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

[Marcelo Schmitt]

Hi,

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

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>

Though, I'm also totally fine if maintainers prefer not to take this change for
whatever reason.

> 
> >>
> >>> And if we are doing this in more places, it would make sense to have a new
> >>> format specifier for integer error values instead of casting them to
> >>> pointers.
> >>
> >> Will _very unlikely_ to happen. This has to be a C standard for that,
> >> otherwise you are suggesting to always have a kernel warning for each
> >> of these cases. The only way we can customize specifiers w/o
> >> introducing a compiler warnings is to continue (and still carefully)
> >> using %p extensions.
> 
> OK, makes sense.
> 
> > 
> > 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.
> > 
> > 
> 
> 

Best regards,
Marcelo