1. Patchew
  2. linux
  3. View series

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

Matti Vaittinen posted 1 patch 9 months ago 
drivers/media/i2c/thp7312.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH v2] media: i2c: thp7312: use fwnode_for_each_child_node()
Posted by Matti Vaittinen 9 months ago 
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(-)

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
Re: [PATCH v2] media: i2c: thp7312: use fwnode_for_each_child_node()
Posted by Laurent Pinchart 9 months ago 
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

-- 
Regards,

Laurent Pinchart
Re: [PATCH v2] media: i2c: thp7312: use fwnode_for_each_child_node()
Posted by Sakari Ailus 8 months, 1 week ago 
Hi Laurent, Matti,

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 discussed this with Rafael and he didn't recall why the original
implementation was like that. The general direction later on has been not
to present unavailable nodes over the fwnode interface.

So I'd say:

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

We should also change the documentation of the fwnode API accordingly.

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

-- 
Regards,

Sakari Ailus
Re: [PATCH v2] media: i2c: thp7312: use fwnode_for_each_child_node()
Posted by Laurent Pinchart 8 months, 1 week ago 
Hi Sakari,

On Tue, Apr 08, 2025 at 08:48:45AM +0000, Sakari Ailus wrote:
> On Fri, Mar 21, 2025 at 12:41:00PM +0200, Laurent Pinchart wrote:
> > 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 discussed this with Rafael and he didn't recall why the original
> implementation was like that. The general direction later on has been not
> to present unavailable nodes over the fwnode interface.
> 
> So I'd say:
> 
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> We should also change the documentation of the fwnode API accordingly.

Does that also mean that the fwnode_for_each_available_child_node()
function will be dropped ? It's used by few drivers (5 in addition to
the thp7312 driver, plus 3 call sites in drivers/base/core.c), so a
patch series to drop it should be easy.

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

-- 
Regards,

Laurent Pinchart
Re: [PATCH v2] media: i2c: thp7312: use fwnode_for_each_child_node()
Posted by Matti Vaittinen 8 months, 1 week ago 
On 08/04/2025 13:12, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tue, Apr 08, 2025 at 08:48:45AM +0000, Sakari Ailus wrote:
>> On Fri, Mar 21, 2025 at 12:41:00PM +0200, Laurent Pinchart wrote:
>>> 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 discussed this with Rafael and he didn't recall why the original
>> implementation was like that. The general direction later on has been not
>> to present unavailable nodes over the fwnode interface.
>>
>> So I'd say:
>>
>> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>
>> We should also change the documentation of the fwnode API accordingly.
> 
> Does that also mean that the fwnode_for_each_available_child_node()
> function will be dropped ? It's used by few drivers (5 in addition to
> the thp7312 driver, plus 3 call sites in drivers/base/core.c), so a
> patch series to drop it should be easy.
> 

I assume the fwnode_for_each_available_child_node() still makes sense 
for ACPI backed users, no?

Yours,
	-- Matti
Re: [PATCH v2] media: i2c: thp7312: use fwnode_for_each_child_node()
Posted by Sakari Ailus 8 months, 1 week ago 
Hei Laurent, Matti,

On Tue, Apr 08, 2025 at 01:26:42PM +0300, Matti Vaittinen wrote:
> On 08/04/2025 13:12, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > On Tue, Apr 08, 2025 at 08:48:45AM +0000, Sakari Ailus wrote:
> > > On Fri, Mar 21, 2025 at 12:41:00PM +0200, Laurent Pinchart wrote:
> > > > 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 discussed this with Rafael and he didn't recall why the original
> > > implementation was like that. The general direction later on has been not
> > > to present unavailable nodes over the fwnode interface.
> > > 
> > > So I'd say:
> > > 
> > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > 
> > > We should also change the documentation of the fwnode API accordingly.
> > 
> > Does that also mean that the fwnode_for_each_available_child_node()
> > function will be dropped ? It's used by few drivers (5 in addition to
> > the thp7312 driver, plus 3 call sites in drivers/base/core.c), so a
> > patch series to drop it should be easy.
> > 
> 
> I assume the fwnode_for_each_available_child_node() still makes sense for
> ACPI backed users, no?

