Re: [RFC PATCH] gpiolib: call gpiochip_(un)lock_as_irq for dis/enable_irq()

[Hans Verkuil <hverkuil@xxxxxxxxx>]

On 18/07/18 09:59, Linus Walleij wrote:
> On Fri, Jul 6, 2018 at 1:25 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> 
>> If a gpio pin has an interrupt associated with it, but you need to set the
>> direction of the pin to output, then what you want to do is to disable the
>> interrupt and change direction to output. And after changing the direction
>> back to input, then you call enable_irq again.
> 
> OK so that kind of works already, but you want to make it more
> seamless and convenient (which is nice).
> 
>> This does not work today since FLAG_USED_AS_IRQ is set when the interrupt
>> is first requested and gpiod_direction_output() checks for that. So the
>> only option is to free the interrupt and re-request it, which is painful.
> 
> It's not the only option (hehe). If you study:
> drivers/w1/masters/w1-gpio.c
> you see that this uses gpiod_set_raw_value() which will ignore
> all flags and protection and just drive the line.
> 
> In the W1 case this is done to "recharge" the line.
> 
> So when you know exactly what you're doing it's fine to just
> drive the line.

But the problem is not with setting the gpio, it's with changing the
direction.

I just noticed that there is a gpiod_direction_output_raw() as well
that skips the IRQ test, but gpiod_direction_output() does more things
than gpiod_direction_output_raw(), in particular it is calling
gpio_set_drive_single_ended().

It looks like I can work around this by:

1) calling gpiod_direction_output() to ensure gpio_set_drive_single_ended()
   is called.
2) call gpiod_direction_input() so it is safe to add the interrupt.
3) call request_irq
4) If I now want to switch from input to output I can disable the irq
   and call gpiod_direction_output_raw() and use gpiod_is_active_low() to
   determine whether active is high or low (since I still need that).

I don't see where gpiod_set_raw_value() comes in. In fact, it seems that
gpiod_set_value() just changes the direction automatically and that there
is no need to explicitly call gpiod_direction_output() at all, except once
during initialization to ensure that gpio_set_drive_single_ended() is called.
If true, then in point 4 above I can just call gpiod_set_value and drop the
gpiod_direction_output()/_input() calls altogether.

Or am I missing something?

Note: the CEC pin used in cec-gpio.c is open drain.

> 
>> When gpiochip_irq_enable() is called, I currently just WARN if
>> gpiochip_lock_as_irq() fails and continue after that. It really is
>> a driver bug, but I wonder if it is also possible to automatically
>> change the direction to input before enabling the interrupt.
> 
> I think I initially had gpiod_lock_as_irq() set direction to
> input unless it was already but there was some problem
> with that.
> 
>> I have no
>> idea how to do this, though, since gpiod_direction_input() needs a
>> struct gpio_desc whereas in irq_enable() I have a struct gpio_chip.
> 
> That is not working anyway, you need to handle this on a gpio_desc
> level.

I suspected as much, but it is good to have confirmation of this.

> 
>> In drivers/media/platform/cec-gpio/cec-gpio.c I have to free and
>> re-request the irq (cec_gpio_dis/enable_irq()). This driver implements
>> low-level support for the HDMI CEC bus. When waiting for something to
>> happen the gpio pin is set to input with an interrupt to avoid polling.
>> When writing to the bus it is set to output and the interrupt obviously
>> has to be removed (or, as I would like to see, just disabled). Requesting
>> an irq can fail, and handling that is really problematic. Just disabling
>> and enabling is much easier and cleaner.
> 
> Yeah. But it works, kind of atleast...
> 
>> The other driver is drivers/gpu/drm/i2c/tda998x_drv.c, specifically the
>> tda998x_cec_calibration() function: this is also a CEC driver but here
>> the tda998x interrupt pin is overloaded: when calibrating the CEC line
>> the interrupt pin of the chip is used as a gpio calibration output signal.
> 
> That sounds like you should use the raw accessor in this case.
> 
>> +static void gpiochip_irq_enable(struct irq_data *d)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +
>> +       WARN_ON(gpiochip_lock_as_irq(chip, d->hwirq));
>> +}
>> +
>> +static void gpiochip_irq_disable(struct irq_data *d)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +
>> +       gpiochip_unlock_as_irq(chip, d->hwirq);
>> +}
> 
> This will not work other than with drivers that use GPIOLIB_IRQCHIP
> right? Since there are a lot of drivers that don't used that,
> this is not going to work.

Correct. But it is this specific case that causes the problem.
If either of these CEC drivers are used in non-GPIOLIB_IRQCHIP situations,
then those should be fixed there as well, i.e. by adding irq_enable/irq_disable
hooks.

Looking at 'git grep gpiochip_lock_as_irq' the only relevant gpio driver
for cec-gpio might be the tegra driver.

> The key is that not all GPIO controllers enumerate their IRQs from
> zero, and some have "holes" in the IRQ or GPIO rangem so
> d->hwirq is not generally the same as the GPIO offset, it just happens
> to be like that in many cases, but it is a hardware specific.
> 
>> +               if (!irqchip->irq_enable && !irqchip->irq_disable) {
>> +                       irqchip->irq_enable = gpiochip_irq_enable;
>> +                       irqchip->irq_disable = gpiochip_irq_disable;
>> +               }
> 
> So these hooks would have to be in the irqchip driver and opt-in.
> But I don't know about that.
> 
> The lock_as_irq() callback is in the irqchip .request_resources since
> it is the only safe slowpath call from irqchip.
> 
> I initially had these hooks in startup/shutdown IRQ but it didn't work
> because almost all irqchip callbacks are implicitly callable from
> fastpath IIRC.
> 
> See commits:
> commit c1bacbae8192dd2a9ebadd22d793b68054f6c6e5
> "genirq: Provide irq_request/release_resources chip callbacks"
> commit 57ef04288abd27a717287a652d324f95cb77c3c6
> "gpio: switch drivers to use new callback"

If the proposed work-around above works, then I'm happy to use that.
Alternatively we could make a gpiod_direction_output_ignore_irq()
call that just skips the FLAG_USED_AS_IRQ check.

Regards,

	Hans

> 
> Yours,
> Linus Walleij
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html