On Sun, 9 Feb 2025 16:47:44 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> On Sat, Feb 08, 2025 at 04:13:24PM -0500, Aren Moynihan wrote:
> > Using dev_err_probe instead of dev_err and return makes the errors
>
> Use dev_err_probe()
> dev_err()
>
> > easier to understand by including the error name, and saves a little
> > code.
>
> I believe this patch will make more sense before switching to local 'dev'
> variable. Then the previous one will have an additional justification as
> the "struct device *dev = ...;" lines in some cases will be added already
> by this patch.
I'm not sure I follow this one comment.
The only line that has struct device *dev = added in this patch is
replacing an existing client->dev lookup that could have been pushed
to previous patch if this patch ordering was maintained.
For dev_err() to dev_err_probe() the number of references to dev
is the same after all. The only additional justification this patch
makes is some longer lines that using a local dev pointer shortens
again.
>
> ...
>
> > indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > - if (!indio_dev) {
> > - dev_err(&client->dev, "iio allocation failed!\n");
> > - return -ENOMEM;
> > - }
> > + if (!indio_dev)
> > + return dev_err_probe(dev, -ENOMEM, "iio allocation failed!\n");
>
> We don't issue the messages for -ENOMEM.
>
> If it's in the current code, add a new patch to drop this message and return an
> error code directly.
I'd be fine with that dev_err() dropped in this patch as long as the
description mentions it.
>
> ...
>
> > + if (ret < 0)
>
> Perhaps, while at it, drop these ' < 0' parts where they are not hinting about
> anything.
That would be a separate patch and indeed makes sense to me as well.
Jonathan
>
> > + return dev_err_probe(dev, ret, "device_register failed\n");
>
