[PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.

[PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.

[Jonathan Cameron]

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

Chances since RFC.
- Add if (!IS_ERR_OR_NULL(_T)) check as suggested by Andy Shevchenko.
  This may allow the compiler to optimize cases where it can tell that
  this check will fail rather than calling into fwnode_handle_put().

Note I don't have the hardware so this is compile tested only.
Hence I'd appreciate some Tested-by tags if anyone can poke one of the
effected drivers.

I have tested the device tree only version:
https://lore.kernel.org/linux-iio/20231217184648.185236-1-jic23@kernel.org/
which is very similar.

Failing to release the references on early exit from loops over child nodes
and similar are a fairly common source of bugs. The need to explicitly
release the references via fwnode_handle_put() also complicate the code.

The first patch enables

	struct fwnode_handle *child __free(fwnode_handle) = NULL;

	device_for_each_child_node(dev, child) {
		if (err)
			/*
			 * Previously needed a fwnode_handle_put() here,
			 * will now be called automatically as well leave
			 * the scope within which the cleanup is registered
			 */
			return err;
	}

/*
 * The if (!IS_ERR_OR_NULL(_T)) check will fail here as the pointer will be
 * NULL and thus fwnode_handle_put() will not be called. It would be
 * functionally correct to call it with a NULL pointer but that reduces
 * the opportunities for the compiler to optimize out the call.
 */
}

As can be seen by the examples from IIO that follow this can save
a reasonable amount of complexity and boiler plate code, often enabling
additional cleanups in related code such as use of
return dev_err_probe().

Merge wise (assuming everyone is happy), I'd propose an immutable branch
(in IIO or elsewhere) with the first patch on it, so that we can start making
use of this in other areas of the kernel without having to wait too long.

Jonathan Cameron (13):
  device property: Add cleanup.h based fwnode_handle_put() scope based
    cleanup.
  iio: adc: max11410: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls
  iio: adc: mcp3564: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls
  iio: adc: qcom-spmi-adc5: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls
  iio: adc: rzg2l_adc: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls
  iio: adc: stm32: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls
  iio: adc: ti-ads1015: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls
  iio: adc: ti-ads131e08: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls
  iio: addac: ad74413r: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls
  iio: dac: ad3552: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls
  iio: dac: ad5770r: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls
  iio: dac: ltc2688: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls
  iio: temp: ltc2983: Use __free(fwnode_handle) to replace
    fwnode_handle_put() calls

 drivers/iio/adc/max11410.c        | 26 ++++--------
 drivers/iio/adc/mcp3564.c         | 15 ++++---
 drivers/iio/adc/qcom-spmi-adc5.c  |  6 +--
 drivers/iio/adc/rzg2l_adc.c       | 10 ++---
 drivers/iio/adc/stm32-adc.c       | 62 +++++++++++----------------
 drivers/iio/adc/ti-ads1015.c      |  4 +-
 drivers/iio/adc/ti-ads131e08.c    | 12 ++----
 drivers/iio/addac/ad74413r.c      |  9 +---
 drivers/iio/dac/ad3552r.c         | 50 +++++++++-------------
 drivers/iio/dac/ad5770r.c         | 18 +++-----
 drivers/iio/dac/ltc2688.c         | 23 +++-------
 drivers/iio/temperature/ltc2983.c | 70 ++++++++++---------------------
 include/linux/property.h          |  3 ++
 13 files changed, 105 insertions(+), 203 deletions(-)

-- 
2.43.0

[PATCH 01/13] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.

[Jonathan Cameron]

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

This allows the following

struct fwnode_handle *child __free(kfree) = NULL;

device_for_each_child_node(dev, child) {
	if (false)
		return -EINVAL;
}

without the fwnode_handle_put() call which tends to complicate early
exits from such loops and lead to resource leak bugs.

Can also be used where the fwnode_handle was obtained from a call
such as fwnode_find_reference() as it will safely do nothing if
IS_ERR() is true.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v1: Thanks to Andy for reviewing the RFC.
    Add check for if (!IS_ERR_OR_NULL(_T)) to allow the compiler to optimize
    cases where it knows the passed in parameter is NULL or an error pointer.
---
 include/linux/property.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/property.h b/include/linux/property.h
index 2b8f07fc68a9..9f3190d902ab 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -12,6 +12,7 @@
 
 #include <linux/args.h>
 #include <linux/bits.h>
+#include <linux/cleanup.h>
 #include <linux/fwnode.h>
 #include <linux/stddef.h>
 #include <linux/types.h>
@@ -161,6 +162,8 @@ struct fwnode_handle *device_get_named_child_node(const struct device *dev,
 
 struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
 void fwnode_handle_put(struct fwnode_handle *fwnode);
+DEFINE_FREE(fwnode_handle, struct fwnode_handle *,
+	    if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T))
 
 int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
 int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);
-- 
2.43.0

Re: [PATCH 01/13] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.

[Andy Shevchenko]

On Sun, Jan 14, 2024 at 05:19:57PM +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This allows the following
> 
> struct fwnode_handle *child __free(kfree) = NULL;
> 
> device_for_each_child_node(dev, child) {
> 	if (false)
> 		return -EINVAL;
> }
> 
> without the fwnode_handle_put() call which tends to complicate early
> exits from such loops and lead to resource leak bugs.
> 
> Can also be used where the fwnode_handle was obtained from a call
> such as fwnode_find_reference() as it will safely do nothing if
> IS_ERR() is true.

...

>  struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
>  void fwnode_handle_put(struct fwnode_handle *fwnode);

I would add a blank line here

> +DEFINE_FREE(fwnode_handle, struct fwnode_handle *,
> +	    if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T))
>  
>  int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
>  int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);

With the above,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/13] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.

[Jonathan Cameron]

On Sun, 21 Jan 2024 14:28:47 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sun, Jan 14, 2024 at 05:19:57PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > This allows the following
> > 
> > struct fwnode_handle *child __free(kfree) = NULL;

That's garbage.  Should be __free(fwnode_handle)!

> > 
> > device_for_each_child_node(dev, child) {
> > 	if (false)
> > 		return -EINVAL;
> > }
> > 
> > without the fwnode_handle_put() call which tends to complicate early
> > exits from such loops and lead to resource leak bugs.
> > 
> > Can also be used where the fwnode_handle was obtained from a call
> > such as fwnode_find_reference() as it will safely do nothing if
> > IS_ERR() is true.  
> 
> ...
> 
> >  struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
> >  void fwnode_handle_put(struct fwnode_handle *fwnode);  
> 
> I would add a blank line here
> 
> > +DEFINE_FREE(fwnode_handle, struct fwnode_handle *,
> > +	    if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T))
> >  
> >  int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
> >  int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);  
> 
> With the above,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
Thanks Andy - however..

The discussion with Rob about the DT equivalent took an interesting turn.

He raised the concern that the __free was not always tightly coupled with the
equivalent of  device_for_each_child_node() which as per similar
discussions elsewhere results in:
a) Potentially wrong ordering if there is other cleanup.h based stuff going on
   in the same function.
b) A lack of association between the setup of the free and what it is undoing.
  (this was the one Rob pointed at).

I proposed two options that here map to
1) Always drag the declaration next to the device_for_each_child_node()
   and intentionally don't set it to NULL.

{
	.... stuff....

	struct fwnode_handle *child __free(fwnode);
	device_for_each_child_node(dev, child) {
	}

2) Scoped version of the loops themselves.

#define device_for_each_child_node_scoped(dev, child)				\
	for (struct fw_node_handle *child __free(fwnode_handle)                 \
		 = device_get_next_child_node(dev, NULL);                       \
	    child; child = device_get_next_child_node(dev, child))

So that the child only exists at all in the scope of the loop.

What do you think of the options?

DT thread is here:
https://lore.kernel.org/linux-iio/20240114165358.119916-1-jic23@kernel.org/T/#t

Jonathan

Re: [PATCH 01/13] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.

[Lukas Wunner]

On Sun, Jan 14, 2024 at 05:19:57PM +0000, Jonathan Cameron wrote:
> This allows the following
> 
> struct fwnode_handle *child __free(fwnode_handle) = NULL;
> 
> device_for_each_child_node(dev, child) {
> 	if (false)
> 		return -EINVAL;
> }
> 
> without the fwnode_handle_put() call which tends to complicate early
> exits from such loops and lead to resource leak bugs.
> 
> Can also be used where the fwnode_handle was obtained from a call
> such as fwnode_find_reference() as it will safely do nothing if
> IS_ERR() is true.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v1: Thanks to Andy for reviewing the RFC.
>     Add check for if (!IS_ERR_OR_NULL(_T)) to allow the compiler to optimize
>     cases where it knows the passed in parameter is NULL or an error pointer.

Heads-up:  Using IS_ERR_OR_NULL() in DEFINE_FREE() macros bloats
the code with additional IS_ERR() checks and NULL pointer checks.

See the detailed explanation in this patch which adds a DEFINE_FREE()
macro for x509_free_certificate():

https://lore.kernel.org/all/70ecd3904a70d2b92f8f1e04365a2b9ce66fac25.1705857475.git.lukas@wunner.de/

I'm wondering if a solution might be to stop returning IS_ERR()
from "constructors" such as x509_cert_parse() and instead assign
the created "object" (x509_certificate) to a call-by-reference
pointer and return an integer.  If the returned integer is not 0,
inhibit "destruction" of the "object" with no_free_ptr().

Thoughts?


> +DEFINE_FREE(fwnode_handle, struct fwnode_handle *,
> +	    if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T))

If you do not align the "if" to the opening parenthesis,
checkpatch --strict complains:
"CHECK: Alignment should match open parenthesis"

If you do align to the opening parenthesis, it complains:
"WARNING: Statements should start on a tabstop"

I chose the latter for x509_free_certificate() for aesthetic reasons.

Either way, checkpatch still emits:

ERROR: trailing statements should be on next line"
#183: FILE: crypto/asymmetric_keys/x509_parser.h:49:
+	    if (!IS_ERR_OR_NULL(_T)) x509_free_certificate(_T))

Can't make it happy with these new-fangled DEFINE_FREE macros it seems. :(

Thanks,

Lukas

Re: [PATCH 01/13] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.

[Lukas Wunner]

On Sun, Jan 21, 2024 at 07:06:03PM +0100, Lukas Wunner wrote:
> On Sun, Jan 14, 2024 at 05:19:57PM +0000, Jonathan Cameron wrote:
> > v1: Thanks to Andy for reviewing the RFC.
> >     Add check for if (!IS_ERR_OR_NULL(_T)) to allow the compiler to optimize
> >     cases where it knows the passed in parameter is NULL or an error pointer.
> 
> Heads-up:  Using IS_ERR_OR_NULL() in DEFINE_FREE() macros bloats
> the code with additional IS_ERR() checks and NULL pointer checks.
> 
> See the detailed explanation in this patch which adds a DEFINE_FREE()
> macro for x509_free_certificate():
> 
> https://lore.kernel.org/all/70ecd3904a70d2b92f8f1e04365a2b9ce66fac25.1705857475.git.lukas@wunner.de/
> 
> I'm wondering if a solution might be to stop returning IS_ERR()
> from "constructors" such as x509_cert_parse() and instead assign
> the created "object" (x509_certificate) to a call-by-reference
> pointer and return an integer.  If the returned integer is not 0,
> inhibit "destruction" of the "object" with no_free_ptr().

Another idea would be to use a call-by-reference pointer and check
the pointer instead of the return code.

E.g.:

DEFINE_FREE(x509_free_certificate, struct x509_certificate *,
	    if (_T) x509_free_certificate(_T))

...

	struct x509_certificate __free(x509_free_certificate) = NULL;
	int ret;

	ret = x509_cert_parse(&cert, buf, len);
	if (!cert)
		return ret;

[PATCH 02/13] iio: adc: max11410: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions within the
device_for_each_child_node(dev, child) loop.  On normal exit from that
loop no fwnode_handle reference will be held and the child pointer
will be NULL thus making the automatically run fwnode_handle_put() a
noop whereas it will release the reference held on early breaking or
returning from within the loop.

Cc: Nuno Sá <nuno.sa@analog.com>
Cc: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/max11410.c | 26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/adc/max11410.c b/drivers/iio/adc/max11410.c
index 6af829349b4e..790a95474bb9 100644
--- a/drivers/iio/adc/max11410.c
+++ b/drivers/iio/adc/max11410.c
@@ -696,7 +696,7 @@ static int max11410_parse_channels(struct max11410_state *st,
 	struct device *dev = &st->spi_dev->dev;
 	struct max11410_channel_config *cfg;
 	struct iio_chan_spec *channels;
-	struct fwnode_handle *child;
+	struct fwnode_handle *child __free(fwnode_handle) = NULL;
 	u32 reference, sig_path;
 	const char *node_name;
 	u32 inputs[2], scale;
@@ -735,47 +735,37 @@ static int max11410_parse_channels(struct max11410_state *st,
 			inputs[1] = 0;
 			chanspec.differential = 0;
 		}
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
 
 		if (inputs[0] > MAX11410_CHANNEL_INDEX_MAX ||
-		    inputs[1] > MAX11410_CHANNEL_INDEX_MAX) {
-			fwnode_handle_put(child);
+		    inputs[1] > MAX11410_CHANNEL_INDEX_MAX)
 			return dev_err_probe(&indio_dev->dev, -EINVAL,
 					     "Invalid channel index for %s, should be less than %d\n",
 					     node_name,
 					     MAX11410_CHANNEL_INDEX_MAX + 1);
-		}
 
 		cfg = &st->channels[chan_idx];
 
 		reference = MAX11410_REFSEL_AVDD_AGND;
 		fwnode_property_read_u32(child, "adi,reference", &reference);
-		if (reference > MAX11410_REFSEL_MAX) {
-			fwnode_handle_put(child);
+		if (reference > MAX11410_REFSEL_MAX)
 			return dev_err_probe(&indio_dev->dev, -EINVAL,
 					     "Invalid adi,reference value for %s, should be less than %d.\n",
 					     node_name, MAX11410_REFSEL_MAX + 1);
-		}
 
 		if (!max11410_get_vrefp(st, reference) ||
-		    (!max11410_get_vrefn(st, reference) && reference <= 2)) {
-			fwnode_handle_put(child);
+		    (!max11410_get_vrefn(st, reference) && reference <= 2))
 			return dev_err_probe(&indio_dev->dev, -EINVAL,
 					     "Invalid VREF configuration for %s, either specify corresponding VREF regulators or change adi,reference property.\n",
 					     node_name);
-		}
 
 		sig_path = MAX11410_PGA_SIG_PATH_BUFFERED;
 		fwnode_property_read_u32(child, "adi,input-mode", &sig_path);
-		if (sig_path > MAX11410_SIG_PATH_MAX) {
-			fwnode_handle_put(child);
+		if (sig_path > MAX11410_SIG_PATH_MAX)
 			return dev_err_probe(&indio_dev->dev, -EINVAL,
 					     "Invalid adi,input-mode value for %s, should be less than %d.\n",
 					     node_name, MAX11410_SIG_PATH_MAX + 1);
-		}
 
 		fwnode_property_read_u32(child, "settling-time-us",
 					 &cfg->settling_time_us);
@@ -793,10 +783,8 @@ static int max11410_parse_channels(struct max11410_state *st,
 			cfg->scale_avail = devm_kcalloc(dev, MAX11410_SCALE_AVAIL_SIZE * 2,
 							sizeof(*cfg->scale_avail),
 							GFP_KERNEL);
-			if (!cfg->scale_avail) {
-				fwnode_handle_put(child);
+			if (!cfg->scale_avail)
 				return -ENOMEM;
-			}
 
 			scale = max11410_get_scale(st, *cfg);
 			for (i = 0; i < MAX11410_SCALE_AVAIL_SIZE; i++) {
-- 
2.43.0

[PATCH 03/13] iio: adc: mcp3564: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions within the
device_for_each_child_node(dev, child) loop. On normal exit from that
loop no fwnode_handle reference will be held and the child pointer
will be NULL thus making the automatically run fwnode_handle_put() a
noop whereas it will release the reference held on early break or
return from the loop.

Return a little earlier in some error paths to avoid performing
pointless activity. Likely motivation for doing this previously was
to avoid repeating the fwnode_handle_put() calls.

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

diff --git a/drivers/iio/adc/mcp3564.c b/drivers/iio/adc/mcp3564.c
index e3f1de5fcc5a..9a1029c93fb2 100644
--- a/drivers/iio/adc/mcp3564.c
+++ b/drivers/iio/adc/mcp3564.c
@@ -998,7 +998,7 @@ static int mcp3564_parse_fw_children(struct iio_dev *indio_dev)
 	struct mcp3564_state *adc = iio_priv(indio_dev);
 	struct device *dev = &adc->spi->dev;
 	struct iio_chan_spec *channels;
-	struct fwnode_handle *child;
+	struct fwnode_handle *child __free(fwnode_handle) = NULL;
 	struct iio_chan_spec chanspec = mcp3564_channel_template;
 	struct iio_chan_spec temp_chanspec = mcp3564_temp_channel_template;
 	struct iio_chan_spec burnout_chanspec = mcp3564_burnout_channel_template;
@@ -1033,26 +1033,25 @@ static int mcp3564_parse_fw_children(struct iio_dev *indio_dev)
 							     "diff-channels",
 							     inputs,
 							     ARRAY_SIZE(inputs));
+			if (ret)
+				return ret;
+
 			chanspec.differential = 1;
 		} else {
 			ret = fwnode_property_read_u32(child, "reg", &inputs[0]);
+			if (ret)
+				return ret;
 
 			chanspec.differential = 0;
 			inputs[1] = MCP3564_AGND;
 		}
