Re: [PATCH] driver core: platform: Rename platform_get_irq_optional() to platform_get_irq_silent()

[Uwe Kleine-König]

On Fri, Jan 14, 2022 at 10:58:51AM +0100, Geert Uytterhoeven wrote:
> Hi Uwe,
> 
> On Thu, Jan 13, 2022 at 11:43 PM Uwe Kleine-König
> <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > On Thu, Jan 13, 2022 at 11:57:43PM +0300, Sergey Shtylyov wrote:
> > > On 1/13/22 11:17 PM, Mark Brown wrote:
> > > >> The subsystems regulator, clk and gpio have the concept of a dummy
> > > >> resource. For regulator, clk and gpio there is a semantic difference
> > > >> between the regular _get() function and the _get_optional() variant.
> > > >> (One might return the dummy resource, the other won't. Unfortunately
> > > >> which one implements which isn't the same for these three.) The
> > > >> difference between platform_get_irq() and platform_get_irq_optional() is
> > > >> only that the former might emit an error message and the later won't.
> > >
> > >    This is only a current difference but I'm still going to return 0 ISO
> > > -ENXIO from latform_get_irq_optional(), no way I'd leave that -ENXIO there
> > > alone... :-)
> >
> > This would address a bit of the critic in my commit log. But as 0 isn't
> > a dummy value like the dummy values that exist for clk, gpiod and
> > regulator I still think that the naming is a bad idea because it's not
> > in the spirit of the other *_get_optional functions.
> >
> > Seeing you say that -ENXIO is a bad return value for
> > platform_get_irq_optional() and 0 should be used instead, I wonder why
> > not changing platform_get_irq() to return 0 instead of -ENXIO, too.
> > This question is for now only about a sensible semantic. That actually
> > changing platform_get_irq() is probably harder than changing
> > platform_get_irq_optional() is a different story.
> >
> > If only platform_get_irq_optional() is changed and given that the
> > callers have to do something like:
> >
> >         if (this_irq_exists()):
> >                 ... (e.g. request_irq)
> >         else:
> >                 ... (e.g. setup polling)
> >
> > I really think it's a bad idea that this_irq_exists() has to be
> > different for platform_get_irq() vs. platform_get_irq_optional().
> 
> For platform_get_irq(), the IRQ being absent is an error condition,
> hence it should return an error code.
> For platform_get_irq_optional(), the IRQ being absent is not an error
> condition, hence it should not return an error code, and 0 is OK.

Please show a few examples how this simplifies the code. If it's only
that a driver has to check for == 0 instead of == -ENXIO, than that's
not a good enough motivation to make platform_get_irq_optional()
different to platform_get_irq().

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature