[PATCH 0/4] use device_for_each_child_node_scoped to access device child nodes

[PATCH 0/4] use device_for_each_child_node_scoped to access device child nodes

[Javier Carrasco]

This series removes accesses to the device `fwnode` to iterate over its
own child nodes. Using the `device_for_each_child_node` macro provides
direct access to the device child nodes, and given that in all cases
they are only required within the loop, the scoped variant of the macro
can be used.

It has been stated in previous discussions [1] that `device_for_each_*`
should be used to access device child nodes, removing the need to access
its internal fwnode, and restricting `fwnode_for_each_*` to traversing
subnodes when required.

Note that `device_for_each_*` implies availability, which means that
after this conversion, unavailable nodes will not be accessible within
the loop. The affected drivers does not seem to have any reason to
iterate over unavailable nodes, though. But if someone has a case where
the affected drivers might require accessing unavailable nodes, please
let me know.

Link: https://lore.kernel.org/linux-hwmon/cffb5885-3cbc-480c-ab6d-4a442d1afb8a@gmail.com/ [1]
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (4):
      coresight: cti: use device_* to iterate over device child nodes
      iio: adc: ad7768-1: use device_* to iterate over device child nodes
      iio: adc: xilinx-ams: use device_* to iterate over device child nodes
      leds: as3645a: use device_* to iterate over device child nodes

 drivers/hwtracing/coresight/coresight-cti-platform.c | 10 +++-------
 drivers/iio/adc/ad7768-1.c                           |  5 +----
 drivers/iio/adc/xilinx-ams.c                         |  7 ++-----
 drivers/leds/flash/leds-as3645a.c                    |  8 +++-----
 4 files changed, 9 insertions(+), 21 deletions(-)
---
base-commit: cd19ac2f903276b820f5d0d89de0c896c27036ed
change-id: 20240725-device_child_node_access-442533910a6f

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 1/4] coresight: cti: use device_* to iterate over device child nodes

[Javier Carrasco]

Drop the manual access to the fwnode of the device to iterate over its
child nodes. `device_for_each_child_node` macro provides direct access
to the child nodes, and given that they are only required within the
loop, the scoped variant of the macro can be used.

Use the `device_for_each_child_node_scoped` macro to iterate over the
direct child nodes of the device.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwtracing/coresight/coresight-cti-platform.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/drivers/hwtracing/coresight/coresight-cti-platform.c
index ccef04f27f12..d0ae10bf6128 100644
--- a/drivers/hwtracing/coresight/coresight-cti-platform.c
+++ b/drivers/hwtracing/coresight/coresight-cti-platform.c
@@ -416,20 +416,16 @@ static int cti_plat_create_impdef_connections(struct device *dev,
 					      struct cti_drvdata *drvdata)
 {
 	int rc = 0;
-	struct fwnode_handle *fwnode = dev_fwnode(dev);
-	struct fwnode_handle *child = NULL;
 
-	if (IS_ERR_OR_NULL(fwnode))
+	if (IS_ERR_OR_NULL(dev_fwnode(dev)))
 		return -EINVAL;
 
-	fwnode_for_each_child_node(fwnode, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		if (cti_plat_node_name_eq(child, CTI_DT_CONNS))
-			rc = cti_plat_create_connection(dev, drvdata,
-							child);
+			rc = cti_plat_create_connection(dev, drvdata, child);
 		if (rc != 0)
 			break;
 	}
-	fwnode_handle_put(child);
 
 	return rc;
 }

-- 
2.43.0

Re: [PATCH 1/4] coresight: cti: use device_* to iterate over device child nodes

[Suzuki K Poulose]