-		if (ret) {
-			fwnode_handle_put(child);
-			return ret;
-		}
 
 		if (inputs[0] > MCP3564_INTERNAL_VCM ||
-		    inputs[1] > MCP3564_INTERNAL_VCM) {
-			fwnode_handle_put(child);
+		    inputs[1] > MCP3564_INTERNAL_VCM)
 			return dev_err_probe(&indio_dev->dev, -EINVAL,
 					     "Channel index > %d, for %s\n",
 					     MCP3564_INTERNAL_VCM + 1,
 					     node_name);
-		}
 
 		chanspec.address = (inputs[0] << 4) | inputs[1];
 		chanspec.channel = inputs[0];
-- 
2.43.0

[PATCH 04/13] iio: adc: qcom-spmi-adc5: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions within the
device_for_each_child_node(dev, child) loop. On normal exit from that
loop no fwnode_handle reference will be held and the child pointer
will be NULL thus making the automatically run fwnode_handle_put() a
noop.

A slightly less convincing usecase than many as all the failure paths
are wrapped up in a call to a per fwnode_handle utility function.
The complexity in that function is sufficient that it makes sense to
factor it out even if it this new auto cleanup would enable simpler
returns if the code was inline at the call site. Hence I've left it alone.

Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/qcom-spmi-adc5.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
index b6b612d733ff..36d3912d36df 100644
--- a/drivers/iio/adc/qcom-spmi-adc5.c
+++ b/drivers/iio/adc/qcom-spmi-adc5.c
@@ -825,7 +825,7 @@ static int adc5_get_fw_data(struct adc5_chip *adc)
 	const struct adc5_channels *adc_chan;
 	struct iio_chan_spec *iio_chan;
 	struct adc5_channel_prop prop, *chan_props;
-	struct fwnode_handle *child;
+	struct fwnode_handle *child __free(fwnode_handle) = NULL;
 	unsigned int index = 0;
 	int ret;
 
@@ -851,10 +851,8 @@ static int adc5_get_fw_data(struct adc5_chip *adc)
 
 	device_for_each_child_node(adc->dev, child) {
 		ret = adc5_get_fw_channel_data(adc, &prop, child, adc->data);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
 
 		prop.scale_fn_type =
 			adc->data->adc_chans[prop.channel].scale_fn_type;
-- 
2.43.0

[PATCH 05/13] iio: adc: rzg2l_adc: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions within the
device_for_each_child_node(dev, child) loop. On normal exit from that
loop no fwnode_handle reference will be held and the child pointer
will be NULL thus making the automatically run fwnode_handle_put() a
noop.

Cc: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/rzg2l_adc.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index 0921ff2d9b3a..08af54824f25 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -302,7 +302,7 @@ static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
 static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l_adc *adc)
 {
 	struct iio_chan_spec *chan_array;
-	struct fwnode_handle *fwnode;
+	struct fwnode_handle *fwnode __free(fwnode_handle) = NULL;
 	struct rzg2l_adc_data *data;
 	unsigned int channel;
 	int num_channels;
@@ -332,15 +332,11 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
 	i = 0;
 	device_for_each_child_node(&pdev->dev, fwnode) {
 		ret = fwnode_property_read_u32(fwnode, "reg", &channel);
-		if (ret) {
-			fwnode_handle_put(fwnode);
+		if (ret)
 			return ret;
-		}
 
-		if (channel >= RZG2L_ADC_MAX_CHANNELS) {
-			fwnode_handle_put(fwnode);
+		if (channel >= RZG2L_ADC_MAX_CHANNELS)
 			return -EINVAL;
-		}
 
 		chan_array[i].type = IIO_VOLTAGE;
 		chan_array[i].indexed = 1;
-- 
2.43.0

[PATCH 06/13] iio: adc: stm32: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions within the
device_for_each_child_node(dev, child) loop. On normal exit from that
loop no fwnode_handle reference will be held and the child pointer
will be NULL thus making the automatically run fwnode_handle_put() a
noop.

Note this includes a probably fix as in one path an error message was
printed with ret == 0.

Took advantage of dev_err_probe() to futher simplify things given no
longer a need for the goto err.

Cc: Olivier Moysan <olivier.moysan@foss.st.com>
Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/stm32-adc.c | 62 ++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 38 deletions(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index b5d3c9cea5c4..c2306aeb5641 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -2187,7 +2187,7 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev,
 				       struct iio_chan_spec *channels)
 {
 	const struct stm32_adc_info *adc_info = adc->cfg->adc_info;
-	struct fwnode_handle *child;
+	struct fwnode_handle *child __free(fwnode_handle) = NULL;
 	const char *name;
 	int val, scan_index = 0, ret;
 	bool differential;
@@ -2195,50 +2195,43 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev,
 
 	device_for_each_child_node(&indio_dev->dev, child) {
 		ret = fwnode_property_read_u32(child, "reg", &val);
-		if (ret) {
-			dev_err(&indio_dev->dev, "Missing channel index %d\n", ret);
-			goto err;
-		}
+		if (ret)
+			return dev_err_probe(&indio_dev->dev, ret,
+					     "Missing channel index\n");
 
 		ret = fwnode_property_read_string(child, "label", &name);
 		/* label is optional */
 		if (!ret) {
-			if (strlen(name) >= STM32_ADC_CH_SZ) {
-				dev_err(&indio_dev->dev, "Label %s exceeds %d characters\n",
-					name, STM32_ADC_CH_SZ);
-				ret = -EINVAL;
-				goto err;
-			}
+			if (strlen(name) >= STM32_ADC_CH_SZ)
+				return dev_err_probe(&indio_dev->dev, -EINVAL,
+						     "Label %s exceeds %d characters\n",
+						     name, STM32_ADC_CH_SZ);
+
 			strscpy(adc->chan_name[val], name, STM32_ADC_CH_SZ);
 			ret = stm32_adc_populate_int_ch(indio_dev, name, val);
 			if (ret == -ENOENT)
 				continue;
 			else if (ret)
-				goto err;
-		} else if (ret != -EINVAL) {
-			dev_err(&indio_dev->dev, "Invalid label %d\n", ret);
-			goto err;
-		}
+				return ret;
+		} else if (ret != -EINVAL)
+			return dev_err_probe(&indio_dev->dev, ret, "Invalid label\n");
 
-		if (val >= adc_info->max_channels) {
-			dev_err(&indio_dev->dev, "Invalid channel %d\n", val);
-			ret = -EINVAL;
-			goto err;
-		}
+		if (val >= adc_info->max_channels)
+			return dev_err_probe(&indio_dev->dev, -EINVAL,
+					     "Invalid channel %d\n", val);
 
 		differential = false;
 		ret = fwnode_property_read_u32_array(child, "diff-channels", vin, 2);
 		/* diff-channels is optional */
 		if (!ret) {
 			differential = true;
-			if (vin[0] != val || vin[1] >= adc_info->max_channels) {
-				dev_err(&indio_dev->dev, "Invalid channel in%d-in%d\n",
-					vin[0], vin[1]);
-				goto err;
-			}
+			if (vin[0] != val || vin[1] >= adc_info->max_channels)
+				return dev_err_probe(&indio_dev->dev, -EINVAL,
+						     "Invalid channel in%d-in%d\n",
+						     vin[0], vin[1]);
 		} else if (ret != -EINVAL) {
-			dev_err(&indio_dev->dev, "Invalid diff-channels property %d\n", ret);
-			goto err;
+			return dev_err_probe(&indio_dev->dev, ret,
+					     "Invalid diff-channels property\n");
 		}
 
 		stm32_adc_chan_init_one(indio_dev, &channels[scan_index], val,
@@ -2247,11 +2240,9 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev,
 		val = 0;
 		ret = fwnode_property_read_u32(child, "st,min-sample-time-ns", &val);
 		/* st,min-sample-time-ns is optional */
-		if (ret && ret != -EINVAL) {
-			dev_err(&indio_dev->dev, "Invalid st,min-sample-time-ns property %d\n",
-				ret);
-			goto err;
-		}
+		if (ret && ret != -EINVAL)
+			return dev_err_probe(&indio_dev->dev, ret,
+					     "Invalid st,min-sample-time-ns property\n");
 
 		stm32_adc_smpr_init(adc, channels[scan_index].channel, val);
 		if (differential)
@@ -2261,11 +2252,6 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev,
 	}
 
 	return scan_index;
-
-err:
-	fwnode_handle_put(child);
-
-	return ret;
 }
 
 static int stm32_adc_chan_fw_init(struct iio_dev *indio_dev, bool timestamping)
-- 
2.43.0

[PATCH 07/13] iio: adc: ti-ads1015: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions within the
device_for_each_child_node(dev, child) loop. On normal exit from that
loop no fwnode_handle reference will be held and the child pointer
will be NULL thus making the automatically run fwnode_handle_put() a
noop.

Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti-ads1015.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 6ae967e4d8fa..b3a8a5b2c013 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -902,7 +902,7 @@ static int ads1015_client_get_channels_config(struct i2c_client *client)
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	struct ads1015_data *data = iio_priv(indio_dev);
 	struct device *dev = &client->dev;
-	struct fwnode_handle *node;
+	struct fwnode_handle *node __free(fwnode_handle) = NULL;
 	int i = -1;
 
 	device_for_each_child_node(dev, node) {
@@ -927,7 +927,6 @@ static int ads1015_client_get_channels_config(struct i2c_client *client)
 			pga = pval;
 			if (pga > 5) {
 				dev_err(dev, "invalid gain on %pfw\n", node);
-				fwnode_handle_put(node);
 				return -EINVAL;
 			}
 		}
@@ -936,7 +935,6 @@ static int ads1015_client_get_channels_config(struct i2c_client *client)
 			data_rate = pval;
 			if (data_rate > 7) {
 				dev_err(dev, "invalid data_rate on %pfw\n", node);
-				fwnode_handle_put(node);
 				return -EINVAL;
 			}
 		}
-- 
2.43.0

[PATCH 08/13] iio: adc: ti-ads131e08: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions within the
device_for_each_child_node(dev, child) loop. On normal exit from that
loop no fwnode_handle reference will be held and the child pointer
will be NULL thus making the automatically run fwnode_handle_put() a
noop.

Cc: Tomislav Denis <tomislav.denis@avl.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti-ads131e08.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ti-ads131e08.c b/drivers/iio/adc/ti-ads131e08.c
index fcfc46254313..fb1ab0b9a9db 100644
--- a/drivers/iio/adc/ti-ads131e08.c
+++ b/drivers/iio/adc/ti-ads131e08.c
@@ -694,7 +694,7 @@ static int ads131e08_alloc_channels(struct iio_dev *indio_dev)
 	struct ads131e08_channel_config *channel_config;
 	struct device *dev = &st->spi->dev;
 	struct iio_chan_spec *channels;
-	struct fwnode_handle *node;
+	struct fwnode_handle *node __free(fwnode_handle) = NULL;
 	unsigned int channel, tmp;
 	int num_channels, i, ret;
 
@@ -739,7 +739,7 @@ static int ads131e08_alloc_channels(struct iio_dev *indio_dev)
 	device_for_each_child_node(dev, node) {
 		ret = fwnode_property_read_u32(node, "reg", &channel);
 		if (ret)
-			goto err_child_out;
+			return ret;
 
 		ret = fwnode_property_read_u32(node, "ti,gain", &tmp);
 		if (ret) {
@@ -747,7 +747,7 @@ static int ads131e08_alloc_channels(struct iio_dev *indio_dev)
 		} else {
 			ret = ads131e08_pga_gain_to_field_value(st, tmp);
 			if (ret < 0)
-				goto err_child_out;
+				return ret;
 
 			channel_config[i].pga_gain = tmp;
 		}
@@ -758,7 +758,7 @@ static int ads131e08_alloc_channels(struct iio_dev *indio_dev)
 		} else {
 			ret = ads131e08_validate_channel_mux(st, tmp);
 			if (ret)
-				goto err_child_out;
+				return ret;
 
 			channel_config[i].mux = tmp;
 		}
@@ -784,10 +784,6 @@ static int ads131e08_alloc_channels(struct iio_dev *indio_dev)
 	st->channel_config = channel_config;
 
 	return 0;
-
-err_child_out:
-	fwnode_handle_put(node);
-	return ret;
 }
 
 static void ads131e08_regulator_disable(void *data)
