[PATCH] media: i2c: thp7312: Don't require node availability

[PATCH] media: i2c: thp7312: Don't require node availability

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

It appears that the concept of available firmware nodes is not really
applicable to the scenarios where a specific name is required from a
node.

As explained[1] by Sakari:
"OF only enumerates available nodes via the fwnode API, software nodes
don't have the concept but on ACPI I guess you could have a difference
in nodes where you have device sub-nodes that aren't available. Still,
these ACPI device nodes don't have meaningful names in this context
(they're 4-character object names) so you wouldn't use them like this
anyway."

Use the fwnode_for_each_child_node() instead of the
fwnode_for_each_available_child_node() In order to make it clearly
visible that the 'availability' of the nodes does not need to be
considered here. This will make it clearly visible that the code in
this driver is suitable candidate to be converted to use the new
fwnode_for_each_named_child_node()[2] when it gets merged.

[1]:
https://lore.kernel.org/all/Z9rhfJUlCbi7kA2m@kekkonen.localdomain/
[2]:
https://lore.kernel.org/all/9c3880f74476436f39d796b5c10c540ae50b722c.1742225817.git.mazziesaccount@gmail.com/

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
NOTE: The change is compile tested only! Proper testing and reviewing is
highly appreciated (as always).

---
 drivers/media/i2c/thp7312.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/thp7312.c b/drivers/media/i2c/thp7312.c
index 8852c56431fe..4b66f64f8d65 100644
--- a/drivers/media/i2c/thp7312.c
+++ b/drivers/media/i2c/thp7312.c
@@ -2067,7 +2067,7 @@ static int thp7312_parse_dt(struct thp7312_device *thp7312)
 		return -EINVAL;
 	}
 
