lkml.org 
[lkml]   [2022]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] driver core: platform: Rename platform_get_irq_optional() to platform_get_irq_silent()
    From
    Date
    On 1/13/22 10:43 PM, Uwe Kleine-König 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.
    >
    > To prevent people's expectations that there is a semantic difference
    > between these too, rename platform_get_irq_optional() to
    > platform_get_irq_silent() to make the actual difference more obvious.
    >
    > The #define for the old name can and should be removed once all patches
    > currently in flux still relying on platform_get_irq_optional() are
    > fixed.

    Hm... I'm afraid that with this #define they would never get fixed... :-)

    > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
    > ---
    > Hello,
    >
    > On Thu, Jan 13, 2022 at 02:45:30PM +0000, Mark Brown wrote:
    >> On Thu, Jan 13, 2022 at 12:08:31PM +0100, Uwe Kleine-König wrote:
    >>
    >>> This is all very unfortunate. In my eyes b) is the most sensible
    >>> sense, but the past showed that we don't agree here. (The most annoying
    >>> part of regulator_get is the warning that is emitted that regularily
    >>> makes customers ask what happens here and if this is fixable.)
    >>
    >> Fortunately it can be fixed, and it's safer to clearly specify things.
    >> The prints are there because when the description is wrong enough to
    >> cause things to blow up we can fail to boot or run messily and
    >> forgetting to describe some supplies (or typoing so they haven't done
    >> that) and people were having a hard time figuring out what might've
    >> happened.
    >
    > Yes, that's right. I sent a patch for such a warning in 2019 and pinged
    > occationally. Still waiting for it to be merged :-\
    > (https://lore.kernel.org/r/20190625100412.11815-1-u.kleine-koenig@pengutronix.de)
    >
    >>> I think at least c) is easy to resolve because
    >>> platform_get_irq_optional() isn't that old yet and mechanically
    >>> replacing it by platform_get_irq_silent() should be easy and safe.
    >>> And this is orthogonal to the discussion if -ENOXIO is a sensible return
    >>> value and if it's as easy as it could be to work with errors on irq
    >>> lookups.
    >>
    >> It'd certainly be good to name anything that doesn't correspond to one
    >> of the existing semantics for the API (!) something different rather
    >> than adding yet another potentially overloaded meaning.
    >
    > It seems we're (at least) three who agree about this. Here is a patch
    > fixing the name.

    I can't say I genrally agree with this patch...

    [...]
    > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
    > index 7c96f169d274..6d495f15f717 100644
    > --- a/include/linux/platform_device.h
    > +++ b/include/linux/platform_device.h
    > @@ -69,7 +69,14 @@ extern void __iomem *
    > devm_platform_ioremap_resource_byname(struct platform_device *pdev,
    > const char *name);
    > extern int platform_get_irq(struct platform_device *, unsigned int);
    > -extern int platform_get_irq_optional(struct platform_device *, unsigned int);
    > +extern int platform_get_irq_silent(struct platform_device *, unsigned int);
    > +
    > +/*
    > + * platform_get_irq_optional was recently renamed to platform_get_irq_silent.
    > + * Fixup users to not break patches that were created before the rename.
    > + */
    > +#define platform_get_irq_optional(pdev, index) platform_get_irq_silent(pdev, index)
    > +

    Yeah, why bother fixing if it compiles anyway?
    I think an inline wrapper with an indication to gcc that the function is deprecated
    (I just forgot how it should look) would be better instead...

    > extern int platform_irq_count(struct platform_device *);
    > extern int devm_platform_get_irqs_affinity(struct platform_device *dev,
    > struct irq_affinity *affd,
    [...]
    MBR, Sergey

    \
     
     \ /
      Last update: 2022-01-14 20:47    [W:4.334 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site