-- 
2.43.0

[PATCH 09/13] iio: addac: ad74413r: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions within the
fwnode_for_each_available_child_node(dev, child) loop. On normal exit
from that loop no fwnode_handle reference will be held and the child
pointer will be NULL thus making the automatically run
fwnode_handle_put() a noop.

Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/addac/ad74413r.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index 7af3e4b8fe3b..ec9a466e118d 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -1255,21 +1255,16 @@ static int ad74413r_parse_channel_config(struct iio_dev *indio_dev,
 static int ad74413r_parse_channel_configs(struct iio_dev *indio_dev)
 {
 	struct ad74413r_state *st = iio_priv(indio_dev);
-	struct fwnode_handle *channel_node = NULL;
+	struct fwnode_handle *channel_node __free(fwnode_handle) = NULL;
 	int ret;
 
 	fwnode_for_each_available_child_node(dev_fwnode(st->dev), channel_node) {
 		ret = ad74413r_parse_channel_config(indio_dev, channel_node);
 		if (ret)
-			goto put_channel_node;
+			return ret;
 	}
 
 	return 0;
-
-put_channel_node:
-	fwnode_handle_put(channel_node);
-
-	return ret;
 }
 
 static int ad74413r_setup_channels(struct iio_dev *indio_dev)
-- 
2.43.0

Re: [PATCH 09/13] iio: addac: ad74413r: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Nuno Sá]

On Sun, 2024-01-14 at 17:20 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This use of the new cleanup.h scope based freeing infrastructure allows
> us to exit directly from error conditions within the
> fwnode_for_each_available_child_node(dev, child) loop. On normal exit
> from that loop no fwnode_handle reference will be held and the child
> pointer will be NULL thus making the automatically run
> fwnode_handle_put() a noop.
> 
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

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

