[PATCH v2 00/12] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows

[PATCH v2 00/12] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows

[Daniel Scally]

Hello all

Previous version:
https://lore.kernel.org/linux-media/20201130133129.1024662-1-djrscally@gmail.com/T/#m91934e12e3d033da2e768e952ea3b4a125ee3e67
The RFC version before that:
https://lore.kernel.org/linux-media/20201019225903.14276-1-djrscally@gmail.com/

This series is to start adding support for webcams on laptops with ACPI tables
designed for use with CIO2 on Windows. This problem has two main parts; the
first part, which is handled in this series, is extending the ipu3-cio2
driver to allow for patching the firmware via software_nodes if endpoints
aren't defined by ACPI. The second is adding a new driver to handle power,
clocks and GPIO pins defined in DSDT tables in an awkward way. I decided to
split that second part out from this series, and instead give it its own
series (a v2 of which should land "soon"). The reasons for that are:

1. It's a logically separate change anyway
2. The recipients list was getting really long and
3. That probably meant that handling merge for all of this in one go was
   going to be impractically awkward.

Given how few comments the remaining patches of this series received in the
last posting, I'm hopeful that most or all of it could get picked up for 5.12.
We touch a few different areas:

lib (with an ack already)
  lib/test_printf.c: Use helper function to unwind array of
    software_nodes

drivers/base
  software_node: Fix refcounts in software_node_get_next_child()
  property: Return true in fwnode_device_is_available for NULL ops
  property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary
  software_node: Enforce parent before child ordering of nodes arrays
  software_node: unregister software_nodes in reverse order

drivers/acpi
  acpi: Add acpi_dev_get_next_match_dev() and helper macro

drivers/media
  media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in
    match_fwnode()
  ipu3-cio2: Add T: entry to MAINTAINERS
  ipu3-cio2: Rename ipu3-cio2.c
  ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

Given that, it feels sensible to me to try and merge them all through a single
tree; I was hoping the other maintainers would be amenable to having everything
merged through the media tree. Mauro; if that plan is ok (and of course assuming
that the rest of the patches are acked by their respective maintainers too),
could we get a dedicated feature branch just in case the following series ends
up being ready in time too? 

Series-level changelog:
	- Squashed the patches enforcing ordering in register/unregister_nodes()

More details of changes on each patch.

Comments as always very welcome - and thanks to everyone for all your help on
this so far, hope I've addressed everything from last time.

Dan

Daniel Scally (11):
  software_node: Fix refcounts in software_node_get_next_child()
  property: Return true in fwnode_device_is_available for NULL ops
  property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary
  software_node: Enforce parent before child ordering of nodes arrays
  software_node: unregister software_nodes in reverse order
  lib/test_printf.c: Use helper function to unwind array of
    software_nodes
  ipu3-cio2: Add T: entry to MAINTAINERS
  ipu3-cio2: Rename ipu3-cio2.c
  media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in
    match_fwnode()
  acpi: Add acpi_dev_get_next_match_dev() and helper macro
  ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

Heikki Krogerus (1):
  software_node: Add support for fwnode_graph*() family of functions

 MAINTAINERS                                   |   2 +
 drivers/acpi/utils.c                          |  30 +-
 drivers/base/property.c                       |  15 +-
 drivers/base/swnode.c                         | 173 +++++++++--
 drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
 drivers/media/pci/intel/ipu3/Makefile         |   3 +
 drivers/media/pci/intel/ipu3/cio2-bridge.c    | 274 ++++++++++++++++++
 drivers/media/pci/intel/ipu3/cio2-bridge.h    | 122 ++++++++
 .../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c}    |  34 +++
 drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
 drivers/media/v4l2-core/v4l2-async.c          |   8 +
 include/acpi/acpi_bus.h                       |   7 +
 lib/test_printf.c                             |   4 +-
 13 files changed, 669 insertions(+), 27 deletions(-)
 create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
 create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
 rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (98%)

-- 
2.25.1

[PATCH v2 01/12] software_node: Fix refcounts in software_node_get_next_child()

[Daniel Scally]

The software_node_get_next_child() function currently does not hold
references to the child software_node that it finds or put the ref that
is held against the old child - fix that.

Fixes: 59abd83672f7 ("drivers: base: Introducing software nodes to the firmware node framework")
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- None

 drivers/base/swnode.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 010828fc785b..615a0c93e116 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -443,14 +443,18 @@ software_node_get_next_child(const struct fwnode_handle *fwnode,
 	struct swnode *c = to_swnode(child);
 
 	if (!p || list_empty(&p->children) ||
-	    (c && list_is_last(&c->entry, &p->children)))
+	    (c && list_is_last(&c->entry, &p->children))) {
+		fwnode_handle_put(child);
 		return NULL;
+	}
 
 	if (c)
 		c = list_next_entry(c, entry);
 	else
 		c = list_first_entry(&p->children, struct swnode, entry);
-	return &c->fwnode;
+
+	fwnode_handle_put(child);
+	return fwnode_handle_get(&c->fwnode);
 }
 
 static struct fwnode_handle *
-- 
2.25.1

Re: [PATCH v2 01/12] software_node: Fix refcounts in software_node_get_next_child()

[Sakari Ailus]

Hi Daniel,

Thanks for the update.

On Thu, Dec 17, 2020 at 11:43:26PM +0000, Daniel Scally wrote:
> The software_node_get_next_child() function currently does not hold
> references to the child software_node that it finds or put the ref that
> is held against the old child - fix that.
> 
> Fixes: 59abd83672f7 ("drivers: base: Introducing software nodes to the firmware node framework")
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

[PATCH v2 02/12] property: Return true in fwnode_device_is_available for NULL ops

[Daniel Scally]

Some types of fwnode_handle do not implement the device_is_available()
check, such as those created by software_nodes. There isn't really a
meaningful way to check for the availability of a device that doesn't
actually exist, so if the check isn't implemented just assume that the
"device" is present.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- None

 drivers/base/property.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 4c43d30145c6..bc9c634df6df 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -785,9 +785,15 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
 /**
  * fwnode_device_is_available - check if a device is available for use
  * @fwnode: Pointer to the fwnode of the device.
+ *
+ * For fwnode node types that don't implement the .device_is_available()
+ * operation, this function returns true.
  */
 bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
 {
+	if (!fwnode_has_op(fwnode, device_is_available))
+		return true;
+
 	return fwnode_call_bool_op(fwnode, device_is_available);
 }
 EXPORT_SYMBOL_GPL(fwnode_device_is_available);
-- 
2.25.1

Re: [PATCH v2 02/12] property: Return true in fwnode_device_is_available for NULL ops

[Sakari Ailus]

On Thu, Dec 17, 2020 at 11:43:27PM +0000, Daniel Scally wrote:
> Some types of fwnode_handle do not implement the device_is_available()
> check, such as those created by software_nodes. There isn't really a
> meaningful way to check for the availability of a device that doesn't
> actually exist, so if the check isn't implemented just assume that the
> "device" is present.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>

For this and the 3rd patch:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

[PATCH v2 03/12] property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary

[Daniel Scally]

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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- Some rearranging of the conditionals

 drivers/base/property.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

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

[PATCH v2 04/12] software_node: Enforce parent before child ordering of nodes arrays

[Daniel Scally]

Registering software_nodes with the .parent member set to point to a
currently unregistered software_node has the potential for problems,
so enforce parent -> child ordering in arrays passed in to
software_node_register_nodes().

Software nodes that are children of another software node should be
unregistered before their parent. To allow easy unregistering of an array
of software_nodes ordered parent to child, reverse the order in which
software_node_unregister_nodes() unregisters software_nodes.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- Squashed the patches that originally touched these separately
	- Updated documentation

 drivers/base/swnode.c | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 615a0c93e116..cfd1faea48a7 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -692,7 +692,10 @@ swnode_register(const struct software_node *node, struct swnode *parent,
  * software_node_register_nodes - Register an array of software nodes
  * @nodes: Zero terminated array of software nodes to be registered
  *
- * Register multiple software nodes at once.
+ * Register multiple software nodes at once. If any node in the array
+ * has it's .parent pointer set, then it's parent **must** have been
+ * registered before it is; either outside of this function or by
+ * ordering the array such that parent comes before child.
  */
 int software_node_register_nodes(const struct software_node *nodes)
 {
@@ -700,33 +703,47 @@ int software_node_register_nodes(const struct software_node *nodes)
 	int i;
 
 	for (i = 0; nodes[i].name; i++) {
-		ret = software_node_register(&nodes[i]);
-		if (ret) {
-			software_node_unregister_nodes(nodes);
-			return ret;
+		const struct software_node *parent = nodes[i].parent;
+
+		if (parent && !software_node_to_swnode(parent)) {
+			ret = -EINVAL;
+			goto err_unregister_nodes;
 		}
+
+		ret = software_node_register(&nodes[i]);
+		if (ret)
+			goto err_unregister_nodes;
 	}
 
 	return 0;
+
+err_unregister_nodes:
+	software_node_unregister_nodes(nodes);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(software_node_register_nodes);
 
 /**
  * software_node_unregister_nodes - Unregister an array of software nodes
- * @nodes: Zero terminated array of software nodes to be unregistered
+ * @nodes: Zero terminated array of software nodes to be unregistered.
  *
- * Unregister multiple software nodes at once.
+ * Unregister multiple software nodes at once. If parent pointers are set up
+ * in any of the software nodes then the array MUST be ordered such that
+ * parents come before their children.
  *
- * NOTE: Be careful using this call if the nodes had parent pointers set up in
- * them before registering.  If so, it is wiser to remove the nodes
- * individually, in the correct order (child before parent) instead of relying
- * on the sequential order of the list of nodes in the array.
+ * NOTE: If you are uncertain whether the array is ordered such that
+ * parents will be unregistered before their children, it is wiser to
+ * remove the nodes individually, in the correct order (child before
+ * parent).
  */
 void software_node_unregister_nodes(const struct software_node *nodes)
 {
-	int i;
+	unsigned int i = 0;
+
+	while (nodes[i].name)
+		i++;
 
-	for (i = 0; nodes[i].name; i++)
+	while (i--)
 		software_node_unregister(&nodes[i]);
 }
 EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
-- 
2.25.1

Re: [PATCH v2 04/12] software_node: Enforce parent before child ordering of nodes arrays

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Thu, Dec 17, 2020 at 11:43:29PM +0000, Daniel Scally wrote:
> Registering software_nodes with the .parent member set to point to a
> currently unregistered software_node has the potential for problems,
> so enforce parent -> child ordering in arrays passed in to
> software_node_register_nodes().
> 
> Software nodes that are children of another software node should be
> unregistered before their parent. To allow easy unregistering of an array
> of software_nodes ordered parent to child, reverse the order in which
> software_node_unregister_nodes() unregisters software_nodes.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v2:
> 
> 	- Squashed the patches that originally touched these separately
> 	- Updated documentation
> 
>  drivers/base/swnode.c | 43 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 615a0c93e116..cfd1faea48a7 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -692,7 +692,10 @@ swnode_register(const struct software_node *node, struct swnode *parent,
>   * software_node_register_nodes - Register an array of software nodes
>   * @nodes: Zero terminated array of software nodes to be registered
>   *
> - * Register multiple software nodes at once.
> + * Register multiple software nodes at once. If any node in the array
> + * has it's .parent pointer set, then it's parent **must** have been
> + * registered before it is; either outside of this function or by
> + * ordering the array such that parent comes before child.
>   */
>  int software_node_register_nodes(const struct software_node *nodes)
>  {
> @@ -700,33 +703,47 @@ int software_node_register_nodes(const struct software_node *nodes)
>  	int i;
>  
>  	for (i = 0; nodes[i].name; i++) {
> -		ret = software_node_register(&nodes[i]);
> -		if (ret) {
> -			software_node_unregister_nodes(nodes);
> -			return ret;
> +		const struct software_node *parent = nodes[i].parent;
> +
> +		if (parent && !software_node_to_swnode(parent)) {
> +			ret = -EINVAL;
> +			goto err_unregister_nodes;
>  		}
> +
> +		ret = software_node_register(&nodes[i]);
> +		if (ret)
> +			goto err_unregister_nodes;
>  	}
>  
>  	return 0;
> +
> +err_unregister_nodes:
> +	software_node_unregister_nodes(nodes);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(software_node_register_nodes);
>  
>  /**
>   * software_node_unregister_nodes - Unregister an array of software nodes
> - * @nodes: Zero terminated array of software nodes to be unregistered
> + * @nodes: Zero terminated array of software nodes to be unregistered.

Not sure if this is needed.

>   *
> - * Unregister multiple software nodes at once.
> + * Unregister multiple software nodes at once. If parent pointers are set up
> + * in any of the software nodes then the array MUST be ordered such that

I'd either replace **must** above with MUST, or use **must** here. I'm
not sure if kerneldoc handles emphasis with **must**, if it does that
seems a bit nicer to me, but it's really up to you.

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

> + * parents come before their children.
>   *
> - * NOTE: Be careful using this call if the nodes had parent pointers set up in
> - * them before registering.  If so, it is wiser to remove the nodes
> - * individually, in the correct order (child before parent) instead of relying
> - * on the sequential order of the list of nodes in the array.
> + * NOTE: If you are uncertain whether the array is ordered such that
> + * parents will be unregistered before their children, it is wiser to
> + * remove the nodes individually, in the correct order (child before
> + * parent).
>   */
>  void software_node_unregister_nodes(const struct software_node *nodes)
>  {
> -	int i;
> +	unsigned int i = 0;
> +
> +	while (nodes[i].name)
> +		i++;
>  
> -	for (i = 0; nodes[i].name; i++)
> +	while (i--)
>  		software_node_unregister(&nodes[i]);
>  }
>  EXPORT_SYMBOL_GPL(software_node_unregister_nodes);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 04/12] software_node: Enforce parent before child ordering of nodes arrays

[Daniel Scally]

Hi Laurent

On 18/12/2020 16:02, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Thu, Dec 17, 2020 at 11:43:29PM +0000, Daniel Scally wrote:
>> Registering software_nodes with the .parent member set to point to a
>> currently unregistered software_node has the potential for problems,
>> so enforce parent -> child ordering in arrays passed in to
>> software_node_register_nodes().
>>
>> Software nodes that are children of another software node should be
>> unregistered before their parent. To allow easy unregistering of an array
>> of software_nodes ordered parent to child, reverse the order in which
>> software_node_unregister_nodes() unregisters software_nodes.
>>
>> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>> Changes in v2:
>>
>> 	- Squashed the patches that originally touched these separately
>> 	- Updated documentation
>>
>>  drivers/base/swnode.c | 43 ++++++++++++++++++++++++++++++-------------
>>  1 file changed, 30 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>> index 615a0c93e116..cfd1faea48a7 100644
>> --- a/drivers/base/swnode.c
>> +++ b/drivers/base/swnode.c
>> @@ -692,7 +692,10 @@ swnode_register(const struct software_node *node, struct swnode *parent,
>>   * software_node_register_nodes - Register an array of software nodes
>>   * @nodes: Zero terminated array of software nodes to be registered
>>   *
>> - * Register multiple software nodes at once.
>> + * Register multiple software nodes at once. If any node in the array
>> + * has it's .parent pointer set, then it's parent **must** have been
>> + * registered before it is; either outside of this function or by
>> + * ordering the array such that parent comes before child.
>>   */
>>  int software_node_register_nodes(const struct software_node *nodes)
>>  {
>> @@ -700,33 +703,47 @@ int software_node_register_nodes(const struct software_node *nodes)
>>  	int i;
>>  
>>  	for (i = 0; nodes[i].name; i++) {
>> -		ret = software_node_register(&nodes[i]);
>> -		if (ret) {
>> -			software_node_unregister_nodes(nodes);
>> -			return ret;
>> +		const struct software_node *parent = nodes[i].parent;
>> +
>> +		if (parent && !software_node_to_swnode(parent)) {
>> +			ret = -EINVAL;
>> +			goto err_unregister_nodes;
>>  		}
>> +
>> +		ret = software_node_register(&nodes[i]);
>> +		if (ret)
>> +			goto err_unregister_nodes;
>>  	}
>>  
>>  	return 0;
>> +
>> +err_unregister_nodes:
>> +	software_node_unregister_nodes(nodes);
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(software_node_register_nodes);
>>  
>>  /**
>>   * software_node_unregister_nodes - Unregister an array of software nodes
>> - * @nodes: Zero terminated array of software nodes to be unregistered
>> + * @nodes: Zero terminated array of software nodes to be unregistered.
> 
> Not sure if this is needed.

Hah, of course. Hangover from the last version (when I had made that
line two sentences)
> 
>>   *
>> - * Unregister multiple software nodes at once.
>> + * Unregister multiple software nodes at once. If parent pointers are set up
>> + * in any of the software nodes then the array MUST be ordered such that
> 
> I'd either replace **must** above with MUST, or use **must** here. I'm
> not sure if kerneldoc handles emphasis with **must**, if it does that
> seems a bit nicer to me, but it's really up to you.

Honestly I haven't delved into kerneldoc yet, but either way I think
**must** is better in both places - will change.

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

Thank you!
> 
>> + * parents come before their children.
>>   *
>> - * NOTE: Be careful using this call if the nodes had parent pointers set up in
>> - * them before registering.  If so, it is wiser to remove the nodes
>> - * individually, in the correct order (child before parent) instead of relying
>> - * on the sequential order of the list of nodes in the array.
>> + * NOTE: If you are uncertain whether the array is ordered such that
>> + * parents will be unregistered before their children, it is wiser to
>> + * remove the nodes individually, in the correct order (child before
>> + * parent).
>>   */
>>  void software_node_unregister_nodes(const struct software_node *nodes)
>>  {
>> -	int i;
>> +	unsigned int i = 0;
>> +
>> +	while (nodes[i].name)
>> +		i++;
>>  
>> -	for (i = 0; nodes[i].name; i++)
>> +	while (i--)
>>  		software_node_unregister(&nodes[i]);
>>  }
>>  EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
> 

Re: [PATCH v2 04/12] software_node: Enforce parent before child ordering of nodes arrays

[Andy Shevchenko]

On Thu, Dec 17, 2020 at 11:43:29PM +0000, Daniel Scally wrote:
> Registering software_nodes with the .parent member set to point to a
> currently unregistered software_node has the potential for problems,
> so enforce parent -> child ordering in arrays passed in to
> software_node_register_nodes().
> 
> Software nodes that are children of another software node should be
> unregistered before their parent. To allow easy unregistering of an array
> of software_nodes ordered parent to child, reverse the order in which
> software_node_unregister_nodes() unregisters software_nodes.

...

> + * Register multiple software nodes at once. If any node in the array
> + * has it's .parent pointer set, then it's parent **must** have been

it's => its in both cases?


> + * registered before it is; either outside of this function or by
> + * ordering the array such that parent comes before child.
>   */

...

> +		const struct software_node *parent = nodes[i].parent;
> +
> +		if (parent && !software_node_to_swnode(parent)) {

Can we have parent of swnode in an array not being an swnode?
Either comment that parent for swnode can be swnode only (Heikki, was it an
idea?) or check if parent is of swnode type and only that apply this
requirement.

> +			ret = -EINVAL;
> +			goto err_unregister_nodes;
>  		}

...

> + * Unregister multiple software nodes at once. If parent pointers are set up
> + * in any of the software nodes then the array MUST be ordered such that
> + * parents come before their children.

Shouldn't be consistent with above, i.e. **must** ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 04/12] software_node: Enforce parent before child ordering of nodes arrays

[Daniel Scally]

On 18/12/2020 20:29, Andy Shevchenko wrote:
>> + * Register multiple software nodes at once. If any node in the array
>> + * has it's .parent pointer set, then it's parent **must** have been
> 
> it's => its in both cases?

Done, ty

>> + * registered before it is; either outside of this function or by
>> + * ordering the array such that parent comes before child.
>>   */
> 
> ...
> 
>> +		const struct software_node *parent = nodes[i].parent;
>> +
>> +		if (parent && !software_node_to_swnode(parent)) {
> 
> Can we have parent of swnode in an array not being an swnode?
> Either comment that parent for swnode can be swnode only (Heikki, was it an
> idea?) or check if parent is of swnode type and only that apply this
> requirement.

.parent can be a pointer to software_node only yes; I can add that to
the document comment.

>> +			ret = -EINVAL;
>> +			goto err_unregister_nodes;
>>  		}
> 
> ...
> 
>> + * Unregister multiple software nodes at once. If parent pointers are set up
>> + * in any of the software nodes then the array MUST be ordered such that
>> + * parents come before their children.
> 
> Shouldn't be consistent with above, i.e. **must** ?

Done also

[PATCH v2 05/12] software_node: unregister software_nodes in reverse order

[Daniel Scally]

To maintain consistency with software_node_unregister_nodes(), reverse
the order in which the software_node_unregister_node_group() function
unregisters nodes.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- Initialised i properly

 drivers/base/swnode.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index cfd1faea48a7..2b90d380039b 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -778,16 +778,22 @@ EXPORT_SYMBOL_GPL(software_node_register_node_group);
  * software_node_unregister_node_group - Unregister a group of software nodes
  * @node_group: NULL terminated array of software node pointers to be unregistered
  *
- * Unregister multiple software nodes at once.
+ * Unregister multiple software nodes at once. The array will be unwound in
+ * reverse order (I.E. last entry first) and thus if any member of the array
+ * has its .parent member set then they should appear later in the array such
+ * that they are unregistered first.
  */
 void software_node_unregister_node_group(const struct software_node **node_group)
 {
-	unsigned int i;
+	unsigned int i = 0;
 
 	if (!node_group)
 		return;
 
-	for (i = 0; node_group[i]; i++)
+	while (node_group[i]->name)
+		i++;
+
+	while (i--)
 		software_node_unregister(node_group[i]);
 }
 EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
-- 
2.25.1

Re: [PATCH v2 05/12] software_node: unregister software_nodes in reverse order

[Andy Shevchenko]

On Thu, Dec 17, 2020 at 11:43:30PM +0000, Daniel Scally wrote:
> To maintain consistency with software_node_unregister_nodes(), reverse
> the order in which the software_node_unregister_node_group() function
> unregisters nodes.

...

> + * Unregister multiple software nodes at once. The array will be unwound in
> + * reverse order (I.E. last entry first) and thus if any member of the array

A nit: I.E. -> i.e.

> + * has its .parent member set then they should appear later in the array such
> + * that they are unregistered first.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 05/12] software_node: unregister software_nodes in reverse order

[Sakari Ailus]

Hi Daniel,

On Thu, Dec 17, 2020 at 11:43:30PM +0000, Daniel Scally wrote:
> To maintain consistency with software_node_unregister_nodes(), reverse
> the order in which the software_node_unregister_node_group() function
> unregisters nodes.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v2:
> 
> 	- Initialised i properly
> 
>  drivers/base/swnode.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index cfd1faea48a7..2b90d380039b 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -778,16 +778,22 @@ EXPORT_SYMBOL_GPL(software_node_register_node_group);
>   * software_node_unregister_node_group - Unregister a group of software nodes
>   * @node_group: NULL terminated array of software node pointers to be unregistered
>   *
> - * Unregister multiple software nodes at once.
> + * Unregister multiple software nodes at once. The array will be unwound in
> + * reverse order (I.E. last entry first) and thus if any member of the array
> + * has its .parent member set then they should appear later in the array such
> + * that they are unregistered first.
>   */
>  void software_node_unregister_node_group(const struct software_node **node_group)
>  {
> -	unsigned int i;
> +	unsigned int i = 0;
>  
>  	if (!node_group)
>  		return;
>  
> -	for (i = 0; node_group[i]; i++)
> +	while (node_group[i]->name)

Why is this change made? node_group is a NULL-terminated array, and the
above accesses the name pointer on each entry before checking the entry is
non-NULL. Or do I miss something here?

> +		i++;
> +
> +	while (i--)
>  		software_node_unregister(node_group[i]);
>  }
>  EXPORT_SYMBOL_GPL(software_node_unregister_node_group);

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 05/12] software_node: unregister software_nodes in reverse order

[Andy Shevchenko]

On Mon, Dec 21, 2020 at 11:21:16AM +0200, Sakari Ailus wrote:
> On Thu, Dec 17, 2020 at 11:43:30PM +0000, Daniel Scally wrote:
> > To maintain consistency with software_node_unregister_nodes(), reverse
> > the order in which the software_node_unregister_node_group() function
> > unregisters nodes.

...

> >  void software_node_unregister_node_group(const struct software_node **node_group)
> >  {
> > -	unsigned int i;
> > +	unsigned int i = 0;
> >  
> >  	if (!node_group)
> >  		return;
> >  
> > -	for (i = 0; node_group[i]; i++)
> > +	while (node_group[i]->name)
> 
> Why is this change made? node_group is a NULL-terminated array, and the
> above accesses the name pointer on each entry before checking the entry is
> non-NULL. Or do I miss something here?

I believe it's a copy'n'paste typo.

> > +		i++;
> > +
> > +	while (i--)
> >  		software_node_unregister(node_group[i]);
> >  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 05/12] software_node: unregister software_nodes in reverse order

[Daniel Scally]

On 21/12/2020 11:26, Andy Shevchenko wrote:
> On Mon, Dec 21, 2020 at 11:21:16AM +0200, Sakari Ailus wrote:
>> On Thu, Dec 17, 2020 at 11:43:30PM +0000, Daniel Scally wrote:
>>> To maintain consistency with software_node_unregister_nodes(), reverse
>>> the order in which the software_node_unregister_node_group() function
>>> unregisters nodes.
> 
> ...
> 
>>>  void software_node_unregister_node_group(const struct software_node **node_group)
>>>  {
>>> -	unsigned int i;
>>> +	unsigned int i = 0;
>>>  
>>>  	if (!node_group)
>>>  		return;
>>>  
>>> -	for (i = 0; node_group[i]; i++)
>>> +	while (node_group[i]->name)
>>
>> Why is this change made? node_group is a NULL-terminated array, and the
>> above accesses the name pointer on each entry before checking the entry is
>> non-NULL. Or do I miss something here?
> 
> I believe it's a copy'n'paste typo.

Careless copy and paste yeah, my bad. I was doing it for consistency but
really should've just changed the ordering; I'll just drop that part.

[PATCH v2 06/12] software_node: Add support for fwnode_graph*() family of functions

[Daniel Scally]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This implements the remaining .graph_* callbacks in the
fwnode operations structure for the software nodes. That makes
the fwnode_graph*() functions available in the drivers also
when software nodes are used.

The implementation tries to mimic the "OF graph" as much as
possible, but there is no support for the "reg" device
property. The ports will need to have the index in their
name which starts with "port@" (for example "port@0", "port@1",
...) and endpoints will use the index of the software node
that is given to them during creation. The port nodes can
also be grouped under a specially named "ports" subnode,
just like in DT, if necessary.

The remote-endpoints are reference properties under the
endpoint nodes that are named "remote-endpoint".

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Co-developed-by: Daniel Scally <djrscally@gmail.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- Changed commit to specify port name prefix as port@
	- Accounted for that rename in *parse_endpoint()

 drivers/base/swnode.c | 110 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 1 deletion(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 2b90d380039b..0d14d5ebe441 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -540,6 +540,110 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	return 0;
 }
 
+static struct fwnode_handle *
+swnode_graph_find_next_port(const struct fwnode_handle *parent,
+			    struct fwnode_handle *port)
+{
+	struct fwnode_handle *old = port;
+
+	while ((port = software_node_get_next_child(parent, old))) {
+		if (!strncmp(to_swnode(port)->node->name, "port", 4))
+			return port;
+		old = port;
+	}
+
+	return NULL;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
+				      struct fwnode_handle *endpoint)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	struct fwnode_handle *old = endpoint;
+	struct fwnode_handle *parent;
+	struct fwnode_handle *port;
+
+	if (!swnode)
+		return NULL;
+
+	if (endpoint) {
+		port = software_node_get_parent(endpoint);
+		parent = software_node_get_parent(port);
+	} else {
+		parent = software_node_get_named_child_node(fwnode, "ports");
+		if (!parent)
+			parent = software_node_get(&swnode->fwnode);
+
+		port = swnode_graph_find_next_port(parent, NULL);
+	}
+
+	for (; port; port = swnode_graph_find_next_port(parent, port)) {
+		endpoint = software_node_get_next_child(port, old);
+		if (endpoint) {
+			fwnode_handle_put(port);
+			break;
+		}
+
+		/* No more endpoints for that port, so stop passing old */
+		old = NULL;
+	}
+
+	fwnode_handle_put(parent);
+
+	return endpoint;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	const struct software_node_ref_args *ref;
+	const struct property_entry *prop;
+
+	if (!swnode)
+		return NULL;
+
+	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
+	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
+		return NULL;
+
+	ref = prop->pointer;
+
+	return software_node_get(software_node_fwnode(ref[0].node));
+}
+
+static struct fwnode_handle *
+software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	struct fwnode_handle *parent;
+
+	if (!strcmp(swnode->parent->node->name, "ports"))
+		parent = &swnode->parent->parent->fwnode;
+	else
+		parent = &swnode->parent->fwnode;
+
+	return software_node_get(parent);
+}
+
+static int
+software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
+				   struct fwnode_endpoint *endpoint)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	int ret;
+
+	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
+	if (ret)
+		return ret;
+
+	endpoint->id = swnode->id;
+	endpoint->local_fwnode = fwnode;
+
+	return 0;
+}
+
 static const struct fwnode_operations software_node_ops = {
 	.get = software_node_get,
 	.put = software_node_put,
@@ -551,7 +655,11 @@ static const struct fwnode_operations software_node_ops = {
 	.get_parent = software_node_get_parent,
 	.get_next_child_node = software_node_get_next_child,
 	.get_named_child_node = software_node_get_named_child_node,
-	.get_reference_args = software_node_get_reference_args
+	.get_reference_args = software_node_get_reference_args,
+	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
+	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
+	.graph_get_port_parent = software_node_graph_get_port_parent,
+	.graph_parse_endpoint = software_node_graph_parse_endpoint,
 };
 
 /* -------------------------------------------------------------------------- */
-- 
2.25.1

Re: [PATCH v2 06/12] software_node: Add support for fwnode_graph*() family of functions

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> This implements the remaining .graph_* callbacks in the
> fwnode operations structure for the software nodes. That makes
> the fwnode_graph*() functions available in the drivers also
> when software nodes are used.
> 
> The implementation tries to mimic the "OF graph" as much as
> possible, but there is no support for the "reg" device
> property. The ports will need to have the index in their
> name which starts with "port@" (for example "port@0", "port@1",
> ...) and endpoints will use the index of the software node
> that is given to them during creation. The port nodes can
> also be grouped under a specially named "ports" subnode,
> just like in DT, if necessary.
> 
> The remote-endpoints are reference properties under the
> endpoint nodes that are named "remote-endpoint".
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Co-developed-by: Daniel Scally <djrscally@gmail.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v2:
> 
> 	- Changed commit to specify port name prefix as port@
> 	- Accounted for that rename in *parse_endpoint()
> 
>  drivers/base/swnode.c | 110 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 2b90d380039b..0d14d5ebe441 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -540,6 +540,110 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  	return 0;
>  }
>  
> +static struct fwnode_handle *
> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
> +			    struct fwnode_handle *port)
> +{
> +	struct fwnode_handle *old = port;
> +
> +	while ((port = software_node_get_next_child(parent, old))) {
> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))

Maybe we'll need to limit this to matching on "port" or "port@[0-9]+" to
avoid false positives, but that can be done later, if needed.

> +			return port;
> +		old = port;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
> +				      struct fwnode_handle *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *old = endpoint;
> +	struct fwnode_handle *parent;
> +	struct fwnode_handle *port;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	if (endpoint) {
> +		port = software_node_get_parent(endpoint);

Here the reference count to port is incremented.

> +		parent = software_node_get_parent(port);
> +	} else {
> +		parent = software_node_get_named_child_node(fwnode, "ports");
> +		if (!parent)
> +			parent = software_node_get(&swnode->fwnode);
> +
> +		port = swnode_graph_find_next_port(parent, NULL);

But here it isn't, software_node_get_next_child() doesn't deal with
reference counts.

> +	}
> +
> +	for (; port; port = swnode_graph_find_next_port(parent, port)) {

So if the loop terminates normally, the reference acquired in the first
branch of the if will be leaked.

> +		endpoint = software_node_get_next_child(port, old);
> +		if (endpoint) {
> +			fwnode_handle_put(port);

While in this case the reference not acquired in the second branch of
the if will be released incorrectly.

I think it's software_node_get_next_child() that needs to be fixed if
I'm not mistaken.

> +			break;
> +		}
> +
> +		/* No more endpoints for that port, so stop passing old */
> +		old = NULL;

I wonder if you could drop the 'old' variable and use 'enpoint' in the
call to software_node_get_next_child(). You could then drop these two
lines.

> +	}
> +
> +	fwnode_handle_put(parent);
> +
> +	return endpoint;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	const struct software_node_ref_args *ref;
> +	const struct property_entry *prop;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
> +	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
> +		return NULL;
> +
> +	ref = prop->pointer;
> +
> +	return software_node_get(software_node_fwnode(ref[0].node));
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *parent;
> +
> +	if (!strcmp(swnode->parent->node->name, "ports"))
> +		parent = &swnode->parent->parent->fwnode;
> +	else
> +		parent = &swnode->parent->fwnode;
> +
> +	return software_node_get(parent);
> +}
> +
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> +				   struct fwnode_endpoint *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	int ret;
> +
> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
> +	if (ret)
> +		return ret;
> +
> +	endpoint->id = swnode->id;
> +	endpoint->local_fwnode = fwnode;
> +
> +	return 0;
> +}
> +
>  static const struct fwnode_operations software_node_ops = {
>  	.get = software_node_get,
>  	.put = software_node_put,
> @@ -551,7 +655,11 @@ static const struct fwnode_operations software_node_ops = {
>  	.get_parent = software_node_get_parent,
>  	.get_next_child_node = software_node_get_next_child,
>  	.get_named_child_node = software_node_get_named_child_node,
> -	.get_reference_args = software_node_get_reference_args
> +	.get_reference_args = software_node_get_reference_args,
> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
> +	.graph_get_port_parent = software_node_graph_get_port_parent,
> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>  };
>  
>  /* -------------------------------------------------------------------------- */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 06/12] software_node: Add support for fwnode_graph*() family of functions

