[PATCH 0/8] IIO: More use of device_for_each_child_node_scoped() and __free()

[PATCH 0/8] IIO: More use of device_for_each_child_node_scoped() and __free()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Mixture of new code that entered in parallel with the original conversion
set to use this new scoped cleanup and cases I missed.  Note the
relevant code didn't make the 6.9 merge window but is queued up for
6.10 in the togreg branch of iio.git and linux-next.

Includes allowing easier building of the ab8500-gpadc driver to enable
testing the changes. Seems a sensible change to make in general.

This series is making us of the automated cleanup introduced in
linux/cleanup.h and new device_for_each_child_node_scoped() to avoid
the need to remember to call fwnode_handle_put() in early exits from
loops over the child nodes. This can also be used for simple scope
based cleanup as seen in the ad3552 patch. In general this makes it
easier to avoid a common class of bug.

Jonathan Cameron (8):
  iio: adc: ab8500-gpadc: Allow COMPILE_TEST builds
  iio: adc: ab8500-gpadc: Fix kernel-doc parameter names.
  iio: adc: ab8500-gpadc: Use device_for_each_child_node_scoped() to
    simplify erorr paths.
  iio: adc: ad4130: Use device_for_each_child_node_scoped() to simplify
    error paths.
  iio: adc: ad7173: Use device_for_each_child_node_scoped() to simplify
    error paths.
  iio: frequency: admfm2000: Use device_for_each_child_node_scoped() to
    simplify error paths.
  iio: dac: ad3552: Use __free(fwnode_handle) to simplify error
    handling.
  iio: adc: pac1934: Use device_for_each_available_child_node_scoped()
    to simplify error handling.

 drivers/iio/adc/Kconfig           |  2 +-
 drivers/iio/adc/ab8500-gpadc.c    |  8 ++--
 drivers/iio/adc/ad4130.c          |  7 +--
 drivers/iio/adc/ad7173.c          | 24 +++-------
 drivers/iio/adc/pac1934.c         | 77 +++++++++++++------------------
 drivers/iio/dac/ad3552r.c         | 59 +++++++++--------------
 drivers/iio/frequency/admfm2000.c | 24 +++-------
 7 files changed, 73 insertions(+), 128 deletions(-)

-- 
2.44.0

[PATCH 1/8] iio: adc: ab8500-gpadc: Allow COMPILE_TEST builds

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The dependencies on various ab8500 components prevent this driver
being useful but they don't seem to prevent it being built.
Improve build coverage by allowing COMPILE_TEST.

Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..e648198b0a0a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -8,7 +8,7 @@ menu "Analog to digital converters"
 
 config AB8500_GPADC
 	bool "ST-Ericsson AB8500 GPADC driver"
-	depends on AB8500_CORE && REGULATOR_AB8500
+	depends on (AB8500_CORE && REGULATOR_AB8500) || COMPILE_TEST
 	default y
 	help
 	  AB8500 Analog Baseband, mixed signal integrated circuit GPADC
-- 
2.44.0

Re: [PATCH 1/8] iio: adc: ab8500-gpadc: Allow COMPILE_TEST builds

[Linus Walleij]

On Sat, Mar 30, 2024 at 8:09 PM Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The dependencies on various ab8500 components prevent this driver
> being useful but they don't seem to prevent it being built.
> Improve build coverage by allowing COMPILE_TEST.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

...but I think AB8500_CORE is a hard requirement because
<linux/mfd/abx500.h> does not provide register accessor stubs?

Yours,
Linus Walleij

Re: [PATCH 1/8] iio: adc: ab8500-gpadc: Allow COMPILE_TEST builds

[Jonathan Cameron]

On Thu, 4 Apr 2024 13:36:15 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Sat, Mar 30, 2024 at 8:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > The dependencies on various ab8500 components prevent this driver
> > being useful but they don't seem to prevent it being built.
> > Improve build coverage by allowing COMPILE_TEST.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> ...but I think AB8500_CORE is a hard requirement because
> <linux/mfd/abx500.h> does not provide register accessor stubs?
> 
hmm. I clearly didn't test enough.  Ah well, I'll drop this one.
> Yours,
> Linus Walleij

Re: [PATCH 1/8] iio: adc: ab8500-gpadc: Allow COMPILE_TEST builds

[Jonathan Cameron]

On Fri, 5 Apr 2024 11:36:22 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Thu, 4 Apr 2024 13:36:15 +0200
> Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > On Sat, Mar 30, 2024 at 8:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > The dependencies on various ab8500 components prevent this driver
> > > being useful but they don't seem to prevent it being built.
> > > Improve build coverage by allowing COMPILE_TEST.
> > >
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>    
> > 
> > ...but I think AB8500_CORE is a hard requirement because
> > <linux/mfd/abx500.h> does not provide register accessor stubs?
> >   
> hmm. I clearly didn't test enough.  Ah well, I'll drop this one.
> > Yours,
> > Linus Walleij  