>  drivers/iio/addac/ad74413r.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> index 7af3e4b8fe3b..ec9a466e118d 100644
> --- a/drivers/iio/addac/ad74413r.c
> +++ b/drivers/iio/addac/ad74413r.c
> @@ -1255,21 +1255,16 @@ static int ad74413r_parse_channel_config(struct
> iio_dev *indio_dev,
>  static int ad74413r_parse_channel_configs(struct iio_dev *indio_dev)
>  {
>  	struct ad74413r_state *st = iio_priv(indio_dev);
> -	struct fwnode_handle *channel_node = NULL;
> +	struct fwnode_handle *channel_node __free(fwnode_handle) = NULL;
>  	int ret;
>  
>  	fwnode_for_each_available_child_node(dev_fwnode(st->dev),
> channel_node) {
>  		ret = ad74413r_parse_channel_config(indio_dev, channel_node);
>  		if (ret)
> -			goto put_channel_node;
> +			return ret;
>  	}
>  
>  	return 0;
> -
> -put_channel_node:
> -	fwnode_handle_put(channel_node);
> -
> -	return ret;
>  }
>  
>  static int ad74413r_setup_channels(struct iio_dev *indio_dev)

Re: [PATCH 09/13] iio: addac: ad74413r: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

On Mon, 15 Jan 2024 11:17:12 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sun, 2024-01-14 at 17:20 +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > This use of the new cleanup.h scope based freeing infrastructure allows
> > us to exit directly from error conditions within the
> > fwnode_for_each_available_child_node(dev, child) loop. On normal exit
> > from that loop no fwnode_handle reference will be held and the child
> > pointer will be NULL thus making the automatically run
> > fwnode_handle_put() a noop.
> > 
> > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---  
> 
> Acked-by: Nuno Sa <nuno.sa@analog.com>
> 
> >  drivers/iio/addac/ad74413r.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> > index 7af3e4b8fe3b..ec9a466e118d 100644
> > --- a/drivers/iio/addac/ad74413r.c
> > +++ b/drivers/iio/addac/ad74413r.c
> > @@ -1255,21 +1255,16 @@ static int ad74413r_parse_channel_config(struct
> > iio_dev *indio_dev,
> >  static int ad74413r_parse_channel_configs(struct iio_dev *indio_dev)
> >  {
> >  	struct ad74413r_state *st = iio_priv(indio_dev);
> > -	struct fwnode_handle *channel_node = NULL;
> > +	struct fwnode_handle *channel_node __free(fwnode_handle) = NULL;
> >  	int ret;
> >  
> >  	fwnode_for_each_available_child_node(dev_fwnode(st->dev),
This should have been
device_for_each_child_node() because that ultimately calls
of_fwnode_get_next_child_node() which calls the available form anyway.
https://lore.kernel.org/lkml/20211205190101.26de4a57@jic23-huawei/T/#u

So I'll just switch to the new
device_for_each_child_node_scoped() that I'm about to propose.


> > channel_node) {
> >  		ret = ad74413r_parse_channel_config(indio_dev, channel_node);
> >  		if (ret)
> > -			goto put_channel_node;
> > +			return ret;
> >  	}
> >  
> >  	return 0;
> > -
> > -put_channel_node:
> > -	fwnode_handle_put(channel_node);
> > -
> > -	return ret;
> >  }
> >  
> >  static int ad74413r_setup_channels(struct iio_dev *indio_dev)  
> 

[PATCH 10/13] iio: dac: ad3552: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions within the
device_for_each_child_node(dev, child) loop. On normal exit from that
loop no fwnode_handle reference will be held and the child pointer
will be NULL thus making the automatically run fwnode_handle_put() a
noop.

Removing the goto err; statements also allows more extensive use of
dev_err_probe() further simplifying the code.

Cc: Mihail Chindris <mihail.chindris@analog.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/dac/ad3552r.c | 50 +++++++++++++++------------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c
index a492e8f2fc0f..f21c88cb480a 100644
--- a/drivers/iio/dac/ad3552r.c
+++ b/drivers/iio/dac/ad3552r.c
@@ -880,7 +880,7 @@ static void ad3552r_reg_disable(void *reg)
 static int ad3552r_configure_device(struct ad3552r_desc *dac)
 {
 	struct device *dev = &dac->spi->dev;
-	struct fwnode_handle *child;
+	struct fwnode_handle *child __free(fwnode_handle) = NULL;
 	struct regulator *vref;
 	int err, cnt = 0, voltage, delta = 100000;
 	u32 vals[2], val, ch;
@@ -951,51 +951,43 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
 
 	device_for_each_child_node(dev, child) {
 		err = fwnode_property_read_u32(child, "reg", &ch);
-		if (err) {
-			dev_err(dev, "mandatory reg property missing\n");
-			goto put_child;
-		}
-		if (ch >= AD3552R_NUM_CH) {
-			dev_err(dev, "reg must be less than %d\n",
-				AD3552R_NUM_CH);
-			err = -EINVAL;
-			goto put_child;
-		}
+		if (err)
+			return dev_err_probe(dev, err,
+					     "mandatory reg property missing\n");
+		if (ch >= AD3552R_NUM_CH)
+			return dev_err_probe(dev, -EINVAL,
+					     "reg must be less than %d\n",
+					     AD3552R_NUM_CH);
 
 		if (fwnode_property_present(child, "adi,output-range-microvolt")) {
 			err = fwnode_property_read_u32_array(child,
 							     "adi,output-range-microvolt",
 							     vals,
 							     2);
-			if (err) {
-				dev_err(dev,
+			if (err)
+				return dev_err_probe(dev, err,
 					"adi,output-range-microvolt property could not be parsed\n");
-				goto put_child;
-			}
 
 			err = ad3552r_find_range(dac->chip_id, vals);
-			if (err < 0) {
-				dev_err(dev,
-					"Invalid adi,output-range-microvolt value\n");
-				goto put_child;
-			}
+			if (err < 0)
+				return dev_err_probe(dev, err,
+						     "Invalid adi,output-range-microvolt value\n");
+
 			val = err;
 			err = ad3552r_set_ch_value(dac,
 						   AD3552R_CH_OUTPUT_RANGE_SEL,
 						   ch, val);
 			if (err)
-				goto put_child;
+				return err;
 
 			dac->ch_data[ch].range = val;
 		} else if (dac->chip_id == AD3542R_ID) {
-			dev_err(dev,
-				"adi,output-range-microvolt is required for ad3542r\n");
-			err = -EINVAL;
-			goto put_child;
+			return dev_err_probe(dev, -EINVAL,
+					     "adi,output-range-microvolt is required for ad3542r\n");
 		} else {
 			err = ad3552r_configure_custom_gain(dac, child, ch);
 			if (err)
-				goto put_child;
+				return err;
 		}
 
 		ad3552r_calc_gain_and_offset(dac, ch);
@@ -1003,7 +995,7 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
 
 		err = ad3552r_set_ch_value(dac, AD3552R_CH_SELECT, ch, 1);
 		if (err < 0)
-			goto put_child;
+			return err;
 
 		dac->channels[cnt] = AD3552R_CH_DAC(ch);
 		++cnt;
@@ -1021,10 +1013,6 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
 	dac->num_ch = cnt;
 
 	return 0;
-put_child:
-	fwnode_handle_put(child);
-
-	return err;
 }
 
 static int ad3552r_init(struct ad3552r_desc *dac)
-- 
2.43.0

Re: [PATCH 10/13] iio: dac: ad3552: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Nuno Sá]

On Sun, 2024-01-14 at 17:20 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This use of the new cleanup.h scope based freeing infrastructure allows
> us to exit directly from error conditions within the
> device_for_each_child_node(dev, child) loop. On normal exit from that
> loop no fwnode_handle reference will be held and the child pointer
> will be NULL thus making the automatically run fwnode_handle_put() a
> noop.
> 
> Removing the goto err; statements also allows more extensive use of
> dev_err_probe() further simplifying the code.
> 
> Cc: Mihail Chindris <mihail.chindris@analog.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

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

>  drivers/iio/dac/ad3552r.c | 50 +++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c
> index a492e8f2fc0f..f21c88cb480a 100644
> --- a/drivers/iio/dac/ad3552r.c
> +++ b/drivers/iio/dac/ad3552r.c
> @@ -880,7 +880,7 @@ static void ad3552r_reg_disable(void *reg)
>  static int ad3552r_configure_device(struct ad3552r_desc *dac)
>  {
>  	struct device *dev = &dac->spi->dev;
> -	struct fwnode_handle *child;
> +	struct fwnode_handle *child __free(fwnode_handle) = NULL;
>  	struct regulator *vref;
>  	int err, cnt = 0, voltage, delta = 100000;
>  	u32 vals[2], val, ch;
> @@ -951,51 +951,43 @@ static int ad3552r_configure_device(struct ad3552r_desc
> *dac)
>  
>  	device_for_each_child_node(dev, child) {
>  		err = fwnode_property_read_u32(child, "reg", &ch);
> -		if (err) {
> -			dev_err(dev, "mandatory reg property missing\n");
> -			goto put_child;
> -		}
> -		if (ch >= AD3552R_NUM_CH) {
> -			dev_err(dev, "reg must be less than %d\n",
> -				AD3552R_NUM_CH);
> -			err = -EINVAL;
> -			goto put_child;
> -		}
> +		if (err)
> +			return dev_err_probe(dev, err,
> +					     "mandatory reg property
> missing\n");
> +		if (ch >= AD3552R_NUM_CH)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "reg must be less than %d\n",
> +					     AD3552R_NUM_CH);
>  
>  		if (fwnode_property_present(child, "adi,output-range-
> microvolt")) {
>  			err = fwnode_property_read_u32_array(child,
>  							     "adi,output-
> range-microvolt",
>  							     vals,
>  							     2);
> -			if (err) {
> -				dev_err(dev,
> +			if (err)
> +				return dev_err_probe(dev, err,
>  					"adi,output-range-microvolt property
> could not be parsed\n");
> -				goto put_child;
> -			}
>  
>  			err = ad3552r_find_range(dac->chip_id, vals);
> -			if (err < 0) {
> -				dev_err(dev,
> -					"Invalid adi,output-range-microvolt
> value\n");
> -				goto put_child;
> -			}
> +			if (err < 0)
> +				return dev_err_probe(dev, err,
> +						     "Invalid adi,output-
> range-microvolt value\n");
> +
>  			val = err;
>  			err = ad3552r_set_ch_value(dac,
>  						  
> AD3552R_CH_OUTPUT_RANGE_SEL,
>  						   ch, val);
>  			if (err)
> -				goto put_child;
> +				return err;
>  
>  			dac->ch_data[ch].range = val;
>  		} else if (dac->chip_id == AD3542R_ID) {
> -			dev_err(dev,
> -				"adi,output-range-microvolt is required for
> ad3542r\n");
> -			err = -EINVAL;
> -			goto put_child;
> +			return dev_err_probe(dev, -EINVAL,
> +					     "adi,output-range-microvolt is
> required for ad3542r\n");
>  		} else {
>  			err = ad3552r_configure_custom_gain(dac, child, ch);
>  			if (err)
> -				goto put_child;
> +				return err;
>  		}
>  
>  		ad3552r_calc_gain_and_offset(dac, ch);
> @@ -1003,7 +995,7 @@ static int ad3552r_configure_device(struct ad3552r_desc
> *dac)
>  
>  		err = ad3552r_set_ch_value(dac, AD3552R_CH_SELECT, ch, 1);
>  		if (err < 0)
> -			goto put_child;
> +			return err;
>  
>  		dac->channels[cnt] = AD3552R_CH_DAC(ch);
>  		++cnt;
> @@ -1021,10 +1013,6 @@ static int ad3552r_configure_device(struct ad3552r_desc
> *dac)
>  	dac->num_ch = cnt;
>  
>  	return 0;
> -put_child:
> -	fwnode_handle_put(child);
> -
> -	return err;
>  }
>  
>  static int ad3552r_init(struct ad3552r_desc *dac)

[PATCH 11/13] iio: dac: ad5770r: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions within the
device_for_each_child_node(dev, child) loop. On normal exit from that
loop no fwnode_handle reference will be held and the child pointer
will be NULL thus making the automatically run fwnode_handle_put() a
noop.

Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/dac/ad5770r.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/dac/ad5770r.c b/drivers/iio/dac/ad5770r.c
index f66d67402e43..782a04406576 100644
--- a/drivers/iio/dac/ad5770r.c
+++ b/drivers/iio/dac/ad5770r.c
@@ -515,7 +515,7 @@ static int ad5770r_channel_config(struct ad5770r_state *st)
 {
 	int ret, tmp[2], min, max;
 	unsigned int num;
-	struct fwnode_handle *child;
+	struct fwnode_handle *child __free(fwnode_handle) = NULL;
 
 	num = device_get_child_node_count(&st->spi->dev);
 	if (num != AD5770R_MAX_CHANNELS)
@@ -524,30 +524,24 @@ static int ad5770r_channel_config(struct ad5770r_state *st)
 	device_for_each_child_node(&st->spi->dev, child) {
 		ret = fwnode_property_read_u32(child, "reg", &num);
 		if (ret)
-			goto err_child_out;
-		if (num >= AD5770R_MAX_CHANNELS) {
-			ret = -EINVAL;
-			goto err_child_out;
-		}
+			return ret;
+		if (num >= AD5770R_MAX_CHANNELS)
+			return -EINVAL;
 
 		ret = fwnode_property_read_u32_array(child,
 						     "adi,range-microamp",
 						     tmp, 2);
 		if (ret)
-			goto err_child_out;
+			return ret;
 
 		min = tmp[0] / 1000;
 		max = tmp[1] / 1000;
 		ret = ad5770r_store_output_range(st, min, max, num);
 		if (ret)
-			goto err_child_out;
+			return ret;
 	}
 
 	return 0;
-
-err_child_out:
-	fwnode_handle_put(child);
-	return ret;
 }
 
 static int ad5770r_init(struct ad5770r_state *st)
-- 
2.43.0

[PATCH 12/13] iio: dac: ltc2688: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions within the
device_for_each_child_node(dev, child) loop. On normal exit from that
loop no fwnode_handle reference will be held and the child pointer
will be NULL thus making the automatically run fwnode_handle_put() a
noop.

Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/dac/ltc2688.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c
index fc8eb53c65be..e8add3636af9 100644
--- a/drivers/iio/dac/ltc2688.c
+++ b/drivers/iio/dac/ltc2688.c
@@ -746,7 +746,7 @@ static int ltc2688_span_lookup(const struct ltc2688_state *st, int min, int max)
 static int ltc2688_channel_config(struct ltc2688_state *st)
 {
 	struct device *dev = &st->spi->dev;
-	struct fwnode_handle *child;
+	struct fwnode_handle *child __free(fwnode_handle) = NULL;
 	u32 reg, clk_input, val, tmp[2];
 	int ret, span;
 
@@ -754,18 +754,14 @@ static int ltc2688_channel_config(struct ltc2688_state *st)
 		struct ltc2688_chan *chan;
 
 		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 >= LTC2688_DAC_CHANNELS) {
-			fwnode_handle_put(child);
+		if (reg >= LTC2688_DAC_CHANNELS)
 			return dev_err_probe(dev, -EINVAL,
 					     "reg bigger than: %d\n",
 					     LTC2688_DAC_CHANNELS);
-		}
 
 		val = 0;
 		chan = &st->channels[reg];
@@ -786,12 +782,10 @@ static int ltc2688_channel_config(struct ltc2688_state *st)
 		if (!ret) {
 			span = ltc2688_span_lookup(st, (int)tmp[0] / 1000,
 						   tmp[1] / 1000);
-			if (span < 0) {
-				fwnode_handle_put(child);
+			if (span < 0)
 				return dev_err_probe(dev, -EINVAL,
 						     "output range not valid:[%d %d]\n",
 						     tmp[0], tmp[1]);
-			}
 
 			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span);
 		}
@@ -800,17 +794,14 @@ static int ltc2688_channel_config(struct ltc2688_state *st)
 					       &clk_input);
 		if (!ret) {
 			if (clk_input >= LTC2688_CH_TGP_MAX) {
-				fwnode_handle_put(child);
 				return dev_err_probe(dev, -EINVAL,
 						     "toggle-dither-input inv value(%d)\n",
 						     clk_input);
 			}
 
 			ret = ltc2688_tgp_clk_setup(st, chan, child, clk_input);
-			if (ret) {
-				fwnode_handle_put(child);
+			if (ret)
 				return ret;
-			}
 
 			/*
 			 * 0 means software toggle which is the default mode.
@@ -844,11 +835,9 @@ static int ltc2688_channel_config(struct ltc2688_state *st)
 
 		ret = regmap_write(st->regmap, LTC2688_CMD_CH_SETTING(reg),
 				   val);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return dev_err_probe(dev, -EINVAL,
 					     "failed to set chan settings\n");
-		}
 	}
 
 	return 0;
-- 
2.43.0

Re: [PATCH 12/13] iio: dac: ltc2688: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Nuno Sá]

On Sun, 2024-01-14 at 17:20 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This use of the new cleanup.h scope based freeing infrastructure allows
> us to exit directly from error conditions within the
> device_for_each_child_node(dev, child) loop. On normal exit from that
> loop no fwnode_handle reference will be held and the child pointer
> will be NULL thus making the automatically run fwnode_handle_put() a
> noop.
> 
> Cc: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

Just one minor comment on my side. With that:

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

I might be able to give this a quick test. Let's see if I can find the time for
it :)

>  drivers/iio/dac/ltc2688.c | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c
> index fc8eb53c65be..e8add3636af9 100644
> --- a/drivers/iio/dac/ltc2688.c
> +++ b/drivers/iio/dac/ltc2688.c
> @@ -746,7 +746,7 @@ static int ltc2688_span_lookup(const struct ltc2688_state
> *st, int min, int max)
>  static int ltc2688_channel_config(struct ltc2688_state *st)
>  {
>  	struct device *dev = &st->spi->dev;
> -	struct fwnode_handle *child;
> +	struct fwnode_handle *child __free(fwnode_handle) = NULL;
>  	u32 reg, clk_input, val, tmp[2];
>  	int ret, span;
>  
> @@ -754,18 +754,14 @@ static int ltc2688_channel_config(struct ltc2688_state
> *st)
>  		struct ltc2688_chan *chan;
>  
>  		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 >= LTC2688_DAC_CHANNELS) {
> -			fwnode_handle_put(child);
> +		if (reg >= LTC2688_DAC_CHANNELS)
>  			return dev_err_probe(dev, -EINVAL,
>  					     "reg bigger than: %d\n",
>  					     LTC2688_DAC_CHANNELS);
> -		}
>  
>  		val = 0;
>  		chan = &st->channels[reg];
> @@ -786,12 +782,10 @@ static int ltc2688_channel_config(struct ltc2688_state
> *st)
>  		if (!ret) {
>  			span = ltc2688_span_lookup(st, (int)tmp[0] / 1000,
>  						   tmp[1] / 1000);
> -			if (span < 0) {
> -				fwnode_handle_put(child);
> +			if (span < 0)
>  				return dev_err_probe(dev, -EINVAL,
>  						     "output range not
> valid:[%d %d]\n",
>  						     tmp[0], tmp[1]);
> -			}
>  
>  			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span);
>  		}
> @@ -800,17 +794,14 @@ static int ltc2688_channel_config(struct ltc2688_state
> *st)
>  					       &clk_input);
>  		if (!ret) {
>  			if (clk_input >= LTC2688_CH_TGP_MAX) {

We can now remove the brackets...

> -				fwnode_handle_put(child);
>  				return dev_err_probe(dev, -EINVAL,
>  						     "toggle-dither-input inv
> value(%d)\n",
>  						     clk_input);
>  			}
>  
>  			ret = ltc2688_tgp_clk_setup(st, chan, child,
> clk_input);
> -			if (ret) {
> -				fwnode_handle_put(child);
> +			if (ret)
>  				return ret;
> -			}
>  
>  			/*
>  			 * 0 means software toggle which is the default mode.
> @@ -844,11 +835,9 @@ static int ltc2688_channel_config(struct ltc2688_state
> *st)
>  
>  		ret = regmap_write(st->regmap, LTC2688_CMD_CH_SETTING(reg),
>  				   val);
> -		if (ret) {
> -			fwnode_handle_put(child);
> +		if (ret)
>  			return dev_err_probe(dev, -EINVAL,
>  					     "failed to set chan
> settings\n");
> -		}
>  	}
>  
>  	return 0;

[PATCH 13/13] iio: temp: ltc2983: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Jonathan Cameron]

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

This use of the new cleanup.h scope based freeing infrastructure allows
us to exit directly from error conditions and in the good path with
the reference obtained from fwnode_find_reference() (which may be an error
pointer) automatically released.

Cc: Cosmin Tanislav <cosmin.tanislav@analog.com>
Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/temperature/ltc2983.c | 70 ++++++++++---------------------
 1 file changed, 21 insertions(+), 49 deletions(-)

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index fcb96c44d954..4357364e611e 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -656,7 +656,7 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
 			 const struct ltc2983_sensor *sensor)
 {
 	struct ltc2983_thermocouple *thermo;
-	struct fwnode_handle *ref;
+	struct fwnode_handle *ref __free(fwnode_handle) = NULL;
 	u32 oc_current;
 	int ret;
 
@@ -714,7 +714,7 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
 			 * the error right away.
 			 */
 			dev_err(&st->spi->dev, "Property reg must be given\n");
-			goto fail;
+			return ERR_PTR(ret);
 		}
 	}
 
@@ -725,22 +725,15 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
 		thermo->custom = __ltc2983_custom_sensor_new(st, child,
 							     propname, false,
 							     16384, true);
-		if (IS_ERR(thermo->custom)) {
-			ret = PTR_ERR(thermo->custom);
-			goto fail;
-		}
+		if (IS_ERR(thermo->custom))
+			return ERR_CAST(thermo->custom);
 	}
 
 	/* set common parameters */
 	thermo->sensor.fault_handler = ltc2983_thermocouple_fault_handler;
 	thermo->sensor.assign_chan = ltc2983_thermocouple_assign_chan;
 
-	fwnode_handle_put(ref);
 	return &thermo->sensor;
-
-fail:
-	fwnode_handle_put(ref);
-	return ERR_PTR(ret);
 }
 
 static struct ltc2983_sensor *
@@ -750,7 +743,7 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 	struct ltc2983_rtd *rtd;
 	int ret = 0;
 	struct device *dev = &st->spi->dev;
-	struct fwnode_handle *ref;
+	struct fwnode_handle *ref __free(fwnode_handle) = NULL;
 	u32 excitation_current = 0, n_wires = 0;
 
 	rtd = devm_kzalloc(dev, sizeof(*rtd), GFP_KERNEL);
@@ -766,7 +759,7 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 	ret = fwnode_property_read_u32(ref, "reg", &rtd->r_sense_chan);
 	if (ret) {
 		dev_err(dev, "Property reg must be given\n");
-		goto fail;
+		return ERR_PTR(ret);
 	}
 
 	ret = fwnode_property_read_u32(child, "adi,number-of-wires", &n_wires);
@@ -787,8 +780,7 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 			break;
 		default:
 			dev_err(dev, "Invalid number of wires:%u\n", n_wires);
-			ret = -EINVAL;
-			goto fail;
+			return ERR_PTR(-EINVAL);
 		}
 	}
 
@@ -798,8 +790,7 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 			if (n_wires == 2 || n_wires == 3) {
 				dev_err(dev,
 					"Rotation not allowed for 2/3 Wire RTDs");
-				ret = -EINVAL;
-				goto fail;
+				return ERR_PTR(-EINVAL);
 			}
 			rtd->sensor_config |= LTC2983_RTD_C_ROTATE(1);
 		} else {
@@ -829,16 +820,14 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 				"Invalid rsense chann:%d to use in kelvin rsense",
 				rtd->r_sense_chan);
 
-			ret = -EINVAL;
-			goto fail;
+			return ERR_PTR(-EINVAL);
 		}
 
 		if (sensor->chan < min || sensor->chan > max) {
 			dev_err(dev, "Invalid chann:%d for the rtd config",
 				sensor->chan);
 
-			ret = -EINVAL;
-			goto fail;
+			return ERR_PTR(-EINVAL);
 		}
 	} else {
 		/* same as differential case */
@@ -846,8 +835,7 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 			dev_err(&st->spi->dev,
 				"Invalid chann:%d for RTD", sensor->chan);
 
-			ret = -EINVAL;
-			goto fail;
+			return ERR_PTR(-EINVAL);
 		}
 	}
 
