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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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 <[email protected]>
> 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
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);
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
On Fri, Jul 30, 2021 at 12:34 PM Andy Shevchenko
<[email protected]> 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
>
>