[PATCH 0/2] Check for endpoints in fwnode->secondary more sensibly

[PATCH 0/2] Check for endpoints in fwnode->secondary more sensibly

[Daniel Scally]

Hello all

A while ago I patched fwnode_graph_get_endpoint_by_id() to check for endpoints
against fwnode->secondary if none was found against the primary. It's actually
better to do this in fwnode_graph_get_next_endpoint() instead, since that
function is called by fwnode_graph_get_endpoint_by_id() and also directly called
in a bunch of other places (primarily sensor drivers checking that they have
endpoints connected during probe). This small series just adds the equivalent
functionality to fwnode_graph_get_next_endpoint() and reverts the earlier
commit.

Thanks
Dan

Daniel Scally (2):
  device property: Check fwnode->secondary in
    fwnode_graph_get_next_endpoint()
  Revert "media: device property: Call fwnode_graph_get_endpoint_by_id()
    for fwnode->secondary"

 drivers/base/property.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH 1/2] device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()

[Daniel Scally]

Sensor drivers often check for an endpoint to make sure that they're
connected to a consuming device like a CIO2 during .probe(). Some of
those endpoints might be in the form of software_nodes assigned as
a secondary to the device's fwnode_handle. Account for this possibility
in fwnode_graph_get_next_endpoint() to avoid having to do it in the
sensor drivers themselves.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/base/property.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 1421e9548857..e3aceb3a9a0d 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1036,7 +1036,26 @@ struct fwnode_handle *
 fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
 			       struct fwnode_handle *prev)
 {
-	return fwnode_call_ptr_op(fwnode, graph_get_next_endpoint, prev);
+	const struct fwnode_handle *parent;
+	struct fwnode_handle *ep;
+
+	/*
+	 * If this function is in a loop and the previous iteration returned
+	 * an endpoint from fwnode->secondary, then we need to use the secondary
+	 * as parent rather than @fwnode.
+	 */
+	if (prev)
+		parent = fwnode_graph_get_port_parent(prev);
+	else
+		parent = fwnode;
+
+	ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
+
+	if (IS_ERR_OR_NULL(ep) && !IS_ERR_OR_NULL(parent) &&
+	    !IS_ERR_OR_NULL(parent->secondary))
+		ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
+
+	return ep;
 }
 EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
 
-- 
2.25.1

Re: [PATCH 1/2] device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()

[Andy Shevchenko]

On Thu, Jul 22, 2021 at 09:19:28PM +0100, Daniel Scally wrote:
> Sensor drivers often check for an endpoint to make sure that they're
> connected to a consuming device like a CIO2 during .probe(). Some of
> those endpoints might be in the form of software_nodes assigned as
> a secondary to the device's fwnode_handle. Account for this possibility
> in fwnode_graph_get_next_endpoint() to avoid having to do it in the
> sensor drivers themselves.

...

> +	ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
> +
> +	if (IS_ERR_OR_NULL(ep) && !IS_ERR_OR_NULL(parent) &&
> +	    !IS_ERR_OR_NULL(parent->secondary))

Nit-pick, I would put it like:

	if (!IS_ERR_OR_NULL(parent->secondary) && !IS_ERR_OR_NULL(parent) &&
	    IS_ERR_OR_NULL(ep))

or

	if (IS_ERR_OR_NULL(ep) &&
	    !IS_ERR_OR_NULL(parent->secondary) && !IS_ERR_OR_NULL(parent))

for the sake of logical split.

> +		ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()

[Daniel Scally]

On 23/07/2021 13:32, Andy Shevchenko wrote:
> On Thu, Jul 22, 2021 at 09:19:28PM +0100, Daniel Scally wrote:
>> Sensor drivers often check for an endpoint to make sure that they're
>> connected to a consuming device like a CIO2 during .probe(). Some of
>> those endpoints might be in the form of software_nodes assigned as
>> a secondary to the device's fwnode_handle. Account for this possibility
>> in fwnode_graph_get_next_endpoint() to avoid having to do it in the
>> sensor drivers themselves.
> ...
>
>> +	ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
>> +
>> +	if (IS_ERR_OR_NULL(ep) && !IS_ERR_OR_NULL(parent) &&
>> +	    !IS_ERR_OR_NULL(parent->secondary))
> Nit-pick, I would put it like:
>
> 	if (!IS_ERR_OR_NULL(parent->secondary) && !IS_ERR_OR_NULL(parent) &&
> 	    IS_ERR_OR_NULL(ep))
>
> or
>
> 	if (IS_ERR_OR_NULL(ep) &&
> 	    !IS_ERR_OR_NULL(parent->secondary) && !IS_ERR_OR_NULL(parent))
>
> for the sake of logical split.


OK; I'll do the second one, feel like it's better to have ep as the
first check.

>
>> +		ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);

Re: [PATCH 1/2] device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()

[Andy Shevchenko]