Not really (see my earlier explanation in
<Z9mQPJwnKAkPHriT@kekkonen.localdomain>). I think all the *available* stuff
should be removed from include/linux/property.h, apart from
fwnode_device_is_availble(), which should be turned to work on struct
device to signal its availability for device nodes only.

-- 
Terveisin,

Sakari Ailus
Re: [PATCH v2] media: i2c: thp7312: use fwnode_for_each_child_node()
Posted by Matti Vaittinen 8 months, 1 week ago 
On 08/04/2025 13:36, Sakari Ailus wrote:
> Hei Laurent, Matti,
> 
> On Tue, Apr 08, 2025 at 01:26:42PM +0300, Matti Vaittinen wrote:
>> On 08/04/2025 13:12, Laurent Pinchart wrote:
>>> Hi Sakari,
>>>
>>> On Tue, Apr 08, 2025 at 08:48:45AM +0000, Sakari Ailus wrote:
>>>> On Fri, Mar 21, 2025 at 12:41:00PM +0200, Laurent Pinchart wrote:
>>>>> 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 discussed this with Rafael and he didn't recall why the original
>>>> implementation was like that. The general direction later on has been not
>>>> to present unavailable nodes over the fwnode interface.
>>>>
>>>> So I'd say:
>>>>
>>>> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>
>>>> We should also change the documentation of the fwnode API accordingly.
>>>
>>> Does that also mean that the fwnode_for_each_available_child_node()
>>> function will be dropped ? It's used by few drivers (5 in addition to
>>> the thp7312 driver, plus 3 call sites in drivers/base/core.c), so a
>>> patch series to drop it should be easy.
>>>
>>
>> I assume the fwnode_for_each_available_child_node() still makes sense for
>> ACPI backed users, no?
> 
> Not really (see my earlier explanation in
> <Z9mQPJwnKAkPHriT@kekkonen.localdomain>).

I capture that the _named_ available nodes don't have value as ACPI 
names aren't really what is expected by the _named_ callers. What I 
didn't pick is that the fwnode_for_each_available_child_node() - which 
should iterate all available child nodes ignoring the name - wouldn't be 
useful.

> I think all the *available* stuff
> should be removed from include/linux/property.h, apart from
> fwnode_device_is_availble(), which should be turned to work on struct
> device to signal its availability for device nodes only.

I am not saying I have any understanding of the uses of the 
'unavailable' nodes. As such I am not arguing over what you say here :)

Yours,
	-- Matti
Re: [PATCH v2] media: i2c: thp7312: use fwnode_for_each_child_node()
Posted by Sakari Ailus 8 months, 1 week ago 
Moi,

On Tue, Apr 08, 2025 at 01:42:12PM +0300, Matti Vaittinen wrote:
> On 08/04/2025 13:36, Sakari Ailus wrote:
> > Hei Laurent, Matti,
> > 
> > On Tue, Apr 08, 2025 at 01:26:42PM +0300, Matti Vaittinen wrote:
> > > On 08/04/2025 13:12, Laurent Pinchart wrote:
> > > > Hi Sakari,
> > > > 
> > > > On Tue, Apr 08, 2025 at 08:48:45AM +0000, Sakari Ailus wrote:
> > > > > On Fri, Mar 21, 2025 at 12:41:00PM +0200, Laurent Pinchart wrote:
> > > > > > 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 discussed this with Rafael and he didn't recall why the original
> > > > > implementation was like that. The general direction later on has been not
> > > > > to present unavailable nodes over the fwnode interface.
> > > > > 
> > > > > So I'd say:
> > > > > 
> > > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > 
> > > > > We should also change the documentation of the fwnode API accordingly.
> > > > 
> > > > Does that also mean that the fwnode_for_each_available_child_node()
> > > > function will be dropped ? It's used by few drivers (5 in addition to
> > > > the thp7312 driver, plus 3 call sites in drivers/base/core.c), so a
> > > > patch series to drop it should be easy.
> > > > 
> > > 
> > > I assume the fwnode_for_each_available_child_node() still makes sense for
> > > ACPI backed users, no?
> > 
> > Not really (see my earlier explanation in
> > <Z9mQPJwnKAkPHriT@kekkonen.localdomain>).
> 
> I capture that the _named_ available nodes don't have value as ACPI names
> aren't really what is expected by the _named_ callers. What I didn't pick is
> that the fwnode_for_each_available_child_node() - which should iterate all
> available child nodes ignoring the name - wouldn't be useful.