Tested again, nope, AB8500_CORE isn't needed as far as I can tell.
What register accessor stubs were you referring to? There are some calls for debug dumps
in that header, but those aren't used by the ADC driver.

Jonathan


> 

Re: [PATCH 1/8] iio: adc: ab8500-gpadc: Allow COMPILE_TEST builds

[Linus Walleij]

On Sat, Apr 6, 2024 at 12:27 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Fri, 5 Apr 2024 11:36:22 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>
> > On Thu, 4 Apr 2024 13:36:15 +0200
> > Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > > On Sat, Mar 30, 2024 at 8:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >
> > > > The dependencies on various ab8500 components prevent this driver
> > > > being useful but they don't seem to prevent it being built.
> > > > Improve build coverage by allowing COMPILE_TEST.
> > > >
> > > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > ...but I think AB8500_CORE is a hard requirement because
> > > <linux/mfd/abx500.h> does not provide register accessor stubs?
> > >
> > hmm. I clearly didn't test enough.  Ah well, I'll drop this one.
> > > Yours,
> > > Linus Walleij
>
> Tested again, nope, AB8500_CORE isn't needed as far as I can tell.

Oh? Weird.

> What register accessor stubs were you referring to? There are some calls for debug dumps
> in that header, but those aren't used by the ADC driver.

<linux/mfd/abx500.h> has for example:
(...)
int abx500_get_register_interruptible(struct device *dev, u8 bank, u8 reg,
        u8 *value);
(...)

