lkml.org 
[lkml]   [2022]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] platform: make platform_get_irq_optional() optional
    Hello Sergey,

    On Thu, Jan 13, 2022 at 11:35:34PM +0300, Sergey Shtylyov wrote:
    > On 1/13/22 12:45 AM, Mark Brown wrote:
    > >>> To me it sounds much more logical for the driver to check if an
    > >>> optional irq is non-zero (available) or zero (not available), than to
    > >>> sprinkle around checks for -ENXIO. In addition, you have to remember
    > >>> that this one returns -ENXIO, while other APIs use -ENOENT or -ENOSYS
    > >>> (or some other error code) to indicate absence. I thought not having
    > >>> to care about the actual error code was the main reason behind the
    > >>> introduction of the *_optional() APIs.
    > >
    > >> No, the main benefit of gpiod_get_optional() (and clk_get_optional()) is
    > >> that you can handle an absent GPIO (or clk) as if it were available.
    >
    > Hm, I've just looked at these and must note that they match 1:1 with
    > platform_get_irq_optional(). Unfortunately, we can't however behave the
    > same way in request_irq() -- because it has to support IRQ0 for the sake
    > of i8253 drivers in arch/...

    Let me reformulate your statement to the IMHO equivalent:

    If you set aside the differences between
    platform_get_irq_optional() and gpiod_get_optional(),
    platform_get_irq_optional() is like gpiod_get_optional().

    The introduction of gpiod_get_optional() made it possible to simplify
    the following code:

    reset_gpio = gpiod_get(...)
    if IS_ERR(reset_gpio):
    error = PTR_ERR(reset_gpio)
    if error != -ENDEV:
    return error
    else:
    gpiod_set_direction(reset_gpiod, INACTIVE)

    to

    reset_gpio = gpiod_get_optional(....)
    if IS_ERR(reset_gpio):
    return reset_gpio
    gpiod_set_direction(reset_gpiod, INACTIVE)

    and I never need to actually know if the reset_gpio actually exists.
    Either the line is put into its inactive state, or it doesn't exist and
    then gpiod_set_direction is a noop. For a regulator or a clk this works
    in a similar way.

    However for an interupt this cannot work. You will always have to check
    if the irq is actually there or not because if it's not you cannot just
    ignore that. So there is no benefit of an optional irq.

    Leaving error message reporting aside, the introduction of
    platform_get_irq_optional() allows to change

    irq = platform_get_irq(...);
    if (irq < 0 && irq != -ENXIO) {
    return irq;
    } else if (irq >= 0) {
    ... setup irq operation ...
    } else { /* irq == -ENXIO */
    ... setup polling ...
    }

    to

    irq = platform_get_irq_optional(...);
    if (irq < 0 && irq != -ENXIO) {
    return irq;
    } else if (irq >= 0) {
    ... setup irq operation ...
    } else { /* irq == -ENXIO */
    ... setup polling ...
    }

    which isn't a win. When changing the return value as you suggest, it can
    be changed instead to:

    irq = platform_get_irq_optional(...);
    if (irq < 0) {
    return irq;
    } else if (irq > 0) {
    ... setup irq operation ...
    } else { /* irq == 0 */
    ... setup polling ...
    }

    which is a tad nicer. If that is your goal however I ask you to also
    change the semantic of platform_get_irq() to return 0 on "not found".
    Note the win is considerably less compared to gpiod_get_optional vs
    gpiod_get however. And then it still lacks the semantic of a dummy irq
    which IMHO forfeits the right to call it ..._optional().

    Now I'm unwilling to continue the discussion unless there pops up a
    suggestion that results in a considerable part (say > 10%) of the
    drivers using platform_get_irq_optional not having to check if the
    return value corresponds to "not found".

    Best regards
    Uwe

    --
    Pengutronix e.K. | Uwe Kleine-König |
    Industrial Linux Solutions | https://www.pengutronix.de/ |
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2022-01-14 10:28    [W:4.514 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site