iio: adc: ad4170-4: Use ERR_PTR() with %pe to improve error logging

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

Posted by Salah Triki 1 month, 4 weeks ago

Use `ERR_PTR(ret)` with `%pe` in `ad4170_read_sample()` to properly display
symbolic error codes (e.g. `-ENOMEM`) instead of raw integers (e.g. `-12`),
improving readability and debug clarity.

Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
Changes in v2:
   - Improve commit title

 drivers/iio/adc/ad4170-4.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad4170-4.c b/drivers/iio/adc/ad4170-4.c
index 6cd84d6fb08b..6296b5dc930b 100644
--- a/drivers/iio/adc/ad4170-4.c
+++ b/drivers/iio/adc/ad4170-4.c
@@ -1253,11 +1253,11 @@ static int ad4170_read_sample(struct iio_dev *indio_dev,
 
 	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;
 	}
-- 
2.43.0

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

Posted by David Lechner 1 month, 4 weeks ago

On 8/7/25 3:05 AM, Salah Triki wrote:
> Use `ERR_PTR(ret)` with `%pe` in `ad4170_read_sample()` to properly display
> symbolic error codes (e.g. `-ENOMEM`) instead of raw integers (e.g. `-12`),
> improving readability and debug clarity.
> 
> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
> Changes in v2:
>    - Improve commit title
> 
>  drivers/iio/adc/ad4170-4.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4170-4.c b/drivers/iio/adc/ad4170-4.c
> index 6cd84d6fb08b..6296b5dc930b 100644
> --- a/drivers/iio/adc/ad4170-4.c
> +++ b/drivers/iio/adc/ad4170-4.c
> @@ -1253,11 +1253,11 @@ static int ad4170_read_sample(struct iio_dev *indio_dev,
>  
>  	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.

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.

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

Posted by Andy Shevchenko 1 month, 4 weeks ago

On Thu, Aug 7, 2025 at 6:03 PM David Lechner <dlechner@baylibre.com> 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.

> 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.

-- 
With Best Regards,
Andy Shevchenko

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

Posted by Andy Shevchenko 1 month, 4 weeks ago

On Thu, Aug 7, 2025 at 11:01 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Aug 7, 2025 at 6:03 PM David Lechner <dlechner@baylibre.com> 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.
>
> > 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.

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.


-- 
With Best Regards,
Andy Shevchenko

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

Posted by David Lechner 1 month, 4 weeks ago

On 8/7/25 4:02 PM, Andy Shevchenko wrote:
> On Thu, Aug 7, 2025 at 11:01 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Aug 7, 2025 at 6:03 PM David Lechner <dlechner@baylibre.com> 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.

>>
>>> 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.
> 
>

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

Posted by Marcelo Schmitt 1 month, 3 weeks ago

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@gmail.com> wrote:
> >> On Thu, Aug 7, 2025 at 6:03 PM David Lechner <dlechner@baylibre.com> 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@analog.com>

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

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

Posted by Andy Shevchenko 1 month, 3 weeks ago

On Fri, Aug 8, 2025 at 2:07 PM Marcelo Schmitt
<marcelo.schmitt1@gmail.com> 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@gmail.com> wrote:
> > >> On Thu, Aug 7, 2025 at 6:03 PM David Lechner <dlechner@baylibre.com> 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@analog.com>

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.

-- 
With Best Regards,
Andy Shevchenko

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

Posted by Jonathan Cameron 1 month, 3 weeks ago

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

> On Fri, Aug 8, 2025 at 2:07 PM Marcelo Schmitt
> <marcelo.schmitt1@gmail.com> 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@gmail.com> wrote:  
> > > >> On Thu, Aug 7, 2025 at 6:03 PM David Lechner <dlechner@baylibre.com> 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@analog.com>  
> 
> 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

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

Posted by Andy Shevchenko 1 month, 4 weeks ago

On Thu, Aug 7, 2025 at 11:14 PM David Lechner <dlechner@baylibre.com> wrote:
> On 8/7/25 4:02 PM, Andy Shevchenko wrote:
> > On Thu, Aug 7, 2025 at 11:01 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Thu, Aug 7, 2025 at 6:03 PM David Lechner <dlechner@baylibre.com> 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.

But it's how we usually do things in the kernel,
subsystem-by-subsystem or even driver-by-driver depending on the
cases. And currently it's used in many already. But if we are talking
about IIO, we need to agree which cases are okay to move to and which
are not (besides the fact that this should be applicable only for
error pointers without additional castings).

> > 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.

-- 
With Best Regards,
Andy Shevchenko