and the ADC driver has:

        /* Check if ADC is not busy, lock and proceed */
        do {
                ret = abx500_get_register_interruptible(gpadc->dev,
                        AB8500_GPADC, AB8500_GPADC_STAT_REG, &val);


$ git grep abx500_get_register_interruptible include/linux/
include/linux/mfd/abx500.h:int
abx500_get_register_interruptible(struct device *dev, u8 bank, u8 reg,

No stubs! I think there is just some other Kconfig stuff bringing the
AB8500 MFD core into
the build at any attempt.

Yours,
Linus Walleij

Re: [PATCH 1/8] iio: adc: ab8500-gpadc: Allow COMPILE_TEST builds

[Jonathan Cameron]

On Mon, 8 Apr 2024 11:16:17 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Sat, Apr 6, 2024 at 12:27 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Fri, 5 Apr 2024 11:36:22 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >  
> > > On Thu, 4 Apr 2024 13:36:15 +0200
> > > Linus Walleij <linus.walleij@linaro.org> wrote:
> > >  
> > > > On Sat, Mar 30, 2024 at 8:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > >  
> > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > >
> > > > > The dependencies on various ab8500 components prevent this driver
> > > > > being useful but they don't seem to prevent it being built.
> > > > > Improve build coverage by allowing COMPILE_TEST.
> > > > >
> > > > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> > > >
> > > > ...but I think AB8500_CORE is a hard requirement because
> > > > <linux/mfd/abx500.h> does not provide register accessor stubs?
> > > >  
> > > hmm. I clearly didn't test enough.  Ah well, I'll drop this one.  
> > > > Yours,
> > > > Linus Walleij  
> >
> > Tested again, nope, AB8500_CORE isn't needed as far as I can tell.  
> 
> Oh? Weird.

> 
> > What register accessor stubs were you referring to? There are some calls for debug dumps
> > in that header, but those aren't used by the ADC driver.  
> 
> <linux/mfd/abx500.h> has for example:
> (...)
> int abx500_get_register_interruptible(struct device *dev, u8 bank, u8 reg,
>         u8 *value);
> (...)
> 
> and the ADC driver has:
> 
>         /* Check if ADC is not busy, lock and proceed */
>         do {
>                 ret = abx500_get_register_interruptible(gpadc->dev,
>                         AB8500_GPADC, AB8500_GPADC_STAT_REG, &val);
> 
> 
> $ git grep abx500_get_register_interruptible include/linux/
> include/linux/mfd/abx500.h:int
> abx500_get_register_interruptible(struct device *dev, u8 bank, u8 reg,
> 
> No stubs! I think there is just some other Kconfig stuff bringing the
> AB8500 MFD core into
> the build at any attempt.

ah got it.  AB8500_CORE isn't needed, but ABX500_CORE is and that's indirectly
depended on by AB8500_CORE.  How about I change the dependency to
depends on ABX500_CORE && (AB8500_REGULATOR || COMPILE_TEST) instead?

As ABX500_CORE doesn't have a horrible indirect arch dependency that
AB8500_CORE does via db8500-prcmu.  I can build it on x86 but obviously
would want to hit this with a lot more build tests.

DB8500_PRCMU has some stubs but not a complete enough set to allow
AB5800_CORE to build.  Could fix that but it's a bigger change than
relaxing requirements of the ADC driver to the ones needed for building.

Jonathan


> 
> Yours,
> Linus Walleij

Re: [PATCH 1/8] iio: adc: ab8500-gpadc: Allow COMPILE_TEST builds

[Linus Walleij]

On Sat, Apr 13, 2024 at 11:57 AM Jonathan Cameron <jic23@kernel.org> wrote:

> > $ git grep abx500_get_register_interruptible include/linux/
> > include/linux/mfd/abx500.h:int
> > abx500_get_register_interruptible(struct device *dev, u8 bank, u8 reg,
> >
> > No stubs! I think there is just some other Kconfig stuff bringing the
> > AB8500 MFD core into
> > the build at any attempt.
>
> ah got it.  AB8500_CORE isn't needed, but ABX500_CORE is and that's indirectly
> depended on by AB8500_CORE.  How about I change the dependency to
> depends on ABX500_CORE && (AB8500_REGULATOR || COMPILE_TEST) instead?

That looks like it will work!

> As ABX500_CORE doesn't have a horrible indirect arch dependency that
> AB8500_CORE does via db8500-prcmu.  I can build it on x86 but obviously
> would want to hit this with a lot more build tests.

Actually I want to drop the ABX500 indirection layer because that was
as the name says intended as an abstraction across a multitude of
ABX500 chips whereof AB8500 would be the first, however the others
never materialized.

But that's for another day, this works right now.

Yours,
Linus Walleij

[PATCH 2/8] iio: adc: ab8500-gpadc: Fix kernel-doc parameter names.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Seems these got renamed at somepoint but the documentation wasn't
updated to match.

Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ab8500-gpadc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ab8500-gpadc.c b/drivers/iio/adc/ab8500-gpadc.c
index 80645fee79a4..0bc1550c7f11 100644
--- a/drivers/iio/adc/ab8500-gpadc.c
+++ b/drivers/iio/adc/ab8500-gpadc.c
@@ -1021,8 +1021,8 @@ static int ab8500_gpadc_parse_channel(struct device *dev,
 /**
  * ab8500_gpadc_parse_channels() - Parse the GPADC channels from DT
  * @gpadc: the GPADC to configure the channels for
- * @chans: the IIO channels we parsed
- * @nchans: the number of IIO channels we parsed
+ * @chans_parsed: the IIO channels we parsed
+ * @nchans_parsed: the number of IIO channels we parsed
  */
 static int ab8500_gpadc_parse_channels(struct ab8500_gpadc *gpadc,
 				       struct iio_chan_spec **chans_parsed,
-- 
2.44.0

Re: [PATCH 2/8] iio: adc: ab8500-gpadc: Fix kernel-doc parameter names.

[Linus Walleij]

On Sat, Mar 30, 2024 at 8:09 PM Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Seems these got renamed at somepoint but the documentation wasn't
> updated to match.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 3/8] iio: adc: ab8500-gpadc: Use device_for_each_child_node_scoped() to simplify erorr paths.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This new loop definition automatically releases the handle on early exit
reducing the chance of bugs that cause resource leaks.

Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ab8500-gpadc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ab8500-gpadc.c b/drivers/iio/adc/ab8500-gpadc.c
index 0bc1550c7f11..59f66e9cb0e8 100644
--- a/drivers/iio/adc/ab8500-gpadc.c
+++ b/drivers/iio/adc/ab8500-gpadc.c
@@ -1028,7 +1028,6 @@ static int ab8500_gpadc_parse_channels(struct ab8500_gpadc *gpadc,
 				       struct iio_chan_spec **chans_parsed,
 				       unsigned int *nchans_parsed)
 {
-	struct fwnode_handle *child;
 	struct ab8500_gpadc_chan_info *ch;
 	struct iio_chan_spec *iio_chans;
 	unsigned int nchans;
@@ -1052,7 +1051,7 @@ static int ab8500_gpadc_parse_channels(struct ab8500_gpadc *gpadc,
 		return -ENOMEM;
 
 	i = 0;
-	device_for_each_child_node(gpadc->dev, child) {
+	device_for_each_child_node_scoped(gpadc->dev, child) {
 		struct iio_chan_spec *iio_chan;
 		int ret;
 
@@ -1062,7 +1061,6 @@ static int ab8500_gpadc_parse_channels(struct ab8500_gpadc *gpadc,
 		ret = ab8500_gpadc_parse_channel(gpadc->dev, child, ch,
 						 iio_chan);
 		if (ret) {
-			fwnode_handle_put(child);
 			return ret;
 		}
 		i++;
-- 
2.44.0

Re: [PATCH 3/8] iio: adc: ab8500-gpadc: Use device_for_each_child_node_scoped() to simplify erorr paths.

[Linus Walleij]

On Sat, Mar 30, 2024 at 8:09 PM Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> This new loop definition automatically releases the handle on early exit
> reducing the chance of bugs that cause resource leaks.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Neat! I love it.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 4/8] iio: adc: ad4130: Use device_for_each_child_node_scoped() to simplify error paths.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This loop definition removes the need for manual releasing of the fwnode_handle
in early exit paths (here an error path) allow simplfication of the code
and reducing the chance of future modificiations not releasing
fwnode_handle correctly.

Cc: Cosmin Tanislav <demonsingur@gmail.com>
Cc: Nuno Sa <nuno.sa@analog.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ad4130.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
index febb64e67955..aaf1fb0ac447 100644
--- a/drivers/iio/adc/ad4130.c
+++ b/drivers/iio/adc/ad4130.c
@@ -1600,17 +1600,14 @@ static int ad4130_parse_fw_children(struct iio_dev *indio_dev)
 {
 	struct ad4130_state *st = iio_priv(indio_dev);
 	struct device *dev = &st->spi->dev;
-	struct fwnode_handle *child;
 	int ret;
 
 	indio_dev->channels = st->chans;
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		ret = ad4130_parse_fw_channel(indio_dev, child);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
 	}
 
 	return 0;
-- 
2.44.0

[PATCH 5/8] iio: adc: ad7173: Use device_for_each_child_node_scoped() to simplify error paths.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This loop definition automatically releases the fwnode_handle on early
exit such as the error cases seen here.  This reducing boilerplate and
the chance of a resource leak being introduced in future.

Cc: Dumitru Ceclan <mitrutzceclan@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ad7173.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 4ff6ce46b02c..f6d29abe1d04 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -910,7 +910,6 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 	struct device *dev = indio_dev->dev.parent;
 	struct iio_chan_spec *chan_arr, *chan;
 	unsigned int ain[2], chan_index = 0;
-	struct fwnode_handle *child;
 	int ref_sel, ret;
 
 	chan_arr = devm_kcalloc(dev, sizeof(*indio_dev->channels),
@@ -940,23 +939,19 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		chan_index++;
 	}
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		chan = &chan_arr[chan_index];
 		chan_st_priv = &chans_st_arr[chan_index];
 		ret = fwnode_property_read_u32_array(child, "diff-channels",
 						     ain, ARRAY_SIZE(ain));
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
 
 		if (ain[0] >= st->info->num_inputs ||
-		    ain[1] >= st->info->num_inputs) {
-			fwnode_handle_put(child);
+		    ain[1] >= st->info->num_inputs)
 			return dev_err_probe(dev, -EINVAL,
 				"Input pin number out of range for pair (%d %d).\n",
 				ain[0], ain[1]);
-		}
 
 		ret = fwnode_property_match_property_string(child,
 							    "adi,reference-select",
@@ -968,24 +963,19 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 			ref_sel = ret;
 
 		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF &&
-		    !st->info->has_int_ref) {
-			fwnode_handle_put(child);
+		    !st->info->has_int_ref)
 			return dev_err_probe(dev, -EINVAL,
 				"Internal reference is not available on current model.\n");
-		}
 
-		if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2) {
-			fwnode_handle_put(child);
+		if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
 			return dev_err_probe(dev, -EINVAL,
 				"External reference 2 is not available on current model.\n");
-		}
 
 		ret = ad7173_get_ref_voltage_milli(st, ref_sel);
-		if (ret < 0) {
-			fwnode_handle_put(child);
+		if (ret < 0)
 			return dev_err_probe(dev, ret,
 					     "Cannot use reference %u\n", ref_sel);
-		}
+
 		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
 			st->adc_mode |= AD7173_ADC_MODE_REF_EN;
 		chan_st_priv->cfg.ref_sel = ref_sel;
-- 
2.44.0

[PATCH 6/8] iio: frequency: admfm2000: Use device_for_each_child_node_scoped() to simplify error paths.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This loop definition automatically release the fwnode_handle on early exit
simplifying error handling paths.

Cc: Kim Seer Paller <kimseer.paller@analog.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/frequency/admfm2000.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/frequency/admfm2000.c b/drivers/iio/frequency/admfm2000.c
index c34d79e55a7c..b2263b9afeda 100644
--- a/drivers/iio/frequency/admfm2000.c
+++ b/drivers/iio/frequency/admfm2000.c
@@ -160,26 +160,21 @@ static int admfm2000_channel_config(struct admfm2000_state *st,
 {
 	struct platform_device *pdev = to_platform_device(indio_dev->dev.parent);
 	struct device *dev = &pdev->dev;
-	struct fwnode_handle *child;
 	struct gpio_desc **dsa;
 	struct gpio_desc **sw;
 	int ret, i;
 	bool mode;
 	u32 reg;
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		ret = fwnode_property_read_u32(child, "reg", &reg);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return dev_err_probe(dev, ret,
 					     "Failed to get reg property\n");
-		}
 
-		if (reg >= indio_dev->num_channels) {
-			fwnode_handle_put(child);
+		if (reg >= indio_dev->num_channels)
 			return dev_err_probe(dev, -EINVAL, "reg bigger than: %d\n",
 					     indio_dev->num_channels);
-		}
 
 		if (fwnode_property_present(child, "adi,mixer-mode"))
 			mode = ADMFM2000_MIXER_MODE;
@@ -196,36 +191,29 @@ static int admfm2000_channel_config(struct admfm2000_state *st,
 			dsa = st->dsa2_gpios;
 			break;
 		default:
-			fwnode_handle_put(child);
 			return -EINVAL;
 		}
 
 		for (i = 0; i < ADMFM2000_MODE_GPIOS; i++) {
 			sw[i] = devm_fwnode_gpiod_get_index(dev, child, "switch",
 							    i, GPIOD_OUT_LOW, NULL);
-			if (IS_ERR(sw[i])) {
-				fwnode_handle_put(child);
+			if (IS_ERR(sw[i]))
 				return dev_err_probe(dev, PTR_ERR(sw[i]),
 						     "Failed to get gpios\n");
-			}
 		}
 
 		for (i = 0; i < ADMFM2000_DSA_GPIOS; i++) {
 			dsa[i] = devm_fwnode_gpiod_get_index(dev, child,
 							     "attenuation", i,
 							     GPIOD_OUT_LOW, NULL);
-			if (IS_ERR(dsa[i])) {
-				fwnode_handle_put(child);
+			if (IS_ERR(dsa[i]))
 				return dev_err_probe(dev, PTR_ERR(dsa[i]),
 						     "Failed to get gpios\n");
-			}
 		}
 
 		ret = admfm2000_mode(indio_dev, reg, mode);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
 	}
 
 	return 0;
-- 
2.44.0

[PATCH 7/8] iio: dac: ad3552: Use __free(fwnode_handle) to simplify error handling.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

By using this scoped based cleanup direct returns on errors are enabled
simplifying the code.

Cc: Mihail Chindris <mihail.chindris@analog.com>
Cc: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/dac/ad3552r.c | 59 +++++++++++++++------------------------
 1 file changed, 23 insertions(+), 36 deletions(-)

diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c
index e14a065b29ca..8aa942896b5b 100644
--- a/drivers/iio/dac/ad3552r.c
+++ b/drivers/iio/dac/ad3552r.c
@@ -801,51 +801,45 @@ static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
 					 u32 ch)
 {
 	struct device *dev = &dac->spi->dev;
-	struct fwnode_handle *gain_child;
 	u32 val;
 	int err;
 	u8 addr;
 	u16 reg = 0, offset;
 
-	gain_child = fwnode_get_named_child_node(child,
-						 "custom-output-range-config");
-	if (!gain_child) {
-		dev_err(dev,
-			"mandatory custom-output-range-config property missing\n");
-		return -EINVAL;
-	}
+	struct fwnode_handle *gain_child __free(fwnode_handle)
+		= fwnode_get_named_child_node(child,
+					      "custom-output-range-config");
+	if (!gain_child)
+		return dev_err_probe(dev, -EINVAL,
+				     "mandatory custom-output-range-config property missing\n");
 
 	dac->ch_data[ch].range_override = 1;
 	reg |= ad3552r_field_prep(1, AD3552R_MASK_CH_RANGE_OVERRIDE);
 
 	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-p", &val);
-	if (err) {
-		dev_err(dev, "mandatory adi,gain-scaling-p property missing\n");
-		goto put_child;
-	}
+	if (err)
+		return dev_err_probe(dev, err,
+				     "mandatory adi,gain-scaling-p property missing\n");
 	reg |= ad3552r_field_prep(val, AD3552R_MASK_CH_GAIN_SCALING_P);
 	dac->ch_data[ch].p = val;
 
 	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val);
-	if (err) {
-		dev_err(dev, "mandatory adi,gain-scaling-n property missing\n");
-		goto put_child;
-	}
+	if (err)
+		return dev_err_probe(dev, err,
+				     "mandatory adi,gain-scaling-n property missing\n");
 	reg |= ad3552r_field_prep(val, AD3552R_MASK_CH_GAIN_SCALING_N);
 	dac->ch_data[ch].n = val;
 
 	err = fwnode_property_read_u32(gain_child, "adi,rfb-ohms", &val);
-	if (err) {
-		dev_err(dev, "mandatory adi,rfb-ohms property missing\n");
-		goto put_child;
-	}
+	if (err)
+		return dev_err_probe(dev, err,
+				     "mandatory adi,rfb-ohms property missing\n");
 	dac->ch_data[ch].rfb = val;
 
 	err = fwnode_property_read_u32(gain_child, "adi,gain-offset", &val);
-	if (err) {
-		dev_err(dev, "mandatory adi,gain-offset property missing\n");
-		goto put_child;
-	}
+	if (err)
+		return dev_err_probe(dev, err,
+				     "mandatory adi,gain-offset property missing\n");
 	dac->ch_data[ch].gain_offset = val;
 
 	offset = abs((s32)val);
@@ -855,21 +849,14 @@ static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
 	addr = AD3552R_REG_ADDR_CH_GAIN(ch);
 	err = ad3552r_write_reg(dac, addr,
 				offset & AD3552R_MASK_CH_OFFSET_BITS_0_7);
-	if (err) {
-		dev_err(dev, "Error writing register\n");
-		goto put_child;
-	}
+	if (err)
+		return dev_err_probe(dev, err, "Error writing register\n");
 
 	err = ad3552r_write_reg(dac, addr, reg);
-	if (err) {
-		dev_err(dev, "Error writing register\n");
-		goto put_child;
-	}
-
-put_child:
-	fwnode_handle_put(gain_child);
+	if (err)
+		return dev_err_probe(dev, err, "Error writing register\n");
 
-	return err;
+	return 0;
 }
 
 static void ad3552r_reg_disable(void *reg)
-- 
2.44.0

[PATCH 8/8] iio: adc: pac1934: Use device_for_each_available_child_node_scoped() to simplify error handling.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Also change both firmware parsing functions to return meaningful errors.
Whilst for the ACPI case this isn't that useful, for the generic fwnode
case we can return the errors coming from the property parsing instead
of just whether we succeeded or not.

Cc: Marius Cristea <marius.cristea@microchip.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/pac1934.c | 77 ++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 46 deletions(-)

diff --git a/drivers/iio/adc/pac1934.c b/drivers/iio/adc/pac1934.c
index e0c2742da523..f751260605e4 100644
--- a/drivers/iio/adc/pac1934.c
+++ b/drivers/iio/adc/pac1934.c
@@ -1079,8 +1079,8 @@ static int pac1934_chip_identify(struct pac1934_chip_info *info)
  * documentation related to the ACPI device definition
  * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ApplicationNotes/ApplicationNotes/PAC1934-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf
  */
-static bool pac1934_acpi_parse_channel_config(struct i2c_client *client,
-					      struct pac1934_chip_info *info)
+static int pac1934_acpi_parse_channel_config(struct i2c_client *client,
+					     struct pac1934_chip_info *info)
 {
 	acpi_handle handle;
 	union acpi_object *rez;
@@ -1095,7 +1095,7 @@ static bool pac1934_acpi_parse_channel_config(struct i2c_client *client,
 
 	rez = acpi_evaluate_dsm(handle, &guid, 0, PAC1934_ACPI_GET_NAMES_AND_MOHMS_VALS, NULL);
 	if (!rez)
-		return false;
+		return -EINVAL;
 
 	for (i = 0; i < rez->package.count; i += 2) {
 		idx = i / 2;
@@ -1118,7 +1118,7 @@ static bool pac1934_acpi_parse_channel_config(struct i2c_client *client,
 		 * and assign the default sampling rate
 		 */
 		info->sample_rate_value = PAC1934_DEFAULT_CHIP_SAMP_SPEED_HZ;
-		return true;
+		return 0;
 	}
 
 	for (i = 0; i < rez->package.count; i++) {
@@ -1131,7 +1131,7 @@ static bool pac1934_acpi_parse_channel_config(struct i2c_client *client,
 
 	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1934_ACPI_GET_BIPOLAR_SETTINGS, NULL);
 	if (!rez)
-		return false;
+		return -EINVAL;
 
 	bi_dir_mask = rez->package.elements[0].integer.value;
 	info->bi_dir[0] = ((bi_dir_mask & (1 << 3)) | (bi_dir_mask & (1 << 7))) != 0;
@@ -1143,19 +1143,18 @@ static bool pac1934_acpi_parse_channel_config(struct i2c_client *client,
 
 	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1934_ACPI_GET_SAMP, NULL);
 	if (!rez)
-		return false;
+		return -EINVAL;
 
 	info->sample_rate_value = rez->package.elements[0].integer.value;
 
 	ACPI_FREE(rez);
 
-	return true;
+	return 0;
 }
 
-static bool pac1934_of_parse_channel_config(struct i2c_client *client,
-					    struct pac1934_chip_info *info)
+static int pac1934_fw_parse_channel_config(struct i2c_client *client,
+					   struct pac1934_chip_info *info)
 {
-	struct fwnode_handle *node, *fwnode;
 	struct device *dev = &client->dev;
 	unsigned int current_channel;
 	int idx, ret;
@@ -1163,46 +1162,38 @@ static bool pac1934_of_parse_channel_config(struct i2c_client *client,
 	info->sample_rate_value = 1024;
 	current_channel = 1;
 
-	fwnode = dev_fwnode(dev);
-	fwnode_for_each_available_child_node(fwnode, node) {
+	device_for_each_child_node_scoped(dev, node) {
 		ret = fwnode_property_read_u32(node, "reg", &idx);
-		if (ret) {
-			dev_err_probe(dev, ret,
-				      "reading invalid channel index\n");
-			goto err_fwnode;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "reading invalid channel index\n");
+
 		/* adjust idx to match channel index (1 to 4) from the datasheet */
 		idx--;
 
 		if (current_channel >= (info->phys_channels + 1) ||
-		    idx >= info->phys_channels || idx < 0) {
-			dev_err_probe(dev, -EINVAL,
-				      "%s: invalid channel_index %d value\n",
-				      fwnode_get_name(node), idx);
-			goto err_fwnode;
-		}
+		    idx >= info->phys_channels || idx < 0)
+			return dev_err_probe(dev, -EINVAL,
+					     "%s: invalid channel_index %d value\n",
+					     fwnode_get_name(node), idx);
 
 		/* enable channel */
 		info->active_channels[idx] = true;
 
 		ret = fwnode_property_read_u32(node, "shunt-resistor-micro-ohms",
 					       &info->shunts[idx]);
-		if (ret) {
-			dev_err_probe(dev, ret,
-				      "%s: invalid shunt-resistor value: %d\n",
-				      fwnode_get_name(node), info->shunts[idx]);
-			goto err_fwnode;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "%s: invalid shunt-resistor value: %d\n",
+					     fwnode_get_name(node), info->shunts[idx]);
 
 		if (fwnode_property_present(node, "label")) {
 			ret = fwnode_property_read_string(node, "label",
 							  (const char **)&info->labels[idx]);
-			if (ret) {
-				dev_err_probe(dev, ret,
-					      "%s: invalid rail-name value\n",
-					      fwnode_get_name(node));
-				goto err_fwnode;
-			}
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "%s: invalid rail-name value\n",
+						     fwnode_get_name(node));
 		}
 
 		info->bi_dir[idx] = fwnode_property_read_bool(node, "bipolar");
@@ -1210,12 +1201,7 @@ static bool pac1934_of_parse_channel_config(struct i2c_client *client,
 		current_channel++;
 	}
 
-	return true;
-
-err_fwnode:
-	fwnode_handle_put(node);
-
-	return false;
+	return 0;
 }
 
 static void pac1934_cancel_delayed_work(void *dwork)
@@ -1485,7 +1471,6 @@ static int pac1934_probe(struct i2c_client *client)
 	const struct pac1934_features *chip;
 	struct iio_dev *indio_dev;
 	int cnt, ret;
-	bool match = false;
 	struct device *dev = &client->dev;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
@@ -1519,16 +1504,16 @@ static int pac1934_probe(struct i2c_client *client)
 	}
 
 	if (acpi_match_device(dev->driver->acpi_match_table, dev))
-		match = pac1934_acpi_parse_channel_config(client, info);
+		ret = pac1934_acpi_parse_channel_config(client, info);
 	else
 		/*
 		 * This makes it possible to use also ACPI PRP0001 for
 		 * registering the device using device tree properties.
 		 */
-		match = pac1934_of_parse_channel_config(client, info);
+		ret = pac1934_fw_parse_channel_config(client, info);
 
-	if (!match)
-		return dev_err_probe(dev, -EINVAL,
+	if (ret)
+		return dev_err_probe(dev, ret,
 				     "parameter parsing returned an error\n");
 
 	mutex_init(&info->lock);
-- 
2.44.0

Re: [PATCH 0/8] IIO: More use of device_for_each_child_node_scoped() and __free()

[Nuno Sá]

On Sat, 2024-03-30 at 19:08 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Mixture of new code that entered in parallel with the original conversion
> set to use this new scoped cleanup and cases I missed.  Note the
> relevant code didn't make the 6.9 merge window but is queued up for
> 6.10 in the togreg branch of iio.git and linux-next.
> 
> Includes allowing easier building of the ab8500-gpadc driver to enable
> testing the changes. Seems a sensible change to make in general.
> 
> This series is making us of the automated cleanup introduced in
> linux/cleanup.h and new device_for_each_child_node_scoped() to avoid
> the need to remember to call fwnode_handle_put() in early exits from
> loops over the child nodes. This can also be used for simple scope
> based cleanup as seen in the ad3552 patch. In general this makes it
> easier to avoid a common class of bug.
> 
> Jonathan Cameron (8):
>   iio: adc: ab8500-gpadc: Allow COMPILE_TEST builds
>   iio: adc: ab8500-gpadc: Fix kernel-doc parameter names.
>   iio: adc: ab8500-gpadc: Use device_for_each_child_node_scoped() to
>     simplify erorr paths.
>   iio: adc: ad4130: Use device_for_each_child_node_scoped() to simplify
>     error paths.
>   iio: adc: ad7173: Use device_for_each_child_node_scoped() to simplify
>     error paths.
>   iio: frequency: admfm2000: Use device_for_each_child_node_scoped() to
>     simplify error paths.
>   iio: dac: ad3552: Use __free(fwnode_handle) to simplify error
>     handling.
>   iio: adc: pac1934: Use device_for_each_available_child_node_scoped()
>     to simplify error handling.
> 
>  drivers/iio/adc/Kconfig           |  2 +-
>  drivers/iio/adc/ab8500-gpadc.c    |  8 ++--
>  drivers/iio/adc/ad4130.c          |  7 +--
>  drivers/iio/adc/ad7173.c          | 24 +++-------
>  drivers/iio/adc/pac1934.c         | 77 +++++++++++++------------------
>  drivers/iio/dac/ad3552r.c         | 59 +++++++++--------------
>  drivers/iio/frequency/admfm2000.c | 24 +++-------
>  7 files changed, 73 insertions(+), 128 deletions(-)
> 

LGTM,

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

Re: [PATCH 0/8] IIO: More use of device_for_each_child_node_scoped() and __free()

[Jonathan Cameron]

On Thu, 04 Apr 2024 11:09:31 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sat, 2024-03-30 at 19:08 +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Mixture of new code that entered in parallel with the original conversion
> > set to use this new scoped cleanup and cases I missed.  Note the
> > relevant code didn't make the 6.9 merge window but is queued up for
> > 6.10 in the togreg branch of iio.git and linux-next.
> > 
> > Includes allowing easier building of the ab8500-gpadc driver to enable
> > testing the changes. Seems a sensible change to make in general.
> > 
> > This series is making us of the automated cleanup introduced in
> > linux/cleanup.h and new device_for_each_child_node_scoped() to avoid
> > the need to remember to call fwnode_handle_put() in early exits from
> > loops over the child nodes. This can also be used for simple scope
> > based cleanup as seen in the ad3552 patch. In general this makes it
> > easier to avoid a common class of bug.
> > 
> > Jonathan Cameron (8):
> >   iio: adc: ab8500-gpadc: Allow COMPILE_TEST builds
> >   iio: adc: ab8500-gpadc: Fix kernel-doc parameter names.
> >   iio: adc: ab8500-gpadc: Use device_for_each_child_node_scoped() to
> >     simplify erorr paths.
> >   iio: adc: ad4130: Use device_for_each_child_node_scoped() to simplify
> >     error paths.
> >   iio: adc: ad7173: Use device_for_each_child_node_scoped() to simplify
> >     error paths.
> >   iio: frequency: admfm2000: Use device_for_each_child_node_scoped() to
> >     simplify error paths.
> >   iio: dac: ad3552: Use __free(fwnode_handle) to simplify error
> >     handling.
> >   iio: adc: pac1934: Use device_for_each_available_child_node_scoped()
> >     to simplify error handling.
> > 
> >  drivers/iio/adc/Kconfig           |  2 +-
> >  drivers/iio/adc/ab8500-gpadc.c    |  8 ++--
> >  drivers/iio/adc/ad4130.c          |  7 +--
> >  drivers/iio/adc/ad7173.c          | 24 +++-------
> >  drivers/iio/adc/pac1934.c         | 77 +++++++++++++------------------
> >  drivers/iio/dac/ad3552r.c         | 59 +++++++++--------------
> >  drivers/iio/frequency/admfm2000.c | 24 +++-------
> >  7 files changed, 73 insertions(+), 128 deletions(-)
> >   
> 
> LGTM,
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 

Series applies with exception of patch 1 where discussion is ongoing.

Thanks,

Jonathan