Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Stephen Boyd]

Quoting Maulik Shah (2020-05-28 06:11:23)
> Hi,
> 
> On 5/28/2020 6:38 AM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2020-05-27 04:26:14)
> >> On 5/27/2020 3:14 PM, Stephen Boyd wrote:
> >>> Quoting Maulik Shah (2020-05-23 10:11:10)
> >>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >>>> index eaa0e20..3810cd0 100644
> >>>> --- a/drivers/gpio/gpiolib.c
> >>>> +++ b/drivers/gpio/gpiolib.c
> >>>> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d)
> >>>>           gpiochip_relres_irq(gc, d->hwirq);
> >>>>    }
> >>>>    
> >>>> +static void gpiochip_irq_mask(struct irq_data *d)
> >>>> +{
> >>>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> >>>> +
> >>>> +       if (gc->irq.irq_mask)
> >>>> +               gc->irq.irq_mask(d);
> >>>> +       gpiochip_disable_irq(gc, d->hwirq);
> >>> How does this work in the lazy case when I want to drive the GPIO? Say I
> >>> have a GPIO that is also an interrupt. The code would look like
> >>>
> >>>    struct gpio_desc *gpio = gpiod_get(...)
> >>>    unsigned int girq = gpiod_to_irq(gpio)
> >>>
> >>>    request_irq(girq, ...);
> >>>
> >>>    disable_irq(girq);
> >>>    gpiod_direction_output(gpio, 1);
> >>>
> >>> In the lazy case genirq wouldn't call the mask function until the first
> >>> interrupt arrived on the GPIO line. If that never happened then wouldn't
> >>> we be blocked in gpiod_direction_output() when the test_bit() sees
> >>> FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving
> >>> gpios?
> >> The client driver can decide to unlazy disable IRQ with below API...
> >>
> >>   Âirq_set_status_flags(girq, IRQ_DISABLE_UNLAZY);
> >>
> >> This will immediatly invoke mask function (unlazy disable) from genirq,
> >> even though irq_disable is not implemented.
> >>
> > Sure a consumer can disable the lazy feature, but that shouldn't be
> > required to make this work. The flag was introduced in commit
> > e9849777d0e2 ("genirq: Add flag to force mask in
> > disable_irq[_nosync]()") specifically to help devices that can't disable
> > the interrupt in their own device avoid a double interrupt.
> i don't think this will be a problem.
> 
> Case 1) Client driver have locked gpio to be used as IRQ using 
> gpiochip_lock_as_irq()
> 
> In this case, When client driver want to change the direction for a 
> gpio, they will invoke gpiod_direction_output().
> I see it checks for two flags (pasted below), if GPIO is used as IRQ and 
> whether its enabled IRQ or not.
> 
>  ÂÂÂÂÂÂ /* GPIOs used for enabled IRQs shall not be set as output */
>  ÂÂÂÂÂÂÂ if (test_bit(FLAG_USED_AS_IRQ, &desc->flags) &&
>  ÂÂÂÂÂÂÂÂÂÂÂ test_bit(FLAG_IRQ_IS_ENABLED, &desc->flags)) {
> 
> The first one (FLAG_USED_AS_IRQ) is set only if client driver in past 
> have locked gpio to use as IRQ with a call to gpiochip_lock_as_irq()
> then it never gets unlocked until clients invoke gpiochip_unlock_as_irq().
> 
> So i presume the client driver which in past locked gpio to be used as 
> IRQ, now wants to change direction then it will
> a. first unlock to use as IRQ
> b. then change the direction.

How does a client driver unlock to use as an IRQ though? I don't
understand how that is done. gpiochip_lock_as_irq() isn't a gpio
consumer API, it's a gpiochip/gpio provider API.

> 
> Once it unlocks in step (a), both these flags will be cleared and there 
> won't be any error in changing direction in step (b).