[Daniel Scally]

Hi Laurent - thanks for comments as always

On 18/12/2020 16:22, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>> This implements the remaining .graph_* callbacks in the
>> fwnode operations structure for the software nodes. That makes
>> the fwnode_graph*() functions available in the drivers also
>> when software nodes are used.
>>
>> The implementation tries to mimic the "OF graph" as much as
>> possible, but there is no support for the "reg" device
>> property. The ports will need to have the index in their
>> name which starts with "port@" (for example "port@0", "port@1",
>> ...) and endpoints will use the index of the software node
>> that is given to them during creation. The port nodes can
>> also be grouped under a specially named "ports" subnode,
>> just like in DT, if necessary.
>>
>> The remote-endpoints are reference properties under the
>> endpoint nodes that are named "remote-endpoint".
>>
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Co-developed-by: Daniel Scally <djrscally@gmail.com>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>> Changes in v2:
>>
>> 	- Changed commit to specify port name prefix as port@
>> 	- Accounted for that rename in *parse_endpoint()
>>
>>  drivers/base/swnode.c | 110 +++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 109 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>> index 2b90d380039b..0d14d5ebe441 100644
>> --- a/drivers/base/swnode.c
>> +++ b/drivers/base/swnode.c
>> @@ -540,6 +540,110 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>>  	return 0;
>>  }
>>  
>> +static struct fwnode_handle *
>> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
>> +			    struct fwnode_handle *port)
>> +{
>> +	struct fwnode_handle *old = port;
>> +
>> +	while ((port = software_node_get_next_child(parent, old))) {
>> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))
> 
> Maybe we'll need to limit this to matching on "port" or "port@[0-9]+" to
> avoid false positives, but that can be done later, if needed.

Hmm yeah I guess that's a danger - ok, I'll stick it on the list.


>> +			return port;
>> +		old = port;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
>> +				      struct fwnode_handle *endpoint)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	struct fwnode_handle *old = endpoint;
>> +	struct fwnode_handle *parent;
>> +	struct fwnode_handle *port;
>> +
>> +	if (!swnode)
>> +		return NULL;
>> +
>> +	if (endpoint) {
>> +		port = software_node_get_parent(endpoint);
> 
> Here the reference count to port is incremented.
> 
>> +		parent = software_node_get_parent(port);
>> +	} else {
>> +		parent = software_node_get_named_child_node(fwnode, "ports");
>> +		if (!parent)
>> +			parent = software_node_get(&swnode->fwnode);
>> +
>> +		port = swnode_graph_find_next_port(parent, NULL);
> 
> But here it isn't, software_node_get_next_child() doesn't deal with
> reference counts.

Not as in the kernel right now, but after patch one of this series, it does:

[PATCH v2 01/12] software_node: Fix refcounts in
software_node_get_next_child()

I'm not sure that one linked to the thread correctly, but it's here if
you haven't seen it:

https://lore.kernel.org/linux-media/20201217234337.1983732-2-djrscally@gmail.com/T/#u

The tl;dr of the change is that it will now get() the next node (if
found) and **always** put() if one is passed.


>> +	}
>> +
>> +	for (; port; port = swnode_graph_find_next_port(parent, port)) {
> 
> So if the loop terminates normally, the reference acquired in the first
> branch of the if will be leaked.
> 
>> +		endpoint = software_node_get_next_child(port, old);
>> +		if (endpoint) {
>> +			fwnode_handle_put(port);
> 
> While in this case the reference not acquired in the second branch of
> the if will be released incorrectly.
> 
> I think it's software_node_get_next_child() that needs to be fixed if
> I'm not mistaken.

I think that's all handled in software_node_get_next_child() as amended
by 01/12. The net effect of get_next_endpoint() should be one refcount
increased for any endpoint returned, and 0 change to parent and any ports.


>> +			break;
>> +		}
>> +
>> +		/* No more endpoints for that port, so stop passing old */
>> +		old = NULL;
> 
> I wonder if you could drop the 'old' variable and use 'enpoint' in the
> call to software_node_get_next_child(). You could then drop these two
> lines.

That won't work, because endpoint would at that point not be a child of
the port we're passing, and the function relies on it being one:

	if (!p || list_empty(&p->children) ||
	    (c && list_is_last(&c->entry, &p->children))) {
		fwnode_handle_put(child);
		return NULL;
	}

>> +	}
>> +
>> +	fwnode_handle_put(parent);
>> +
>> +	return endpoint;
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	const struct software_node_ref_args *ref;
>> +	const struct property_entry *prop;
>> +
>> +	if (!swnode)
>> +		return NULL;
>> +
>> +	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
>> +	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
>> +		return NULL;
>> +
>> +	ref = prop->pointer;
>> +
>> +	return software_node_get(software_node_fwnode(ref[0].node));
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	struct fwnode_handle *parent;
>> +
>> +	if (!strcmp(swnode->parent->node->name, "ports"))
>> +		parent = &swnode->parent->parent->fwnode;
>> +	else
>> +		parent = &swnode->parent->fwnode;
>> +
>> +	return software_node_get(parent);
>> +}
>> +
>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> +				   struct fwnode_endpoint *endpoint)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	int ret;
>> +
>> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
>> +	if (ret)
>> +		return ret;
>> +
>> +	endpoint->id = swnode->id;
>> +	endpoint->local_fwnode = fwnode;
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct fwnode_operations software_node_ops = {
>>  	.get = software_node_get,
>>  	.put = software_node_put,
>> @@ -551,7 +655,11 @@ static const struct fwnode_operations software_node_ops = {
>>  	.get_parent = software_node_get_parent,
>>  	.get_next_child_node = software_node_get_next_child,
>>  	.get_named_child_node = software_node_get_named_child_node,
>> -	.get_reference_args = software_node_get_reference_args
>> +	.get_reference_args = software_node_get_reference_args,
>> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
>> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
>> +	.graph_get_port_parent = software_node_graph_get_port_parent,
>> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>>  };
>>  
>>  /* -------------------------------------------------------------------------- */
> 

Re: [PATCH v2 06/12] software_node: Add support for fwnode_graph*() family of functions

[Daniel Scally]

On 18/12/2020 22:13, Daniel Scally wrote:

>>> +			break;
>>> +		}
>>> +
>>> +		/* No more endpoints for that port, so stop passing old */
>>> +		old = NULL;
>>
>> I wonder if you could drop the 'old' variable and use 'enpoint' in the
>> call to software_node_get_next_child(). You could then drop these two
>> lines.
> 
> That won't work, because endpoint would at that point not be a child of
> the port we're passing, and the function relies on it being one:
> 
> 	if (!p || list_empty(&p->children) ||
> 	    (c && list_is_last(&c->entry, &p->children))) {
> 		fwnode_handle_put(child);
> 		return NULL;
> 	}
> 

Wait, that's nonsense of course, because endpoint gets set to NULL when
software_node_get_next_child() finds nothing - I'll double check but
pretty sure you're right.

Re: [PATCH v2 06/12] software_node: Add support for fwnode_graph*() family of functions

[Andy Shevchenko]

On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> This implements the remaining .graph_* callbacks in the
> fwnode operations structure for the software nodes. That makes
> the fwnode_graph*() functions available in the drivers also
> when software nodes are used.
> 
> The implementation tries to mimic the "OF graph" as much as
> possible, but there is no support for the "reg" device
> property. The ports will need to have the index in their
> name which starts with "port@" (for example "port@0", "port@1",
> ...) and endpoints will use the index of the software node
> that is given to them during creation. The port nodes can
> also be grouped under a specially named "ports" subnode,
> just like in DT, if necessary.
> 
> The remote-endpoints are reference properties under the
> endpoint nodes that are named "remote-endpoint".

...

> +	while ((port = software_node_get_next_child(parent, old))) {
> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))
> +			return port;
> +		old = port;
> +	}

Dunno if we need defines for port and its length here.

...

> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);

But here at least comment is needed what 5 means ('port@' I suppose).

> +	if (ret)
> +		return ret;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 06/12] software_node: Add support for fwnode_graph*() family of functions

[Daniel Scally]

On 18/12/2020 20:37, Andy Shevchenko wrote:
> On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>> This implements the remaining .graph_* callbacks in the
>> fwnode operations structure for the software nodes. That makes
>> the fwnode_graph*() functions available in the drivers also
>> when software nodes are used.
>>
>> The implementation tries to mimic the "OF graph" as much as
>> possible, but there is no support for the "reg" device
>> property. The ports will need to have the index in their
>> name which starts with "port@" (for example "port@0", "port@1",
>> ...) and endpoints will use the index of the software node
>> that is given to them during creation. The port nodes can
>> also be grouped under a specially named "ports" subnode,
>> just like in DT, if necessary.
>>
>> The remote-endpoints are reference properties under the
>> endpoint nodes that are named "remote-endpoint".
> 
> ...
> 
>> +	while ((port = software_node_get_next_child(parent, old))) {
>> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))
>> +			return port;
>> +		old = port;
>> +	}
> 
> Dunno if we need defines for port and its length here.

Mmm, maybe a comment?

> ...
> 
>> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
> 
> But here at least comment is needed what 5 means ('port@' I suppose).

Ack - I'll add an explanatory comment (and yep, it's 'port@')

>> +	if (ret)
>> +		return ret;
> 
> 

Re: [PATCH v2 06/12] software_node: Add support for fwnode_graph*() family of functions

[Sakari Ailus]

Hi Daniel and Heikki,

On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> This implements the remaining .graph_* callbacks in the
> fwnode operations structure for the software nodes. That makes
> the fwnode_graph*() functions available in the drivers also
> when software nodes are used.
> 
> The implementation tries to mimic the "OF graph" as much as
> possible, but there is no support for the "reg" device
> property. The ports will need to have the index in their
> name which starts with "port@" (for example "port@0", "port@1",
> ...) and endpoints will use the index of the software node
> that is given to them during creation. The port nodes can
> also be grouped under a specially named "ports" subnode,
> just like in DT, if necessary.
> 
> The remote-endpoints are reference properties under the
> endpoint nodes that are named "remote-endpoint".
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Co-developed-by: Daniel Scally <djrscally@gmail.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v2:
> 
> 	- Changed commit to specify port name prefix as port@
> 	- Accounted for that rename in *parse_endpoint()
> 
>  drivers/base/swnode.c | 110 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 2b90d380039b..0d14d5ebe441 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -540,6 +540,110 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  	return 0;
>  }
>  
> +static struct fwnode_handle *
> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
> +			    struct fwnode_handle *port)
> +{
> +	struct fwnode_handle *old = port;
> +
> +	while ((port = software_node_get_next_child(parent, old))) {
> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))
> +			return port;
> +		old = port;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
> +				      struct fwnode_handle *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *old = endpoint;
> +	struct fwnode_handle *parent;
> +	struct fwnode_handle *port;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	if (endpoint) {
> +		port = software_node_get_parent(endpoint);
> +		parent = software_node_get_parent(port);
> +	} else {
> +		parent = software_node_get_named_child_node(fwnode, "ports");
> +		if (!parent)
> +			parent = software_node_get(&swnode->fwnode);
> +
> +		port = swnode_graph_find_next_port(parent, NULL);
> +	}
> +
> +	for (; port; port = swnode_graph_find_next_port(parent, port)) {
> +		endpoint = software_node_get_next_child(port, old);
> +		if (endpoint) {
> +			fwnode_handle_put(port);
> +			break;
> +		}
> +
> +		/* No more endpoints for that port, so stop passing old */
> +		old = NULL;
> +	}
> +
> +	fwnode_handle_put(parent);
> +
> +	return endpoint;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	const struct software_node_ref_args *ref;
> +	const struct property_entry *prop;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
> +	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
> +		return NULL;
> +
> +	ref = prop->pointer;
> +
> +	return software_node_get(software_node_fwnode(ref[0].node));
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *parent;
> +
> +	if (!strcmp(swnode->parent->node->name, "ports"))
> +		parent = &swnode->parent->parent->fwnode;
> +	else
> +		parent = &swnode->parent->fwnode;

If you happen to call this function on a non-port node for whatever reason,
you may end up accessing a pointer that's NULL, can't you? Instead I'd do
something like:

swnode = swnode->parent;
if (swnode && !strcmp(swnode->node->name, "ports"))
	swnode = swnode->parent;

return swnode ? software_node_get(&swnode->fwnode) : NULL;

You can also drop parent as a by-product of this.

> +
> +	return software_node_get(parent);
> +}
> +
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> +				   struct fwnode_endpoint *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	int ret;
> +
> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
> +	if (ret)
> +		return ret;
> +
> +	endpoint->id = swnode->id;
> +	endpoint->local_fwnode = fwnode;
> +
> +	return 0;
> +}
> +
>  static const struct fwnode_operations software_node_ops = {
>  	.get = software_node_get,
>  	.put = software_node_put,
> @@ -551,7 +655,11 @@ static const struct fwnode_operations software_node_ops = {
>  	.get_parent = software_node_get_parent,
>  	.get_next_child_node = software_node_get_next_child,
>  	.get_named_child_node = software_node_get_named_child_node,
> -	.get_reference_args = software_node_get_reference_args
> +	.get_reference_args = software_node_get_reference_args,
> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
> +	.graph_get_port_parent = software_node_graph_get_port_parent,
> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>  };
>  
>  /* -------------------------------------------------------------------------- */

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 06/12] software_node: Add support for fwnode_graph*() family of functions

[Daniel Scally]

Hi Sakari - thanks for the reviews in previous emails

On 21/12/2020 09:34, Sakari Ailus wrote:
> Hi Daniel and Heikki,
>
> On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
>
> +static struct fwnode_handle *
> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *parent;
> +
> +	if (!strcmp(swnode->parent->node->name, "ports"))
> +		parent = &swnode->parent->parent->fwnode;
> +	else
> +		parent = &swnode->parent->fwnode;
> If you happen to call this function on a non-port node for whatever reason,
> you may end up accessing a pointer that's NULL, can't you?

Yes, actually.

> Instead I'd do
> something like:
>
> swnode = swnode->parent;
> if (swnode && !strcmp(swnode->node->name, "ports"))
> 	swnode = swnode->parent;
>
> return swnode ? software_node_get(&swnode->fwnode) : NULL;
>
> You can also drop parent as a by-product of this.
Yes ok, that looks good to me - thanks.
>> +
>> +	return software_node_get(parent);
>> +}
>> +
>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> +				   struct fwnode_endpoint *endpoint)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	int ret;
>> +
>> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
>> +	if (ret)
>> +		return ret;
>> +
>> +	endpoint->id = swnode->id;
>> +	endpoint->local_fwnode = fwnode;
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct fwnode_operations software_node_ops = {
>>  	.get = software_node_get,
>>  	.put = software_node_put,
>> @@ -551,7 +655,11 @@ static const struct fwnode_operations software_node_ops = {
>>  	.get_parent = software_node_get_parent,
>>  	.get_next_child_node = software_node_get_next_child,
>>  	.get_named_child_node = software_node_get_named_child_node,
>> -	.get_reference_args = software_node_get_reference_args
>> +	.get_reference_args = software_node_get_reference_args,
>> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
>> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
>> +	.graph_get_port_parent = software_node_graph_get_port_parent,
>> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>>  };
>>  
>>  /* -------------------------------------------------------------------------- */

[PATCH v2 07/12] lib/test_printf.c: Use helper function to unwind array of software_nodes

[Daniel Scally]

Use the software_node_unregister_nodes() helper function to unwind this
array in a cleaner way.

Acked-by: Petr Mladek <pmladek@suse.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- None

 lib/test_printf.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7ac87f18a10f..7d60f24240a4 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -644,9 +644,7 @@ static void __init fwnode_pointer(void)
 	test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
 	test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
 
-	software_node_unregister(&softnodes[2]);
-	software_node_unregister(&softnodes[1]);
-	software_node_unregister(&softnodes[0]);
+	software_node_unregister_nodes(softnodes);
 }
 
 static void __init
-- 
2.25.1

[PATCH v2 08/12] ipu3-cio2: Add T: entry to MAINTAINERS

[Daniel Scally]

Development for the ipu3-cio2 driver is taking place in media_tree, but
there's no T: entry in MAINTAINERS to denote that - rectify that oversight

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- None

 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 80881fb36404..16b544624577 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8946,6 +8946,7 @@ M:	Bingbu Cao <bingbu.cao@intel.com>
 R:	Tianshu Qiu <tian.shu.qiu@intel.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
+T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/userspace-api/media/v4l/pixfmt-srggb10-ipu3.rst
 F:	drivers/media/pci/intel/ipu3/
 
-- 
2.25.1

[PATCH v2 09/12] ipu3-cio2: Rename ipu3-cio2.c

[Daniel Scally]

ipu3-cio2 driver needs extending with multiple files; rename the main
source file and specify the renamed file in Makefile to accommodate that.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- None

 drivers/media/pci/intel/ipu3/Makefile                          | 2 ++
 drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 0
 2 files changed, 2 insertions(+)
 rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (100%)

diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
index 98ddd5beafe0..429d516452e4 100644
--- a/drivers/media/pci/intel/ipu3/Makefile
+++ b/drivers/media/pci/intel/ipu3/Makefile
@@ -1,2 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
+
+ipu3-cio2-y += ipu3-cio2-main.o
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
similarity index 100%
rename from drivers/media/pci/intel/ipu3/ipu3-cio2.c
rename to drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
-- 
2.25.1

[PATCH v2 10/12] media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in match_fwnode()

[Daniel Scally]

Where the fwnode graph is comprised of software_nodes, these will be
assigned as the secondary to dev->fwnode. Check the v4l2_subdev's fwnode
for a secondary and attempt to match against it during match_fwnode() to
accommodate that possibility.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- None

 drivers/media/v4l2-core/v4l2-async.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index e3ab003a6c85..9dd896d085ec 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -87,6 +87,14 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
 	if (sd->fwnode == asd->match.fwnode)
 		return true;
 
+	/*
+	 * Check the same situation for any possible secondary assigned to the
+	 * subdev's fwnode
+	 */
+	if (!IS_ERR_OR_NULL(sd->fwnode->secondary) &&
+	    sd->fwnode->secondary == asd->match.fwnode)
+		return true;
+
 	/*
 	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
 	 * endpoint or a device. If they're of the same type, there's no match.
-- 
2.25.1

[PATCH v2 11/12] acpi: Add acpi_dev_get_next_match_dev() and helper macro

[Daniel Scally]

To ensure we handle situations in which multiple sensors of the same
model (and therefore _HID) are present in a system, we need to be able
to iterate over devices matching a known _HID but unknown _UID and _HRV
 - add acpi_dev_get_next_match_dev() to accommodate that possibility and
change acpi_dev_get_first_match_dev() to simply call the new function
with a NULL starting point. Add an iterator macro for convenience.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- None

 drivers/acpi/utils.c    | 30 ++++++++++++++++++++++++++----
 include/acpi/acpi_bus.h |  7 +++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index d5411a166685..c177165c8db2 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -843,12 +843,13 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
 EXPORT_SYMBOL(acpi_dev_present);
 
 /**
- * acpi_dev_get_first_match_dev - Return the first match of ACPI device
+ * acpi_dev_get_next_match_dev - Return the next match of ACPI device
+ * @adev: Pointer to the previous acpi_device matching this hid, uid and hrv
  * @hid: Hardware ID of the device.
  * @uid: Unique ID of the device, pass NULL to not check _UID
  * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
  *
- * Return the first match of ACPI device if a matching device was present
+ * Return the next match of ACPI device if another matching device was present
  * at the moment of invocation, or NULL otherwise.
  *
  * The caller is responsible to call put_device() on the returned device.
@@ -856,8 +857,9 @@ EXPORT_SYMBOL(acpi_dev_present);
  * See additional information in acpi_dev_present() as well.
  */
 struct acpi_device *
-acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
+acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv)
 {
+	struct device *start = adev ? &adev->dev : NULL;
 	struct acpi_dev_match_info match = {};
 	struct device *dev;
 
@@ -865,9 +867,29 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
 	match.uid = uid;
 	match.hrv = hrv;
 
-	dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
+	dev = bus_find_device(&acpi_bus_type, start, &match, acpi_dev_match_cb);
 	return dev ? to_acpi_device(dev) : NULL;
 }
