diff mbox series

media: i2c: thp7312: Don't require node availability

Message ID Z9vTV7tS2ZI3tM6m@mva-rohm (mailing list archive)
State New
Headers show
Series media: i2c: thp7312: Don't require node availability | expand

Commit Message

Matti Vaittinen March 20, 2025, 8:35 a.m. UTC 
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(-)


base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6

Comments

Laurent Pinchart March 20, 2025, 2:26 p.m. UTC | #1 
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
Matti Vaittinen March 21, 2025, 6:35 a.m. UTC | #2 
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
Matti Vaittinen March 21, 2025, 8:29 a.m. UTC | #3 
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
diff mbox series

Patch

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