device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
Subsystems (13)
Jonathan Cameron
maintainerv2 cover device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
From: Jonathan Cameron <[email protected]>
Changes since v1:
- Introduced device_for_each_child_node_scoped()
We may need equivalents for fwnode_for_each_child_node_scoped() etc, but this is the only one I needed so far.
This followed from a discussion of the equivalent patch set for device_for_each_of_node() which lead to bringing the declaration of the handle we are applying the __free() to into the for_* loop initialization. The avoided issues with the declaration (which also effects cleanup order) being nowhere near where it was first set to something non NULL. The disadvantage is that the declaration of that local variable is not obvious from the macro parameters. Bugs due to variable shadowing might occur, though in many cases those are apparent as compiler warnings about use of uninitialized variables.
- Reordered patches to drag the ltc2983 which is teh one case that wasn't a loop next to the patch that enables that simpler handling. Also move the struct fwnode_handle *ref declarations to where they are intialized. This may look odd, but Linus and others have stated this is how they prefer this to be done.
- Converted the one instance of fwnode_for_each_available_child_node() over to device_for_each_child_node_scoped() as it never needed to be the fwnode version in the first place - that was probably a misunderstanding of _available_ or not.
- Dropped tags other than Andy's on the first patch (as that was unchanged other than simplifying the patch description). The code changed too much for me to carry them forwards.
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 1st and 3rd patches on it, so that we can start making use of this in other areas of the kernel without having to wait too long.
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.
Julia Lawal has posted some nice coccinelle magic for the DT equivalents. Referenced from that cover letter. Similar may help us convert more drivers to use this new approach, but often hand tweaking can take additional advantage of other cleanup.h based magic, or things like return dev_err_probe().
https://lore.kernel.org/all/[email protected]/
Jonathan Cameron (14):
device property: Add cleanup.h based fwnode_handle_put() scope based
cleanup.
iio: temp: ltc2983: Use __free(fwnode_handle) to replace
fwnode_handle_put() calls
device property: Introduce device_for_each_child_node_scoped()
iio: adc: max11410: Use device_for_each_child_node_scoped()
iio: adc: mcp3564: Use device_for_each_child_node_scopd()
iio: adc: qcom-spmi-adc5: Use device_for_each_child_node_scopd()
iio: adc: rzg2l_adc: Use device_for_each_child_node_scopd()
iio: adc: stm32: Use device_for_each_child_node_scoped()
iio: adc: ti-ads1015: Use device_for_each_child_node_scoped()
iio: adc: ti-ads131e08: Use device_for_each_child_node_scoped()
iio: addac: ad74413r: Use device_for_each_child_node_scoped()
iio: dac: ad3552r: Use device_for_each_child_node_scoped()
iio: dac: ad5770r: Use device_for_each_child_node_scoped()
iio: dac: ltc2688: Use device_for_each_child_node_scoped()
drivers/iio/adc/max11410.c | 27 +++-------- drivers/iio/adc/mcp3564.c | 16 +++---- drivers/iio/adc/qcom-spmi-adc5.c | 7 +-- drivers/iio/adc/rzg2l_adc.c | 11 ++--- drivers/iio/adc/stm32-adc.c | 63 ++++++++++--------------- drivers/iio/adc/ti-ads1015.c | 5 +- drivers/iio/adc/ti-ads131e08.c | 13 ++---- drivers/iio/addac/ad74413r.c | 10 +--- drivers/iio/dac/ad3552r.c | 51 ++++++++------------- drivers/iio/dac/ad5770r.c | 19 +++----- drivers/iio/dac/ltc2688.c | 24 +++------- drivers/iio/temperature/ltc2983.c | 76 ++++++++++--------------------- include/linux/property.h | 8 ++++ 13 files changed, 113 insertions(+), 217 deletions(-)
Signature · 1 line
2.43.1
Jonathan Cameron
maintainerPATCH v2 1/14 device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
From: Jonathan Cameron <[email protected]>
Useful 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 and will automatically release the reference on the variable leaving scope.
--- include/linux/property.h | 3 +++ 1 file changed, 3 insertions(+)
include/linux/property.h +3 -0
@@ -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>
@@ -188,6 +189,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);
Signature · 1 line
2.43.1
Sakari Ailus
reviewerRe: [PATCH v2 01/14] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
Hi Jonathan,
Quoted 26 lines · Jonathan Cameron
From: Jonathan Cameron <[email protected]>
Useful where the
fwnode_handlewas obtained from a call such asfwnode_find_reference()as it will safely do nothing ifIS_ERR()is true and will automatically release the reference on the variable leaving scope.Reviewed-by Andy Shevchenko <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>--- include/linux/property.h | 3 +++ 1 file changed, 3 insertions(+)include/linux/property.h +1
-0@@ -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>
include/linux/property.h struct fwnode_handle *device_get_named_child_node(const struct device *dev, · line 193
Quoted 5 lines
include/linux/property.h +2
-0@@ -188,6 +189,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))
fwnode_handle_put() can be safely called on NULL or error pointer fwnode so you can remove the check.
Quoted 3 lines
include/linux/property.h +0
-0int fwnode_irq_get (const struct fwnode_handle *fwnode,unsigned int index);int fwnode_irq_get_byname (const struct fwnode_handle *fwnode,const char *name);
Signature · 3 lines
Kind regards, Sakari Ailus
Re: [PATCH v2 01/14] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
Quoted 30 lines · On Mon, 12 Feb 2024 08:49:23 +0000 Sakari Ailus <[email protected]>…
Hi Jonathan,
Quoted 27 lines · Jonathan Cameron
From: Jonathan Cameron <[email protected]>
Useful where the
fwnode_handlewas obtained from a call such asfwnode_find_reference()as it will safely do nothing ifIS_ERR()is true and will automatically release the reference on the variable leaving scope.Reviewed-by Andy Shevchenko <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>--- include/linux/property.h | 3 +++ 1 file changed, 3 insertions(+)include/linux/property.h +1
-0@@ -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> @@ -188,6 +189,8 @@ struct fwnode_handle *device_get_named_child_node(const struct device *dev,
Quoted 7 lines
include/linux/property.h struct fwnode_handle *device_get_named_child_node(const struct device *dev, · line 193
Quoted 4 lines
include/linux/property.h +2
-0struct 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))
fwnode_handle_put()can be safely called on NULL or error pointer fwnode so you can remove the check.
Was discussed in the RFC thread (where i didn't have this protection)
https://lore.kernel.org/linux-iio/[email protected]/
includes a reference to Linus Torvald's view on this.
All comes down to compiler visibility and optimization opportunities, which are improved if the check is in the macro definition.
Jonathan
Quoted 5 lines
Quoted 4 lines
include/linux/property.h +0
-0int fwnode_irq_get (const struct fwnode_handle *fwnode,unsigned int index);int fwnode_irq_get_byname (const struct fwnode_handle *fwnode,const char *name);
Sakari Ailus
reviewerRe: [PATCH v2 01/14] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
Hi Jonathan,
Quoted 41 lines · Jonathan Cameron
Quoted 30 lines · On Mon, 12 Feb 2024 08:49:23 +0000 Sakari Ailus <[email protected]>…
Hi Jonathan,
Quoted 27 lines · Jonathan Cameron
From: Jonathan Cameron <[email protected]>
Useful where the
fwnode_handlewas obtained from a call such asfwnode_find_reference()as it will safely do nothing ifIS_ERR()is true and will automatically release the reference on the variable leaving scope.Reviewed-by Andy Shevchenko <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>--- include/linux/property.h | 3 +++ 1 file changed, 3 insertions(+)include/linux/property.h +1
-0@@ -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> @@ -188,6 +189,8 @@ struct fwnode_handle *device_get_named_child_node(const struct device *dev,Quoted 7 lines
include/linux/property.h struct fwnode_handle *device_get_named_child_node(const struct device *dev, · line 193
Quoted 4 lines
include/linux/property.h +2
-0struct 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))
fwnode_handle_put()can be safely called on NULL or error pointer fwnode so you can remove the check.
Quoted 7 lines
Was discussed in the RFC thread (where i didn't have this protection)
https://lore.kernel.org/linux-iio/[email protected]/
includes a reference to Linus Torvald's view on this.All comes down to compiler visibility and optimization opportunities, which are improved if the check is in the macro definition.
Hmm. In that case I'd rather make fwnode_handle_put() and similar trivial functions macros.
There's no need to add cruft here and about a 100-fold number of callers will get the same benefit.
Signature · 1 line
Sakari Ailus
Andy Shevchenko
reviewerRe: [PATCH v2 01/14] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
Quoted 1 line · Sakari Ailus
On Mon, Feb 12, 2024 at 11:42:06AM +0000, Jonathan Cameron wrote:
...
Quoted 2 lines
Hmm. In that case I'd rather make
fwnode_handle_put()and similar trivial functions macros.
This will kill the type-checking opportunity, so I'm against this move.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Sakari Ailus
reviewerRe: [PATCH v2 01/14] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
Quoted 9 lines · Andy Shevchenko
Quoted 1 line · Sakari Ailus
On Mon, Feb 12, 2024 at 11:42:06AM +0000, Jonathan Cameron wrote:
...
Quoted 2 lines
Hmm. In that case I'd rather make
fwnode_handle_put()and similar trivial functions macros.This will kill the type-checking opportunity, so I'm against this move.
Then it could be made static inline and moved to the header. I suppose for modern compilers there should be no difference in between the two optimisation-wise.
Signature · 1 line
Sakari Ailus
Re: [PATCH v2 01/14] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
Quoted 6 lines · On Mon, 12 Feb 2024 12:58:03 +0000 Sakari Ailus <[email protected]>…
Quoted 5 lines · Andy Shevchenko
Quoted 1 line · Sakari Ailus
On Mon, Feb 12, 2024 at 11:42:06AM +0000, Jonathan Cameron wrote:
...
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
Hmm. In that case I'd rather make
fwnode_handle_put()and similar trivial functions macros.This will kill the type-checking opportunity, so I'm against this move.
Then it could be made static inline and moved to the header. I suppose for modern compilers there should be no difference in between the two optimisation-wise.
Sure - will be a bit fiddly as this is only worth doing if we drop the internal check that buried several macros deep.
1. rename existing fwnode_handle_put() to __fwnode_handle_put()
2. Make __fwnode_handle_put() call a new set of macros
#define fwnode_has_op_nocheck(fwnode, op) \ (fwnode)->ops && (fwnode)->ops->op
#define fwnode_call_void_op_nocheck(fwnode, op, .... \
do {
if (fwnode_had_op_nocheck(fwnode, op)) \
(fwnode)->ops->op(fwnode, ## __VA_ARGS__);
} while (false);3. Add new
static inline fwnode_handle_put(struct fwnode_handle *fwnode)
{
if (!IS_ERR_OR_NULL(fwnode))
__fwnode_handle_put(fwnode);
}Or something like that.
I'm fine with doing that if conclusion is the complexity of the change is worth it.
Jonathan
Re: [PATCH v2 01/14] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
Quoted 14 lines · On Tue, 13 Feb 2024 10:22:45 +0000 Jonathan Cameron…
Quoted 11 lines · On Mon, 12 Feb 2024 12:58:03 +0000 Sakari Ailus <[email protected]>…
Quoted 9 lines · Andy Shevchenko
Quoted 1 line · Sakari Ailus
On Mon, Feb 12, 2024 at 11:42:06AM +0000, Jonathan Cameron wrote:
...
Quoted 2 lines
Hmm. In that case I'd rather make
fwnode_handle_put()and similar trivial functions macros.This will kill the type-checking opportunity, so I'm against this move.
Quoted 7 lines
Quoted 4 lines
Then it could be made static inline and moved to the header. I suppose for modern compilers there should be no difference in between the two optimisation-wise.
Sure - will be a bit fiddly as this is only worth doing if we drop the internal check that buried several macros deep.
Not enough coffee yesterday. We can just move the the existing fwnode_handle_put() to property.h as that includes fwnode.h has all the definitions in it which we need to be able to see.
I think that should be uncontroversial?
Jonathan
Quoted 26 lines
1. rename existing
fwnode_handle_put()to__fwnode_handle_put()
2. Make__fwnode_handle_put()call a new set of macros#define fwnode_has_op_nocheck(fwnode, op) \ (fwnode)->ops && (fwnode)->ops->op#define fwnode_call_void_op_nocheck(fwnode, op, .... \ do { if (fwnode_had_op_nocheck(fwnode, op)) \ (fwnode)->ops->op(fwnode, ## __VA_ARGS__); } while (false);3. Add new
static inlinefwnode_handle_put(struct fwnode_handle *fwnode){ if (!IS_ERR_OR_NULL(fwnode)) __fwnode_handle_put(fwnode); }Or something like that.
I'm fine with doing that if conclusion is the complexity of the change is worth it.
Jonathan
Sakari Ailus
reviewerRe: [PATCH v2 01/14] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
Quoted 22 lines · Jonathan Cameron
Quoted 19 lines · On Tue, 13 Feb 2024 10:22:45 +0000 Jonathan Cameron…
Quoted 15 lines · On Mon, 12 Feb 2024 12:58:03 +0000 Sakari Ailus <[email protected]>…
Quoted 9 lines · Andy Shevchenko
Quoted 1 line · Sakari Ailus
On Mon, Feb 12, 2024 at 11:42:06AM +0000, Jonathan Cameron wrote:
...
Quoted 2 lines
Hmm. In that case I'd rather make
fwnode_handle_put()and similar trivial functions macros.This will kill the type-checking opportunity, so I'm against this move.
Then it could be made static inline and moved to the header. I suppose for modern compilers there should be no difference in between the two optimisation-wise.
Quoted 8 lines
Quoted 2 lines
Sure - will be a bit fiddly as this is only worth doing if we drop the internal check that buried several macros deep.
Not enough coffee yesterday. We can just move the the existing
fwnode_handle_put()toproperty.has that includesfwnode.hhas all the definitions in it which we need to be able to see.I think that should be uncontroversial?
I agree.
Signature · 1 line
Sakari Ailus
Andy Shevchenko
reviewerRe: [PATCH v2 01/14] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
In reply to Sakari AilusQuoted 1 line · Sakari Ailus
On Sun, Feb 11, 2024 at 07:25:27PM +0000, Jonathan Cameron wrote:
...
Quoted 4 lines
include/linux/property.h struct fwnode_handle *device_get_named_child_node(const struct device *dev, · line 193
Quoted 2 lines
include/linux/property.h +2
-0+DEFINE_FREE(fwnode_handle, struct fwnode_handle *, + if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T))
fwnode_handle_put()can be safely called on NULL or error pointer fwnode
Yes.
Quoted 1 line
so you can remove the check.
No. (Technically "yes", but better "no".)
This has been discussed a lot, including the LWN wrap-up about the cleanup.h.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Andy Shevchenko
reviewerRe: [PATCH v2 01/14] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
In reply to Jonathan CameronQuoted 6 lines · Jonathan Cameron
From: Jonathan Cameron <[email protected]>
Useful where the
fwnode_handlewas obtained from a call such asfwnode_find_reference()as it will safely do nothing ifIS_ERR()is true and will automatically release the reference on the variable leaving scope.
...
Quoted 2 lines
struct
fwnode_handle*fwnode_handle_get(structfwnode_handle*fwnode); voidfwnode_handle_put(struct fwnode_handle *fwnode);
I would add a blank line here, but it's a minor comment anyway.
Quoted 2 lines
include/linux/property.h +2
-0+DEFINE_FREE(fwnode_handle, struct fwnode_handle *, + if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T))
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerPATCH v2 2/14 iio: temp: ltc2983: Use __free(fwnode_handle) to replace fwnode_handle_put() calls
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
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.
---
v2: Move the declarations down to where they are assigned. This both clearly associates the cleanup with the action that it is cleaning up and avoid potential future missordering of cleanup.
--- drivers/iio/temperature/ltc2983.c | 76 ++++++++++--------------------- 1 file changed, 24 insertions(+), 52 deletions(-)
drivers/iio/temperature/ltc2983.c +24 -52
@@ -656,7 +656,6 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data const struct ltc2983_sensor *sensor) {struct ltc2983_thermocouple *thermo; -struct fwnode_handle *ref;u32 oc_current;int ret;
@@ -703,7 +702,8 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_datareturn ERR_PTR (-EINVAL ); } - ref =fwnode_find_reference (child,"adi,cold-junction-handle" ,0 ); + struct fwnode_handle *ref __free(fwnode_handle) = +fwnode_find_reference (child,"adi,cold-junction-handle" ,0 );if (IS_ERR (ref)) { ref =NULL ; }else {
@@ -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,14 +743,14 @@ 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;u32 excitation_current =0 , n_wires =0 ; rtd =devm_kzalloc (dev,sizeof (*rtd),GFP_KERNEL );if (!rtd)return ERR_PTR (-ENOMEM ); - ref =fwnode_find_reference (child,"adi,rsense-handle" ,0 ); + struct fwnode_handle *ref __free(fwnode_handle) = +fwnode_find_reference (child,"adi,rsense-handle" ,0 );if (IS_ERR (ref)) {dev_err (dev,"Property adi,rsense-handle missing or invalid" );return ERR_CAST (ref);
@@ -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,6 @@ 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;u32 excitation_current =0 ;int ret =0 ;
@@ -929,7 +909,8 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *sif (!thermistor)return ERR_PTR (-ENOMEM ); - ref =fwnode_find_reference (child,"adi,rsense-handle" ,0 ); + struct fwnode_handle *ref __free(fwnode_handle) = +fwnode_find_reference (child,"adi,rsense-handle" ,0 );if (IS_ERR (ref)) {dev_err (dev,"Property adi,rsense-handle missing or invalid" );return ERR_CAST (ref);
@@ -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 *sdev_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 *sdev_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 *
Signature · 1 line
2.43.1
Jonathan Cameron
maintainerPATCH v2 3/14 device property: Introduce device_for_each_child_node_scoped()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Similar to recently propose for_each_child_of_node_scoped() this new version of the loop macro instantiates a new local struct fwnode_handle * that uses the __free(fwnode_handle) auto cleanup handling so that if a reference to a node is held on early exit from the loop the reference will be released. If the loop runs to completion, the child pointer will be NULL and no action will be taken.
The reason this is useful is that it removes the need for fwnode_handle_put() on early loop exits. If there is a need to retain the reference, then return_ptr(child) or no_free_ptr(child) may be used to safely disable the auto cleanup.
--- include/linux/property.h | 5 +++++ 1 file changed, 5 insertions(+)
include/linux/property.h +5 -0
@@ -182,6 +182,11 @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev,for (child =device_get_next_child_node (dev,NULL ); child; \ child = device_get_next_child_node(dev, child)) +#define device_for_each_child_node_scoped (dev, child)\ + for (struct fwnode_handle *child __free(fwnode_handle) = \ + device_get_next_child_node(dev, NULL); child; \ + child = device_get_next_child_node(dev, child)) +struct fwnode_handle *fwnode_get_named_child_node (const struct fwnode_handle *fwnode,const char *childname); struct fwnode_handle *device_get_named_child_node(const struct device *dev,
Signature · 1 line
2.43.1
Re: [PATCH v2 03/14] device property: Introduce device_for_each_child_node_scoped()
Quoted 14 lines · Jonathan Cameron
From: Jonathan Cameron <[email protected]>
Similar to recently propose
for_each_child_of_node_scoped()this new version of the loop macro instantiates a new local structfwnode_handle* that uses the__free(fwnode_handle)auto cleanup handling so that if a reference to a node is held on early exit from the loop the reference will be released. If the loop runs to completion, the child pointer will be NULL and no action will be taken.The reason this is useful is that it removes the need for
fwnode_handle_put()on early loop exits. If there is a need to retain the reference, thenreturn_ptr(child)orno_free_ptr(child)may be used to safely disable the auto cleanup.
...
Quoted 1 line
+#define
device_for_each_child_node_scoped(dev, child)\
Missing space before backslash, but I would rather to make them to be TABed to the same column.
include/linux/property.h struct fwnode_handle *device_get_next_child_node(const struct device *dev, · line 187
Quoted 2 lines
include/linux/property.h +2
-0+ for (struct fwnode_handle *child __free(fwnode_handle) = \ + device_get_next_child_node(dev, NULL); child; \
Please, move child to a separate line, so we will easily see the all three parameters of the for-loop. That said, indent the assignment to the right as well.
include/linux/property.h struct fwnode_handle *device_get_next_child_node(const struct device *dev, · line 188
Quoted 1 line
include/linux/property.h +1
-0+ child = device_get_next_child_node(dev, child))
With the above addressed,
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 03/14] device property: Introduce device_for_each_child_node_scoped()
Quoted 16 lines · On Mon, 12 Feb 2024 14:10:57 +0200 Andy Shevchenko…
Quoted 10 lines · Jonathan Cameron
From: Jonathan Cameron <[email protected]>
Similar to recently propose
for_each_child_of_node_scoped()this new version of the loop macro instantiates a new local structfwnode_handle* that uses the__free(fwnode_handle)auto cleanup handling so that if a reference to a node is held on early exit from the loop the reference will be released. If the loop runs to completion, the child pointer will be NULL and no action will be taken.Quoted 4 lines
The reason this is useful is that it removes the need for
fwnode_handle_put()on early loop exits. If there is a need to retain the reference, thenreturn_ptr(child)orno_free_ptr(child)may be used to safely disable the auto cleanup.
Quoted 6 lines
...
Quoted 1 line
+#define
device_for_each_child_node_scoped(dev, child)\Missing space before backslash, but I would rather to make them to be TABed to the same column.
Oops. I spotted I messed this up bug clearly failed to fix it before sending out.
Quoted 7 lines
include/linux/property.h struct fwnode_handle *device_get_next_child_node(const struct device *dev, · line 187
Quoted 3 lines
include/linux/property.h +2
-0+ for (struct fwnode_handle *child __free(fwnode_handle) = \ + device_get_next_child_node(dev, NULL); child; \Please, move child to a separate line, so we will easily see the all three parameters of the for-loop. That said, indent the assignment to the right as well.
Indent makes sense - but (to save another respin) how far? Next tab stop will be a bit random looking but I guess nothing else makes more sense.
Quoted 5 lines
include/linux/property.h struct fwnode_handle *device_get_next_child_node(const struct device *dev, · line 188
Quoted 2 lines
include/linux/property.h +1
-0+ child = device_get_next_child_node(dev, child))With the above addressed,
Reviewed-by Andy Shevchenko <[email protected]>
Thanks,
Quoted 1 line
Andy Shevchenko
reviewerRe: [PATCH v2 03/14] device property: Introduce device_for_each_child_node_scoped()
Quoted 3 lines · Jonathan Cameron
Quoted 1 line · On Mon, 12 Feb 2024 14:10:57 +0200 Andy Shevchenko…
On Sun, Feb 11, 2024 at 07:25:29PM +0000, Jonathan Cameron wrote:
...
Quoted 5 lines
Quoted 5 lines
include/linux/property.h struct fwnode_handle *device_get_next_child_node(const struct device *dev, · line 187
Quoted 2 lines
include/linux/property.h +2
-0+ for (struct fwnode_handle *child __free(fwnode_handle) = \ + device_get_next_child_node(dev, NULL); child; \Please, move child to a separate line, so we will easily see the all three parameters of the for-loop.
Oh, I should withdraw above, we have other for_each macros there with a child being combined with previous line.
Quoted 6 lines
Quoted 2 lines
That said, indent the assignment to the right as
well.Indent makes sense - but (to save another respin) how far? Next tab stop will be a bit random looking but I guess nothing else makes more sense.
Just make whatever TAB stop that doesn't require adding any spaces.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerRe: [PATCH v2 03/14] device property: Introduce device_for_each_child_node_scoped()
Quoted 7 lines · On Tue, 13 Feb 2024 19:12:46 +0200 Andy Shevchenko…
Quoted 3 lines · Jonathan Cameron
Quoted 1 line · On Mon, 12 Feb 2024 14:10:57 +0200 Andy Shevchenko…
On Sun, Feb 11, 2024 at 07:25:29PM +0000, Jonathan Cameron wrote:
...
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
include/linux/property.h struct fwnode_handle *device_get_next_child_node(const struct device *dev, · line 187
Quoted 2 lines
include/linux/property.h +2
-0+ for (struct fwnode_handle *child __free(fwnode_handle) = \ + device_get_next_child_node(dev, NULL); child; \Please, move child to a separate line, so we will easily see the all three parameters of the for-loop.
Oh, I should withdraw above, we have other
for_eachmacros there with a child being combined with previous line.
I ended up moving it down to the next line (so it shares with the update term).
That seemed better than having it on the end of the line that is still finishing the initialization term and felt similar enough to local style.
Quoted 10 lines
Quoted 7 lines
Quoted 3 lines
That said, indent the assignment to the right as
well.Indent makes sense - but (to save another respin) how far? Next tab stop will be a bit random looking but I guess nothing else makes more sense.
Just make whatever TAB stop that doesn't require adding any spaces.
Jonathan Cameron
maintainerPATCH v2 4/14 iio: adc: max11410: Use device_for_each_child_node_scoped()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Switching to the _scoped() version removes the need for manual calling of fwnode_handle_put() in the paths where the code exits the loop early. In this case that's all in error paths.
--- drivers/iio/adc/max11410.c | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-)
drivers/iio/adc/max11410.c +7 -20
@@ -696,7 +696,6 @@ 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;u32 reference, sig_path;const char *node_name;u32 inputs[2 ], scale;
@@ -720,7 +719,7 @@ static int max11410_parse_channels(struct max11410_state *st,if (!st->channels)return -ENOMEM ; -device_for_each_child_node (dev, child) { +device_for_each_child_node_scoped (dev, child) { node_name =fwnode_get_name (child);if (fwnode_property_present (child,"diff-channels" )) { ret = fwnode_property_read_u32_array(child,
@@ -735,47 +734,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 +782,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++) {
Signature · 1 line
2.43.1
Jonathan Cameron
maintainerPATCH v2 5/14 iio: adc: mcp3564: Use device_for_each_child_node_scopd()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Switching to the _scoped() version removes the need for manual calling of fwnode_handle_put() in the paths where the code exits the loop early. In this case that's all in error paths.
--- drivers/iio/adc/mcp3564.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
drivers/iio/adc/mcp3564.c +7 -9
@@ -998,7 +998,6 @@ 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 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;
@@ -1025,7 +1024,7 @@ static int mcp3564_parse_fw_children(struct iio_dev *indio_dev)if (!channels)return dev_err_probe (dev, -ENOMEM ,"Can't allocate memory\n" ); -device_for_each_child_node (dev, child) { +device_for_each_child_node_scoped (dev, child) { node_name =fwnode_get_name (child);if (fwnode_property_present (child,"diff-channels" )) {
@@ -1033,26 +1032,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 ];
Signature · 1 line
2.43.1
Jonathan Cameron
maintainerPATCH v2 6/14 iio: adc: qcom-spmi-adc5: Use device_for_each_child_node_scopd()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Switching to the _scoped() version removes the need for manual calling of fwnode_handle_put() in the paths where the code exits the loop early. In this case that's all in error paths.
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.
--- drivers/iio/adc/qcom-spmi-adc5.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/iio/adc/qcom-spmi-adc5.c +2 -5
@@ -825,7 +825,6 @@ 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;unsigned int index =0 ;int ret;
@@ -849,12 +848,10 @@ static int adc5_get_fw_data(struct adc5_chip *adc)if (!adc->data) adc->data = &adc5_data_pmic; -device_for_each_child_node (adc->dev, child) { +device_for_each_child_node_scoped (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;
Signature · 1 line
2.43.1
Re: [PATCH v2 06/14] iio: adc: qcom-spmi-adc5: Use device_for_each_child_node_scopd()
Quoted 13 lines · Jonathan Cameron
From: Jonathan Cameron <[email protected]>
Switching to the
_scoped()version removes the need for manual calling offwnode_handle_put()in the paths where the code exits the loop early. In this case that's all in error paths.A slightly less convincing usecase than many as all the failure paths are wrapped up in a call to a per
fwnode_handleutility 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.
Quoted 7 lines
Cc Dmitry Baryshkov <[email protected]>Cc Marijn Suijten <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>--- drivers/iio/adc/qcom-spmi-adc5.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
Signature · 2 lines
With best wishes Dmitry
Jonathan Cameron
maintainerPATCH v2 7/14 iio: adc: rzg2l_adc: Use device_for_each_child_node_scopd()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Switching to the _scoped() version removes the need for manual calling of fwnode_handle_put() in the paths where the code exits the loop early. In this case that's all in error paths.
--- drivers/iio/adc/rzg2l_adc.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
drivers/iio/adc/rzg2l_adc.c +3 -8
@@ -302,7 +302,6 @@ 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 rzg2l_adc_data *data;unsigned int channel;int num_channels;
@@ -330,17 +329,13 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2lreturn -ENOMEM ; i =0 ; -device_for_each_child_node (&pdev->dev, fwnode) { +device_for_each_child_node_scoped (&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 ;
Signature · 1 line
2.43.1
Jonathan Cameron
maintainerPATCH v2 8/14 iio: adc: stm32: Use device_for_each_child_node_scoped()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Switching to the _scoped() version removes the need for manual calling of fwnode_handle_put() in the paths where the code exits the loop early. In this case that's all in error paths.
Note this includes a probable 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.
--- drivers/iio/adc/stm32-adc.c | 63 ++++++++++++++----------------------- 1 file changed, 24 insertions(+), 39 deletions(-)
drivers/iio/adc/stm32-adc.c +24 -39
@@ -2187,58 +2187,50 @@ 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;const char *name;int val, scan_index =0 , ret;bool differential;u32 vin[2 ]; -device_for_each_child_node (&indio_dev->dev, child) { +device_for_each_child_node_scoped (&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 +2239,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 +2251,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)
Signature · 1 line
2.43.1
Jonathan Cameron
maintainerPATCH v2 9/14 iio: adc: ti-ads1015: Use device_for_each_child_node_scoped()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Switching to the _scoped() version removes the need for manual calling of fwnode_handle_put() in the paths where the code exits the loop early. In this case that's all in error paths.
--- drivers/iio/adc/ti-ads1015.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/iio/adc/ti-ads1015.c +1 -4
@@ -902,10 +902,9 @@ 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;int i =-1 ; -device_for_each_child_node (dev, node) { +device_for_each_child_node_scoped (dev, node) {u32 pval;unsigned int channel;unsigned int pga =ADS1015_DEFAULT_PGA ;
@@ -927,7 +926,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 +934,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 ; } }
Signature · 1 line
2.43.1
Jonathan Cameron
maintainerPATCH v2 10/14 iio: adc: ti-ads131e08: Use device_for_each_child_node_scoped()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Switching to the _scoped() version removes the need for manual calling of fwnode_handle_put() in the paths where the code exits the loop early. In this case that's all in error paths.
--- drivers/iio/adc/ti-ads131e08.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
drivers/iio/adc/ti-ads131e08.c +4 -9
@@ -694,7 +694,6 @@ 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;unsigned int channel, tmp;int num_channels, i, ret;
@@ -736,10 +735,10 @@ static int ads131e08_alloc_channels(struct iio_dev *indio_dev)return -ENOMEM ; i =0 ; -device_for_each_child_node (dev, node) { +device_for_each_child_node_scoped (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 +746,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 +757,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 +783,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)
Signature · 1 line
2.43.1
Jonathan Cameron
maintainerPATCH v2 11/14 iio: addac: ad74413r: Use device_for_each_child_node_scoped()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Switching to the _scoped() version removes the need for manual calling of fwnode_handle_put() in the paths where the code exits the loop early. In this case that's all in error paths.
The use of fwnode_for_each_available_child_node() here is assumed to have been down to a false assumption that device_for_each_child_node() doesn't check avaialble - so this transition to the scoped device_for_each_child_node_scoped() is equivalent.
--- drivers/iio/addac/ad74413r.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
drivers/iio/addac/ad74413r.c +2 -8
@@ -1255,21 +1255,15 @@ 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 ;int ret; -fwnode_for_each_available_child_node (dev_fwnode (st->dev), channel_node) { +device_for_each_child_node_scoped (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)
Signature · 1 line
2.43.1
Jonathan Cameron
maintainerPATCH v2 12/14 iio: dac: ad3552r: Use device_for_each_child_node_scoped()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Switching to the _scoped() version removes the need for manual calling of fwnode_handle_put() in the paths where the code exits the loop early. In this case that's all in error paths.
Removing the goto err; statements also allows more extensive use of dev_err_probe() further simplifying the code.
--- drivers/iio/dac/ad3552r.c | 51 +++++++++++++++------------------------ 1 file changed, 19 insertions(+), 32 deletions(-)
drivers/iio/dac/ad3552r.c +19 -32
@@ -880,7 +880,6 @@ 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 regulator *vref;int err, cnt =0 , voltage, delta =100000 ;u32 vals[2 ], val, ch;
@@ -949,53 +948,45 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)return -ENODEV ; } -device_for_each_child_node (dev, child) { +device_for_each_child_node_scoped (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 +994,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 +1012,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)
Signature · 1 line
2.43.1
Jonathan Cameron
maintainerPATCH v2 13/14 iio: dac: ad5770r: Use device_for_each_child_node_scoped()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Switching to the _scoped() version removes the need for manual calling of fwnode_handle_put() in the paths where the code exits the loop early. In this case that's all in error paths.
--- drivers/iio/dac/ad5770r.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)
drivers/iio/dac/ad5770r.c +6 -13
@@ -515,39 +515,32 @@ static int ad5770r_channel_config(struct ad5770r_state *st) {int ret, tmp[2 ], min, max;unsigned int num; -struct fwnode_handle *child; num =device_get_child_node_count (&st->spi->dev);if (num !=AD5770R_MAX_CHANNELS )return -EINVAL ; -device_for_each_child_node (&st->spi->dev, child) { +device_for_each_child_node_scoped (&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)
Signature · 1 line
2.43.1
Jonathan Cameron
maintainerPATCH v2 14/14 iio: dac: ltc2688: Use device_for_each_child_node_scoped()
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Switching to the _scoped() version removes the need for manual calling of fwnode_handle_put() in the paths where the code exits the loop early. In this case that's all in error paths.
--- drivers/iio/dac/ltc2688.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
drivers/iio/dac/ltc2688.c +6 -18
@@ -746,26 +746,21 @@ 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;u32 reg, clk_input, val, tmp[2 ];int ret, span; -device_for_each_child_node (dev, child) { +device_for_each_child_node_scoped (dev, child) {struct ltc2688_chan *chan; ret =fwnode_property_read_u32 (child,"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 +781,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 +793,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 +834,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 ;
Signature · 1 line
2.43.1