+EXPORT_SYMBOL(acpi_dev_get_next_match_dev);
+
+/**
+ * acpi_dev_get_first_match_dev - Return the first match of ACPI device
+ * @hid: Hardware ID of the device.
+ * @uid: Unique ID of the device, pass NULL to not check _UID
+ * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
+ *
+ * Return the first match of ACPI device if a matching device was present
+ * at the moment of invocation, or NULL otherwise.
+ *
+ * The caller is responsible to call put_device() on the returned device.
+ *
+ * See additional information in acpi_dev_present() as well.
+ */
+struct acpi_device *
+acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
+{
+	return acpi_dev_get_next_match_dev(NULL, hid, uid, hrv);
+}
 EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
 
 /*
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a3abcc4b7d9f..0a028ba967d3 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -688,9 +688,16 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
 
 bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
 
+struct acpi_device *
+acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
 struct acpi_device *
 acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv);
 
+#define for_each_acpi_dev_match(adev, hid, uid, hrv)			\
+	for (adev = acpi_dev_get_first_match_dev(hid, uid, hrv);	\
+	     adev;							\
+	     adev = acpi_dev_get_next_match_dev(adev, hid, uid, hrv))
+
 static inline void acpi_dev_put(struct acpi_device *adev)
 {
 	put_device(&adev->dev);
-- 
2.25.1

Re: [PATCH v2 11/12] acpi: Add acpi_dev_get_next_match_dev() and helper macro

[Andy Shevchenko]

On Thu, Dec 17, 2020 at 11:43:36PM +0000, Daniel Scally wrote:
> To ensure we handle situations in which multiple sensors of the same
> model (and therefore _HID) are present in a system, we need to be able
> to iterate over devices matching a known _HID but unknown _UID and _HRV
>  - add acpi_dev_get_next_match_dev() to accommodate that possibility and
> change acpi_dev_get_first_match_dev() to simply call the new function
> with a NULL starting point. Add an iterator macro for convenience.

...

> - * acpi_dev_get_first_match_dev - Return the first match of ACPI device
> + * acpi_dev_get_next_match_dev - Return the next match of ACPI device
> + * @adev: Pointer to the previous acpi_device matching this hid, uid and hrv

A nit: @hid, @uid and @hrv

>   * @hid: Hardware ID of the device.
>   * @uid: Unique ID of the device, pass NULL to not check _UID
>   * @hrv: Hardware Revision of the device, pass -1 to not check _HRV

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 11/12] acpi: Add acpi_dev_get_next_match_dev() and helper macro

[Sakari Ailus]

Hi Daniel,

On Thu, Dec 17, 2020 at 11:43:36PM +0000, Daniel Scally wrote:
> To ensure we handle situations in which multiple sensors of the same
> model (and therefore _HID) are present in a system, we need to be able
> to iterate over devices matching a known _HID but unknown _UID and _HRV
>  - add acpi_dev_get_next_match_dev() to accommodate that possibility and
> change acpi_dev_get_first_match_dev() to simply call the new function
> with a NULL starting point. Add an iterator macro for convenience.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

[PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

Currently on platforms designed for Windows, connections between CIO2 and
sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
driver to compensate by building software_node connections, parsing the
connection properties from the sensor's SSDB buffer.

Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- Dropped some headers
	- Added support for specifying link-frequencies in the array of
	cio2_supported_sensors and added that property to the endpoint.
	- Replaced strcpy with strscpy (Laurent, I liked your change better
	stylistically but ofc the string literals are lost when the module
	is reloaded)
	- Named the ports/endpoints "port@%u"
	- Added an overflow check to cio2_bridge_connect_sensors()
	- A bunch of cosmetic changes

For the cio2_supported_sensors array, specify link frequencies in this
manner: 

	CIO2_SENSOR_CONFIG("OVTI5648", 2, 16800000, 2100000)

 MAINTAINERS                                   |   1 +
 drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
 drivers/media/pci/intel/ipu3/Makefile         |   1 +
 drivers/media/pci/intel/ipu3/cio2-bridge.c    | 274 ++++++++++++++++++
 drivers/media/pci/intel/ipu3/cio2-bridge.h    | 122 ++++++++
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  34 +++
 drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
 7 files changed, 456 insertions(+)
 create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
 create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 16b544624577..e7784b4bc8ea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8943,6 +8943,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
 M:	Yong Zhi <yong.zhi@intel.com>
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
 M:	Bingbu Cao <bingbu.cao@intel.com>
+M:	Dan Scally <djrscally@gmail.com>
 R:	Tianshu Qiu <tian.shu.qiu@intel.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
index 82d7f17e6a02..dcf5c4b74673 100644
--- a/drivers/media/pci/intel/ipu3/Kconfig
+++ b/drivers/media/pci/intel/ipu3/Kconfig
@@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
 	  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
 	  connected camera.
 	  The module will be called ipu3-cio2.
+
+config CIO2_BRIDGE
+	bool "IPU3 CIO2 Sensors Bridge"
+	depends on VIDEO_IPU3_CIO2
+	help
+	  This extension provides an API for the ipu3-cio2 driver to create
+	  connections to cameras that are hidden in SSDB buffer in ACPI. It
+	  can be used to enable support for cameras in detachable / hybrid
+	  devices that ship with Windows.
+
+	  Say Y here if your device is a detachable / hybrid laptop that comes
+	  with Windows installed by the OEM, for example:
+
+		- Microsoft Surface models (except Surface Pro 3)
+		- The Lenovo Miix line (for example the 510, 520, 710 and 720)
+		- Dell 7285
+
+	  If in doubt, say N here.
diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
index 429d516452e4..933777e6ea8a 100644
--- a/drivers/media/pci/intel/ipu3/Makefile
+++ b/drivers/media/pci/intel/ipu3/Makefile
@@ -2,3 +2,4 @@
 obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
 
 ipu3-cio2-y += ipu3-cio2-main.o
+ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
new file mode 100644
index 000000000000..3f0e2d7eab20
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -0,0 +1,274 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Dan Scally <djrscally@gmail.com> */
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/pci.h>
+#include <linux/property.h>
+
+#include "cio2-bridge.h"
+
+/*
+ * Extend this array with ACPI Hardware ID's of devices known to be working
+ * plus the number of link-frequencies expected by their drivers, along with
+ * the frequency values in hertz. This is somewhat opportunistic way of adding
+ * support for this for now in the hopes of a better source for the information
+ * (possibly some encoded value in the SSDB buffer that we're unaware of)
+ * becoming apparent in the future.
+ *
+ * Do not add an entry for a sensor that is not actually supported.
+ */
+static const struct cio2_sensor_config cio2_supported_sensors[] = {
+	CIO2_SENSOR_CONFIG("INT33BE", 0),
+	CIO2_SENSOR_CONFIG("OVTI2680", 0),
+};
+
+static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
+					void *data, u32 size)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	acpi_status status;
+	int ret;
+
+	status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	obj = buffer.pointer;
+	if (!obj) {
+		dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
+		return -ENODEV;
+	}
+
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		dev_err(&adev->dev, "Not an ACPI buffer\n");
+		ret = -ENODEV;
+		goto out_free_buff;
+	}
+
+	if (obj->buffer.length > size) {
+		dev_err(&adev->dev, "Given buffer is too small\n");
+		ret = -EINVAL;
+		goto out_free_buff;
+	}
+
+	memcpy(data, obj->buffer.pointer, obj->buffer.length);
+	ret = obj->buffer.length;
+
+out_free_buff:
+	kfree(buffer.pointer);
+	return ret;
+}
+
+static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
+{
+	strscpy(sensor->prop_names.clock_frequency, "clock-frequency",
+		sizeof(sensor->prop_names.clock_frequency));
+	strscpy(sensor->prop_names.rotation, "rotation",
+		sizeof(sensor->prop_names.rotation));
+	strscpy(sensor->prop_names.bus_type, "bus-type",
+		sizeof(sensor->prop_names.bus_type));
+	strscpy(sensor->prop_names.data_lanes, "data-lanes",
+		sizeof(sensor->prop_names.data_lanes));
+	strscpy(sensor->prop_names.remote_endpoint, "remote-endpoint",
+		sizeof(sensor->prop_names.remote_endpoint));
+	strscpy(sensor->prop_names.link_frequencies, "link-frequencies",
+		sizeof(sensor->prop_names.link_frequencies));
+}
+
+static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor,
+						 const struct cio2_sensor_config *cfg)
+{
+	unsigned int i;
+
+	cio2_bridge_init_property_names(sensor);
+
+	for (i = 0; i < 4; i++)
+		sensor->data_lanes[i] = i + 1;
+
+	sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];
+	sensor->remote_ref[0].node = &sensor->swnodes[SWNODE_SENSOR_ENDPOINT];
+
+	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
+						       sensor->ssdb.mclkspeed);
+	sensor->dev_properties[1] = PROPERTY_ENTRY_U8(sensor->prop_names.rotation,
+						      sensor->ssdb.degree);
+
+	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 4);
+	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
+								sensor->data_lanes,
+								sensor->ssdb.lanes);
+	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
+							    sensor->local_ref);
+
+	if (cfg->nr_link_freqs > 0)
+		sensor->ep_properties[3] = PROPERTY_ENTRY_U64_ARRAY_LEN(
+						sensor->prop_names.link_frequencies,
+						cfg->link_freqs,
+						cfg->nr_link_freqs);
+
+	sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
+								  sensor->data_lanes,
+								  sensor->ssdb.lanes);
+	sensor->cio2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
+							      sensor->remote_ref);
+}
+
+static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
+{
+	snprintf(sensor->node_names.remote_port, 7, "port@%u", sensor->ssdb.link);
+	strscpy(sensor->node_names.port, "port@0", sizeof(sensor->node_names.port));
+	strscpy(sensor->node_names.endpoint, "endpoint@0", sizeof(sensor->node_names.endpoint));
+}
+
+static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
+						  struct cio2_sensor *sensor)
+{
+	struct software_node *nodes = sensor->swnodes;
+
+	cio2_bridge_init_swnode_names(sensor);
+
+	nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name,
+					       sensor->dev_properties);
+	nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
+					      &nodes[SWNODE_SENSOR_HID]);
+	nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
+						      &nodes[SWNODE_SENSOR_PORT],
+						      sensor->ep_properties);
+	nodes[SWNODE_CIO2_PORT] = NODE_PORT(sensor->node_names.remote_port,
+					    &bridge->cio2_hid_node);
+	nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
+						    &nodes[SWNODE_CIO2_PORT],
+						    sensor->cio2_properties);
+}
+
+static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
+{
+	struct cio2_sensor *sensor;
+	unsigned int i;
+
+	for (i = 0; i < bridge->n_sensors; i++) {
+		sensor = &bridge->sensors[i];
+		software_node_unregister_nodes(sensor->swnodes);
+		acpi_dev_put(sensor->adev);
+	}
+}
+
+static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
+				       struct pci_dev *cio2)
+{
+	struct fwnode_handle *fwnode;
+	struct cio2_sensor *sensor;
+	struct acpi_device *adev;
+	unsigned int i;
+	int ret = 0;
+
+	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
+		const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
+
+		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
+			if (bridge->n_sensors >= CIO2_NUM_PORTS) {
+				dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");
+				/* overflow i so outer loop ceases */
+				i = ARRAY_SIZE(cio2_supported_sensors);
+				break;
+			}
+
+			if (!adev->status.enabled)
+				continue;
+
+			sensor = &bridge->sensors[bridge->n_sensors];
+			sensor->adev = adev;
+			strscpy(sensor->name, cfg->hid, sizeof(sensor->name));
+
+			ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
+							   &sensor->ssdb,
+							   sizeof(sensor->ssdb));
+			if (ret < 0)
+				goto err_put_adev;
+
+			if (sensor->ssdb.lanes > 4) {
+				dev_err(&adev->dev,
+					"Number of lanes in SSDB is invalid\n");
+				goto err_put_adev;
+			}
+
+			cio2_bridge_create_fwnode_properties(sensor, cfg);
+			cio2_bridge_create_connection_swnodes(bridge, sensor);
+
+			ret = software_node_register_nodes(sensor->swnodes);
+			if (ret)
+				goto err_put_adev;
+
+			fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]);
+			if (!fwnode) {
+				ret = -ENODEV;
+				goto err_free_swnodes;
+			}
+
+			adev->fwnode.secondary = fwnode;
+
+			dev_info(&cio2->dev, "Found supported sensor %s\n",
+				 acpi_dev_name(adev));
+
+			bridge->n_sensors++;
+		}
+	}
+
+	return ret;
+
+err_free_swnodes:
+	software_node_unregister_nodes(sensor->swnodes);
+err_put_adev:
+	acpi_dev_put(sensor->adev);
+
+	return ret;
+}
+
+int cio2_bridge_init(struct pci_dev *cio2)
+{
+	struct device *dev = &cio2->dev;
+	struct fwnode_handle *fwnode;
+	struct cio2_bridge *bridge;
+	int ret;
+
+	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
+	if (!bridge)
+		return -ENOMEM;
+
+	strscpy(bridge->cio2_node_name, CIO2_HID, sizeof(bridge->cio2_node_name));
+	bridge->cio2_hid_node.name = bridge->cio2_node_name;
+
+	ret = software_node_register(&bridge->cio2_hid_node);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register the CIO2 HID node\n");
+		goto err_free_bridge;
+	}
+
+	ret = cio2_bridge_connect_sensors(bridge, cio2);
+	if (ret || bridge->n_sensors == 0)
+		goto err_unregister_cio2;
+
+	dev_info(dev, "Connected %d cameras\n", bridge->n_sensors);
+
+	fwnode = software_node_fwnode(&bridge->cio2_hid_node);
+	if (!fwnode) {
+		dev_err(dev, "Error getting fwnode from cio2 software_node\n");
+		ret = -ENODEV;
+		goto err_unregister_sensors;
+	}
+
+	set_secondary_fwnode(dev, fwnode);
+
+	return 0;
+
+err_unregister_sensors:
+	cio2_bridge_unregister_sensors(bridge);
+err_unregister_cio2:
+	software_node_unregister(&bridge->cio2_hid_node);
+err_free_bridge:
+	kfree(bridge);
+
+	return ret;
+}
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
new file mode 100644
index 000000000000..f89a8e33f82c
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
@@ -0,0 +1,122 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Author: Dan Scally <djrscally@gmail.com> */
+#ifndef __CIO2_BRIDGE_H
+#define __CIO2_BRIDGE_H
+
+#include <linux/property.h>
+
+#define CIO2_HID				"INT343E"
+#define CIO2_NUM_PORTS				4
+#define MAX_NUM_LINK_FREQS			3
+
+#define CIO2_SENSOR_CONFIG(_HID, _NR, ...)	\
+	{					\
+		.hid = _HID,			\
+		.nr_link_freqs = _NR,		\
+		.link_freqs = { __VA_ARGS__ }	\
+	}
+
+#define NODE_SENSOR(_HID, _PROPS)		\
+	((const struct software_node) {		\
+		.name = _HID,			\
+		.properties = _PROPS,		\
+	})
+
+#define NODE_PORT(_PORT, _SENSOR_NODE)		\
+	((const struct software_node) {		\
+		_PORT,				\
+		_SENSOR_NODE,			\
+	})
+
+#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
+	((const struct software_node) {		\
+		_EP,				\
+		_PORT,				\
+		_PROPS,				\
+	})
+
+enum cio2_sensor_swnodes {
+	SWNODE_SENSOR_HID,
+	SWNODE_SENSOR_PORT,
+	SWNODE_SENSOR_ENDPOINT,
+	SWNODE_CIO2_PORT,
+	SWNODE_CIO2_ENDPOINT,
+	NR_OF_SENSOR_SWNODES
+};
+
+/* Data representation as it is in ACPI SSDB buffer */
+struct cio2_sensor_ssdb {
+	u8 version;				/* 0000 */
+	u8 sku;					/* 0001 */
+	u8 guid_csi2[16];			/* 0002 */
+	u8 devfunction;				/* 0003 */
+	u8 bus;					/* 0004 */
+	u32 dphylinkenfuses;			/* 0005 */
+	u32 clockdiv;				/* 0009 */
+	u8 link;				/* 0013 */
+	u8 lanes;				/* 0014 */
+	u32 csiparams[10];			/* 0015 */
+	u32 maxlanespeed;			/* 0019 */
+	u8 sensorcalibfileidx;			/* 0023 */
+	u8 sensorcalibfileidxInMBZ[3];		/* 0024 */
+	u8 romtype;				/* 0025 */
+	u8 vcmtype;				/* 0026 */
+	u8 platforminfo;			/* 0027 */
+	u8 platformsubinfo;
+	u8 flash;
+	u8 privacyled;
+	u8 degree;
+	u8 mipilinkdefined;
+	u32 mclkspeed;
+	u8 controllogicid;
+	u8 reserved1[3];
+	u8 mclkport;
+	u8 reserved2[13];
+} __packed;
+
+struct cio2_property_names {
+	char clock_frequency[16];
+	char rotation[9];
+	char bus_type[9];
+	char data_lanes[11];
+	char remote_endpoint[16];
+	char link_frequencies[17];
+};
+
+struct cio2_node_names {
+	char port[7];
+	char endpoint[11];
+	char remote_port[7];
+};
+
+struct cio2_sensor_config {
+	const char *hid;
+	const u8 nr_link_freqs;
+	const u64 link_freqs[MAX_NUM_LINK_FREQS];
+};
+
+struct cio2_sensor {
+	char name[ACPI_ID_LEN];
+	struct acpi_device *adev;
+
+	struct software_node swnodes[6];
+	struct cio2_node_names node_names;
+
+	u32 data_lanes[4];
+	struct cio2_sensor_ssdb ssdb;
+	struct cio2_property_names prop_names;
+	struct property_entry ep_properties[5];
+	struct property_entry dev_properties[3];
+	struct property_entry cio2_properties[3];
+	struct software_node_ref_args local_ref[1];
+	struct software_node_ref_args remote_ref[1];
+};
+
+struct cio2_bridge {
+	char cio2_node_name[ACPI_ID_LEN];
+	struct software_node cio2_hid_node;
+	unsigned int n_sensors;
+	struct cio2_sensor sensors[CIO2_NUM_PORTS];
+};
+
+#endif
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 36e354ecf71e..68ff28abc6a3 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1702,11 +1702,28 @@ static void cio2_queues_exit(struct cio2_device *cio2)
 		cio2_queue_exit(cio2, &cio2->queue[i]);
 }
 
+static bool cio2_check_fwnode_graph(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *endpoint;
+
+	if (IS_ERR_OR_NULL(fwnode))
+		return false;
+
+	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
+	if (endpoint) {
+		fwnode_handle_put(endpoint);
+		return true;
+	}
+
+	return cio2_check_fwnode_graph(fwnode->secondary);
+}
+
 /**************** PCI interface ****************/
 
 static int cio2_pci_probe(struct pci_dev *pci_dev,
 			  const struct pci_device_id *id)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(&pci_dev->dev);
 	struct cio2_device *cio2;
 	int r;
 
@@ -1715,6 +1732,23 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 		return -ENOMEM;
 	cio2->pci_dev = pci_dev;
 
+	/*
+	 * On some platforms no connections to sensors are defined in firmware,
+	 * if the device has no endpoints then we can try to build those as
+	 * software_nodes parsed from SSDB.
+	 */
+	if (!cio2_check_fwnode_graph(fwnode)) {
+		if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) {
+			dev_err(&pci_dev->dev,
+				"fwnode graph has no endpoints connected\n");
+			return -EINVAL;
+		}
+
+		r = cio2_bridge_init(pci_dev);
+		if (r)
+			return r;
+	}
+
 	r = pcim_enable_device(pci_dev);
 	if (r) {
 		dev_err(&pci_dev->dev, "failed to enable device (%d)\n", r);
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
index ccf0b85ae36f..520a27c9cdad 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -437,4 +437,10 @@ static inline struct cio2_queue *vb2q_to_cio2_queue(struct vb2_queue *vq)
 	return container_of(vq, struct cio2_queue, vbq);
 }
 
+#if IS_ENABLED(CONFIG_CIO2_BRIDGE)
+int cio2_bridge_init(struct pci_dev *cio2);
+#else
+int cio2_bridge_init(struct pci_dev *cio2) { return 0; }
+#endif
+
 #endif