Fair enough. I don't think we need to support enumerating unavailable ACPI
device nodes in this API. I'd indeed change the behaviour so that only
available nodes are enumerated. I can post a patch for that.

> 
> > I think all the *available* stuff
> > should be removed from include/linux/property.h, apart from
> > fwnode_device_is_availble(), which should be turned to work on struct
> > device to signal its availability for device nodes only.
> 
> I am not saying I have any understanding of the uses of the 'unavailable'
> nodes. As such I am not arguing over what you say here :)

-- 
Kind regards,

Sakari Ailus
Re: [PATCH v2] media: i2c: thp7312: use fwnode_for_each_child_node()
Posted by Laurent Pinchart 8 months, 1 week ago 
On Tue, Apr 08, 2025 at 11:08:10AM +0000, Sakari Ailus wrote:
> Moi,
> 
> On Tue, Apr 08, 2025 at 01:42:12PM +0300, Matti Vaittinen wrote:
> > On 08/04/2025 13:36, Sakari Ailus wrote:
> > > Hei Laurent, Matti,
> > > 
> > > On Tue, Apr 08, 2025 at 01:26:42PM +0300, Matti Vaittinen wrote:
> > > > On 08/04/2025 13:12, Laurent Pinchart wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > On Tue, Apr 08, 2025 at 08:48:45AM +0000, Sakari Ailus wrote:
> > > > > > On Fri, Mar 21, 2025 at 12:41:00PM +0200, Laurent Pinchart wrote:
> > > > > > > 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 discussed this with Rafael and he didn't recall why the original
> > > > > > implementation was like that. The general direction later on has been not
> > > > > > to present unavailable nodes over the fwnode interface.
> > > > > > 
> > > > > > So I'd say:
> > > > > > 
> > > > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > 
> > > > > > We should also change the documentation of the fwnode API accordingly.
> > > > > 
> > > > > Does that also mean that the fwnode_for_each_available_child_node()
> > > > > function will be dropped ? It's used by few drivers (5 in addition to
> > > > > the thp7312 driver, plus 3 call sites in drivers/base/core.c), so a
> > > > > patch series to drop it should be easy.
> > > > > 
> > > > 
> > > > I assume the fwnode_for_each_available_child_node() still makes sense for
> > > > ACPI backed users, no?
> > > 
> > > Not really (see my earlier explanation in
> > > <Z9mQPJwnKAkPHriT@kekkonen.localdomain>).
> > 
> > I capture that the _named_ available nodes don't have value as ACPI names
> > aren't really what is expected by the _named_ callers. What I didn't pick is
> > that the fwnode_for_each_available_child_node() - which should iterate all
> > available child nodes ignoring the name - wouldn't be useful.
> 
> Fair enough. I don't think we need to support enumerating unavailable ACPI
> device nodes in this API. I'd indeed change the behaviour so that only
> available nodes are enumerated. I can post a patch for that.

