On Mon, 19 Feb 2024 13:45:03 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> On Sat, Feb 17, 2024 at 04:42:43PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> > 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
