diff mbox series

[v2] media: i2c: thp7312: use fwnode_for_each_child_node()

Message ID Z90qM33DvkTMGg_x@mva-rohm (mailing list archive)
State New
Headers show
Series [v2] media: i2c: thp7312: use fwnode_for_each_child_node() | expand

Commit Message

Matti Vaittinen March 21, 2025, 8:58 a.m. UTC 
When fwnode_for_each_available_child_node() is used on the device-tree
backed systems, it renders to same operation as the
fwnode_for_each_child_node(), because the fwnode_for_each_child_node()
does only iterate through those device-tree nodes which are available.

The thp7312 uses the fwnode_for_each_available_child_node() to look up
and handle nodes with specific names. This means the code is used only
on the device-tree backed systems because the node names have little
meaning on ACPI or swnode backed systems.

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
explicitly considered here. This will also 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>

---
Revision history:
v1 => v2:
 - rephrase the commit message to not claim the 'availability' has no
   well defined meaning on the DT backed systems. Instead, explain that
   the fwnode_for_each_available_child_node() only iterates through the
   available nodes on the DT backed systems and is thus functionally
   equivalent to the fwnode_for_each_child_node().

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 21, 2025, 10:41 a.m. UTC | #1 
Hi Matti,

Thank you for the patch.

On Fri, Mar 21, 2025 at 10:58:27AM +0200, Matti Vaittinen wrote:
> When fwnode_for_each_available_child_node() is used on the device-tree
> backed systems, it renders to same operation as the
> fwnode_for_each_child_node(), because the fwnode_for_each_child_node()
> does only iterate through those device-tree nodes which are available.

This makes me wonder why the OF backend implements
fwnode_for_each_child_node() as fwnode_for_each_available_child_node().
Is that on purpose, or is it a bug ?

> The thp7312 uses the fwnode_for_each_available_child_node() to look up
> and handle nodes with specific names. This means the code is used only
> on the device-tree backed systems because the node names have little
> meaning on ACPI or swnode backed systems.
> 
> 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
> explicitly considered here. This will also 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>
> 
> ---
> Revision history:
> v1 => v2:
>  - rephrase the commit message to not claim the 'availability' has no
>    well defined meaning on the DT backed systems. Instead, explain that
>    the fwnode_for_each_available_child_node() only iterates through the
>    available nodes on the DT backed systems and is thus functionally
>    equivalent to the fwnode_for_each_child_node().
> 
> 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
Sakari Ailus March 24, 2025, 9:28 a.m. UTC | #2 
Hi Laurent,

On Fri, Mar 21, 2025 at 12:41:00PM +0200, Laurent Pinchart wrote:
> Hi Matti,
> 
> Thank you for the patch.
> 
> On Fri, Mar 21, 2025 at 10:58:27AM +0200, Matti Vaittinen wrote:
> > When fwnode_for_each_available_child_node() is used on the device-tree
> > backed systems, it renders to same operation as the
> > fwnode_for_each_child_node(), because the fwnode_for_each_child_node()
> > does only iterate through those device-tree nodes which are available.
> 
> This makes me wonder why the OF backend implements
> fwnode_for_each_child_node() as fwnode_for_each_available_child_node().
> Is that on purpose, or is it a bug ?

I went back to see where this came from,I understand
<URL:https://lkml.org/lkml/2014/10/17/185> is the first version of the
patch adding device_get_next_child_node(). Since then the behaviour has
been carried forward.

Cc Rafael.

> 
> > The thp7312 uses the fwnode_for_each_available_child_node() to look up
> > and handle nodes with specific names. This means the code is used only
> > on the device-tree backed systems because the node names have little
> > meaning on ACPI or swnode backed systems.
> > 
> > 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
> > explicitly considered here. This will also 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>
> > 
> > ---
> > Revision history:
> > v1 => v2:
> >  - rephrase the commit message to not claim the 'availability' has no
> >    well defined meaning on the DT backed systems. Instead, explain that
> >    the fwnode_for_each_available_child_node() only iterates through the
> >    available nodes on the DT backed systems and is thus functionally
> >    equivalent to the fwnode_for_each_child_node().
> > 
> > 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
>
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++;