-	fwnode_for_each_available_child_node(sensors, node) {
+	fwnode_for_each_child_node(sensors, node) {
 		if (fwnode_name_eq(node, "sensor")) {
 			if (!thp7312_sensor_parse_dt(thp7312, node))
 				num_sensors++;

base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] media: i2c: thp7312: Don't require node availability

[Laurent Pinchart]

Hi Matti,

On Thu, Mar 20, 2025 at 10:35:35AM +0200, Matti Vaittinen wrote:
> It appears that the concept of available firmware nodes is not really
> applicable to the scenarios where a specific name is required from a
> node.
> 
> As explained[1] by Sakari:
> "OF only enumerates available nodes via the fwnode API, software nodes
> don't have the concept but on ACPI I guess you could have a difference
> in nodes where you have device sub-nodes that aren't available. Still,
> these ACPI device nodes don't have meaningful names in this context
> (they're 4-character object names) so you wouldn't use them like this
> anyway."
> 
> Use the fwnode_for_each_child_node() instead of the
> fwnode_for_each_available_child_node() In order to make it clearly
> visible that the 'availability' of the nodes does not need to be
> considered here.

Why not ? Node availability is a concept that exists in DT, and this
driver has only been tested on DT-based systems. Why can't we keep the
code as-is ?

> This will make it clearly visible that the code in
> this driver is suitable candidate to be converted to use the new
> fwnode_for_each_named_child_node()[2] when it gets merged.
> 
> [1]:
> https://lore.kernel.org/all/Z9rhfJUlCbi7kA2m@kekkonen.localdomain/
> [2]:
> https://lore.kernel.org/all/9c3880f74476436f39d796b5c10c540ae50b722c.1742225817.git.mazziesaccount@gmail.com/
> 
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> NOTE: The change is compile tested only! Proper testing and reviewing is
> highly appreciated (as always).
> 
> ---
>  drivers/media/i2c/thp7312.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/thp7312.c b/drivers/media/i2c/thp7312.c
> index 8852c56431fe..4b66f64f8d65 100644
> --- a/drivers/media/i2c/thp7312.c
> +++ b/drivers/media/i2c/thp7312.c
> @@ -2067,7 +2067,7 @@ static int thp7312_parse_dt(struct thp7312_device *thp7312)
>  		return -EINVAL;
>  	}
>  
> -	fwnode_for_each_available_child_node(sensors, node) {
> +	fwnode_for_each_child_node(sensors, node) {
>  		if (fwnode_name_eq(node, "sensor")) {
>  			if (!thp7312_sensor_parse_dt(thp7312, node))
>  				num_sensors++;
> 
> base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: i2c: thp7312: Don't require node availability

[Matti Vaittinen]

Hi dee Ho Laurent,

On 20/03/2025 16:26, Laurent Pinchart wrote:
> Hi Matti,
> 
> On Thu, Mar 20, 2025 at 10:35:35AM +0200, Matti Vaittinen wrote:
>> It appears that the concept of available firmware nodes is not really
>> applicable to the scenarios where a specific name is required from a
>> node.
>>
>> As explained[1] by Sakari:
>> "OF only enumerates available nodes via the fwnode API, software nodes
>> don't have the concept but on ACPI I guess you could have a difference
>> in nodes where you have device sub-nodes that aren't available. Still,
>> these ACPI device nodes don't have meaningful names in this context
>> (they're 4-character object names) so you wouldn't use them like this
>> anyway."
>>
>> Use the fwnode_for_each_child_node() instead of the
>> fwnode_for_each_available_child_node() In order to make it clearly
>> visible that the 'availability' of the nodes does not need to be
>> considered here.
> 
> Why not ? Node availability is a concept that exists in DT, and this
> driver has only been tested on DT-based systems.

I admit I need to study this then. I just took what Sakari said for 
granted, without taking any further look at this.

I mean following quote:
"OF only enumerates available nodes via the fwnode API".

I interpreted it as if, in the dt based systems, the nodes which aren't 
available, wouldn't be enumerated and available via the fwnode APIs. If 
this is not true, then we probably need to re-re-re-consider also the 
need for the fwnode_for_each_available_named_child_node().

> Why can't we keep the
> code as-is ?

If I am mistaken and the 'availability' has a meaning - then we can and 
should. If not, then this discussion should serve as a good example why 
the code should be changed ;)

I hope Sakari can share his view :)

Yours,
	-- Matti

Re: [PATCH] media: i2c: thp7312: Don't require node availability

[Matti Vaittinen]

On 21/03/2025 08:35, Matti Vaittinen wrote:
> Hi dee Ho Laurent,
> 
> On 20/03/2025 16:26, Laurent Pinchart wrote:
>> Hi Matti,
>>
>> On Thu, Mar 20, 2025 at 10:35:35AM +0200, Matti Vaittinen wrote:
>>> It appears that the concept of available firmware nodes is not really
>>> applicable to the scenarios where a specific name is required from a
>>> node.
>>>
>>> As explained[1] by Sakari:
>>> "OF only enumerates available nodes via the fwnode API, software nodes
>>> don't have the concept but on ACPI I guess you could have a difference
>>> in nodes where you have device sub-nodes that aren't available. Still,
>>> these ACPI device nodes don't have meaningful names in this context
>>> (they're 4-character object names) so you wouldn't use them like this
>>> anyway."
>>>
>>> Use the fwnode_for_each_child_node() instead of the
>>> fwnode_for_each_available_child_node() In order to make it clearly
>>> visible that the 'availability' of the nodes does not need to be
>>> considered here.
>>
>> Why not ? Node availability is a concept that exists in DT, and this
>> driver has only been tested on DT-based systems.
> 
> I admit I need to study this then. I just took what Sakari said for 
> granted, without taking any further look at this.
> 

I took a peek in the 'availability' concept and found:
https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/of/base.c#L468

So, the availability indeed has a well defined meaning in the DT, 
boiling down to the value of the 'status' -property.

Then I took further look at the fwnode_for_each_child_node(), and if I'm 
not mistaken, it calls:

fwnode_for_each_child_node()
	fwnode_get_next_child_node()
		fwnode_call_ptr_op(fwnode, get_next_child_node, child);
			 of_fwnode_get_next_child_node() (dt-based)
				of_get_next_available_child() (dt-based)

where the of_get_next_available_child() skips all the disabled nodes.

So, in that regard I agree with Sakari. On DT based systems, the

fwnode_for_each_child_node() seems to equal the
fwnode_for_each_available_child_node().

And, since the 'thp7312' driver requires specific names for the nodes, 
it indeed seems to me that only the device-tree use-case needs to be 
considered.

After all this I'd say this patch is still valid - but the commit 
message is misleading. If no one objects I'll rewrite the commit msg and 
respin :)

Yours,
	-- Matti