diff mbox series

[02/18] property: Add support for calling fwnode_graph_get_endpoint_by_id() for fwnode->secondary

Message ID 20201130133129.1024662-3-djrscally@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows | expand

Commit Message

Daniel Scally Nov. 30, 2020, 1:31 p.m. UTC
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(+)

Comments

Laurent Pinchart Nov. 30, 2020, 4:08 p.m. UTC | #1
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);
Laurent Pinchart Nov. 30, 2020, 5:28 p.m. UTC | #2
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);
Andy Shevchenko Nov. 30, 2020, 5:29 p.m. UTC | #3
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);
Andy Shevchenko Nov. 30, 2020, 5:53 p.m. UTC | #4
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()
Laurent Pinchart Nov. 30, 2020, 6:41 p.m. UTC | #5
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()
Andy Shevchenko Nov. 30, 2020, 7:21 p.m. UTC | #6
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()
Daniel Scally Dec. 2, 2020, 10:13 a.m. UTC | #7
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 mbox series

Patch

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);