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

[PATCH v2 0/3] 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>
---
Changes in v2:
- Rebase onto next-20240808, drop upstreamed patches (changes for ad7768-1)
- xilinx-ams.c: drop fwnode_device_is_available(child) (implicit in the
  loop).
- Link to v1: https://lore.kernel.org/r/20240801-device_child_node_access-v1-0-ddfa21bef6f2@gmail.com

---
Javier Carrasco (3):
      coresight: cti: 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/xilinx-ams.c                         | 15 +++++----------
 drivers/leds/flash/leds-as3645a.c                    |  8 +++-----
 3 files changed, 11 insertions(+), 22 deletions(-)
---
base-commit: 222a3380f92b8791d4eeedf7cd750513ff428adf
change-id: 20240725-device_child_node_access-442533910a6f

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

[PATCH v2 1/3] 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 v2 1/3] coresight: cti: use device_* to iterate over device child nodes

[Mike Leach]

On Thu, 8 Aug 2024 at 16:12, Javier Carrasco
<javier.carrasco.cruz@gmail.com> 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;
>         }
> -       fwnode_handle_put(child);
>
>         return rc;
>  }
>
> --
> 2.43.0
>

Reviewed-by: Mike Leach <mike.leach@linaro.org>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

[PATCH v2 2/3] 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 | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
index f051358d6b50..ebc583b07e0c 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,16 +1296,12 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
 		num_channels += ret;
 	}
 
-	fwnode_for_each_child_node(fwnode, 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);
-				return ret;
-			}
+	device_for_each_child_node_scoped(dev, child) {
+		ret = ams_init_module(indio_dev, child, ams_channels + num_channels);
+		if (ret < 0)
+			return ret;
 
-			num_channels += ret;
-		}
+		num_channels += ret;
 	}
 
 	for (i = 0; i < num_channels; i++) {

-- 
2.43.0

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

[Jonathan Cameron]

On Thu, 08 Aug 2024 17:12:38 +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>

One follow up question for those familiar with the driver.
Why do we check if the main device is available?
	if (fwnode_device_is_available(fwnode)) {
		ret = ams_init_module(indio_dev, fwnode, ams_channels);
		if (ret < 0)
			return ret;

		num_channels += ret;
	}


How is the platform device driver probed if it's not available?

Jonathan


> ---
>  drivers/iio/adc/xilinx-ams.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> index f051358d6b50..ebc583b07e0c 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,16 +1296,12 @@ static int ams_parse_firmware(struct iio_dev *indio_dev)
>  		num_channels += ret;
>  	}
>  
> -	fwnode_for_each_child_node(fwnode, 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);
> -				return ret;
> -			}
> +	device_for_each_child_node_scoped(dev, child) {
> +		ret = ams_init_module(indio_dev, child, ams_channels + num_channels);
> +		if (ret < 0)
> +			return ret;
>  
> -			num_channels += ret;
> -		}
> +		num_channels += ret;
>  	}
>  
>  	for (i = 0; i < num_channels; i++) {
> 

[PATCH v2 3/3] 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.

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
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 v2 3/3] leds: as3645a: use device_* to iterate over device child nodes

[Lee Jones]

On Thu, 08 Aug 2024, 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.
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 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)

Please swap the parameters to have the more senior one (dev) at the start.

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

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 0/3] use device_for_each_child_node_scoped to access device child nodes

[Suzuki K Poulose]

On Thu, 08 Aug 2024 17:12:36 +0200, Javier Carrasco wrote:
> 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.
> 
> [...]

Applied, to coresight next tree. Thanks!

[1/3] coresight: cti: use device_* to iterate over device child nodes
      https://git.kernel.org/coresight/c/daca644d0c9e0

Best regards,
-- 
Suzuki K Poulose <suzuki.poulose@arm.com>