-- 
2.25.1

Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Thu, Dec 17, 2020 at 11:43:37PM +0000, Daniel Scally wrote:
> Currently on platforms designed for Windows, connections between CIO2 and
> sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
> driver to compensate by building software_node connections, parsing the
> connection properties from the sensor's SSDB buffer.
> 
> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v2:
> 
> 	- Dropped some headers
> 	- Added support for specifying link-frequencies in the array of
> 	cio2_supported_sensors and added that property to the endpoint.
> 	- Replaced strcpy with strscpy (Laurent, I liked your change better
> 	stylistically but ofc the string literals are lost when the module
> 	is reloaded)
> 	- Named the ports/endpoints "port@%u"
> 	- Added an overflow check to cio2_bridge_connect_sensors()
> 	- A bunch of cosmetic changes
> 
> For the cio2_supported_sensors array, specify link frequencies in this
> manner: 
> 
> 	CIO2_SENSOR_CONFIG("OVTI5648", 2, 16800000, 2100000)
> 
>  MAINTAINERS                                   |   1 +
>  drivers/media/pci/intel/ipu3/Kconfig          |  18 ++
>  drivers/media/pci/intel/ipu3/Makefile         |   1 +
>  drivers/media/pci/intel/ipu3/cio2-bridge.c    | 274 ++++++++++++++++++
>  drivers/media/pci/intel/ipu3/cio2-bridge.h    | 122 ++++++++
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  34 +++
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h      |   6 +
>  7 files changed, 456 insertions(+)
>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
>  create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 16b544624577..e7784b4bc8ea 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8943,6 +8943,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
>  M:	Yong Zhi <yong.zhi@intel.com>
>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>  M:	Bingbu Cao <bingbu.cao@intel.com>
> +M:	Dan Scally <djrscally@gmail.com>
>  R:	Tianshu Qiu <tian.shu.qiu@intel.com>
>  L:	linux-media@vger.kernel.org
>  S:	Maintained
> diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
> index 82d7f17e6a02..dcf5c4b74673 100644
> --- a/drivers/media/pci/intel/ipu3/Kconfig
> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
>  	  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
>  	  connected camera.
>  	  The module will be called ipu3-cio2.
> +
> +config CIO2_BRIDGE
> +	bool "IPU3 CIO2 Sensors Bridge"
> +	depends on VIDEO_IPU3_CIO2
> +	help
> +	  This extension provides an API for the ipu3-cio2 driver to create
> +	  connections to cameras that are hidden in SSDB buffer in ACPI. It
> +	  can be used to enable support for cameras in detachable / hybrid
> +	  devices that ship with Windows.
> +
> +	  Say Y here if your device is a detachable / hybrid laptop that comes
> +	  with Windows installed by the OEM, for example:
> +
> +		- Microsoft Surface models (except Surface Pro 3)
> +		- The Lenovo Miix line (for example the 510, 520, 710 and 720)
> +		- Dell 7285
> +
> +	  If in doubt, say N here.
> diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
> index 429d516452e4..933777e6ea8a 100644
> --- a/drivers/media/pci/intel/ipu3/Makefile
> +++ b/drivers/media/pci/intel/ipu3/Makefile
> @@ -2,3 +2,4 @@
>  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
>  
>  ipu3-cio2-y += ipu3-cio2-main.o
> +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> new file mode 100644
> index 000000000000..3f0e2d7eab20
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> @@ -0,0 +1,274 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/pci.h>
> +#include <linux/property.h>
> +
> +#include "cio2-bridge.h"
> +
> +/*
> + * Extend this array with ACPI Hardware ID's of devices known to be working
> + * plus the number of link-frequencies expected by their drivers, along with
> + * the frequency values in hertz. This is somewhat opportunistic way of adding
> + * support for this for now in the hopes of a better source for the information
> + * (possibly some encoded value in the SSDB buffer that we're unaware of)
> + * becoming apparent in the future.
> + *
> + * Do not add an entry for a sensor that is not actually supported.
> + */
> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
> +	CIO2_SENSOR_CONFIG("INT33BE", 0),
> +	CIO2_SENSOR_CONFIG("OVTI2680", 0),
> +};
> +
> +static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
> +					void *data, u32 size)
> +{
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	acpi_status status;
> +	int ret;
> +
> +	status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	obj = buffer.pointer;
> +	if (!obj) {
> +		dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
> +		return -ENODEV;
> +	}
> +
> +	if (obj->type != ACPI_TYPE_BUFFER) {
> +		dev_err(&adev->dev, "Not an ACPI buffer\n");
> +		ret = -ENODEV;
> +		goto out_free_buff;
> +	}
> +
> +	if (obj->buffer.length > size) {
> +		dev_err(&adev->dev, "Given buffer is too small\n");
> +		ret = -EINVAL;
> +		goto out_free_buff;
> +	}
> +
> +	memcpy(data, obj->buffer.pointer, obj->buffer.length);
> +	ret = obj->buffer.length;
> +
> +out_free_buff:
> +	kfree(buffer.pointer);
> +	return ret;
> +}
> +
> +static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
> +{
> +	strscpy(sensor->prop_names.clock_frequency, "clock-frequency",
> +		sizeof(sensor->prop_names.clock_frequency));
> +	strscpy(sensor->prop_names.rotation, "rotation",
> +		sizeof(sensor->prop_names.rotation));
> +	strscpy(sensor->prop_names.bus_type, "bus-type",
> +		sizeof(sensor->prop_names.bus_type));
> +	strscpy(sensor->prop_names.data_lanes, "data-lanes",
> +		sizeof(sensor->prop_names.data_lanes));
> +	strscpy(sensor->prop_names.remote_endpoint, "remote-endpoint",
> +		sizeof(sensor->prop_names.remote_endpoint));
> +	strscpy(sensor->prop_names.link_frequencies, "link-frequencies",
> +		sizeof(sensor->prop_names.link_frequencies));

Just curious, was there anything not working correctly with the proposal
I made ?

static const struct cio2_property_names prop_names = {
	.clock_frequency = "clock-frequency",
	.rotation = "rotation",
	.bus_type = "bus-type",
	.data_lanes = "data-lanes",
	.remote_endpoint = "remote-endpoint",
};

static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
{
	sensor->prop_names = prop_names;
}

It generates a warning when the string is too long for the field size,
which should help catching issues at compilation time.

> +}
> +
> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor,
> +						 const struct cio2_sensor_config *cfg)
> +{
> +	unsigned int i;
> +
> +	cio2_bridge_init_property_names(sensor);
> +
> +	for (i = 0; i < 4; i++)
> +		sensor->data_lanes[i] = i + 1;
> +
> +	sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT];
> +	sensor->remote_ref[0].node = &sensor->swnodes[SWNODE_SENSOR_ENDPOINT];
> +
> +	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
> +						       sensor->ssdb.mclkspeed);
> +	sensor->dev_properties[1] = PROPERTY_ENTRY_U8(sensor->prop_names.rotation,
> +						      sensor->ssdb.degree);
> +
> +	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 4);
> +	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> +								sensor->data_lanes,
> +								sensor->ssdb.lanes);
> +	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> +							    sensor->local_ref);
> +
> +	if (cfg->nr_link_freqs > 0)
> +		sensor->ep_properties[3] = PROPERTY_ENTRY_U64_ARRAY_LEN(
> +						sensor->prop_names.link_frequencies,
> +						cfg->link_freqs,
> +						cfg->nr_link_freqs);
> +
> +	sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> +								  sensor->data_lanes,
> +								  sensor->ssdb.lanes);
> +	sensor->cio2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> +							      sensor->remote_ref);
> +}
> +
> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
> +{
> +	snprintf(sensor->node_names.remote_port, 7, "port@%u", sensor->ssdb.link);
> +	strscpy(sensor->node_names.port, "port@0", sizeof(sensor->node_names.port));
> +	strscpy(sensor->node_names.endpoint, "endpoint@0", sizeof(sensor->node_names.endpoint));

I'd wrap lines, but maybe that's because I'm an old-school, 80-columns
programmer :-)

> +}
> +
> +static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
> +						  struct cio2_sensor *sensor)
> +{
> +	struct software_node *nodes = sensor->swnodes;
> +
> +	cio2_bridge_init_swnode_names(sensor);
> +
> +	nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name,
> +					       sensor->dev_properties);
> +	nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
> +					      &nodes[SWNODE_SENSOR_HID]);
> +	nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> +						      &nodes[SWNODE_SENSOR_PORT],
> +						      sensor->ep_properties);
> +	nodes[SWNODE_CIO2_PORT] = NODE_PORT(sensor->node_names.remote_port,
> +					    &bridge->cio2_hid_node);
> +	nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> +						    &nodes[SWNODE_CIO2_PORT],
> +						    sensor->cio2_properties);
> +}
> +
> +static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
> +{
> +	struct cio2_sensor *sensor;
> +	unsigned int i;
> +
> +	for (i = 0; i < bridge->n_sensors; i++) {
> +		sensor = &bridge->sensors[i];
> +		software_node_unregister_nodes(sensor->swnodes);
> +		acpi_dev_put(sensor->adev);
> +	}
> +}
> +
> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
> +				       struct pci_dev *cio2)
> +{
> +	struct fwnode_handle *fwnode;
> +	struct cio2_sensor *sensor;
> +	struct acpi_device *adev;
> +	unsigned int i;
> +	int ret = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
> +		const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
> +
> +		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> +			if (bridge->n_sensors >= CIO2_NUM_PORTS) {
> +				dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");
> +				/* overflow i so outer loop ceases */
> +				i = ARRAY_SIZE(cio2_supported_sensors);
> +				break;

Or just

				return 0;

?

> +			}
> +
> +			if (!adev->status.enabled)
> +				continue;
> +
> +			sensor = &bridge->sensors[bridge->n_sensors];
> +			sensor->adev = adev;
> +			strscpy(sensor->name, cfg->hid, sizeof(sensor->name));
> +
> +			ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
> +							   &sensor->ssdb,
> +							   sizeof(sensor->ssdb));
> +			if (ret < 0)
> +				goto err_put_adev;
> +
> +			if (sensor->ssdb.lanes > 4) {
> +				dev_err(&adev->dev,
> +					"Number of lanes in SSDB is invalid\n");
> +				goto err_put_adev;
> +			}
> +
> +			cio2_bridge_create_fwnode_properties(sensor, cfg);
> +			cio2_bridge_create_connection_swnodes(bridge, sensor);
> +
> +			ret = software_node_register_nodes(sensor->swnodes);
> +			if (ret)
> +				goto err_put_adev;
> +
> +			fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]);
> +			if (!fwnode) {
> +				ret = -ENODEV;
> +				goto err_free_swnodes;
> +			}
> +
> +			adev->fwnode.secondary = fwnode;
> +
> +			dev_info(&cio2->dev, "Found supported sensor %s\n",
> +				 acpi_dev_name(adev));
> +
> +			bridge->n_sensors++;
> +		}
> +	}
> +
> +	return ret;
> +
> +err_free_swnodes:
> +	software_node_unregister_nodes(sensor->swnodes);
> +err_put_adev:
> +	acpi_dev_put(sensor->adev);
> +
> +	return ret;
> +}
> +
> +int cio2_bridge_init(struct pci_dev *cio2)
> +{
> +	struct device *dev = &cio2->dev;
> +	struct fwnode_handle *fwnode;
> +	struct cio2_bridge *bridge;
> +	int ret;
> +
> +	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> +	if (!bridge)
> +		return -ENOMEM;
> +
> +	strscpy(bridge->cio2_node_name, CIO2_HID, sizeof(bridge->cio2_node_name));
> +	bridge->cio2_hid_node.name = bridge->cio2_node_name;
> +
> +	ret = software_node_register(&bridge->cio2_hid_node);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register the CIO2 HID node\n");
> +		goto err_free_bridge;
> +	}
> +
> +	ret = cio2_bridge_connect_sensors(bridge, cio2);
> +	if (ret || bridge->n_sensors == 0)
> +		goto err_unregister_cio2;
> +
> +	dev_info(dev, "Connected %d cameras\n", bridge->n_sensors);
> +
> +	fwnode = software_node_fwnode(&bridge->cio2_hid_node);
> +	if (!fwnode) {
> +		dev_err(dev, "Error getting fwnode from cio2 software_node\n");
> +		ret = -ENODEV;
> +		goto err_unregister_sensors;
> +	}
> +
> +	set_secondary_fwnode(dev, fwnode);
> +
> +	return 0;
> +
> +err_unregister_sensors:
> +	cio2_bridge_unregister_sensors(bridge);
> +err_unregister_cio2:
> +	software_node_unregister(&bridge->cio2_hid_node);
> +err_free_bridge:
> +	kfree(bridge);
> +
> +	return ret;
> +}
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> new file mode 100644
> index 000000000000..f89a8e33f82c
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +#ifndef __CIO2_BRIDGE_H
> +#define __CIO2_BRIDGE_H
> +
> +#include <linux/property.h>
> +
> +#define CIO2_HID				"INT343E"
> +#define CIO2_NUM_PORTS				4
> +#define MAX_NUM_LINK_FREQS			3
> +
> +#define CIO2_SENSOR_CONFIG(_HID, _NR, ...)	\
> +	{					\
> +		.hid = _HID,			\
> +		.nr_link_freqs = _NR,		\
> +		.link_freqs = { __VA_ARGS__ }	\
> +	}
> +
> +#define NODE_SENSOR(_HID, _PROPS)		\
> +	((const struct software_node) {		\
> +		.name = _HID,			\
> +		.properties = _PROPS,		\
> +	})
> +
> +#define NODE_PORT(_PORT, _SENSOR_NODE)		\
> +	((const struct software_node) {		\
> +		_PORT,				\
> +		_SENSOR_NODE,			\
> +	})
> +
> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
> +	((const struct software_node) {		\
> +		_EP,				\
> +		_PORT,				\
> +		_PROPS,				\
> +	})
> +
> +enum cio2_sensor_swnodes {
> +	SWNODE_SENSOR_HID,
> +	SWNODE_SENSOR_PORT,
> +	SWNODE_SENSOR_ENDPOINT,
> +	SWNODE_CIO2_PORT,
> +	SWNODE_CIO2_ENDPOINT,
> +	NR_OF_SENSOR_SWNODES
> +};
> +
> +/* Data representation as it is in ACPI SSDB buffer */
> +struct cio2_sensor_ssdb {
> +	u8 version;				/* 0000 */
> +	u8 sku;					/* 0001 */
> +	u8 guid_csi2[16];			/* 0002 */
> +	u8 devfunction;				/* 0003 */
> +	u8 bus;					/* 0004 */
> +	u32 dphylinkenfuses;			/* 0005 */
> +	u32 clockdiv;				/* 0009 */
> +	u8 link;				/* 0013 */
> +	u8 lanes;				/* 0014 */
> +	u32 csiparams[10];			/* 0015 */
> +	u32 maxlanespeed;			/* 0019 */
> +	u8 sensorcalibfileidx;			/* 0023 */
> +	u8 sensorcalibfileidxInMBZ[3];		/* 0024 */
> +	u8 romtype;				/* 0025 */
> +	u8 vcmtype;				/* 0026 */
> +	u8 platforminfo;			/* 0027 */

Why stop at 27 ? :-) I'd either go all the way, or not at all. It's also
quite customary to represent offset as hex values, as that's what most
hex editors / viewers will show.

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

> +	u8 platformsubinfo;
> +	u8 flash;
> +	u8 privacyled;
> +	u8 degree;
> +	u8 mipilinkdefined;
> +	u32 mclkspeed;
> +	u8 controllogicid;
> +	u8 reserved1[3];
> +	u8 mclkport;
> +	u8 reserved2[13];
> +} __packed;
> +
> +struct cio2_property_names {
> +	char clock_frequency[16];
> +	char rotation[9];
> +	char bus_type[9];
> +	char data_lanes[11];
> +	char remote_endpoint[16];
> +	char link_frequencies[17];
> +};
> +
> +struct cio2_node_names {
> +	char port[7];
> +	char endpoint[11];
> +	char remote_port[7];
> +};
> +
> +struct cio2_sensor_config {
> +	const char *hid;
> +	const u8 nr_link_freqs;
> +	const u64 link_freqs[MAX_NUM_LINK_FREQS];
> +};
> +
> +struct cio2_sensor {
> +	char name[ACPI_ID_LEN];
> +	struct acpi_device *adev;
> +
> +	struct software_node swnodes[6];
> +	struct cio2_node_names node_names;
> +
> +	u32 data_lanes[4];
> +	struct cio2_sensor_ssdb ssdb;
> +	struct cio2_property_names prop_names;
> +	struct property_entry ep_properties[5];
> +	struct property_entry dev_properties[3];
> +	struct property_entry cio2_properties[3];
> +	struct software_node_ref_args local_ref[1];
> +	struct software_node_ref_args remote_ref[1];
> +};
> +
> +struct cio2_bridge {
> +	char cio2_node_name[ACPI_ID_LEN];
> +	struct software_node cio2_hid_node;
> +	unsigned int n_sensors;
> +	struct cio2_sensor sensors[CIO2_NUM_PORTS];
> +};
> +
> +#endif
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> index 36e354ecf71e..68ff28abc6a3 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> @@ -1702,11 +1702,28 @@ static void cio2_queues_exit(struct cio2_device *cio2)
>  		cio2_queue_exit(cio2, &cio2->queue[i]);
>  }
>  
> +static bool cio2_check_fwnode_graph(struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_handle *endpoint;
> +
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return false;
> +
> +	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> +	if (endpoint) {
> +		fwnode_handle_put(endpoint);
> +		return true;
> +	}
> +
> +	return cio2_check_fwnode_graph(fwnode->secondary);
> +}
> +
>  /**************** PCI interface ****************/
>  
>  static int cio2_pci_probe(struct pci_dev *pci_dev,
>  			  const struct pci_device_id *id)
>  {
> +	struct fwnode_handle *fwnode = dev_fwnode(&pci_dev->dev);
>  	struct cio2_device *cio2;
>  	int r;
>  
> @@ -1715,6 +1732,23 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  		return -ENOMEM;
>  	cio2->pci_dev = pci_dev;
>  
> +	/*
> +	 * On some platforms no connections to sensors are defined in firmware,
> +	 * if the device has no endpoints then we can try to build those as
> +	 * software_nodes parsed from SSDB.
> +	 */
> +	if (!cio2_check_fwnode_graph(fwnode)) {
> +		if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) {
> +			dev_err(&pci_dev->dev,
> +				"fwnode graph has no endpoints connected\n");
> +			return -EINVAL;
> +		}
> +
> +		r = cio2_bridge_init(pci_dev);
> +		if (r)
> +			return r;
> +	}
> +
>  	r = pcim_enable_device(pci_dev);
>  	if (r) {
>  		dev_err(&pci_dev->dev, "failed to enable device (%d)\n", r);
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> index ccf0b85ae36f..520a27c9cdad 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -437,4 +437,10 @@ static inline struct cio2_queue *vb2q_to_cio2_queue(struct vb2_queue *vq)
>  	return container_of(vq, struct cio2_queue, vbq);
>  }
>  
> +#if IS_ENABLED(CONFIG_CIO2_BRIDGE)
> +int cio2_bridge_init(struct pci_dev *cio2);
> +#else
> +int cio2_bridge_init(struct pci_dev *cio2) { return 0; }
> +#endif
> +
>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

