Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Davide Hug <d@xxxxxxxxxx>]

I'm really sorry that I come back to this only now.

On Tue, 12 Sep 2017 15:57:47 +0200
Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:

> > At each measurement the bidirectional data line is used by the sensor as data
> > ready signal. So the IRQ is used only to detect the data ready signal. After that
> > the IRQ is disabled to avoid triggering interrupts during the serial
> > communication.
> 
> That is a fully valid usecase and I think something gpiolib
> should be able to support. If it bugs you, it will also soon bug
> somebody else.

Reading through the mailing list from when irq_request/release_resources were
introduced I see how more involved this is than I would first think.
Also, from the discussion above I understand that for the hwmon/sht15 driver it
is acceptable to use a simple polling loop without irq.
But it seems that iio/humidity/dht11 is another driver with a similar
situation.

> Somewhere the backing irqchip is talking to gpiochip by
> issueing gpiochip_lock_as_irq() on this
> line and it should be made to issue gpiochip_unlock_as_irq()
> before used as serial line, and then gpiochip_lock_as_irq()
> again when the go back to using it as irq.
> 
> Currently, this mostly gets done in the .irq_request_resources
> and .irq_release_resources of the irqchip.
> 
> Intuitively I would say it should be done by
> .irq_enable()/.irq_disable() but that has the problem that it
> is fastpath, and not all GPIO irqchips can handle that.

Do I understand this correctly? Some GPIO drivers can be slow and sleep, and
.irq_enable/disable can be called from places where this is not acceptable.

(I see that only gpio/gpio-pcf857x implements .irq_enable/disable and has
.can_sleep=true)

> Maybe we should do it in .irq_enable()/.irq_disable() in case
> the .can_sleep bool on the gpiochip is not set, simply?
> This way the irqchip should be able to lock/unlock the GPIO
> lines dynamically.

So, before a driver relays on locking/unlocking a GPIO line dynamically, the
driver would have to make sure that the GPIO has not .can_sleep=true. Right?

> We would need to patch a few drivers, but the generic code
> for simple gpio irqchips is in gpiolib.c and can be fixed once
> and for all. (Which is kind of why I insist on drivers to use
> that...)

With a default .irq_enable/disable implementation in gpiolib (something as in
the patch below) the GPIO driver implementing their own .irq_enable/disable
would have to be fixed, right?

Grepping around I get 12 drivers that call gpiochip_irqchip_add, that don't
have .can_sleep=true and implement their own .irq_enable/.irq_disable.
Also, 5 of these drivers only implement either .irq_enable or .irq_disable. This
would be a problem with my patch below which assumes that both are implemented
or neither.

> comm -3 <(grep -r 'gpiochip_irqchip_add' drivers/ -l | xargs grep "\.irq_enable" -l |sort) \
>         <(grep -r 'gpiochip_irqchip_add' drivers/ -l | xargs grep "\.irq_disable" -l |sort)
		drivers/gpio/gpio-xlp.c
drivers/gpio/gpio-zynq.c
drivers/pinctrl/intel/pinctrl-intel.c
		drivers/pinctrl/pinctrl-at91.c
		drivers/pinctrl/pinctrl-st.c


Thanks,
Davide




diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index aad84a6306c4..2435654d2ffd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1701,6 +1701,24 @@ static void gpiochip_irq_relres(struct irq_data *d)
 	module_put(chip->gpiodev->owner);
 }
 
+static void gpiochip_irq_disable(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+	gpiochip_unlock_as_irq(chip, data->hwirq);
+	if (data->chip->irq_mask)
+		data->chip->irq_mask(data);
+}
+
+static void gpiochip_irq_enable(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+	gpiochip_lock_as_irq(chip, data->hwirq);
+	if (data->chip->irq_unmask)
+		data->chip->irq_unmask(data);
+}
+
 static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 {
 	if (!gpiochip_irqchip_irq_valid(chip, offset))
@@ -1834,6 +1852,8 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
 	if (gpiochip->irq.chip) {
 		gpiochip->irq.chip->irq_request_resources = NULL;
 		gpiochip->irq.chip->irq_release_resources = NULL;
+		gpiochip->irq.chip->irq_disable = NULL;
+		gpiochip->irq.chip->irq_enable  = NULL;
 		gpiochip->irq.chip = NULL;
 	}
 
@@ -1931,6 +1951,13 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
 		irqchip->irq_release_resources = gpiochip_irq_relres;
 	}
 
+	if (!gpiochip->can_sleep &&
+	    !irqchip->irq_disable &&
+	    !irqchip->irq_enable) {
+		irqchip->irq_disable = gpiochip_irq_disable;
+		irqchip->irq_enable  = gpiochip_irq_enable;
+	}
+
 	acpi_gpiochip_request_interrupts(gpiochip);
 
 	return 0;
--
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