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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

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.

> 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.

> 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.

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"

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