Unless there's a specific reason against it that I wouldn't be aware of,
I would also very much favour merging the fwnode_for_each_child_node()
and fwnode_for_each_available_child_node() functions into a single one
that only enumerates available nodes, with a consistent behaviour across
all backend. Having fwnode_for_each_child_node() return unavailable ACPI
nodes but not unavailable DT nodes would be really confusing.

> > > I think all the *available* stuff
> > > should be removed from include/linux/property.h, apart from
> > > fwnode_device_is_availble(), which should be turned to work on struct
> > > device to signal its availability for device nodes only.
> > 
> > I am not saying I have any understanding of the uses of the 'unavailable'
> > nodes. As such I am not arguing over what you say here :)

-- 
Regards,

Laurent Pinchart
Re: [PATCH v2] media: i2c: thp7312: use fwnode_for_each_child_node()
Posted by Matti Vaittinen 8 months, 1 week ago 
On 08/04/2025 14:16, Laurent Pinchart wrote:
> On Tue, Apr 08, 2025 at 11:08:10AM +0000, Sakari Ailus wrote:
>> Moi,
>>
>> On Tue, Apr 08, 2025 at 01:42:12PM +0300, Matti Vaittinen wrote:
>>> On 08/04/2025 13:36, Sakari Ailus wrote:
>>>> Hei Laurent, Matti,
>>>>
>>>> On Tue, Apr 08, 2025 at 01:26:42PM +0300, Matti Vaittinen wrote:
>>>>> On 08/04/2025 13:12, Laurent Pinchart wrote:
>>>>>> Hi Sakari,
>>>>>>
>>>>>> On Tue, Apr 08, 2025 at 08:48:45AM +0000, Sakari Ailus wrote:
>>>>>>> On Fri, Mar 21, 2025 at 12:41:00PM +0200, Laurent Pinchart wrote:
>>>>>>>> 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 discussed this with Rafael and he didn't recall why the original
>>>>>>> implementation was like that. The general direction later on has been not
>>>>>>> to present unavailable nodes over the fwnode interface.
>>>>>>>
>>>>>>> So I'd say:
>>>>>>>
>>>>>>> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>>>
>>>>>>> We should also change the documentation of the fwnode API accordingly.
>>>>>>
>>>>>> Does that also mean that the fwnode_for_each_available_child_node()
>>>>>> function will be dropped ? It's used by few drivers (5 in addition to
>>>>>> the thp7312 driver, plus 3 call sites in drivers/base/core.c), so a
>>>>>> patch series to drop it should be easy.
>>>>>>
>>>>>
>>>>> I assume the fwnode_for_each_available_child_node() still makes sense for
>>>>> ACPI backed users, no?
>>>>
>>>> Not really (see my earlier explanation in
>>>> <Z9mQPJwnKAkPHriT@kekkonen.localdomain>).
>>>
>>> I capture that the _named_ available nodes don't have value as ACPI names
>>> aren't really what is expected by the _named_ callers. What I didn't pick is
>>> that the fwnode_for_each_available_child_node() - which should iterate all
>>> available child nodes ignoring the name - wouldn't be useful.
>>
>> Fair enough. I don't think we need to support enumerating unavailable ACPI
>> device nodes in this API. I'd indeed change the behaviour so that only
>> available nodes are enumerated. I can post a patch for that.
> 
> Unless there's a specific reason against it that I wouldn't be aware of,
> I would also very much favour merging the fwnode_for_each_child_node()
> and fwnode_for_each_available_child_node() functions into a single one
> that only enumerates available nodes, with a consistent behaviour across
> all backend. Having fwnode_for_each_child_node() return unavailable ACPI
> nodes but not unavailable DT nodes would be really confusing.

Absolutely no objections.

Yours,
	-- Matti
Re: [PATCH v2] media: i2c: thp7312: use fwnode_for_each_child_node()
Posted by Sakari Ailus 8 months, 3 weeks ago 
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
> 

-- 
Kind regards,

Sakari Ailus

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.