Re: [PATCH v5 5/9] iio: adc: stm32: Use device_for_each_child_node_scoped()

[Fabrice Gasnier <fabrice.gasnier@foss.st.com>]

On 2/24/24 13:32, 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.

Hi Jonathan,

Indeed, please find later question, inline.

> 
> 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>
> 
> ---
> v5: Use a local struct device pointer.
>     Add brackets back I shouldn't have dropped.
> 
> Andy had a number of other comments but they would be unrelated changes
> so I'm leaving them for a future patch set.
> ---
>  drivers/iio/adc/stm32-adc.c | 61 +++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index b5d3c9cea5c4..36add95212c3 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -2187,58 +2187,52 @@ 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 device *dev = &indio_dev->dev;
>  	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(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(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(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;
> +				return ret;
>  		} else if (ret != -EINVAL) {
> -			dev_err(&indio_dev->dev, "Invalid label %d\n", ret);
> -			goto err;
> +			return dev_err_probe(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(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]);

Good catch! I think you're talking about a missing "ret = -EINVAL;" here.

Do you think this should be split as a precursor fix patch, so it can be
picked into stable release ?

Please let me know. Appart from that, you can add my:

Tested-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Acked-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Thanks,
Best Regards,
Fabrice

> -				goto err;
> -			}
> +			if (vin[0] != val || vin[1] >= adc_info->max_channels)
> +				return dev_err_probe(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(dev, ret,
> +					     "Invalid diff-channels property\n");
>  		}
>  
>  		stm32_adc_chan_init_one(indio_dev, &channels[scan_index], val,
> @@ -2247,11 +2241,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(dev, ret,
> +					     "Invalid st,min-sample-time-ns property\n");
>  
>  		stm32_adc_smpr_init(adc, channels[scan_index].channel, val);
>  		if (differential)
> @@ -2261,11 +2253,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)