Re: [PATCH] iio: humidity: dht11: Improve error reporting in .probe()

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 30 Jun 2022 16:15:05 +0200
Uwe Kleine-König <ukleinek@debian.org> wrote:

> On 6/30/22 12:26, Andy Shevchenko wrote:
> > On Thu, Jun 30, 2022 at 12:25 PM Uwe Kleine-König <ukleinek@debian.org> wrote:  
> >> On 6/30/22 09:17, Andy Shevchenko wrote:  
> >>> On Thu, Jun 30, 2022 at 8:58 AM Uwe Kleine-König <ukleinek@debian.org> wrote:  
> >>>>
> >>>> Don't emit a message for -ENOMEM, the kernel is already loud enough in
> >>>> this case. Add a message if getting the GPIO or registering the iio
> >>>> device fails and use dev_err_probe for improved behaviour on
> >>>> -EPROBE_DEFER.  
> >>>
> >>> Why do we need additional messages?  
> >>
> >> I don't understand the question. Do you really wonder why there is a
> >> benefit of an error message if a resource allocation in probe fails?
> >>
> >> Current state is that for Yves-Alexis (on Cc:) the driver is silent but
> >> unbound. I guess that's because gpiod_get fails, but seeing a
> >> confirmation in the kernel log would be nice.

Giving that example in the patch description makes for a very strong
reasoning.  Mostly I want this captured so that others who might consider
similar patches and see this, understand the reasoning.  Messages for things
that can happen in real life are definitely a good thing!

> >>  
> >>> They should be split in a separate patch, perhaps, with the explanation.
> >>>
> >>> Actually the rest I would split to two: converting to dev_err_probe()
> >>> in the case where it's not right now, and dropping ENOMEM message.  
> >>
> >> So we're at three patches:
> >>
> >>    - drop ENOMEM message
> >>    - convert existing messages to dev_err_probe()
> >>    - introduce errors for devm_gpiod_get() and devm_iio_device_register()

That lines up with how I'd prefer it split. If I'd not wanted the patch description
additions above anyway I 'might' have just picked it up.  Given we are going
around again, let's set a nice example.  In particularly it will make the
description easier to read as separates -ENOMEM case from the new additions.

Jonathan


> >>
> >> I can rework accordingly, but for me this looks a bit over engineered.  
> > 
> > Perhaps, but they three are about different stuff.  
> 
> The different things could be named as follows:
> 
>   - Improve error reporting in .probe() for devm_iio_device_alloc()
>   - Improve error reporting in .probe() for devm_gpiod_get()
>   - Improve error reporting in .probe() for gpiod_to_irq()
>   - Improve error reporting in .probe() for devm_iio_device_register()
> 
> Even after the discussion I think it's ok to summarize that to a single 
> patch "Improve error reporting in .probe()". I'd wait for Jonathan's 
> feedback with his opinion before reworking the patch.



> 
> Best regards
> Uwe