lkml.org 
[lkml]   [2020]   [Jul]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 1/5] pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip
    Hi,

    On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
    >
    > The gpio can be marked for wakeup and drivers can invoke disable_irq()
    > during suspend, in such cases unlazy approach will also disable at HW
    > and such gpios will not wakeup device from suspend to RAM.
    >
    > Remove irq_disable callback to allow gpio interrupts to lazy disabled.
    > The gpio interrupts will get disabled during irq_mask callback.
    >
    > Acked-by: Linus Walleij <linus.walleij@linaro.org>
    > Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
    > ---
    > drivers/pinctrl/qcom/pinctrl-msm.c | 13 -------------
    > 1 file changed, 13 deletions(-)

    While the code of this patch looks fairly correct to me (there's no
    need to implement the irq_disable callback and we can just rely on the
    masking), I'm slightly anxious about the description. It almost feels
    like you're somehow relying on the laziness to "fix" your issue here.
    ...but the laziness is supposed to just be an optimization.
    Specifically if an interrupt happens to fire at any point in time
    after a driver has called disable_irq() then it acts just like a
    non-lazy disable.

    Said another way, I think this is a valid thing for a driver to do and
    it should get woken up if the irq fires in suspend:

    disable_irq();
    msleep(1000);
    enable_irq_wake()

    Specifically if an IRQ comes in during that sleep then it will be just
    like you had a non-lazy IRQ.


    So while I'm for this patch, I'd suggest a simpler description
    (assuming my understanding is correct):

    There is no reason to implement irq_disable() and irq_mask(). Let's just
    use irq_mask() and let the rest of the core handle it.


    Also: it feels really weird to me that you're getting rid of the
    irq_disable() but keeping irq_enable(). That seems like asking for
    trouble, though I'd have to do more research to see if I could figure
    out exactly what might go wrong. Could you remove your irq_enable()
    too?


    -Doug

    \
     
     \ /
      Last update: 2020-07-14 00:26    [W:3.499 / U:0.232 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site