Hi Laurent - thanks for the comments

On 18/12/2020 16:53, Laurent Pinchart wrote:
>> +static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
>> +{
>> +	strscpy(sensor->prop_names.clock_frequency, "clock-frequency",
>> +		sizeof(sensor->prop_names.clock_frequency));
>> +	strscpy(sensor->prop_names.rotation, "rotation",
>> +		sizeof(sensor->prop_names.rotation));
>> +	strscpy(sensor->prop_names.bus_type, "bus-type",
>> +		sizeof(sensor->prop_names.bus_type));
>> +	strscpy(sensor->prop_names.data_lanes, "data-lanes",
>> +		sizeof(sensor->prop_names.data_lanes));
>> +	strscpy(sensor->prop_names.remote_endpoint, "remote-endpoint",
>> +		sizeof(sensor->prop_names.remote_endpoint));
>> +	strscpy(sensor->prop_names.link_frequencies, "link-frequencies",
>> +		sizeof(sensor->prop_names.link_frequencies));
> 
> Just curious, was there anything not working correctly with the proposal
> I made ?
> 
> static const struct cio2_property_names prop_names = {
> 	.clock_frequency = "clock-frequency",
> 	.rotation = "rotation",
> 	.bus_type = "bus-type",
> 	.data_lanes = "data-lanes",
> 	.remote_endpoint = "remote-endpoint",
> };
> 
> static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
> {
> 	sensor->prop_names = prop_names;
> }
> 
> It generates a warning when the string is too long for the field size,
> which should help catching issues at compilation time.

Yes, though I don't know how much of a real-world problem it would have
been - if you recall we have the issue that the device grabs a reference
to the software_nodes (after we stopped delaying until after the
i2c_client is available), which means we can't safely free the
cio2_bridge struct on module unload. That also means we can't rely on
those pointers to string literals existing, because if the ipu3-cio2
module gets unloaded they'll be gone.

Shame, as it's way neater.

>> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
>> +{
>> +	snprintf(sensor->node_names.remote_port, 7, "port@%u", sensor->ssdb.link);
>> +	strscpy(sensor->node_names.port, "port@0", sizeof(sensor->node_names.port));
>> +	strscpy(sensor->node_names.endpoint, "endpoint@0", sizeof(sensor->node_names.endpoint));
> 
> I'd wrap lines, but maybe that's because I'm an old-school, 80-columns
> programmer :-)

Heh sure, I'll wrap them.

>> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
>> +				       struct pci_dev *cio2)
>> +{
>> +	struct fwnode_handle *fwnode;
>> +	struct cio2_sensor *sensor;
>> +	struct acpi_device *adev;
>> +	unsigned int i;
>> +	int ret = 0;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
>> +		const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
>> +
>> +		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
>> +			if (bridge->n_sensors >= CIO2_NUM_PORTS) {
>> +				dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");
>> +				/* overflow i so outer loop ceases */
>> +				i = ARRAY_SIZE(cio2_supported_sensors);
>> +				break;
> 
> Or just
> 
> 				return 0;
> 
> ?

Derp, yes of course.


>> +/* Data representation as it is in ACPI SSDB buffer */
>> +struct cio2_sensor_ssdb {
>> +	u8 version;				/* 0000 */
>> +	u8 sku;					/* 0001 */
>> +	u8 guid_csi2[16];			/* 0002 */
>> +	u8 devfunction;				/* 0003 */
>> +	u8 bus;					/* 0004 */
>> +	u32 dphylinkenfuses;			/* 0005 */
>> +	u32 clockdiv;				/* 0009 */
>> +	u8 link;				/* 0013 */
>> +	u8 lanes;				/* 0014 */
>> +	u32 csiparams[10];			/* 0015 */
>> +	u32 maxlanespeed;			/* 0019 */
>> +	u8 sensorcalibfileidx;			/* 0023 */
>> +	u8 sensorcalibfileidxInMBZ[3];		/* 0024 */
>> +	u8 romtype;				/* 0025 */
>> +	u8 vcmtype;				/* 0026 */
>> +	u8 platforminfo;			/* 0027 */
> 
> Why stop at 27 ? :-) I'd either go all the way, or not at all. It's also
> quite customary to represent offset as hex values, as that's what most
> hex editors / viewers will show.

Oops - that was actually just me debugging...I guess I might actually
finish it, converted to hex. It came in useful reading the DSDT to have
that somewhere easy to refer to.

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

Nice - thank you!

Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Laurent Pinchart]

Hi Daniel,

On Fri, Dec 18, 2020 at 11:57:54PM +0000, Daniel Scally wrote:
> Hi Laurent - thanks for the comments
> 
> On 18/12/2020 16:53, Laurent Pinchart wrote:
> >> +static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
> >> +{
> >> +	strscpy(sensor->prop_names.clock_frequency, "clock-frequency",
> >> +		sizeof(sensor->prop_names.clock_frequency));
> >> +	strscpy(sensor->prop_names.rotation, "rotation",
> >> +		sizeof(sensor->prop_names.rotation));
> >> +	strscpy(sensor->prop_names.bus_type, "bus-type",
> >> +		sizeof(sensor->prop_names.bus_type));
> >> +	strscpy(sensor->prop_names.data_lanes, "data-lanes",
> >> +		sizeof(sensor->prop_names.data_lanes));
> >> +	strscpy(sensor->prop_names.remote_endpoint, "remote-endpoint",
> >> +		sizeof(sensor->prop_names.remote_endpoint));
> >> +	strscpy(sensor->prop_names.link_frequencies, "link-frequencies",
> >> +		sizeof(sensor->prop_names.link_frequencies));
> > 
> > Just curious, was there anything not working correctly with the proposal
> > I made ?
> > 
> > static const struct cio2_property_names prop_names = {
> > 	.clock_frequency = "clock-frequency",
> > 	.rotation = "rotation",
> > 	.bus_type = "bus-type",
> > 	.data_lanes = "data-lanes",
> > 	.remote_endpoint = "remote-endpoint",
> > };
> > 
> > static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
> > {
> > 	sensor->prop_names = prop_names;
> > }
> > 
> > It generates a warning when the string is too long for the field size,
> > which should help catching issues at compilation time.
> 
> Yes, though I don't know how much of a real-world problem it would have
> been - if you recall we have the issue that the device grabs a reference
> to the software_nodes (after we stopped delaying until after the
> i2c_client is available), which means we can't safely free the
> cio2_bridge struct on module unload. That also means we can't rely on
> those pointers to string literals existing, because if the ipu3-cio2
> module gets unloaded they'll be gone.

But the strings above are not stored as literals in .rodata, they're
copied in prop_names (itself in .rodata), which is then copied to
sensor->prop_names.

> Shame, as it's way neater.
> 
> >> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
> >> +{
> >> +	snprintf(sensor->node_names.remote_port, 7, "port@%u", sensor->ssdb.link);
> >> +	strscpy(sensor->node_names.port, "port@0", sizeof(sensor->node_names.port));
> >> +	strscpy(sensor->node_names.endpoint, "endpoint@0", sizeof(sensor->node_names.endpoint));
> > 
> > I'd wrap lines, but maybe that's because I'm an old-school, 80-columns
> > programmer :-)
> 
> Heh sure, I'll wrap them.
> 
> >> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
> >> +				       struct pci_dev *cio2)
> >> +{
> >> +	struct fwnode_handle *fwnode;
> >> +	struct cio2_sensor *sensor;
> >> +	struct acpi_device *adev;
> >> +	unsigned int i;
> >> +	int ret = 0;
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
> >> +		const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
> >> +
> >> +		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> >> +			if (bridge->n_sensors >= CIO2_NUM_PORTS) {
> >> +				dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");
> >> +				/* overflow i so outer loop ceases */
> >> +				i = ARRAY_SIZE(cio2_supported_sensors);
> >> +				break;
> > 
> > Or just
> > 
> > 				return 0;
> > 
> > ?
> 
> Derp, yes of course.
> 
> 
> >> +/* Data representation as it is in ACPI SSDB buffer */
> >> +struct cio2_sensor_ssdb {
> >> +	u8 version;				/* 0000 */
> >> +	u8 sku;					/* 0001 */
> >> +	u8 guid_csi2[16];			/* 0002 */
> >> +	u8 devfunction;				/* 0003 */
> >> +	u8 bus;					/* 0004 */
> >> +	u32 dphylinkenfuses;			/* 0005 */
> >> +	u32 clockdiv;				/* 0009 */
> >> +	u8 link;				/* 0013 */
> >> +	u8 lanes;				/* 0014 */
> >> +	u32 csiparams[10];			/* 0015 */
> >> +	u32 maxlanespeed;			/* 0019 */
> >> +	u8 sensorcalibfileidx;			/* 0023 */
> >> +	u8 sensorcalibfileidxInMBZ[3];		/* 0024 */
> >> +	u8 romtype;				/* 0025 */
> >> +	u8 vcmtype;				/* 0026 */
> >> +	u8 platforminfo;			/* 0027 */
> > 
> > Why stop at 27 ? :-) I'd either go all the way, or not at all. It's also
> > quite customary to represent offset as hex values, as that's what most
> > hex editors / viewers will show.
> 
> Oops - that was actually just me debugging...I guess I might actually
> finish it, converted to hex. It came in useful reading the DSDT to have
> that somewhere easy to refer to.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Nice - thank you!

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

On 19/12/2020 00:39, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Fri, Dec 18, 2020 at 11:57:54PM +0000, Daniel Scally wrote:
>> Hi Laurent - thanks for the comments
>>
>> On 18/12/2020 16:53, Laurent Pinchart wrote:
>>>> +static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
>>>> +{
>>>> +	strscpy(sensor->prop_names.clock_frequency, "clock-frequency",
>>>> +		sizeof(sensor->prop_names.clock_frequency));
>>>> +	strscpy(sensor->prop_names.rotation, "rotation",
>>>> +		sizeof(sensor->prop_names.rotation));
>>>> +	strscpy(sensor->prop_names.bus_type, "bus-type",
>>>> +		sizeof(sensor->prop_names.bus_type));
>>>> +	strscpy(sensor->prop_names.data_lanes, "data-lanes",
>>>> +		sizeof(sensor->prop_names.data_lanes));
>>>> +	strscpy(sensor->prop_names.remote_endpoint, "remote-endpoint",
>>>> +		sizeof(sensor->prop_names.remote_endpoint));
>>>> +	strscpy(sensor->prop_names.link_frequencies, "link-frequencies",
>>>> +		sizeof(sensor->prop_names.link_frequencies));
>>>
>>> Just curious, was there anything not working correctly with the proposal
>>> I made ?
>>>
>>> static const struct cio2_property_names prop_names = {
>>> 	.clock_frequency = "clock-frequency",
>>> 	.rotation = "rotation",
>>> 	.bus_type = "bus-type",
>>> 	.data_lanes = "data-lanes",
>>> 	.remote_endpoint = "remote-endpoint",
>>> };
>>>
>>> static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
>>> {
>>> 	sensor->prop_names = prop_names;
>>> }
>>>
>>> It generates a warning when the string is too long for the field size,
>>> which should help catching issues at compilation time.
>>
>> Yes, though I don't know how much of a real-world problem it would have
>> been - if you recall we have the issue that the device grabs a reference
>> to the software_nodes (after we stopped delaying until after the
>> i2c_client is available), which means we can't safely free the
>> cio2_bridge struct on module unload. That also means we can't rely on
>> those pointers to string literals existing, because if the ipu3-cio2
>> module gets unloaded they'll be gone.
> 
> But the strings above are not stored as literals in .rodata, they're
> copied in prop_names (itself in .rodata), which is then copied to
> sensor->prop_names.

Yeah, my bad; I also had changed the struct definition to:

struct cio2_property_names {
	char *clock_frequency;
	...
};

And that behaves differently - apologies. I'll change to your proposal.

Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Andy Shevchenko]

On Thu, Dec 17, 2020 at 11:43:37PM +0000, Daniel Scally wrote:
> Currently on platforms designed for Windows, connections between CIO2 and
> sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
> driver to compensate by building software_node connections, parsing the
> connection properties from the sensor's SSDB buffer.

...

> +	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 4);

Does 4 has any meaning that can be described by #define ?

...

> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
> +{
> +	snprintf(sensor->node_names.remote_port, 7, "port@%u", sensor->ssdb.link);

Hmm... I think you should use actual size of remote_port instead of 7.

> +	strscpy(sensor->node_names.port, "port@0", sizeof(sensor->node_names.port));

Yeah, I would rather like to see one point of the definition of the format.
If it's the same as per OF case, perhaps some generic header (like fwnode.h?) is good for this?
In this case the 5 in one of the previous patches Also can be derived from the format.

> +	strscpy(sensor->node_names.endpoint, "endpoint@0", sizeof(sensor->node_names.endpoint));

Similar here.

> +}

...

> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
> +		const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
> +
> +		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {

> +			if (bridge->n_sensors >= CIO2_NUM_PORTS) {
> +				dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");

> +				/* overflow i so outer loop ceases */
> +				i = ARRAY_SIZE(cio2_supported_sensors);
> +				break;

Why not to create a new label below and assign ret here with probably comment
why it's not an error?

> +			}

...

> +			ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
> +							   &sensor->ssdb,
> +							   sizeof(sensor->ssdb));
> +			if (ret < 0)

if (ret) (because positive case can be returned just by next conditional).

> +				goto err_put_adev;
> +
> +			if (sensor->ssdb.lanes > 4) {
> +				dev_err(&adev->dev,
> +					"Number of lanes in SSDB is invalid\n");
> +				goto err_put_adev;
> +			}

...

> +			dev_info(&cio2->dev, "Found supported sensor %s\n",
> +				 acpi_dev_name(adev));
> +
> +			bridge->n_sensors++;
> +		}
> +	}

	return 0;

> +err_free_swnodes:
> +	software_node_unregister_nodes(sensor->swnodes);
> +err_put_adev:
> +	acpi_dev_put(sensor->adev);

err_out:

> +	return ret;
> +}

...

> +enum cio2_sensor_swnodes {
> +	SWNODE_SENSOR_HID,
> +	SWNODE_SENSOR_PORT,
> +	SWNODE_SENSOR_ENDPOINT,
> +	SWNODE_CIO2_PORT,
> +	SWNODE_CIO2_ENDPOINT,

> +	NR_OF_SENSOR_SWNODES

Perhaps same namespace, i.e.

	SWNODE_SENSOR_NR

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

Hi Andy, thanks for the comments

On 18/12/2020 21:17, Andy Shevchenko wrote:
> On Thu, Dec 17, 2020 at 11:43:37PM +0000, Daniel Scally wrote:
>> Currently on platforms designed for Windows, connections between CIO2 and
>> sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
>> driver to compensate by building software_node connections, parsing the
>> connection properties from the sensor's SSDB buffer.
> 
> ...
> 
>> +	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 4);
> 
> Does 4 has any meaning that can be described by #define ?

