| Message ID | 20230204133040.1236799-2-treapking@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | Register Type-C mode-switch in DP bridge endpoints | expand |
Hi Pin-yen, On Sat, Feb 04, 2023 at 09:30:32PM +0800, Pin-yen Lin wrote: > From: Prashant Malani <pmalani@chromium.org> > > When searching the device graph for device matches, check the > remote-endpoint itself for a match. > > Some drivers register devices for individual endpoints. This allows > the matcher code to evaluate those for a match too, instead > of only looking at the remote parent devices. This is required when a > device supports two mode switches in its endpoints, so we can't simply > register the mode switch with the parent node. > > Signed-off-by: Prashant Malani <pmalani@chromium.org> > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > Tested-by: Chen-Yu Tsai <wenst@chromium.org> Thanks for the update. I intended to give my Reviewed-by: but there's something still needs to be addressed. See below. > > --- > > Changes in v11: > - Added missing fwnode_handle_put in drivers/base/property.c > > Changes in v10: > - Collected Reviewed-by and Tested-by tags > > Changes in v6: > - New in v6 > > drivers/base/property.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 2a5a37fcd998..e6f915b72eb7 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -1223,6 +1223,22 @@ static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode, > break; > } > > + /* > + * Some drivers may register devices for endpoints. Check > + * the remote-endpoints for matches in addition to the remote > + * port parent. > + */ > + node = fwnode_graph_get_remote_endpoint(ep); Here fwnode_graph_get_remote_endpoint() returns an endpoint... > + if (fwnode_device_is_available(node)) { and you're calling fwnode_device_is_available() on the endpoint node, which always returns true. Shouldn't you call this on the device node instead? What about match() below? > + ret = match(node, con_id, data); > + if (ret) { > + if (matches) > + matches[count] = ret; > + count++; > + } > + } > + fwnode_handle_put(node); > + > node = fwnode_graph_get_remote_port_parent(ep); > if (!fwnode_device_is_available(node)) { > fwnode_handle_put(node);
Hi Sakari, Thanks for the review. On Mon, Feb 6, 2023 at 5:11 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Pin-yen, > > On Sat, Feb 04, 2023 at 09:30:32PM +0800, Pin-yen Lin wrote: > > From: Prashant Malani <pmalani@chromium.org> > > > > When searching the device graph for device matches, check the > > remote-endpoint itself for a match. > > > > Some drivers register devices for individual endpoints. This allows > > the matcher code to evaluate those for a match too, instead > > of only looking at the remote parent devices. This is required when a > > device supports two mode switches in its endpoints, so we can't simply > > register the mode switch with the parent node. > > > > Signed-off-by: Prashant Malani <pmalani@chromium.org> > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > > Tested-by: Chen-Yu Tsai <wenst@chromium.org> > > Thanks for the update. > > I intended to give my Reviewed-by: but there's something still needs to be > addressed. See below. > > > > > --- > > > > Changes in v11: > > - Added missing fwnode_handle_put in drivers/base/property.c > > > > Changes in v10: > > - Collected Reviewed-by and Tested-by tags > > > > Changes in v6: > > - New in v6 > > > > drivers/base/property.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c > > index 2a5a37fcd998..e6f915b72eb7 100644 > > --- a/drivers/base/property.c > > +++ b/drivers/base/property.c > > @@ -1223,6 +1223,22 @@ static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode, > > break; > > } > > > > + /* > > + * Some drivers may register devices for endpoints. Check > > + * the remote-endpoints for matches in addition to the remote > > + * port parent. > > + */ > > + node = fwnode_graph_get_remote_endpoint(ep); > > Here fwnode_graph_get_remote_endpoint() returns an endpoint... > > > + if (fwnode_device_is_available(node)) { > > and you're calling fwnode_device_is_available() on the endpoint node, which > always returns true. > > Shouldn't you call this on the device node instead? What about match() > below? Yes we should have checked the availability on the device node itself instead of the endpoint node. But regarding the match() call, we need to call it with the endpoint node because that's where we put the "mode-switch" properties and register the mode switches on. We can't use the device node because we want to register two mode switches for the same device node. Regards, Pin-yen > > > + ret = match(node, con_id, data); > > + if (ret) { > > + if (matches) > > + matches[count] = ret; > > + count++; > > + } > > + } > > + fwnode_handle_put(node); > > + > > node = fwnode_graph_get_remote_port_parent(ep); > > if (!fwnode_device_is_available(node)) { > > fwnode_handle_put(node); > > -- > Kind regards, > > Sakari Ailus
Hi Pin-yen, On Thu, Feb 09, 2023 at 12:28:33PM +0800, Pin-yen Lin wrote: > Hi Sakari, > > Thanks for the review. > > On Mon, Feb 6, 2023 at 5:11 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Pin-yen, > > > > On Sat, Feb 04, 2023 at 09:30:32PM +0800, Pin-yen Lin wrote: > > > From: Prashant Malani <pmalani@chromium.org> > > > > > > When searching the device graph for device matches, check the > > > remote-endpoint itself for a match. > > > > > > Some drivers register devices for individual endpoints. This allows > > > the matcher code to evaluate those for a match too, instead > > > of only looking at the remote parent devices. This is required when a > > > device supports two mode switches in its endpoints, so we can't simply > > > register the mode switch with the parent node. > > > > > > Signed-off-by: Prashant Malani <pmalani@chromium.org> > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > > > Tested-by: Chen-Yu Tsai <wenst@chromium.org> > > > > Thanks for the update. > > > > I intended to give my Reviewed-by: but there's something still needs to be > > addressed. See below. > > > > > > > > --- > > > > > > Changes in v11: > > > - Added missing fwnode_handle_put in drivers/base/property.c > > > > > > Changes in v10: > > > - Collected Reviewed-by and Tested-by tags > > > > > > Changes in v6: > > > - New in v6 > > > > > > drivers/base/property.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c > > > index 2a5a37fcd998..e6f915b72eb7 100644 > > > --- a/drivers/base/property.c > > > +++ b/drivers/base/property.c > > > @@ -1223,6 +1223,22 @@ static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode, > > > break; > > > } > > > > > > + /* > > > + * Some drivers may register devices for endpoints. Check > > > + * the remote-endpoints for matches in addition to the remote > > > + * port parent. > > > + */ > > > + node = fwnode_graph_get_remote_endpoint(ep); > > > > Here fwnode_graph_get_remote_endpoint() returns an endpoint... > > > > > + if (fwnode_device_is_available(node)) { > > > > and you're calling fwnode_device_is_available() on the endpoint node, which > > always returns true. > > > > Shouldn't you call this on the device node instead? What about match() > > below? > > Yes we should have checked the availability on the device node itself > instead of the endpoint node. But regarding the match() call, we need > to call it with the endpoint node because that's where we put the > "mode-switch" properties and register the mode switches on. We can't > use the device node because we want to register two mode switches for > the same device node. Ok. I think it should be documented for both fwnode_connection_find_match() and fwnode_connection_find_matches() may then be also called with the endpoint node.
diff --git a/drivers/base/property.c b/drivers/base/property.c index 2a5a37fcd998..e6f915b72eb7 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -1223,6 +1223,22 @@ static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode, break; } + /* + * Some drivers may register devices for endpoints. Check + * the remote-endpoints for matches in addition to the remote + * port parent. + */ + node = fwnode_graph_get_remote_endpoint(ep); + if (fwnode_device_is_available(node)) { + ret = match(node, con_id, data); + if (ret) { + if (matches) + matches[count] = ret; + count++; + } + } + fwnode_handle_put(node); + node = fwnode_graph_get_remote_port_parent(ep); if (!fwnode_device_is_available(node)) { fwnode_handle_put(node);