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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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?

-- 
With Best Regards,
Andy Shevchenko