@@ -856,10 +844,8 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 		rtd->custom = __ltc2983_custom_sensor_new(st, child,
 							  "adi,custom-rtd",
 							  false, 2048, false);
-		if (IS_ERR(rtd->custom)) {
-			ret = PTR_ERR(rtd->custom);
-			goto fail;
-		}
+		if (IS_ERR(rtd->custom))
+			return ERR_CAST(rtd->custom);
 	}
 
 	/* set common parameters */
@@ -901,18 +887,13 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 			dev_err(&st->spi->dev,
 				"Invalid value for excitation current(%u)",
 				excitation_current);
-			ret = -EINVAL;
-			goto fail;
+			return ERR_PTR(-EINVAL);
 		}
 	}
 
 	fwnode_property_read_u32(child, "adi,rtd-curve", &rtd->rtd_curve);
 
-	fwnode_handle_put(ref);
 	return &rtd->sensor;
-fail:
-	fwnode_handle_put(ref);
-	return ERR_PTR(ret);
 }
 
 static struct ltc2983_sensor *
@@ -921,7 +902,7 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 {
 	struct ltc2983_thermistor *thermistor;
 	struct device *dev = &st->spi->dev;
-	struct fwnode_handle *ref;
+	struct fwnode_handle *ref __free(fwnode_handle) = NULL;
 	u32 excitation_current = 0;
 	int ret = 0;
 
@@ -938,7 +919,7 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 	ret = fwnode_property_read_u32(ref, "reg", &thermistor->r_sense_chan);
 	if (ret) {
 		dev_err(dev, "rsense channel must be configured...\n");
-		goto fail;
+		return ERR_PTR(ret);
 	}
 
 	if (fwnode_property_read_bool(child, "adi,single-ended")) {
@@ -958,8 +939,7 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 		dev_err(&st->spi->dev,
 			"Invalid chann:%d for differential thermistor",
 			sensor->chan);
-		ret = -EINVAL;
-		goto fail;
+		return ERR_PTR(-EINVAL);
 	}
 
 	/* check custom sensor */
@@ -978,10 +958,8 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 								 propname,
 								 steinhart,
 								 64, false);