It's V4L2_FWNODE_BUS_TYPE_CSI2_DPHY:

https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-fwnode.c#L36

That enum's not in an accessible header, but I can define it in this
module's header

>> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
>> +{
>> +	snprintf(sensor->node_names.remote_port, 7, "port@%u", sensor->ssdb.link);
> 
> Hmm... I think you should use actual size of remote_port instead of 7.

Yes ok


>> +	strscpy(sensor->node_names.port, "port@0", sizeof(sensor->node_names.port));
> 
> Yeah, I would rather like to see one point of the definition of the format.
> If it's the same as per OF case, perhaps some generic header (like fwnode.h?) is good for this?
> In this case the 5 in one of the previous patches Also can be derived from the format.

Okedokey. It is indeed intended to match OF and ACPI case, both of which
mandate that format (though only ACPI's functions seem to enforce it).
fwnode.h seems as good a place as any to me, though I'm not sure there's
anywhere in the driver code for OF or ACPI that would actually use it at
the moment.

>> +	strscpy(sensor->node_names.endpoint, "endpoint@0", sizeof(sensor->node_names.endpoint));
> 
> Similar here.
> 
>> +}
> 
> ...
> 
>> +	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
>> +		const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
>> +
>> +		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> 
>> +			if (bridge->n_sensors >= CIO2_NUM_PORTS) {
>> +				dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");
> 
>> +				/* overflow i so outer loop ceases */
>> +				i = ARRAY_SIZE(cio2_supported_sensors);
>> +				break;
> 
> Why not to create a new label below and assign ret here with probably comment
> why it's not an error?

Sure, I can do that, but since it wouldn't need any cleanup I could also
just return 0 here as Laurent suggest (but with a comment explaining why
that's ok as you say) - do you have a preference?

>> +			}
> 
> ...
> 
>> +			ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
>> +							   &sensor->ssdb,
>> +							   sizeof(sensor->ssdb));
>> +			if (ret < 0)
> 
> if (ret) (because positive case can be returned just by next conditional).

cio2_bridge_read_acpi_buffer() returns the buffer length on success at
the moment, but I can change it to return 0 and have this be if (ret)

>> +				goto err_put_adev;
>> +
>> +			if (sensor->ssdb.lanes > 4) {
>> +				dev_err(&adev->dev,
>> +					"Number of lanes in SSDB is invalid\n");
>> +				goto err_put_adev;
>> +			}
> 
> ...
> 
>> +			dev_info(&cio2->dev, "Found supported sensor %s\n",
>> +				 acpi_dev_name(adev));
>> +
>> +			bridge->n_sensors++;
>> +		}
>> +	}
> 
> 	return 0;

Okedokey

> 
>> +err_free_swnodes:
>> +	software_node_unregister_nodes(sensor->swnodes);
>> +err_put_adev:
>> +	acpi_dev_put(sensor->adev);
> 
> err_out:

Depends on question above I think

>> +	return ret;
>> +}
> 
> ...
> 
>> +enum cio2_sensor_swnodes {
>> +	SWNODE_SENSOR_HID,
>> +	SWNODE_SENSOR_PORT,
>> +	SWNODE_SENSOR_ENDPOINT,
>> +	SWNODE_CIO2_PORT,
>> +	SWNODE_CIO2_ENDPOINT,
> 
>> +	NR_OF_SENSOR_SWNODES
> 
> Perhaps same namespace, i.e.
> 
> 	SWNODE_SENSOR_NR

Yep, will do.

Thanks
Dan

Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Andy Shevchenko]

On Sat, Dec 19, 2020 at 2:25 AM Daniel Scally <djrscally@gmail.com> wrote:
> On 18/12/2020 21:17, Andy Shevchenko wrote:
> > On Thu, Dec 17, 2020 at 11:43:37PM +0000, Daniel Scally wrote:

...

> >> +    sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 4);
> >
> > Does 4 has any meaning that can be described by #define ?
>
> It's V4L2_FWNODE_BUS_TYPE_CSI2_DPHY:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-fwnode.c#L36
>
> That enum's not in an accessible header, but I can define it in this
> module's header

Maybe you can do a preparatory patch to make it visible to v4l2
drivers? (Like moving to one of v4l2 headers)

...

> >> +                    if (bridge->n_sensors >= CIO2_NUM_PORTS) {
> >> +                            dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");
> >
> >> +                            /* overflow i so outer loop ceases */
> >> +                            i = ARRAY_SIZE(cio2_supported_sensors);
> >> +                            break;
> >
> > Why not to create a new label below and assign ret here with probably comment
> > why it's not an error?
>
> Sure, I can do that, but since it wouldn't need any cleanup I could also
> just return 0 here as Laurent suggest (but with a comment explaining why
> that's ok as you say) - do you have a preference?

While it's a good suggestion it will bring a bit of inconsistency into
approach. Everywhere else in the function you are using the goto
approach.
So yes, I have a preference.

> >> +                    }

...

> >> +                    ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
> >> +                                                       &sensor->ssdb,
> >> +                                                       sizeof(sensor->ssdb));
> >> +                    if (ret < 0)
> >
> > if (ret) (because positive case can be returned just by next conditional).
>
> cio2_bridge_read_acpi_buffer() returns the buffer length on success at
> the moment, but I can change it to return 0 and have this be if (ret)

Please correct this somehow, because the next failure returns it
instead of error...

> >> +                            goto err_put_adev;
> >> +
> >> +                    if (sensor->ssdb.lanes > 4) {
> >> +                            dev_err(&adev->dev,
> >> +                                    "Number of lanes in SSDB is invalid\n");

...I'm even thinking that you have to assign ret here to something meaningful.

> >> +                            goto err_put_adev;
> >> +                    }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

On 19/12/2020 18:52, Andy Shevchenko wrote:
> On Sat, Dec 19, 2020 at 2:25 AM Daniel Scally <djrscally@gmail.com> wrote:
>> On 18/12/2020 21:17, Andy Shevchenko wrote:
>>> On Thu, Dec 17, 2020 at 11:43:37PM +0000, Daniel Scally wrote:
> 
> ...
> 
>>>> +    sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 4);
>>>
>>> Does 4 has any meaning that can be described by #define ?
>>
>> It's V4L2_FWNODE_BUS_TYPE_CSI2_DPHY:
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-fwnode.c#L36
>>
>> That enum's not in an accessible header, but I can define it in this
>> module's header
> 
> Maybe you can do a preparatory patch to make it visible to v4l2
> drivers? (Like moving to one of v4l2 headers)

Sure ok, guess media/v4l2-fwnode.h makes the most sense.

> ...
> 
>>>> +                    if (bridge->n_sensors >= CIO2_NUM_PORTS) {
>>>> +                            dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");
>>>
>>>> +                            /* overflow i so outer loop ceases */
>>>> +                            i = ARRAY_SIZE(cio2_supported_sensors);
>>>> +                            break;
>>>
>>> Why not to create a new label below and assign ret here with probably comment
>>> why it's not an error?
>>
>> Sure, I can do that, but since it wouldn't need any cleanup I could also
>> just return 0 here as Laurent suggest (but with a comment explaining why
>> that's ok as you say) - do you have a preference?
> 
> While it's a good suggestion it will bring a bit of inconsistency into
> approach. Everywhere else in the function you are using the goto
> approach.
> So yes, I have a preference.

No problem

>>>> +                    }
> 
> ...
> 
>>>> +                    ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
>>>> +                                                       &sensor->ssdb,
>>>> +                                                       sizeof(sensor->ssdb));
>>>> +                    if (ret < 0)
>>>
>>> if (ret) (because positive case can be returned just by next conditional).
>>
>> cio2_bridge_read_acpi_buffer() returns the buffer length on success at
>> the moment, but I can change it to return 0 and have this be if (ret)
> 
> Please correct this somehow, because the next failure returns it
> instead of error...

Ah! Good spot - thank you. I will fix that yes.

>>>> +                            goto err_put_adev;
>>>> +
>>>> +                    if (sensor->ssdb.lanes > 4) {
>>>> +                            dev_err(&adev->dev,
>>>> +                                    "Number of lanes in SSDB is invalid\n");
> 
> ...I'm even thinking that you have to assign ret here to something meaningful.

Yeah I agree, I will do this too.

>>>> +                            goto err_put_adev;
>>>> +                    }
> 

Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Sakari Ailus]

Hi Daniel, Andy,

On Sat, Dec 19, 2020 at 11:48:51PM +0000, Daniel Scally wrote:
> On 19/12/2020 18:52, Andy Shevchenko wrote:
> > On Sat, Dec 19, 2020 at 2:25 AM Daniel Scally <djrscally@gmail.com> wrote:
> >> On 18/12/2020 21:17, Andy Shevchenko wrote:
> >>> On Thu, Dec 17, 2020 at 11:43:37PM +0000, Daniel Scally wrote:
> > 
> > ...
> > 
> >>>> +    sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 4);
> >>>
> >>> Does 4 has any meaning that can be described by #define ?
> >>
> >> It's V4L2_FWNODE_BUS_TYPE_CSI2_DPHY:
> >>
> >> https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-fwnode.c#L36
> >>
> >> That enum's not in an accessible header, but I can define it in this
> >> module's header
> > 
> > Maybe you can do a preparatory patch to make it visible to v4l2
> > drivers? (Like moving to one of v4l2 headers)
> 
> Sure ok, guess media/v4l2-fwnode.h makes the most sense.

Yes, please.

> 
> > ...
> > 
> >>>> +                    if (bridge->n_sensors >= CIO2_NUM_PORTS) {
> >>>> +                            dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");
> >>>
> >>>> +                            /* overflow i so outer loop ceases */
> >>>> +                            i = ARRAY_SIZE(cio2_supported_sensors);
> >>>> +                            break;
> >>>
> >>> Why not to create a new label below and assign ret here with probably comment
> >>> why it's not an error?
> >>
> >> Sure, I can do that, but since it wouldn't need any cleanup I could also
> >> just return 0 here as Laurent suggest (but with a comment explaining why
> >> that's ok as you say) - do you have a preference?
> > 
> > While it's a good suggestion it will bring a bit of inconsistency into
> > approach. Everywhere else in the function you are using the goto
> > approach.
> > So yes, I have a preference.
> 
> No problem

Laurent also commented on the return code.

I might just handle this as an error. The earlier ports are fine, but
there's also a problem with the data here. It'd be easier to spot that this
way, and we can change this in the future if need be.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Daniel Scally]

On 21/12/2020 10:21, Sakari Ailus wrote:
> Hi Daniel, Andy,
>
> On Sat, Dec 19, 2020 at 11:48:51PM +0000, Daniel Scally wrote:
>> On 19/12/2020 18:52, Andy Shevchenko wrote:
>>> On Sat, Dec 19, 2020 at 2:25 AM Daniel Scally <djrscally@gmail.com> wrote:
>>>> On 18/12/2020 21:17, Andy Shevchenko wrote:
>>>>> On Thu, Dec 17, 2020 at 11:43:37PM +0000, Daniel Scally wrote:
>>> ...
>>>
>>>>>> +    sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 4);
>>>>> Does 4 has any meaning that can be described by #define ?
>>>> It's V4L2_FWNODE_BUS_TYPE_CSI2_DPHY:
>>>>
>>>> https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-fwnode.c#L36
>>>>
>>>> That enum's not in an accessible header, but I can define it in this
>>>> module's header
>>> Maybe you can do a preparatory patch to make it visible to v4l2
>>> drivers? (Like moving to one of v4l2 headers)
>> Sure ok, guess media/v4l2-fwnode.h makes the most sense.
> Yes, please.
Done for the next version
>
>>> ...
>>>
>>>>>> +                    if (bridge->n_sensors >= CIO2_NUM_PORTS) {
>>>>>> +                            dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");
>>>>>> +                            /* overflow i so outer loop ceases */
>>>>>> +                            i = ARRAY_SIZE(cio2_supported_sensors);
>>>>>> +                            break;
>>>>> Why not to create a new label below and assign ret here with probably comment
>>>>> why it's not an error?
>>>> Sure, I can do that, but since it wouldn't need any cleanup I could also
>>>> just return 0 here as Laurent suggest (but with a comment explaining why
>>>> that's ok as you say) - do you have a preference?
>>> While it's a good suggestion it will bring a bit of inconsistency into
>>> approach. Everywhere else in the function you are using the goto
>>> approach.
>>> So yes, I have a preference.
>> No problem
> Laurent also commented on the return code.
>
> I might just handle this as an error. The earlier ports are fine, but
> there's also a problem with the data here. It'd be easier to spot that this
> way, and we can change this in the future if need be.


You mean just raise an error with dev_err()? Or fail the probe and
unwind the 4 sensors that were already connected successfully? I'm fine
with that if so - we have no in scope devices where that will be a
problem at the moment.

Re: [PATCH v2 12/12] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

[Sakari Ailus]

On Mon, Dec 21, 2020 at 10:52:52AM +0000, Daniel Scally wrote:
> 
> On 21/12/2020 10:21, Sakari Ailus wrote:
> > Hi Daniel, Andy,
> >
> > On Sat, Dec 19, 2020 at 11:48:51PM +0000, Daniel Scally wrote:
> >> On 19/12/2020 18:52, Andy Shevchenko wrote:
> >>> On Sat, Dec 19, 2020 at 2:25 AM Daniel Scally <djrscally@gmail.com> wrote:
> >>>> On 18/12/2020 21:17, Andy Shevchenko wrote:
> >>>>> On Thu, Dec 17, 2020 at 11:43:37PM +0000, Daniel Scally wrote:
> >>> ...
> >>>
> >>>>>> +    sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type, 4);
> >>>>> Does 4 has any meaning that can be described by #define ?
> >>>> It's V4L2_FWNODE_BUS_TYPE_CSI2_DPHY:
> >>>>
> >>>> https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-fwnode.c#L36
> >>>>
> >>>> That enum's not in an accessible header, but I can define it in this
> >>>> module's header
> >>> Maybe you can do a preparatory patch to make it visible to v4l2
> >>> drivers? (Like moving to one of v4l2 headers)
> >> Sure ok, guess media/v4l2-fwnode.h makes the most sense.
> > Yes, please.
> Done for the next version
> >
> >>> ...
> >>>
> >>>>>> +                    if (bridge->n_sensors >= CIO2_NUM_PORTS) {
> >>>>>> +                            dev_warn(&cio2->dev, "Exceeded available CIO2 ports\n");
> >>>>>> +                            /* overflow i so outer loop ceases */
> >>>>>> +                            i = ARRAY_SIZE(cio2_supported_sensors);
> >>>>>> +                            break;
> >>>>> Why not to create a new label below and assign ret here with probably comment
> >>>>> why it's not an error?
> >>>> Sure, I can do that, but since it wouldn't need any cleanup I could also
> >>>> just return 0 here as Laurent suggest (but with a comment explaining why
> >>>> that's ok as you say) - do you have a preference?
> >>> While it's a good suggestion it will bring a bit of inconsistency into
> >>> approach. Everywhere else in the function you are using the goto
> >>> approach.
> >>> So yes, I have a preference.
> >> No problem
> > Laurent also commented on the return code.
> >
> > I might just handle this as an error. The earlier ports are fine, but
> > there's also a problem with the data here. It'd be easier to spot that this
> > way, and we can change this in the future if need be.
> 
> 
> You mean just raise an error with dev_err()? Or fail the probe and
> unwind the 4 sensors that were already connected successfully? I'm fine

Both.

> with that if so - we have no in scope devices where that will be a
> problem at the moment.

I guess there will be quite a few additional things to address before
getting anything with four sensors working.

-- 
Regards,

Sakari Ailus