| Message ID | 20201130133129.1024662-3-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows | expand |
Hi Daniel, Thank you for the patch. On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote: > This function is used to find fwnode endpoints against a device. In > some instances those endpoints are software nodes which are children of > fwnode->secondary. Add support to fwnode_graph_get_endpoint_by_id() to > find those endpoints by recursively calling itself passing the ptr to > fwnode->secondary in the event no endpoint is found for the primary. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since RFC v3: > > Patch introduced. In discussion in the last submission I noted > that the CIO2 device doesn't have an ACPI fwnode - that turns > out to be true for _some_ devices but not others, so we need > this function to check the secondary too. > > drivers/base/property.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index a5ca2306796f..4ece6b086e36 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -1162,6 +1162,10 @@ fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode, > best_ep_id = fwnode_ep.id; > } > > + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > + endpoint, flags); > + > return best_ep; > } > EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
On Mon, Nov 30, 2020 at 07:29:00PM +0200, Andy Shevchenko wrote: > On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote: > > This function is used to find fwnode endpoints against a device. In > > some instances those endpoints are software nodes which are children of > > fwnode->secondary. Add support to fwnode_graph_get_endpoint_by_id() to > > find those endpoints by recursively calling itself passing the ptr to > > fwnode->secondary in the event no endpoint is found for the primary. > > One nit below, after addressing: > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > ... > > > + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > > + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > > + endpoint, flags); > > > return best_ep; > > Can we, please, do > > if (best_ep) > return best_ep; > > if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > endpoint, flags); > > return NULL; > > ? > > This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda > idiomatic to the cases when we need to proceed primary followed by the > secondary in cases where it's not already done. We could also move the !fwnode check to the beginning of the function. > > } > > EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote: > This function is used to find fwnode endpoints against a device. In > some instances those endpoints are software nodes which are children of > fwnode->secondary. Add support to fwnode_graph_get_endpoint_by_id() to > find those endpoints by recursively calling itself passing the ptr to > fwnode->secondary in the event no endpoint is found for the primary. One nit below, after addressing: Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> ... > + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > + endpoint, flags); > return best_ep; Can we, please, do if (best_ep) return best_ep; if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, endpoint, flags); return NULL; ? This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda idiomatic to the cases when we need to proceed primary followed by the secondary in cases where it's not already done. > } > EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
On Mon, Nov 30, 2020 at 07:28:57PM +0200, Laurent Pinchart wrote: > On Mon, Nov 30, 2020 at 07:29:00PM +0200, Andy Shevchenko wrote: > > On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote: ... > > > + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > > > + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > > > + endpoint, flags); > > > > > return best_ep; > > > > Can we, please, do > > > > if (best_ep) > > return best_ep; > > > > if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > > return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > > endpoint, flags); > > > > return NULL; > > > > ? > > > > This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda > > idiomatic to the cases when we need to proceed primary followed by the > > secondary in cases where it's not already done. > > We could also move the !fwnode check to the beginning of the function. It's already there (1). What did I miss? 1) via fwnode_graph_get_next_endpoint() -> fwnode_call_ptr_op()
Hi Andy, On Mon, Nov 30, 2020 at 07:53:19PM +0200, Andy Shevchenko wrote: > On Mon, Nov 30, 2020 at 07:28:57PM +0200, Laurent Pinchart wrote: > > On Mon, Nov 30, 2020 at 07:29:00PM +0200, Andy Shevchenko wrote: > > > On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote: > > ... > > > > > + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > > > > + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > > > > + endpoint, flags); > > > > > > > return best_ep; > > > > > > Can we, please, do > > > > > > if (best_ep) > > > return best_ep; > > > > > > if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > > > return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > > > endpoint, flags); > > > > > > return NULL; > > > > > > ? > > > > > > This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda > > > idiomatic to the cases when we need to proceed primary followed by the > > > secondary in cases where it's not already done. > > > > We could also move the !fwnode check to the beginning of the function. > > It's already there (1). What did I miss? It is, but as we need an explicitly check at the end, it feels cleaner to move it to the beginning. No big deal though. > 1) via fwnode_graph_get_next_endpoint() -> fwnode_call_ptr_op()
On Mon, Nov 30, 2020 at 08:41:41PM +0200, Laurent Pinchart wrote: > On Mon, Nov 30, 2020 at 07:53:19PM +0200, Andy Shevchenko wrote: > > On Mon, Nov 30, 2020 at 07:28:57PM +0200, Laurent Pinchart wrote: > > > On Mon, Nov 30, 2020 at 07:29:00PM +0200, Andy Shevchenko wrote: ... > > > We could also move the !fwnode check to the beginning of the function. > > > > It's already there (1). What did I miss? > > It is, but as we need an explicitly check at the end, it feels cleaner > to move it to the beginning. No big deal though. I prefer to stick with a pattern I mentioned because we may easily to find and unify these ones somehow. > > 1) via fwnode_graph_get_next_endpoint() -> fwnode_call_ptr_op()
On 30/11/2020 17:29, Andy Shevchenko wrote: > On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote: >> This function is used to find fwnode endpoints against a device. In >> some instances those endpoints are software nodes which are children of >> fwnode->secondary. Add support to fwnode_graph_get_endpoint_by_id() to >> find those endpoints by recursively calling itself passing the ptr to >> fwnode->secondary in the event no endpoint is found for the primary. > > One nit below, after addressing: > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > ... > >> + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) >> + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, >> + endpoint, flags); > >> return best_ep; > > Can we, please, do > > if (best_ep) > return best_ep; > > if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > endpoint, flags); > > return NULL; > > ? > > This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda > idiomatic to the cases when we need to proceed primary followed by the > secondary in cases where it's not already done. Thanks - I made this change too
diff --git a/drivers/base/property.c b/drivers/base/property.c index a5ca2306796f..4ece6b086e36 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -1162,6 +1162,10 @@ fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode, best_ep_id = fwnode_ep.id; } + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, + endpoint, flags); + return best_ep; } EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
This function is used to find fwnode endpoints against a device. In some instances those endpoints are software nodes which are children of fwnode->secondary. Add support to fwnode_graph_get_endpoint_by_id() to find those endpoints by recursively calling itself passing the ptr to fwnode->secondary in the event no endpoint is found for the primary. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes since RFC v3: Patch introduced. In discussion in the last submission I noted that the CIO2 device doesn't have an ACPI fwnode - that turns out to be true for _some_ devices but not others, so we need this function to check the secondary too. drivers/base/property.c | 4 ++++ 1 file changed, 4 insertions(+)