On Fri, Jul 23, 2021 at 02:04:59PM +0100, Daniel Scally wrote:
> 
> On 23/07/2021 13:32, Andy Shevchenko wrote:
> > On Thu, Jul 22, 2021 at 09:19:28PM +0100, Daniel Scally wrote:
> >> Sensor drivers often check for an endpoint to make sure that they're
> >> connected to a consuming device like a CIO2 during .probe(). Some of
> >> those endpoints might be in the form of software_nodes assigned as
> >> a secondary to the device's fwnode_handle. Account for this possibility
> >> in fwnode_graph_get_next_endpoint() to avoid having to do it in the
> >> sensor drivers themselves.
> > ...
> >
> >> +	ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
> >> +
> >> +	if (IS_ERR_OR_NULL(ep) && !IS_ERR_OR_NULL(parent) &&
> >> +	    !IS_ERR_OR_NULL(parent->secondary))
> > Nit-pick, I would put it like:
> >
> > 	if (!IS_ERR_OR_NULL(parent->secondary) && !IS_ERR_OR_NULL(parent) &&
> > 	    IS_ERR_OR_NULL(ep))
> >
> > or
> >
> > 	if (IS_ERR_OR_NULL(ep) &&
> > 	    !IS_ERR_OR_NULL(parent->secondary) && !IS_ERR_OR_NULL(parent))
> >
> > for the sake of logical split.
> 
> 
> OK; I'll do the second one, feel like it's better to have ep as the
> first check.

Fine, but also I have just noticed that parent should be checked before
parent->secondary.

Something like this

	if (IS_ERR_OR_NULL(ep) &&
	    !IS_ERR_OR_NULL(parent) && IS_ERR_OR_NULL(parent->secondary))

> >> +		ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()

[Daniel Scally]

On Fri, Jul 30, 2021 at 12:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Jul 23, 2021 at 02:04:59PM +0100, Daniel Scally wrote:
> >
> > On 23/07/2021 13:32, Andy Shevchenko wrote:
> > > On Thu, Jul 22, 2021 at 09:19:28PM +0100, Daniel Scally wrote:
> > >> Sensor drivers often check for an endpoint to make sure that they're
> > >> connected to a consuming device like a CIO2 during .probe(). Some of
> > >> those endpoints might be in the form of software_nodes assigned as
> > >> a secondary to the device's fwnode_handle. Account for this possibility
> > >> in fwnode_graph_get_next_endpoint() to avoid having to do it in the
> > >> sensor drivers themselves.
> > > ...
> > >
> > >> +  ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
> > >> +
> > >> +  if (IS_ERR_OR_NULL(ep) && !IS_ERR_OR_NULL(parent) &&
> > >> +      !IS_ERR_OR_NULL(parent->secondary))
> > > Nit-pick, I would put it like:
> > >
> > >     if (!IS_ERR_OR_NULL(parent->secondary) && !IS_ERR_OR_NULL(parent) &&
> > >         IS_ERR_OR_NULL(ep))
> > >
> > > or
> > >
> > >     if (IS_ERR_OR_NULL(ep) &&
> > >         !IS_ERR_OR_NULL(parent->secondary) && !IS_ERR_OR_NULL(parent))
> > >
> > > for the sake of logical split.
> >
> >
> > OK; I'll do the second one, feel like it's better to have ep as the
> > first check.
>
> Fine, but also I have just noticed that parent should be checked before
> parent->secondary.
>
> Something like this
>
>         if (IS_ERR_OR_NULL(ep) &&
>             !IS_ERR_OR_NULL(parent) && IS_ERR_OR_NULL(parent->secondary))
>
> > >> +          ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);

Yes, no problem. I'll send a v2 when I can, It will likely be another
week or so though, my computer's in a cardboard box.

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

[PATCH 2/2] Revert "media: device property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary"

[Daniel Scally]

This reverts commit acd418bfcfc415cf5e6414b6d1c6acfec850f290. Checking for
endpoints against fwnode->secondary in fwnode_graph_get_next_endpoint() is
a better way to do this since that function is also used in a bunch of
other places, for instance sensor drivers checking that they do have an
endpoint connected during probe.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/base/property.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index e3aceb3a9a0d..689276fb0e45 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1234,14 +1234,7 @@ fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode,
 		best_ep_id = fwnode_ep.id;
 	}
 
-	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;
+	return best_ep;
 }
 EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
 
-- 
2.25.1

Re: [PATCH 0/2] Check for endpoints in fwnode->secondary more sensibly

[Andy Shevchenko]

On Thu, Jul 22, 2021 at 09:19:27PM +0100, Daniel Scally wrote:
> Hello all
> 
> A while ago I patched fwnode_graph_get_endpoint_by_id() to check for endpoints
> against fwnode->secondary if none was found against the primary. It's actually
> better to do this in fwnode_graph_get_next_endpoint() instead, since that
> function is called by fwnode_graph_get_endpoint_by_id() and also directly called
> in a bunch of other places (primarily sensor drivers checking that they have
> endpoints connected during probe). This small series just adds the equivalent
> functionality to fwnode_graph_get_next_endpoint() and reverts the earlier
> commit.

Makes sense to me (one nit-pick to patch 1, though).
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Daniel Scally (2):
>   device property: Check fwnode->secondary in
>     fwnode_graph_get_next_endpoint()
>   Revert "media: device property: Call fwnode_graph_get_endpoint_by_id()
>     for fwnode->secondary"
> 
>  drivers/base/property.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko