[PATCH v1 1/3] of: base: Provide of_graph_get_port_parent()

[PATCH v1 1/3] of: base: Provide of_graph_get_port_parent()

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

When handling endpoints, the v4l2 async framework needs to identify the
parent device of a port endpoint.

Adapt the existing of_graph_get_remote_port_parent() such that a caller
can obtain the parent of a port without parsing the remote-endpoint
first.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/of/base.c        | 30 ++++++++++++++++++++++--------
 include/linux/of_graph.h |  1 +
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d7c4629a3a2d..f81100500999 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2455,6 +2455,27 @@ struct device_node *of_graph_get_endpoint_by_regs(
 EXPORT_SYMBOL(of_graph_get_endpoint_by_regs);
 
 /**
+ * of_graph_get_port_parent() - get port's parent node
+ * @node: pointer to a local endpoint device_node
+ *
+ * Return: device node associated with endpoint @node.
+ *	   Use of_node_put() on it when done.
+ */
+struct device_node *of_graph_get_port_parent(struct device_node *node)
+{
+	unsigned int depth;
+
+	/* Walk 3 levels up only if there is 'ports' node. */
+	for (depth = 3; depth && node; depth--) {
+		node = of_get_next_parent(node);
+		if (depth == 2 && of_node_cmp(node->name, "ports"))
+			break;
+	}
+	return node;
+}
+EXPORT_SYMBOL(of_graph_get_port_parent);
+
+/**
  * of_graph_get_remote_port_parent() - get remote port's parent node
  * @node: pointer to a local endpoint device_node
  *
@@ -2465,18 +2486,11 @@ struct device_node *of_graph_get_remote_port_parent(
 			       const struct device_node *node)
 {
 	struct device_node *np;
-	unsigned int depth;
 
 	/* Get remote endpoint node. */
 	np = of_parse_phandle(node, "remote-endpoint", 0);
 
-	/* Walk 3 levels up only if there is 'ports' node. */
-	for (depth = 3; depth && np; depth--) {
-		np = of_get_next_parent(np);
-		if (depth == 2 && of_node_cmp(np->name, "ports"))
-			break;
-	}
-	return np;
+	return of_graph_get_port_parent(np);
 }
 EXPORT_SYMBOL(of_graph_get_remote_port_parent);
 
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index abdb02eaef06..49bf34880ebc 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -48,6 +48,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 					struct device_node *previous);
 struct device_node *of_graph_get_endpoint_by_regs(
 		const struct device_node *parent, int port_reg, int reg);
+struct device_node *of_graph_get_port_parent(struct device_node *node);
 struct device_node *of_graph_get_remote_port_parent(
 					const struct device_node *node);
 struct device_node *of_graph_get_remote_port(const struct device_node *node);
-- 
git-series 0.9.1

Re: [PATCH v1 1/3] of: base: Provide of_graph_get_port_parent()

[Rob Herring]

On Wed, May 17, 2017 at 10:03 AM, Kieran Bingham <kbingham@kernel.org> wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> When handling endpoints, the v4l2 async framework needs to identify the
> parent device of a port endpoint.
>
> Adapt the existing of_graph_get_remote_port_parent() such that a caller
> can obtain the parent of a port without parsing the remote-endpoint
> first.

A similar patch is already applied as part of the ASoC graph card support.

Rob

Re: [PATCH v1 1/3] of: base: Provide of_graph_get_port_parent()

[Kieran Bingham]

On 17/05/17 17:36, Rob Herring wrote:
> On Wed, May 17, 2017 at 10:03 AM, Kieran Bingham <kbingham@kernel.org> wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> When handling endpoints, the v4l2 async framework needs to identify the
>> parent device of a port endpoint.
>>
>> Adapt the existing of_graph_get_remote_port_parent() such that a caller
>> can obtain the parent of a port without parsing the remote-endpoint
>> first.
> 
> A similar patch is already applied as part of the ASoC graph card support.
> 
> Rob

Ah yes, a quick google finds it...
:  https://patchwork.kernel.org/patch/9658907/

Surprisingly similar patch ... and a familiar name.

Morimoto-san - you beat me to it :D !

Thanks Rob, (And Morimoto!)

--
Kieran

Re: [PATCH v1 1/3] of: base: Provide of_graph_get_port_parent()

[Kuninori Morimoto]

Hi Kieran

> >> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>
> >> When handling endpoints, the v4l2 async framework needs to identify the
> >> parent device of a port endpoint.
> >>
> >> Adapt the existing of_graph_get_remote_port_parent() such that a caller
> >> can obtain the parent of a port without parsing the remote-endpoint
> >> first.
> > 
> > A similar patch is already applied as part of the ASoC graph card support.
> > 
> > Rob
> 
> Ah yes, a quick google finds it...
> :  https://patchwork.kernel.org/patch/9658907/
> 
> Surprisingly similar patch ... and a familiar name.
> 
> Morimoto-san - you beat me to it :D !

Interesting.
It was applies today to Mark's (= ALSA SoC Maintainer) branch !
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=topic/of-graph&id=0ef472a973ebbfc20f2f12769e77a8cfd3612778


Best regards
---
Kuninori Morimoto

Re: [PATCH v1 1/3] of: base: Provide of_graph_get_port_parent()

[Laurent Pinchart]

Hi Kieran,

On Wednesday 17 May 2017 21:02:42 Kieran Bingham wrote:
> On 17/05/17 17:36, Rob Herring wrote:
> > On Wed, May 17, 2017 at 10:03 AM, Kieran Bingham wrote:
> >> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> 
> >> When handling endpoints, the v4l2 async framework needs to identify the
> >> parent device of a port endpoint.
> >> 
> >> Adapt the existing of_graph_get_remote_port_parent() such that a caller
> >> can obtain the parent of a port without parsing the remote-endpoint
> >> first.
> > 
> > A similar patch is already applied as part of the ASoC graph card support.
> > 
> > Rob
> 
> Ah yes, a quick google finds it...
> 
> :  https://patchwork.kernel.org/patch/9658907/
> 
> Surprisingly similar patch ... and a familiar name.

Very similar indeed, down to identical problems ;-) Quoting your patch,

>  /**
> + * of_graph_get_port_parent() - get port's parent node
> + * @node: pointer to a local endpoint device_node
> + *
> + * Return: device node associated with endpoint @node.
> + *	   Use of_node_put() on it when done.
> + */

The documentation of the return value is a bit confusing to me. I assume that, 
by device node, you meant the DT node corresponding to the device of the 
endpoint which is passed as an argument. However, device node cal also refer 
to struct device_node. Should this be clarified ?

> Morimoto-san - you beat me to it :D !
> 
> Thanks Rob, (And Morimoto!)

-- 
Regards,

Laurent Pinchart

[PATCH v1 2/3] device property: Add fwnode_graph_get_port_parent

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

V4L2 async notifiers can pass the endpoint fwnode rather than the device
fwnode.

Provide a helper to obtain the parent device fwnode without first
parsing the remote-endpoint as per fwnode_graph_get_remote_port_parent.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/base/property.c  | 25 +++++++++++++++++++++++++
 include/linux/property.h |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 627ebc9b570d..caf4316fe565 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1245,6 +1245,31 @@ fwnode_graph_get_next_endpoint(struct fwnode_handle *fwnode,
 EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
 
 /**
+ * fwnode_graph_get_port_parent - Return device node of a port endpoint
+ * @fwnode: Endpoint firmware node pointing of the port
+ *
+ * Extracts firmware node of the device the @fwnode belongs to.
+ */
+struct fwnode_handle *
+fwnode_graph_get_port_parent(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *parent = NULL;
+
+	if (is_of_node(fwnode)) {
+		struct device_node *node;
+
+		node = of_graph_get_port_parent(to_of_node(fwnode));
+		if (node)
+			parent = &node->fwnode;
+	} else if (is_acpi_node(fwnode)) {
+		parent = acpi_node_get_parent(fwnode);
+	}
+
+	return parent;
+}
+EXPORT_SYMBOL_GPL(fwnode_graph_get_port_parent);
+
+/**
  * fwnode_graph_get_remote_port_parent - Return fwnode of a remote device
  * @fwnode: Endpoint firmware node pointing to the remote endpoint
  *
diff --git a/include/linux/property.h b/include/linux/property.h
index 2f482616a2f2..624129b86c82 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -274,6 +274,8 @@ void *device_get_mac_address(struct device *dev, char *addr, int alen);
 
 struct fwnode_handle *fwnode_graph_get_next_endpoint(
 	struct fwnode_handle *fwnode, struct fwnode_handle *prev);
+struct fwnode_handle *fwnode_graph_get_port_parent(
+	struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_graph_get_remote_port_parent(
 	struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_graph_get_remote_port(
-- 
git-series 0.9.1

Re: [PATCH v1 2/3] device property: Add fwnode_graph_get_port_parent

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 17 May 2017 16:03:38 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> V4L2 async notifiers can pass the endpoint fwnode rather than the device
> fwnode.

I'm not sure I would mention V4L2 in the commit message, as this is generic.

> Provide a helper to obtain the parent device fwnode without first
> parsing the remote-endpoint as per fwnode_graph_get_remote_port_parent.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/base/property.c  | 25 +++++++++++++++++++++++++
>  include/linux/property.h |  2 ++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 627ebc9b570d..caf4316fe565 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1245,6 +1245,31 @@ fwnode_graph_get_next_endpoint(struct fwnode_handle
> *fwnode, EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
> 
>  /**
> + * fwnode_graph_get_port_parent - Return device node of a port endpoint
> + * @fwnode: Endpoint firmware node pointing of the port
> + *
> + * Extracts firmware node of the device the @fwnode belongs to.

I'm not too familiar with the fwnode API, but I know it's written in C, where 
functions don't extract something but return a value :-) How about

Return: the firmware node of the device the @endpoint belongs to.

> + */
> +struct fwnode_handle *
> +fwnode_graph_get_port_parent(struct fwnode_handle *fwnode)

This is akin to writing (unsigned int integer)

How about calling the variable endpoint ? That would also make the 
documentation clearer in my opinion, with "the @fwnode belongs to" replaced 
with "the @endpoint belongs to".

> +{
> +	struct fwnode_handle *parent = NULL;
> +
> +	if (is_of_node(fwnode)) {
> +		struct device_node *node;
> +
> +		node = of_graph_get_port_parent(to_of_node(fwnode));
> +		if (node)
> +			parent = &node->fwnode;

This part looks good to me, with the above small change,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	} else if (is_acpi_node(fwnode)) {
> +		parent = acpi_node_get_parent(fwnode);

I can't comment on this one though.

> +	}
> +
> +	return parent;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_graph_get_port_parent);
> +
> +/**
>   * fwnode_graph_get_remote_port_parent - Return fwnode of a remote device
>   * @fwnode: Endpoint firmware node pointing to the remote endpoint
>   *
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 2f482616a2f2..624129b86c82 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -274,6 +274,8 @@ void *device_get_mac_address(struct device *dev, char
> *addr, int alen);
> 
>  struct fwnode_handle *fwnode_graph_get_next_endpoint(
>  	struct fwnode_handle *fwnode, struct fwnode_handle *prev);
> +struct fwnode_handle *fwnode_graph_get_port_parent(
> +	struct fwnode_handle *fwnode);
>  struct fwnode_handle *fwnode_graph_get_remote_port_parent(
>  	struct fwnode_handle *fwnode);
>  struct fwnode_handle *fwnode_graph_get_remote_port(

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 2/3] device property: Add fwnode_graph_get_port_parent

[Kieran Bingham]

Hi Laurent,

On 18/05/17 14:36, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday 17 May 2017 16:03:38 Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> V4L2 async notifiers can pass the endpoint fwnode rather than the device
>> fwnode.
> 
> I'm not sure I would mention V4L2 in the commit message, as this is generic.

Good point

>> Provide a helper to obtain the parent device fwnode without first
>> parsing the remote-endpoint as per fwnode_graph_get_remote_port_parent.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/base/property.c  | 25 +++++++++++++++++++++++++
>>  include/linux/property.h |  2 ++
>>  2 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index 627ebc9b570d..caf4316fe565 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -1245,6 +1245,31 @@ fwnode_graph_get_next_endpoint(struct fwnode_handle
>> *fwnode, EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
>>
>>  /**
>> + * fwnode_graph_get_port_parent - Return device node of a port endpoint
>> + * @fwnode: Endpoint firmware node pointing of the port
>> + *
>> + * Extracts firmware node of the device the @fwnode belongs to.
> 
> I'm not too familiar with the fwnode API, but I know it's written in C, where 
> functions don't extract something but return a value :-) How about
> 
> Return: the firmware node of the device the @endpoint belongs to.
> 

I'm not averse to the reword - but it is different to the other functions in the
same context:

fwnode_graph_get_remote_endpoint(struct fwnode_handle *fwnode)
 * Extracts firmware node of a remote endpoint the @fwnode points to.

struct fwnode_handle *fwnode_graph_get_remote_port(struct fwnode_handle *fwnode)
 * Extracts firmware node of a remote port the @fwnode points to.

fwnode_graph_get_remote_port_parent(struct fwnode_handle *fwnode)
 * Extracts firmware node of a remote device the @fwnode points to.

Then with this function becoming:

fwnode_graph_get_port_parent(struct fwnode_handle *endpoint)
 * Returns firmware node of the device the @endpoint belongs to.


I guess those could be changed too ...


>> + */
>> +struct fwnode_handle *
>> +fwnode_graph_get_port_parent(struct fwnode_handle *fwnode)
> 
> This is akin to writing (unsigned int integer)

Yes, good point there - I was thinking of the fwnode as an object itself, but
really it's representing the endpoint, and the fwnode is the class type :)

> 
> How about calling the variable endpoint ? That would also make the 
> documentation clearer in my opinion, with "the @fwnode belongs to" replaced 
> with "the @endpoint belongs to".

Agreed

> 
>> +{
>> +	struct fwnode_handle *parent = NULL;
>> +
>> +	if (is_of_node(fwnode)) {
>> +		struct device_node *node;
>> +
>> +		node = of_graph_get_port_parent(to_of_node(fwnode));
>> +		if (node)
>> +			parent = &node->fwnode;
> 
> This part looks good to me, with the above small change,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks,

I'll add this if the code doesn't change drastically based on Sakari's suggestion.

> 
>> +	} else if (is_acpi_node(fwnode)) {
>> +		parent = acpi_node_get_parent(fwnode);
> 
> I can't comment on this one though.
> 
>> +	}
>> +
>> +	return parent;
>> +}
>> +EXPORT_SYMBOL_GPL(fwnode_graph_get_port_parent);
>> +
>> +/**
>>   * fwnode_graph_get_remote_port_parent - Return fwnode of a remote device
>>   * @fwnode: Endpoint firmware node pointing to the remote endpoint
>>   *
>> diff --git a/include/linux/property.h b/include/linux/property.h
>> index 2f482616a2f2..624129b86c82 100644
>> --- a/include/linux/property.h
>> +++ b/include/linux/property.h
>> @@ -274,6 +274,8 @@ void *device_get_mac_address(struct device *dev, char
>> *addr, int alen);
>>
>>  struct fwnode_handle *fwnode_graph_get_next_endpoint(
>>  	struct fwnode_handle *fwnode, struct fwnode_handle *prev);
>> +struct fwnode_handle *fwnode_graph_get_port_parent(
>> +	struct fwnode_handle *fwnode);
>>  struct fwnode_handle *fwnode_graph_get_remote_port_parent(
>>  	struct fwnode_handle *fwnode);
>>  struct fwnode_handle *fwnode_graph_get_remote_port(
> 

Re: [PATCH v1 2/3] device property: Add fwnode_graph_get_port_parent

[Laurent Pinchart]

Hi Kieran,

On Friday 19 May 2017 14:34:33 Kieran Bingham wrote:
> On 18/05/17 14:36, Laurent Pinchart wrote:
> > On Wednesday 17 May 2017 16:03:38 Kieran Bingham wrote:
> >> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> 
> >> V4L2 async notifiers can pass the endpoint fwnode rather than the device
> >> fwnode.
> > 
> > I'm not sure I would mention V4L2 in the commit message, as this is
> > generic.
>
> Good point
> 
> >> Provide a helper to obtain the parent device fwnode without first
> >> parsing the remote-endpoint as per fwnode_graph_get_remote_port_parent.
> >> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> ---
> >> 
> >>  drivers/base/property.c  | 25 +++++++++++++++++++++++++
> >>  include/linux/property.h |  2 ++
> >>  2 files changed, 27 insertions(+)
> >> 
> >> diff --git a/drivers/base/property.c b/drivers/base/property.c
> >> index 627ebc9b570d..caf4316fe565 100644
> >> --- a/drivers/base/property.c
> >> +++ b/drivers/base/property.c
> >> @@ -1245,6 +1245,31 @@ fwnode_graph_get_next_endpoint(struct
> >> fwnode_handle
> >> *fwnode, EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
> >> 
> >>  /**
> >> 
> >> + * fwnode_graph_get_port_parent - Return device node of a port endpoint
> >> + * @fwnode: Endpoint firmware node pointing of the port
> >> + *
> >> + * Extracts firmware node of the device the @fwnode belongs to.
> > 
> > I'm not too familiar with the fwnode API, but I know it's written in C,
> > where functions don't extract something but return a value :-) How about
> > 
> > Return: the firmware node of the device the @endpoint belongs to.
> 
> I'm not averse to the reword - but it is different to the other functions in
> the same context:
> 
> fwnode_graph_get_remote_endpoint(struct fwnode_handle *fwnode)
>  * Extracts firmware node of a remote endpoint the @fwnode points to.
> 
> struct fwnode_handle *fwnode_graph_get_remote_port(struct fwnode_handle
> *fwnode)
>  * Extracts firmware node of a remote port the @fwnode points to.
> 
> fwnode_graph_get_remote_port_parent(struct fwnode_handle *fwnode)
>  * Extracts firmware node of a remote device the @fwnode points to.
> 
> Then with this function becoming:
> 
> fwnode_graph_get_port_parent(struct fwnode_handle *endpoint)
>  * Returns firmware node of the device the @endpoint belongs to.
> 
> 
> I guess those could be changed too ...

My point is that the kerneldoc format documents return values with a "Return:" 
tag. The documentation for the function can still provide extra information.

> >> + */
> >> +struct fwnode_handle *
> >> +fwnode_graph_get_port_parent(struct fwnode_handle *fwnode)
> > 
> > This is akin to writing (unsigned int integer)
> 
> Yes, good point there - I was thinking of the fwnode as an object itself,
> but really it's representing the endpoint, and the fwnode is the class type
> :)
>
> > How about calling the variable endpoint ? That would also make the
> > documentation clearer in my opinion, with "the @fwnode belongs to"
> > replaced with "the @endpoint belongs to".
> 
> Agreed
> 
> >> +{
> >> +	struct fwnode_handle *parent = NULL;
> >> +
> >> +	if (is_of_node(fwnode)) {
> >> +		struct device_node *node;
> >> +
> >> +		node = of_graph_get_port_parent(to_of_node(fwnode));
> >> +		if (node)
> >> +			parent = &node->fwnode;
> > 
> > This part looks good to me, with the above small change,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks,
> 
> I'll add this if the code doesn't change drastically based on Sakari's
> suggestion.
>
> >> +	} else if (is_acpi_node(fwnode)) {
> >> +		parent = acpi_node_get_parent(fwnode);
> > 
> > I can't comment on this one though.
> > 
> >> +	}
> >> +
> >> +	return parent;
> >> +}
> >> +EXPORT_SYMBOL_GPL(fwnode_graph_get_port_parent);
> >> +
> >> +/**
> >> 
> >>   * fwnode_graph_get_remote_port_parent - Return fwnode of a remote
> >>   device
> >>   * @fwnode: Endpoint firmware node pointing to the remote endpoint
> >>   *
> >> 
> >> diff --git a/include/linux/property.h b/include/linux/property.h
> >> index 2f482616a2f2..624129b86c82 100644
> >> --- a/include/linux/property.h
> >> +++ b/include/linux/property.h
> >> @@ -274,6 +274,8 @@ void *device_get_mac_address(struct device *dev, char
> >> *addr, int alen);
> >> 
> >>  struct fwnode_handle *fwnode_graph_get_next_endpoint(
> >>  
> >>  	struct fwnode_handle *fwnode, struct fwnode_handle *prev);
> >> 
> >> +struct fwnode_handle *fwnode_graph_get_port_parent(
> >> +	struct fwnode_handle *fwnode);
> >> 
> >>  struct fwnode_handle *fwnode_graph_get_remote_port_parent(
> >>  
> >>  	struct fwnode_handle *fwnode);
> >>  
> >>  struct fwnode_handle *fwnode_graph_get_remote_port(

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 2/3] device property: Add fwnode_graph_get_port_parent

[Sakari Ailus]

Hi Laurent and Kieran,

On Fri, May 19, 2017 at 05:42:07PM +0300, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Friday 19 May 2017 14:34:33 Kieran Bingham wrote:
> > On 18/05/17 14:36, Laurent Pinchart wrote:
> > > On Wednesday 17 May 2017 16:03:38 Kieran Bingham wrote:
> > >> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > >> 
> > >> V4L2 async notifiers can pass the endpoint fwnode rather than the device
> > >> fwnode.
> > > 
> > > I'm not sure I would mention V4L2 in the commit message, as this is
> > > generic.
> >
> > Good point
> > 
> > >> Provide a helper to obtain the parent device fwnode without first
> > >> parsing the remote-endpoint as per fwnode_graph_get_remote_port_parent.
> > >> 
> > >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > >> ---
> > >> 
> > >>  drivers/base/property.c  | 25 +++++++++++++++++++++++++
> > >>  include/linux/property.h |  2 ++
> > >>  2 files changed, 27 insertions(+)
> > >> 
> > >> diff --git a/drivers/base/property.c b/drivers/base/property.c
> > >> index 627ebc9b570d..caf4316fe565 100644
> > >> --- a/drivers/base/property.c
> > >> +++ b/drivers/base/property.c
> > >> @@ -1245,6 +1245,31 @@ fwnode_graph_get_next_endpoint(struct
> > >> fwnode_handle
> > >> *fwnode, EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
> > >> 
> > >>  /**
> > >> 
> > >> + * fwnode_graph_get_port_parent - Return device node of a port endpoint
> > >> + * @fwnode: Endpoint firmware node pointing of the port
> > >> + *
> > >> + * Extracts firmware node of the device the @fwnode belongs to.
> > > 
> > > I'm not too familiar with the fwnode API, but I know it's written in C,
> > > where functions don't extract something but return a value :-) How about
> > > 
> > > Return: the firmware node of the device the @endpoint belongs to.
> > 
> > I'm not averse to the reword - but it is different to the other functions in
> > the same context:
> > 
> > fwnode_graph_get_remote_endpoint(struct fwnode_handle *fwnode)
> >  * Extracts firmware node of a remote endpoint the @fwnode points to.
> > 
> > struct fwnode_handle *fwnode_graph_get_remote_port(struct fwnode_handle
> > *fwnode)
> >  * Extracts firmware node of a remote port the @fwnode points to.
> > 
> > fwnode_graph_get_remote_port_parent(struct fwnode_handle *fwnode)
> >  * Extracts firmware node of a remote device the @fwnode points to.
> > 
> > Then with this function becoming:
> > 
> > fwnode_graph_get_port_parent(struct fwnode_handle *endpoint)
> >  * Returns firmware node of the device the @endpoint belongs to.
> > 
> > 
> > I guess those could be changed too ...
> 
> My point is that the kerneldoc format documents return values with a "Return:" 
> tag. The documentation for the function can still provide extra information.

Yeah, let's do this right and then fix the rest. I've already submitted the
pull request on this one --- the origin of that text is actually the V4L2 OF
framework. Feel free to submit a patch that fixes it, I can do it as well.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v1 2/3] device property: Add fwnode_graph_get_port_parent

[Sakari Ailus]

Hi Kieran,

On Wed, May 17, 2017 at 04:03:38PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> V4L2 async notifiers can pass the endpoint fwnode rather than the device
> fwnode.
> 
> Provide a helper to obtain the parent device fwnode without first
> parsing the remote-endpoint as per fwnode_graph_get_remote_port_parent.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Could you rebase this on my fwnode cleanup patchset here, please? I'd be
happy to apply it on top of the set.

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=acpi-graph-cleaned>

This function becomes quite simple, see
fwnode_graph_get_remote_port_parent().

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH v1 3/3] v4l: async: Match parent devices

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Devices supporting multiple endpoints on a single device node must set
their subdevice fwnode to the endpoint to allow distinct comparisons.

Adapt the match_fwnode call to compare against the provided fwnodes
first, but also to search for a comparison against the parent fwnode.

This allows notifiers to pass the endpoint for comparison and still
support existing subdevices which store their default parent device
node.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-async.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index e1e181db90f7..65735a5c4350 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -41,14 +41,26 @@ static bool match_devname(struct v4l2_subdev *sd,
 	return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
 }
 
+static bool match_of(struct device_node *a, struct device_node *b)
+{
+	return !of_node_cmp(of_node_full_name(a), of_node_full_name(b));
+}
+
 static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
+	struct device_node *sdnode;
+	struct fwnode_handle *async_device;
+
+	async_device = fwnode_graph_get_port_parent(asd->match.fwnode.fwnode);
+
 	if (!is_of_node(sd->fwnode) || !is_of_node(asd->match.fwnode.fwnode))
-		return sd->fwnode == asd->match.fwnode.fwnode;
+		return sd->fwnode == asd->match.fwnode.fwnode ||
+		       sd->fwnode == async_device;
+
+	sdnode = to_of_node(sd->fwnode);
 
-	return !of_node_cmp(of_node_full_name(to_of_node(sd->fwnode)),
-			    of_node_full_name(
-				    to_of_node(asd->match.fwnode.fwnode)));
+	return match_of(sdnode, to_of_node(asd->match.fwnode.fwnode)) ||
+	       match_of(sdnode, to_of_node(async_device));
 }
 
 static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
-- 
git-series 0.9.1

Re: [PATCH v1 3/3] v4l: async: Match parent devices

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 17 May 2017 16:03:39 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Devices supporting multiple endpoints on a single device node must set
> their subdevice fwnode to the endpoint to allow distinct comparisons.
> 
> Adapt the match_fwnode call to compare against the provided fwnodes
> first, but also to search for a comparison against the parent fwnode.
> 
> This allows notifiers to pass the endpoint for comparison and still
> support existing subdevices which store their default parent device
> node.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index e1e181db90f7..65735a5c4350
> 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -41,14 +41,26 @@ static bool match_devname(struct v4l2_subdev *sd,
>  	return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
>  }
> 
/*
 * Check whether the two device_node pointers refer to the same OF node. We
 * can't compare pointers directly as they can differ if overlays have been
 * applied.
 */

> +static bool match_of(struct device_node *a, struct device_node *b)
> +{
> +	return !of_node_cmp(of_node_full_name(a), of_node_full_name(b));
> +}
> +
>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd)
> {
> +	struct device_node *sdnode;
> +	struct fwnode_handle *async_device;

I would name this asd_fwnode, and to be consistent rename sdnode to sd_ofnode.

> +
> +	async_device = fwnode_graph_get_port_parent(asd->match.fwnode.fwnode);
> +
>  	if (!is_of_node(sd->fwnode) || !is_of_node(asd->match.fwnode.fwnode))
> -		return sd->fwnode == asd->match.fwnode.fwnode;
> +		return sd->fwnode == asd->match.fwnode.fwnode ||
> +		       sd->fwnode == async_device;

I wonder whether we could simplify this by changing the 
fwnode_graph_get_port_parent() API. At the moment the function walks two or 
three levels up depending on whether there's a ports name or not. If we turned 
in into a function that accepts an endpoint, port or device node, and returns 
the device node unconditionally (basically, returning the argument if its name 
is not "port(@[0-9]+)?" or "endpoint(@[0-9]+)?", and walking up until it 
reaches the device node otherwise), you could write the above

	asd_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode.fwnode);

  	if (!is_of_node(sd->fwnode) || !is_of_node(asd_fwnode))
		       sd->fwnode == asd_fwnode;

	sdnode = to_of_node(sd->fwnode);
 
	return match_of(sdnode, to_of_node(asd_node));

> +
> +	sdnode = to_of_node(sd->fwnode);
> 
> -	return !of_node_cmp(of_node_full_name(to_of_node(sd->fwnode)),
> -			    of_node_full_name(
> -				    to_of_node(asd->match.fwnode.fwnode)));
> +	return match_of(sdnode, to_of_node(asd->match.fwnode.fwnode)) ||
> +	       match_of(sdnode, to_of_node(async_device));

This is getting a bit complex, could you document the function ?

>  }
> 
>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 3/3] v4l: async: Match parent devices

[Kieran Bingham]

Hi Laurent,

On 18/05/17 15:01, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday 17 May 2017 16:03:39 Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> Devices supporting multiple endpoints on a single device node must set
>> their subdevice fwnode to the endpoint to allow distinct comparisons.
>>
>> Adapt the match_fwnode call to compare against the provided fwnodes
>> first, but also to search for a comparison against the parent fwnode.
>>
>> This allows notifiers to pass the endpoint for comparison and still
>> support existing subdevices which store their default parent device
>> node.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-async.c | 20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c
>> b/drivers/media/v4l2-core/v4l2-async.c index e1e181db90f7..65735a5c4350
>> 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -41,14 +41,26 @@ static bool match_devname(struct v4l2_subdev *sd,
>>  	return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
>>  }
>>
> /*
>  * Check whether the two device_node pointers refer to the same OF node. We
>  * can't compare pointers directly as they can differ if overlays have been
>  * applied.
>  */

Thanks - that's a good addition - I've put it in.

> 
>> +static bool match_of(struct device_node *a, struct device_node *b)
>> +{
>> +	return !of_node_cmp(of_node_full_name(a), of_node_full_name(b));
>> +}
>> +
>>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev
>> *asd)
>> {
>> +	struct device_node *sdnode;
>> +	struct fwnode_handle *async_device;
> 
> I would name this asd_fwnode, and to be consistent rename sdnode to sd_ofnode.

Actually, now that I agree with Sakari, and the parent of both the SD and the
ASD should be cross-referenced, I have used:

	sd_parent = fwnode_graph_get_port_parent(sd->fwnode);
	asd_parent = fwnode_graph_get_port_parent(asd_fwnode);

> 
>> +
>> +	async_device = fwnode_graph_get_port_parent(asd->match.fwnode.fwnode);
>> +
>>  	if (!is_of_node(sd->fwnode) || !is_of_node(asd->match.fwnode.fwnode))
>> -		return sd->fwnode == asd->match.fwnode.fwnode;
>> +		return sd->fwnode == asd->match.fwnode.fwnode ||
>> +		       sd->fwnode == async_device;
> 
> I wonder whether we could simplify this by changing the 
> fwnode_graph_get_port_parent() API. At the moment the function walks two or 
> three levels up depending on whether there's a ports name or not. If we turned 
> in into a function that accepts an endpoint, port or device node, and returns 
> the device node unconditionally (basically, returning the argument if its name 
> is not "port(@[0-9]+)?" or "endpoint(@[0-9]+)?", and walking up until it 
> reaches the device node otherwise), you could write the above
> 
> 	asd_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode.fwnode);
> 
>   	if (!is_of_node(sd->fwnode) || !is_of_node(asd_fwnode))
> 		       sd->fwnode == asd_fwnode;
> 
> 	sdnode = to_of_node(sd->fwnode);
>  
> 	return match_of(sdnode, to_of_node(asd_node));


I don't think that would help here. I want the function to do comparisons on the
endpoint when provided - I don't want helpers to suddenly bring the comparison
up to the device level.

> 
>> +
>> +	sdnode = to_of_node(sd->fwnode);
>>
>> -	return !of_node_cmp(of_node_full_name(to_of_node(sd->fwnode)),
>> -			    of_node_full_name(
>> -				    to_of_node(asd->match.fwnode.fwnode)));
>> +	return match_of(sdnode, to_of_node(asd->match.fwnode.fwnode)) ||
>> +	       match_of(sdnode, to_of_node(async_device));
> 
> This is getting a bit complex, could you document the function ?

I've added comments, and improved helpers - I think it's looking a lot better now :)

> 
>>  }
>>
>>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
>> *asd)
> 

--
Kieran