On 01/08/2024 07:13, Javier Carrasco wrote:
> Drop the manual access to the fwnode of the device to iterate over its
> child nodes. `device_for_each_child_node` macro provides direct access
> to the child nodes, and given that they are only required within the
> loop, the scoped variant of the macro can be used.
> 
> Use the `device_for_each_child_node_scoped` macro to iterate over the
> direct child nodes of the device.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>   drivers/hwtracing/coresight/coresight-cti-platform.c | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/drivers/hwtracing/coresight/coresight-cti-platform.c
> index ccef04f27f12..d0ae10bf6128 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-platform.c
> @@ -416,20 +416,16 @@ static int cti_plat_create_impdef_connections(struct device *dev,
>   					      struct cti_drvdata *drvdata)
>   {
>   	int rc = 0;
> -	struct fwnode_handle *fwnode = dev_fwnode(dev);
> -	struct fwnode_handle *child = NULL;
>   
> -	if (IS_ERR_OR_NULL(fwnode))
> +	if (IS_ERR_OR_NULL(dev_fwnode(dev)))
>   		return -EINVAL;
>   
> -	fwnode_for_each_child_node(fwnode, child) {
> +	device_for_each_child_node_scoped(dev, child) {
>   		if (cti_plat_node_name_eq(child, CTI_DT_CONNS))
> -			rc = cti_plat_create_connection(dev, drvdata,
> -							child);
> +			rc = cti_plat_create_connection(dev, drvdata, child);
>   		if (rc != 0)
>   			break;

Don't we need to fwnode_handle_put(child) here, since we removed the
outer one ?

Suzuki

Re: [PATCH 1/4] coresight: cti: use device_* to iterate over device child nodes

[Javier Carrasco]

On 01/08/2024 11:20, Suzuki K Poulose wrote:
> On 01/08/2024 07:13, Javier Carrasco wrote:
>> Drop the manual access to the fwnode of the device to iterate over its
>> child nodes. `device_for_each_child_node` macro provides direct access
>> to the child nodes, and given that they are only required within the
>> loop, the scoped variant of the macro can be used.
>>
>> Use the `device_for_each_child_node_scoped` macro to iterate over the
>> direct child nodes of the device.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-cti-platform.c | 10 +++-------
>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/
>> drivers/hwtracing/coresight/coresight-cti-platform.c
>> index ccef04f27f12..d0ae10bf6128 100644
>> --- a/drivers/hwtracing/coresight/coresight-cti-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-cti-platform.c
>> @@ -416,20 +416,16 @@ static int
>> cti_plat_create_impdef_connections(struct device *dev,
>>                             struct cti_drvdata *drvdata)
>>   {
>>       int rc = 0;
>> -    struct fwnode_handle *fwnode = dev_fwnode(dev);
>> -    struct fwnode_handle *child = NULL;
>>   -    if (IS_ERR_OR_NULL(fwnode))
>> +    if (IS_ERR_OR_NULL(dev_fwnode(dev)))
>>           return -EINVAL;
>>   -    fwnode_for_each_child_node(fwnode, child) {
>> +    device_for_each_child_node_scoped(dev, child) {
>>           if (cti_plat_node_name_eq(child, CTI_DT_CONNS))
>> -            rc = cti_plat_create_connection(dev, drvdata,
>> -                            child);
>> +            rc = cti_plat_create_connection(dev, drvdata, child);
>>           if (rc != 0)
>>               break;
> 
> Don't we need to fwnode_handle_put(child) here, since we removed the
> outer one ?
> 
> Suzuki
> 

Hi Suzuki,

No, we don't need fwnode_handle_put(child) anymore because the scoped
variant of the macro is used.

Best regards,
Javier Carrasco

Re: [PATCH 1/4] coresight: cti: use device_* to iterate over device child nodes

[Suzuki K Poulose]

On 01/08/2024 11:37, Javier Carrasco wrote:
> On 01/08/2024 11:20, Suzuki K Poulose wrote:
>> On 01/08/2024 07:13, Javier Carrasco wrote:
>>> Drop the manual access to the fwnode of the device to iterate over its
>>> child nodes. `device_for_each_child_node` macro provides direct access
>>> to the child nodes, and given that they are only required within the
>>> loop, the scoped variant of the macro can be used.
>>>
>>> Use the `device_for_each_child_node_scoped` macro to iterate over the
>>> direct child nodes of the device.
>>>
>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>>> ---
>>>    drivers/hwtracing/coresight/coresight-cti-platform.c | 10 +++-------
>>>    1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/
>>> drivers/hwtracing/coresight/coresight-cti-platform.c
>>> index ccef04f27f12..d0ae10bf6128 100644
>>> --- a/drivers/hwtracing/coresight/coresight-cti-platform.c
>>> +++ b/drivers/hwtracing/coresight/coresight-cti-platform.c
>>> @@ -416,20 +416,16 @@ static int
>>> cti_plat_create_impdef_connections(struct device *dev,
>>>                              struct cti_drvdata *drvdata)
>>>    {
>>>        int rc = 0;
>>> -    struct fwnode_handle *fwnode = dev_fwnode(dev);
>>> -    struct fwnode_handle *child = NULL;
>>>    -    if (IS_ERR_OR_NULL(fwnode))
>>> +    if (IS_ERR_OR_NULL(dev_fwnode(dev)))
>>>            return -EINVAL;
>>>    -    fwnode_for_each_child_node(fwnode, child) {
>>> +    device_for_each_child_node_scoped(dev, child) {
>>>            if (cti_plat_node_name_eq(child, CTI_DT_CONNS))
>>> -            rc = cti_plat_create_connection(dev, drvdata,
>>> -                            child);
>>> +            rc = cti_plat_create_connection(dev, drvdata, child);
>>>            if (rc != 0)
>>>                break;
>>
>> Don't we need to fwnode_handle_put(child) here, since we removed the
>> outer one ?
>>
>> Suzuki
>>
> 
> Hi Suzuki,
> 
> No, we don't need fwnode_handle_put(child) anymore because the scoped
> variant of the macro is used.

Ah, apologies, was looking at the non-scoped version. I will queue this.

Suzuki
> 
> Best regards,
> Javier Carrasco

[PATCH 2/4] iio: adc: ad7768-1: use device_* to iterate over device child nodes

[Javier Carrasco]

Drop the manual access to the fwnode of the device to iterate over its
child nodes. `device_for_each_child_node` macro provides direct access
to the child nodes, and given that they are only required within the
loop, the scoped variant of the macro can be used.

Use the `device_for_each_child_node_scoped` macro to iterate over the
direct child nodes of the device.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/adc/ad7768-1.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 70a25949142c..721672fe84ab 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -544,13 +544,10 @@ static int ad7768_set_channel_label(struct iio_dev *indio_dev,
 {
 	struct ad7768_state *st = iio_priv(indio_dev);
 	struct device *device = indio_dev->dev.parent;
-	struct fwnode_handle *fwnode;
-	struct fwnode_handle *child;
 	const char *label;
 	int crt_ch = 0;
 
-	fwnode = dev_fwnode(device);
-	fwnode_for_each_child_node(fwnode, child) {
+	device_for_each_child_node_scoped(device, child) {
 		if (fwnode_property_read_u32(child, "reg", &crt_ch))
 			continue;
 

-- 
2.43.0

Re: [PATCH 2/4] iio: adc: ad7768-1: use device_* to iterate over device child nodes

[Nuno Sá]

On Thu, 2024-08-01 at 08:13 +0200, Javier Carrasco wrote:
> Drop the manual access to the fwnode of the device to iterate over its
> child nodes. `device_for_each_child_node` macro provides direct access
> to the child nodes, and given that they are only required within the
> loop, the scoped variant of the macro can be used.
> 
> Use the `device_for_each_child_node_scoped` macro to iterate over the
> direct child nodes of the device.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/adc/ad7768-1.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 70a25949142c..721672fe84ab 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -544,13 +544,10 @@ static int ad7768_set_channel_label(struct iio_dev
> *indio_dev,
>  {
>  	struct ad7768_state *st = iio_priv(indio_dev);
>  	struct device *device = indio_dev->dev.parent;
> -	struct fwnode_handle *fwnode;
> -	struct fwnode_handle *child;
>  	const char *label;
>  	int crt_ch = 0;
>  
> -	fwnode = dev_fwnode(device);
> -	fwnode_for_each_child_node(fwnode, child) {
> +	device_for_each_child_node_scoped(device, child) {
>  		if (fwnode_property_read_u32(child, "reg", &crt_ch))
>  			continue;
>  
> 

Re: [PATCH 2/4] iio: adc: ad7768-1: use device_* to iterate over device child nodes

[Jonathan Cameron]

On Thu, 01 Aug 2024 09:31:16 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Thu, 2024-08-01 at 08:13 +0200, Javier Carrasco wrote:
> > Drop the manual access to the fwnode of the device to iterate over its
> > child nodes. `device_for_each_child_node` macro provides direct access
> > to the child nodes, and given that they are only required within the
> > loop, the scoped variant of the macro can be used.
> > 
> > Use the `device_for_each_child_node_scoped` macro to iterate over the
> > direct child nodes of the device.
> > 
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > ---  
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Applied
> 
> >  drivers/iio/adc/ad7768-1.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > index 70a25949142c..721672fe84ab 100644
> > --- a/drivers/iio/adc/ad7768-1.c
> > +++ b/drivers/iio/adc/ad7768-1.c
> > @@ -544,13 +544,10 @@ static int ad7768_set_channel_label(struct iio_dev
> > *indio_dev,
> >  {
> >  	struct ad7768_state *st = iio_priv(indio_dev);
> >  	struct device *device = indio_dev->dev.parent;
> > -	struct fwnode_handle *fwnode;
> > -	struct fwnode_handle *child;
> >  	const char *label;
> >  	int crt_ch = 0;
> >  
> > -	fwnode = dev_fwnode(device);
> > -	fwnode_for_each_child_node(fwnode, child) {
> > +	device_for_each_child_node_scoped(device, child) {
> >  		if (fwnode_property_read_u32(child, "reg", &crt_ch))
> >  			continue;
> >  
> >   
> 

[PATCH 3/4] iio: adc: xilinx-ams: use device_* to iterate over device child nodes

[Javier Carrasco]

Use `device_for_each_child_node_scoped()` in `ams_parse_firmware()`
to explicitly state device child node access, and simplify the child
node handling as it is not required outside the loop.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/adc/xilinx-ams.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
index f051358d6b50..27d5e4a6f9d6 100644
--- a/drivers/iio/adc/xilinx-ams.c
+++ b/drivers/iio/adc/xilinx-ams.c
@@ -1275,7 +1275,6 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
 	struct ams *ams = iio_priv(indio_dev);
 	struct iio_chan_spec *ams_channels, *dev_channels;
 	struct device *dev = indio_dev->dev.parent;
-	struct fwnode_handle *child = NULL;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	size_t ams_size;
 	int ret, ch_cnt = 0, i, rising_off, falling_off;
@@ -1297,13 +1296,11 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
 		num_channels += ret;
 	}
 
-	fwnode_for_each_child_node(fwnode, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		if (fwnode_device_is_available(child)) {
 			ret = ams_init_module(indio_dev, child, ams_channels + num_channels);
-			if (ret < 0) {
-				fwnode_handle_put(child);
+			if (ret < 0)
 				return ret;
-			}
 
 			num_channels += ret;
 		}

-- 
2.43.0

Re: [PATCH 3/4] iio: adc: xilinx-ams: use device_* to iterate over device child nodes

[Jonathan Cameron]

On Thu, 01 Aug 2024 08:13:52 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> Use `device_for_each_child_node_scoped()` in `ams_parse_firmware()`
> to explicitly state device child node access, and simplify the child
> node handling as it is not required outside the loop.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/iio/adc/xilinx-ams.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> index f051358d6b50..27d5e4a6f9d6 100644
> --- a/drivers/iio/adc/xilinx-ams.c
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -1275,7 +1275,6 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
>  	struct ams *ams = iio_priv(indio_dev);
>  	struct iio_chan_spec *ams_channels, *dev_channels;
>  	struct device *dev = indio_dev->dev.parent;
> -	struct fwnode_handle *child = NULL;
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	size_t ams_size;
>  	int ret, ch_cnt = 0, i, rising_off, falling_off;
> @@ -1297,13 +1296,11 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
>  		num_channels += ret;
>  	}
>  
> -	fwnode_for_each_child_node(fwnode, child) {
> +	device_for_each_child_node_scoped(dev, child) {
>  		if (fwnode_device_is_available(child)) {
This is fun.  should have been fwnode_for_each_available_child_node()
in the first place rather than iterating over all nodes, and only
doing stuff for the available ones.

Can use the device_for_each_child_node_scoped() but that only includes
available nodes anyway, so my understanding is that we can also drop
this if statement as it will always be true.


>  			ret = ams_init_module(indio_dev, child, ams_channels + num_channels);
> -			if (ret < 0) {
> -				fwnode_handle_put(child);
> +			if (ret < 0)
>  				return ret;
> -			}
>  
>  			num_channels += ret;
>  		}
> 

[PATCH 4/4] leds: as3645a: use device_* to iterate over device child nodes

[Javier Carrasco]

Drop the manual access to the fwnode of the device to iterate over its
child nodes. `device_for_each_child_node` macro provides direct access
to the child nodes, and given that the `child` variable is only required
within the loop, the scoped variant of the macro can be used.

Use the `device_for_each_child_node_scoped` macro to iterate over the
direct child nodes of the device.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/flash/leds-as3645a.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/flash/leds-as3645a.c b/drivers/leds/flash/leds-as3645a.c
index 2c6ef321b7c8..8e6abedf6e00 100644
--- a/drivers/leds/flash/leds-as3645a.c
+++ b/drivers/leds/flash/leds-as3645a.c
@@ -478,14 +478,12 @@ static int as3645a_detect(struct as3645a *flash)
 	return as3645a_write(flash, AS_BOOST_REG, AS_BOOST_CURRENT_DISABLE);
 }
 
-static int as3645a_parse_node(struct as3645a *flash,
-			      struct fwnode_handle *fwnode)
+static int as3645a_parse_node(struct as3645a *flash, struct device *dev)
 {
 	struct as3645a_config *cfg = &flash->cfg;
-	struct fwnode_handle *child;
 	int rval;
 
-	fwnode_for_each_child_node(fwnode, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		u32 id = 0;
 
 		fwnode_property_read_u32(child, "reg", &id);
@@ -686,7 +684,7 @@ static int as3645a_probe(struct i2c_client *client)
 
 	flash->client = client;
 
-	rval = as3645a_parse_node(flash, dev_fwnode(&client->dev));
+	rval = as3645a_parse_node(flash, &client->dev);
 	if (rval < 0)
 		return rval;
 

-- 
2.43.0

Re: [PATCH 4/4] leds: as3645a: use device_* to iterate over device child nodes

[Sakari Ailus]

On Thu, Aug 01, 2024 at 08:13:53AM +0200, Javier Carrasco wrote:
> Drop the manual access to the fwnode of the device to iterate over its
> child nodes. `device_for_each_child_node` macro provides direct access
> to the child nodes, and given that the `child` variable is only required
> within the loop, the scoped variant of the macro can be used.
> 
> Use the `device_for_each_child_node_scoped` macro to iterate over the
> direct child nodes of the device.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Thanks!

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

-- 
Sakari Ailus