-		if (IS_ERR(thermistor->custom)) {
-			ret = PTR_ERR(thermistor->custom);
-			goto fail;
-		}
+		if (IS_ERR(thermistor->custom))
+			return ERR_CAST(thermistor->custom);
 	}
 	/* set common parameters */
 	thermistor->sensor.fault_handler = ltc2983_common_fault_handler;
@@ -1005,8 +983,7 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 			    LTC2983_SENSOR_THERMISTOR_STEINHART) {
 				dev_err(&st->spi->dev,
 					"Auto Range not allowed for custom sensors\n");
-				ret = -EINVAL;
-				goto fail;
+				return ERR_PTR(-EINVAL);
 			}
 			thermistor->excitation_current = 0x0c;
 			break;
@@ -1047,16 +1024,11 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 			dev_err(&st->spi->dev,
 				"Invalid value for excitation current(%u)",
 				excitation_current);
-			ret = -EINVAL;
-			goto fail;
+			return ERR_PTR(-EINVAL);
 		}
 	}
 
-	fwnode_handle_put(ref);
 	return &thermistor->sensor;
-fail:
-	fwnode_handle_put(ref);
-	return ERR_PTR(ret);
 }
 
 static struct ltc2983_sensor *
-- 
2.43.0

Re: [PATCH 13/13] iio: temp: ltc2983: Use __free(fwnode_handle) to replace fwnode_handle_put() calls

[Nuno Sá]

On Sun, 2024-01-14 at 17:20 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This use of the new cleanup.h scope based freeing infrastructure allows
> us to exit directly from error conditions and in the good path with
> the reference obtained from fwnode_find_reference() (which may be an error
> pointer) automatically released.
> 
> Cc: Cosmin Tanislav <cosmin.tanislav@analog.com>
> Cc: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

I guess this one could also benefit from using dev_err_probe(). Anyways:

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

I might also be able to test this one...

