Re: [PATCH v2] iio: adc: ad4170-4: Use ERR_PTR() with %pe to improve error logging

[Jonathan Cameron]

On Fri, 8 Aug 2025 14:30:53 +0200
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:

> On Fri, Aug 8, 2025 at 2:07 PM Marcelo Schmitt
> <marcelo.schmitt1@xxxxxxxxx> wrote:
> > On 08/07, David Lechner wrote:  
> > > On 8/7/25 4:02 PM, Andy Shevchenko wrote:  
> > > > On Thu, Aug 7, 2025 at 11:01 PM Andy Shevchenko
> > > > <andy.shevchenko@xxxxxxxxx> wrote:  
> > > >> On Thu, Aug 7, 2025 at 6:03 PM David Lechner <dlechner@xxxxxxxxxxxx> wrote:  
> > > >>> On 8/7/25 3:05 AM, Salah Triki wrote:  
> 
> ...
> 
> > > >>>>       ret = __ad4170_read_sample(indio_dev, chan, val);
> > > >>>>       if (ret) {
> > > >>>> -             dev_err(dev, "failed to read sample: %d\n", ret);
> > > >>>> +             dev_err(dev, "failed to read sample: %pe\n", ERR_PTR(ret));
> > > >>>>
> > > >>>>               ret2 = ad4170_set_channel_enable(st, chan->address, false);
> > > >>>>               if (ret2)
> > > >>>> -                     dev_err(dev, "failed to disable channel: %d\n", ret2);
> > > >>>> +                     dev_err(dev, "failed to disable channel: %pe\n", ERR_PTR(ret2));
> > > >>>>
> > > >>>>               return ret;
> > > >>>>       }  
> > > >>>
> > > >>> Interesting, I didn't know we had this format specifier. But I think
> > > >>> this is something we would want to do kernel-wide or not at all to stay
> > > >>> consistent.  
> > > >>
> > > >> I'm sorry but I didn't follow. This is a kernel-wide format specifier.  
> > >
> > > I meant that it would be strange to make this change just in one
> > > driver and not do the same everywhere else.  
> >
> > Casting error values to pointers is already being done by many IIO drivers
> > if we consider the use of dev_err_probe().
> > __dev_probe_failed() does the casting from within dev_err_probe()
> > https://elixir.bootlin.com/linux/v6.15.9/source/drivers/base/core.c#L5026  
> 
> This is a manipulation. The dev_err_probe() and __dev_probe_failed()
> are parts of the core where we specifically bend the rules for all in
> one place just for a reason to avoid this spreading (and avoid
> creating specific APIs).
> 
> > Thus, I think this patch makes the error messaging from ad4170
> > more consistent and, because of that, I also see this as a good change.
> >
> > Reviewed-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>  
> 
> Thanks for the review...
> 
> > Though, I'm also totally fine if maintainers prefer not to take this change for
> > whatever reason.  
> 
> ...but the below still stays...
> 
> > > > And to be clear: I am not in favour of this change exactly due to a
> > > > bit weird (for the reader) castings just for the sake of use of %pe.  
> 

Agreed.  To me having this cast broadly used in drivers is just too weird.

I'd prefer we dig down a little further and use what %pe is using directly.

#include <linux/errname.h>

printk(... %s\n", errname(ret));

It's not much used so far though but there is precedence via a wrapper
in bcachefs. We'd probably want to select CONFIG_SYMBOLIC_ERRNAME for IIO
though (rather than every driver).

Jonathan