On Thu, Aug 20, 2020 at 12:27:03PM +1000, Sam McNally wrote:
[...] diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 1ac874e4e7a1..73a2299c2faa 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2161,11 +2161,23 @@ static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb, int drm_dp_mst_connector_late_register(struct drm_connector *connector, struct drm_dp_mst_port *port) {
int ret; DRM_DEBUG_KMS("registering %s remote bus for %s\n", port->aux.name, connector->kdev->kobj.name); port->aux.dev = connector->kdev;
return drm_dp_aux_register_devnode(&port->aux);
ret = drm_dp_aux_register_devnode(&port->aux);if (ret)return ret;if (port->pdt != DP_PEER_DEVICE_NONE &&drm_dp_mst_is_end_device(port->pdt, port->mcs)) {How can we get here when drm_dp_mst_is_end_device(port) is not true? AFAICS that's only case where we should create a connector and an i2c device. (IOW we don't create them for branch ports.)
I'm not sure what you mean. Wouldn't this condition be checked during the registration of any MST connector? This follows the pattern used in drm_dp_mst_port_add_connector() [0], which seems like it's invoked in the same cases as drm_dp_mst_connector_late_register(), modulo early outs for errors.
Re-reading the code, a DRM connector is created whenever the MST port is an output port, so even in the case of an output branch port.
I'm still not sure why we can't register/unregister the I2C bus whenever creating/removing the DRM connector. That's also the scope of the AUX bus, which is what I2C depends on, and if a port doesn't support I2C messaging then the corresponding AUX messages would be NAKed.
--Imre
[0] https://cgit.freedesktop.org/drm-tip/tree/drivers/gpu/drm/drm_dp_mst_topolog...
ret = sysfs_create_link(&port->connector->kdev->kobj,&port->aux.ddc.dev.kobj, "ddc");if (ret)drm_dp_aux_unregister_devnode(&port->aux);}return ret;} EXPORT_SYMBOL(drm_dp_mst_connector_late_register);
@@ -5490,6 +5502,7 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port) { struct drm_dp_aux *aux = &port->aux; struct device *parent_dev = port->mgr->dev->dev;
int ret; aux->ddc.algo = &drm_dp_mst_i2c_algo; aux->ddc.algo_data = aux;@@ -5504,7 +5517,17 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port) strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(parent_dev), sizeof(aux->ddc.name));
return i2c_add_adapter(&aux->ddc);
ret = i2c_add_adapter(&aux->ddc);if (ret)return ret;if (port->connector && port->connector->kdev) {ret = sysfs_create_link(&port->connector->kdev->kobj,&port->aux.ddc.dev.kobj, "ddc");if (ret)i2c_del_adapter(&port->aux.ddc);}return ret;}
/** @@ -5513,6 +5536,8 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port) */ static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_mst_port *port) {
if (port->connector && port->connector->kdev)sysfs_remove_link(&port->connector->kdev->kobj, "ddc"); i2c_del_adapter(&port->aux.ddc);}
-- 2.28.0.rc0.142.g3c755180ce-goog