diff mbox series

[v4,09/15] iio: adc: stm32: Use device_for_each_child_node_scoped()

Message ID 20240217164249.921878-10-jic23@kernel.org (mailing list archive)
State Superseded
Headers show
Series device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. | expand

Commit Message

Jonathan Cameron Feb. 17, 2024, 4:42 p.m. UTC 
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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.

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 | 63 ++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 39 deletions(-)

Comments

Andy Shevchenko Feb. 19, 2024, 11:45 a.m. UTC | #1 
On Sat, Feb 17, 2024 at 04:42:43PM +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 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.

...

>  	const struct stm32_adc_info *adc_info = adc->cfg->adc_info;

I believe with

	struct device *dev = &indio_dev->dev;

you can make the below neater.
Also see some side notes.

> -	struct fwnode_handle *child;
>  	const char *name;
>  	int val, scan_index = 0, ret;
>  	bool differential;
>  	u32 vin[2];

...

>  		if (!ret) {

Not a fan of this pattern, below we have two different patterns for the cases
like this :-(

> +			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)


This 'else' is redundant.

> +				return ret;
> +		} else if (ret != -EINVAL)

This also...

> +			return dev_err_probe(&indio_dev->dev, ret, "Invalid label\n");

...if you first do like

		if (ret && ret != -EINVAL)
			return dev_err_probe(...);
		if (!ret) {

Another option

		if (ret) {
			if (ret != -EINVAL)
				return dev_err_probe(...);
		} else {

...

>  		differential = false;
>  		ret = fwnode_property_read_u32_array(child, "diff-channels", vin, 2);

ARRAY_SIZE()?

>  		/* diff-channels is optional */

...

>  		if (!ret) {
> +			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");
>  		}

As per above?
Jonathan Cameron Feb. 24, 2024, 12:16 p.m. UTC | #2 
On Mon, 19 Feb 2024 13:45:03 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, Feb 17, 2024 at 04:42:43PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > 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.  
> 
> ...
> 
> >  	const struct stm32_adc_info *adc_info = adc->cfg->adc_info;  
> 
> I believe with
> 
> 	struct device *dev = &indio_dev->dev;
> 
> you can make the below neater.

Agreed.  Given the users of indio_dev are all the error messages I'm touching
anyway, this is in scope for this patch I think.

> Also see some side notes.
> 
> > -	struct fwnode_handle *child;
> >  	const char *name;
> >  	int val, scan_index = 0, ret;
> >  	bool differential;
> >  	u32 vin[2];  
> 
> ...
> 
> >  		if (!ret) {  
> 
> Not a fan of this pattern, below we have two different patterns for the cases
> like this :-(

Likewise not a fan.

Tidying this up is unrelated enough though that I won't do it in this
series.

> 
> > +			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)  
> 
> 
> This 'else' is redundant.
> 
> > +				return ret;
> > +		} else if (ret != -EINVAL)  
>
I shouldn't have dropped the bracket here, so I'll put that back for now.
 
> This also...
> 
> > +			return dev_err_probe(&indio_dev->dev, ret, "Invalid label\n");  
> 
> ...if you first do like
> 
> 		if (ret && ret != -EINVAL)
> 			return dev_err_probe(...);
> 		if (!ret) {
> 
> Another option
> 
> 		if (ret) {
> 			if (ret != -EINVAL)
> 				return dev_err_probe(...);
> 		} else {
> 
> ...
> 
> >  		differential = false;
> >  		ret = fwnode_property_read_u32_array(child, "diff-channels", vin, 2);  
> 
> ARRAY_SIZE()?
> 
> >  		/* diff-channels is optional */  
> 
> ...
> 
> >  		if (!ret) {
> > +			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");
> >  		} 
> 
> As per above?
> 
Agree with all comments, but as you said side notes. Stuff to cleanup in a series
doing a wider driver clean up.

Thanks,

Jonathan
diff mbox series

Patch

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index b5d3c9cea5c4..9fd94a792f42 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -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)