>  drivers/iio/temperature/ltc2983.c | 70 ++++++++++---------------------
>  1 file changed, 21 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/iio/temperature/ltc2983.c
> b/drivers/iio/temperature/ltc2983.c
> index fcb96c44d954..4357364e611e 100644
> --- a/drivers/iio/temperature/ltc2983.c
> +++ b/drivers/iio/temperature/ltc2983.c
> @@ -656,7 +656,7 @@ ltc2983_thermocouple_new(const struct fwnode_handle
> *child, struct ltc2983_data
>  			 const struct ltc2983_sensor *sensor)
>  {
>  	struct ltc2983_thermocouple *thermo;
> -	struct fwnode_handle *ref;
> +	struct fwnode_handle *ref __free(fwnode_handle) = NULL;
>  	u32 oc_current;
>  	int ret;
>  
> @@ -714,7 +714,7 @@ ltc2983_thermocouple_new(const struct fwnode_handle
> *child, struct ltc2983_data
>  			 * the error right away.
>  			 */
>  			dev_err(&st->spi->dev, "Property reg must be
> given\n");
> -			goto fail;
> +			return ERR_PTR(ret);
>  		}
>  	}
>  
> @@ -725,22 +725,15 @@ ltc2983_thermocouple_new(const struct fwnode_handle
> *child, struct ltc2983_data
>  		thermo->custom = __ltc2983_custom_sensor_new(st, child,
>  							     propname, false,
>  							     16384, true);
> -		if (IS_ERR(thermo->custom)) {
> -			ret = PTR_ERR(thermo->custom);
> -			goto fail;
> -		}
> +		if (IS_ERR(thermo->custom))
> +			return ERR_CAST(thermo->custom);
>  	}
>  
>  	/* set common parameters */
>  	thermo->sensor.fault_handler = ltc2983_thermocouple_fault_handler;
>  	thermo->sensor.assign_chan = ltc2983_thermocouple_assign_chan;
>  
> -	fwnode_handle_put(ref);
>  	return &thermo->sensor;
> -
> -fail:
> -	fwnode_handle_put(ref);
> -	return ERR_PTR(ret);
>  }
>  
>  static struct ltc2983_sensor *
> @@ -750,7 +743,7 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct
> ltc2983_data *st,
>  	struct ltc2983_rtd *rtd;
>  	int ret = 0;
>  	struct device *dev = &st->spi->dev;
> -	struct fwnode_handle *ref;
> +	struct fwnode_handle *ref __free(fwnode_handle) = NULL;
>  	u32 excitation_current = 0, n_wires = 0;
>  
>  	rtd = devm_kzalloc(dev, sizeof(*rtd), GFP_KERNEL);
> @@ -766,7 +759,7 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct
> ltc2983_data *st,
>  	ret = fwnode_property_read_u32(ref, "reg", &rtd->r_sense_chan);
>  	if (ret) {
>  		dev_err(dev, "Property reg must be given\n");
> -		goto fail;
> +		return ERR_PTR(ret);
>  	}
>  
>  	ret = fwnode_property_read_u32(child, "adi,number-of-wires",
> &n_wires);
> @@ -787,8 +780,7 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct
> ltc2983_data *st,
>  			break;
>  		default:
>  			dev_err(dev, "Invalid number of wires:%u\n",
> n_wires);
> -			ret = -EINVAL;
> -			goto fail;
> +			return ERR_PTR(-EINVAL);
>  		}
>  	}
>  
> @@ -798,8 +790,7 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct
> ltc2983_data *st,
>  			if (n_wires == 2 || n_wires == 3) {
>  				dev_err(dev,
>  					"Rotation not allowed for 2/3 Wire
> RTDs");
> -				ret = -EINVAL;
> -				goto fail;
> +				return ERR_PTR(-EINVAL);
>  			}
>  			rtd->sensor_config |= LTC2983_RTD_C_ROTATE(1);
>  		} else {
> @@ -829,16 +820,14 @@ ltc2983_rtd_new(const struct fwnode_handle *child,
> struct ltc2983_data *st,
>  				"Invalid rsense chann:%d to use in kelvin
> rsense",
>  				rtd->r_sense_chan);
>  
> -			ret = -EINVAL;
> -			goto fail;
> +			return ERR_PTR(-EINVAL);
>  		}
>  
>  		if (sensor->chan < min || sensor->chan > max) {
>  			dev_err(dev, "Invalid chann:%d for the rtd config",
>  				sensor->chan);
>  
> -			ret = -EINVAL;
> -			goto fail;
> +			return ERR_PTR(-EINVAL);
>  		}
>  	} else {
>  		/* same as differential case */
> @@ -846,8 +835,7 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct
> ltc2983_data *st,
>  			dev_err(&st->spi->dev,
>  				"Invalid chann:%d for RTD", sensor->chan);
>  
> -			ret = -EINVAL;
> -			goto fail;
> +			return ERR_PTR(-EINVAL);
>  		}
>  	}
>  
> @@ -856,10 +844,8 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct
> ltc2983_data *st,
>  		rtd->custom = __ltc2983_custom_sensor_new(st, child,
>  							  "adi,custom-rtd",
>  							  false, 2048,
> false);
> -		if (IS_ERR(rtd->custom)) {
> -			ret = PTR_ERR(rtd->custom);
> -			goto fail;
> -		}
> +		if (IS_ERR(rtd->custom))
> +			return ERR_CAST(rtd->custom);
>  	}
>  
>  	/* set common parameters */
> @@ -901,18 +887,13 @@ ltc2983_rtd_new(const struct fwnode_handle *child,
> struct ltc2983_data *st,
>  			dev_err(&st->spi->dev,
>  				"Invalid value for excitation current(%u)",
>  				excitation_current);
> -			ret = -EINVAL;
> -			goto fail;
> +			return ERR_PTR(-EINVAL);
>  		}
>  	}
>  
>  	fwnode_property_read_u32(child, "adi,rtd-curve", &rtd->rtd_curve);
>  
> -	fwnode_handle_put(ref);
>  	return &rtd->sensor;
> -fail:
> -	fwnode_handle_put(ref);
> -	return ERR_PTR(ret);
>  }
>  
>  static struct ltc2983_sensor *
> @@ -921,7 +902,7 @@ ltc2983_thermistor_new(const struct fwnode_handle *child,
> struct ltc2983_data *s
>  {
>  	struct ltc2983_thermistor *thermistor;
>  	struct device *dev = &st->spi->dev;
> -	struct fwnode_handle *ref;
> +	struct fwnode_handle *ref __free(fwnode_handle) = NULL;
>  	u32 excitation_current = 0;
>  	int ret = 0;
>  
> @@ -938,7 +919,7 @@ ltc2983_thermistor_new(const struct fwnode_handle *child,
> struct ltc2983_data *s
>  	ret = fwnode_property_read_u32(ref, "reg", &thermistor-
> >r_sense_chan);
>  	if (ret) {
>  		dev_err(dev, "rsense channel must be configured...\n");
> -		goto fail;
> +		return ERR_PTR(ret);
>  	}
>  
>  	if (fwnode_property_read_bool(child, "adi,single-ended")) {
> @@ -958,8 +939,7 @@ ltc2983_thermistor_new(const struct fwnode_handle *child,
> struct ltc2983_data *s
>  		dev_err(&st->spi->dev,
>  			"Invalid chann:%d for differential thermistor",
>  			sensor->chan);
> -		ret = -EINVAL;
> -		goto fail;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	/* check custom sensor */
> @@ -978,10 +958,8 @@ ltc2983_thermistor_new(const struct fwnode_handle *child,
> struct ltc2983_data *s
>  								 propname,
>  								 steinhart,
>  								 64, false);
> -		if (IS_ERR(thermistor->custom)) {
> -			ret = PTR_ERR(thermistor->custom);
> -			goto fail;
> -		}
> +		if (IS_ERR(thermistor->custom))
> +			return ERR_CAST(thermistor->custom);
>  	}
>  	/* set common parameters */
>  	thermistor->sensor.fault_handler = ltc2983_common_fault_handler;
> @@ -1005,8 +983,7 @@ ltc2983_thermistor_new(const struct fwnode_handle *child,
> struct ltc2983_data *s
>  			    LTC2983_SENSOR_THERMISTOR_STEINHART) {
>  				dev_err(&st->spi->dev,
>  					"Auto Range not allowed for custom
> sensors\n");
> -				ret = -EINVAL;
> -				goto fail;
> +				return ERR_PTR(-EINVAL);
>  			}
>  			thermistor->excitation_current = 0x0c;
>  			break;
> @@ -1047,16 +1024,11 @@ ltc2983_thermistor_new(const struct fwnode_handle
> *child, struct ltc2983_data *s
>  			dev_err(&st->spi->dev,
>  				"Invalid value for excitation current(%u)",
>  				excitation_current);
> -			ret = -EINVAL;
> -			goto fail;
> +			return ERR_PTR(-EINVAL);
>  		}
>  	}
>  
> -	fwnode_handle_put(ref);
>  	return &thermistor->sensor;
> -fail:
> -	fwnode_handle_put(ref);
> -	return ERR_PTR(ret);
>  }
>  
>  static struct ltc2983_sensor *

Re: [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.

[Andy Shevchenko]

On Sun, Jan 14, 2024 at 05:19:56PM +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Chances since RFC.
> - Add if (!IS_ERR_OR_NULL(_T)) check as suggested by Andy Shevchenko.
>   This may allow the compiler to optimize cases where it can tell that
>   this check will fail rather than calling into fwnode_handle_put().

FWIW, this nuance is mentioned in [1] as:

  "Within the macro, this declaration is creating a new function called
  __free_kfree() that makes a call to kfree() if the passed-in pointer is not
  NULL. Nobody will ever call that function directly, but the declaration makes
  it possible to write code like: ..."

Not so explicit, but still...

[1]: https://lwn.net/Articles/934679/

-- 
With Best Regards,
Andy Shevchenko