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

[Hans Verkuil <hverkuil@xxxxxxxxx>]

Hi Linus,

On 20/07/18 10:06, Linus Walleij wrote:

<snip>

> You would probably think it's fine if you didn't have to do this
> heavy lifting and could just request them on probe and
> call enable_irq() and disable_irq() when needed, and have these
> call down into the irq_chip .irq_enable() and .irq_disable() and
> lock the GPIO line as input *there* instead, and then change
> this everywhere (yes patch all gpio_chips with an irqchip
> driver in that case).
> 
> As drivers have likely sometimes already assigned function
> pointers to .irq_enable() and .irq_disable() these have to
> be copied and stored in struct gpio_irq_chip() by
> gpiochip_set_cascaded_irqchip() and called in sequence.
> But it can be done.
> 
> What about changing the GPIOLIB_IRQCHIP code to just
> do the module_get()/put() part on .irq_request_resources()
> and move the locking to the .irq_enable()/.irq_disable()
> callbacks so that we can enable and disable irqs on the fly
> in a better way? (BIG WIN!)
> 
> What about going and reinvestigating this instead?
> 
> I'm sorry that I can't present any simple solution, but hey,
> I presented a really complicated solution instead, isn't it
> great! :D

I did do some investigation into this, but it made me very unhappy:
it's a lot of low-level changes in a lot of drivers for a lot of
different boards, some of which are probably hard to test. Scary.

But I've come up with a much simpler alternative: add two new
gpiolib functions: gpiod_enable_irq and gpiod_disable_irq.

An RFC patch is below (documentation is missing, but it works fine).

Basically if you have a gpio which has an interrupt, then you can use
these functions to disable and enable it. It enables/disables the interrupt
and calls gpiochip_(un)lock_as_irq() as well.

It is even possible to call gpiod_direction_input in gpiod_enable_irq to ensure
that the direction is set correctly. I have not done so, but if you think it is
useful, then it's trivial to do. Right now it will return an error if the
direction is still set to output.

If there is no associated interrupt, then gpiod_enable_irq just returns 0. Not
sure if that's a bug or a feature. It's easy enough to change.

Let me know what you think. IMHO this avoids a huge amount of churn in code you
really do no want to touch and it does exactly what you want it to do. And it
looks pretty clean as well, both in gpiolib and in the drivers.

Regards,

	Hans

Signed-off-by: Hans Verkuil <hansverk@xxxxxxxxx>
---
 drivers/gpio/gpiolib.c        | 83 ++++++++++++++++++++++++++---------
 include/linux/gpio/consumer.h |  3 ++
 2 files changed, 66 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e11a3bb03820..75ad6177fa95 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3228,28 +3228,16 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_to_irq);

-/**
- * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ
- * @chip: the chip the GPIO to lock belongs to
- * @offset: the offset of the GPIO to lock as IRQ
- *
- * This is used directly by GPIO drivers that want to lock down
- * a certain GPIO line to be used for IRQs.
- */
-int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
+static int gpiodesc_lock_as_irq(struct gpio_desc *desc)
 {
-	struct gpio_desc *desc;
-
-	desc = gpiochip_get_desc(chip, offset);
-	if (IS_ERR(desc))
-		return PTR_ERR(desc);
+	struct gpio_chip *chip = gpiod_to_chip(desc);

 	/*
 	 * If it's fast: flush the direction setting if something changed
 	 * behind our back
 	 */
 	if (!chip->can_sleep && chip->get_direction) {
-		int dir = chip->get_direction(chip, offset);
+		int dir = chip->get_direction(chip, gpio_chip_hwgpio(desc));

 		if (dir)
 			clear_bit(FLAG_IS_OUT, &desc->flags);
@@ -3276,8 +3264,53 @@ int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)

 	return 0;
 }
+
+/**
+ * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ
+ * @chip: the chip the GPIO to lock belongs to
+ * @offset: the offset of the GPIO to lock as IRQ
+ *
+ * This is used directly by GPIO drivers that want to lock down
+ * a certain GPIO line to be used for IRQs.
+ */
+int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpio_desc *desc;
+
+	desc = gpiochip_get_desc(chip, offset);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+	return gpiodesc_lock_as_irq(desc);
+}
 EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq);

+int gpiod_enable_irq(struct gpio_desc *desc)
+{
+	int irq;
+	int ret;
+
+	VALIDATE_DESC(desc);
+
+	irq = gpiod_to_irq(desc);
+	if (irq < 0)
+		return 0;
+
+	ret = gpiodesc_lock_as_irq(desc);
+	if (!ret)
+		enable_irq(irq);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gpiod_enable_irq);
+
+static void gpiodesc_unlock_as_irq(struct gpio_desc *desc)
+{
+	clear_bit(FLAG_USED_AS_IRQ, &desc->flags);
+
+	/* If we only had this marking, erase it */
+	if (desc->label && !strcmp(desc->label, "interrupt"))
+		desc_set_label(desc, NULL);
+}
+
 /**
  * gpiochip_unlock_as_irq() - unlock a GPIO used as IRQ
  * @chip: the chip the GPIO to lock belongs to
@@ -3294,14 +3327,24 @@ void gpiochip_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
 	if (IS_ERR(desc))
 		return;

-	clear_bit(FLAG_USED_AS_IRQ, &desc->flags);
-
-	/* If we only had this marking, erase it */
-	if (desc->label && !strcmp(desc->label, "interrupt"))
-		desc_set_label(desc, NULL);
+	gpiodesc_unlock_as_irq(desc);
 }
 EXPORT_SYMBOL_GPL(gpiochip_unlock_as_irq);

+void gpiod_disable_irq(struct gpio_desc *desc)
+{
+	int irq;
+
+	VALIDATE_DESC_VOID(desc);
+	irq = gpiod_to_irq(desc);
+	if (irq < 0)
+		return;
+
+	disable_irq(irq);
+	gpiodesc_unlock_as_irq(desc);
+}
+EXPORT_SYMBOL_GPL(gpiod_disable_irq);
+
 bool gpiochip_line_is_irq(struct gpio_chip *chip, unsigned int offset)
 {
 	if (offset >= chip->ngpio)
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 243112c7fa7d..c7a9d154df22 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -104,6 +104,9 @@ int gpiod_direction_input(struct gpio_desc *desc);
 int gpiod_direction_output(struct gpio_desc *desc, int value);
 int gpiod_direction_output_raw(struct gpio_desc *desc, int value);

+int gpiod_enable_irq(struct gpio_desc *desc);
+void gpiod_disable_irq(struct gpio_desc *desc);
+
 /* Value get/set from non-sleeping context */
 int gpiod_get_value(const struct gpio_desc *desc);
 int gpiod_get_array_value(unsigned int array_size,
-- 
2.18.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