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

[Hans Verkuil <hverkuil@xxxxxxxxx>]

Ping?

Regards,

	Hans

On 06/07/18 13:25, Hans Verkuil 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.
> 
> 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.
> 
> This RFC patch sets hooks that allow the driver to know when the irq is
> disabled and enabled and it sets FLAG_USED_AS_IRQ accordingly.
> 
> It works, but I have one open question:
> 
> 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 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.
> 
> This patch would be really useful for two CEC drivers:
> 
> 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.
> 
> 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.
> 
> This driver disables the interrupt and switches the direction to output
> before doing the calibration. This fails for the BeagleBone board unless
> I apply this patch. This driver was originally developed for the dove-cubox
> board which uses this gpio driver: drivers/gpio/gpio-mvebu.c. For some
> reason this worked fine (i.e. it is apparently possible to disable the irq
> and change the output without running into the FLAG_USED_AS_IRQ check), but
> I don't understand why. I don't have this hardware, so I can't test it.
> 
> I'm a newbie when it comes to gpio, so I have no idea how much sense this
> patch makes.
> 
> Regards,
> 
> 	Hans
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e11a3bb03820..dcdb457b83b0 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1822,6 +1822,20 @@ static void gpiochip_irq_relres(struct irq_data *d)
>  	module_put(chip->gpiodev->owner);
>  }
> 
> +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);
> +}
> +
>  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
>  	if (!gpiochip_irqchip_irq_valid(chip, offset))
> @@ -1897,6 +1911,10 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
>  	    !irqchip->irq_release_resources) {
>  		irqchip->irq_request_resources = gpiochip_irq_reqres;
>  		irqchip->irq_release_resources = gpiochip_irq_relres;
> +		if (!irqchip->irq_enable && !irqchip->irq_disable) {
> +			irqchip->irq_enable = gpiochip_irq_enable;
> +			irqchip->irq_disable = gpiochip_irq_disable;
> +		}
>  	}
> 
>  	if (gpiochip->irq.parent_handler) {
> @@ -2056,6 +2074,10 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
>  	    !irqchip->irq_release_resources) {
>  		irqchip->irq_request_resources = gpiochip_irq_reqres;
>  		irqchip->irq_release_resources = gpiochip_irq_relres;
> +		if (!irqchip->irq_enable && !irqchip->irq_disable) {
> +			irqchip->irq_enable = gpiochip_irq_enable;
> +			irqchip->irq_disable = gpiochip_irq_disable;
> +		}
>  	}
> 
>  	acpi_gpiochip_request_interrupts(gpiochip);
> 

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