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

Andy Shevchenko
2024-02-19T11:45:14Z

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