diff mbox series

[RFCv2,1/4] gpiolib: (un)mark gpio as irq when dis/enabling irq

Message ID 20180827130620.96232-2-hverkuil@xs4all.nl
State New
Headers show
Series gpiolib: chain irq callbacks | expand

Commit Message

Hans Verkuil Aug. 27, 2018, 1:06 p.m. UTC 
From: Hans Verkuil <hans.verkuil@cisco.com>

Drivers are currently required to call gpiochip_(un)lock_as_irq whenever
they want to use a gpio as an interrupt. Unfortunately, this is generally
done when the irq is requested, not when the irq is enabled/disabled.

This is problematic for cases where a gpio pin is temporarily used as an
interrupt pin, then, after the irq is disabled, is used as a regular
gpio pin again. Currently it is not possible to do this other than by first
freeing the interrupt so gpiochip_unlock_as_irq is called.

There are currently two drivers that would like to be able to do this:
the tda998x_drv.c driver where a regular gpio pin needs to be temporarily
reconfigured as an interrupt pin during CEC calibration, and the cec-gpio
driver where you want to configure the gpio pin as an interrupt while
waiting for traffic over the CEC bus, or as a regular pin when receiving or
transmitting a CEC message.

The core problem is that gpiochip_(un)lock_as_irq is called when the
interrupt is allocated/freed, not when the interrupt is enabled/disabled.

Also, it is hit-and-miss whether drivers call these functions.

This patch hooks gpiochip_irq_(un)lock into the irq_enable/disable
callbacks of the irqchip. That way drivers no longer have to call these
functions and this is all done transparently for drivers.

The old gpiochip_(un)lock_as_irq() functions are now empty stubs that
can be removed once all drivers that call them are updated.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpio/gpiolib.c      | 155 +++++++++++++++++++++++++++---------
 include/linux/gpio/driver.h |   7 ++
 2 files changed, 123 insertions(+), 39 deletions(-)

Comments

Linus Walleij Aug. 29, 2018, 7:15 a.m. UTC | #1 
On Mon, Aug 27, 2018 at 3:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Drivers are currently required to call gpiochip_(un)lock_as_irq whenever
> they want to use a gpio as an interrupt. Unfortunately, this is generally
> done when the irq is requested, not when the irq is enabled/disabled.
>
> This is problematic for cases where a gpio pin is temporarily used as an
> interrupt pin, then, after the irq is disabled, is used as a regular
> gpio pin again. Currently it is not possible to do this other than by first
> freeing the interrupt so gpiochip_unlock_as_irq is called.
>
> There are currently two drivers that would like to be able to do this:
> the tda998x_drv.c driver where a regular gpio pin needs to be temporarily
> reconfigured as an interrupt pin during CEC calibration, and the cec-gpio
> driver where you want to configure the gpio pin as an interrupt while
> waiting for traffic over the CEC bus, or as a regular pin when receiving or
> transmitting a CEC message.
>
> The core problem is that gpiochip_(un)lock_as_irq is called when the
> interrupt is allocated/freed, not when the interrupt is enabled/disabled.
>
> Also, it is hit-and-miss whether drivers call these functions.
>
> This patch hooks gpiochip_irq_(un)lock into the irq_enable/disable
> callbacks of the irqchip. That way drivers no longer have to call these
> functions and this is all done transparently for drivers.
>
> The old gpiochip_(un)lock_as_irq() functions are now empty stubs that
> can be removed once all drivers that call them are updated.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

This looks *REALLY* good to me.

It solves a bunch of long-standing unelegance issues with the GPIO
IRQs by creating an indirection of IRQchip pointers.

I just want to know that Marc Z in on board with this change
and I want to apply it (or if you want to make a v1) early so
we can shake out any bugs in the development cycle for v4.20.

Yours,
Linus Walleij
Linus Walleij Aug. 29, 2018, 7:18 a.m. UTC | #2 
On Mon, Aug 27, 2018 at 3:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> This patch hooks gpiochip_irq_(un)lock into the irq_enable/disable
> callbacks of the irqchip. That way drivers no longer have to call these
> functions and this is all done transparently for drivers.

We need to add a notice that this only concerns the drivers that use
GPIOLIB_IRQCHIP. All others still need to implement all their irqchip
semantics all by themselves.

Yours,
Linus Walleij
Hans Verkuil Aug. 29, 2018, 8:05 a.m. UTC | #3 
On 29/08/18 09:15, Linus Walleij wrote:
> On Mon, Aug 27, 2018 at 3:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Drivers are currently required to call gpiochip_(un)lock_as_irq whenever
>> they want to use a gpio as an interrupt. Unfortunately, this is generally
>> done when the irq is requested, not when the irq is enabled/disabled.
>>
>> This is problematic for cases where a gpio pin is temporarily used as an
>> interrupt pin, then, after the irq is disabled, is used as a regular
>> gpio pin again. Currently it is not possible to do this other than by first
>> freeing the interrupt so gpiochip_unlock_as_irq is called.
>>
>> There are currently two drivers that would like to be able to do this:
>> the tda998x_drv.c driver where a regular gpio pin needs to be temporarily
>> reconfigured as an interrupt pin during CEC calibration, and the cec-gpio
>> driver where you want to configure the gpio pin as an interrupt while
>> waiting for traffic over the CEC bus, or as a regular pin when receiving or
>> transmitting a CEC message.
>>
>> The core problem is that gpiochip_(un)lock_as_irq is called when the
>> interrupt is allocated/freed, not when the interrupt is enabled/disabled.
>>
>> Also, it is hit-and-miss whether drivers call these functions.
>>
>> This patch hooks gpiochip_irq_(un)lock into the irq_enable/disable
>> callbacks of the irqchip. That way drivers no longer have to call these
>> functions and this is all done transparently for drivers.
>>
>> The old gpiochip_(un)lock_as_irq() functions are now empty stubs that
>> can be removed once all drivers that call them are updated.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This looks *REALLY* good to me.

It does? Overriding all these irqchip hooks is scary. But it might be
because this is all new to me...

> It solves a bunch of long-standing unelegance issues with the GPIO
> IRQs by creating an indirection of IRQchip pointers.
> 
> I just want to know that Marc Z in on board with this change
> and I want to apply it (or if you want to make a v1) early so
> we can shake out any bugs in the development cycle for v4.20.

Can you reply to the question in my cover letter? I need to know
the answer before spinning a non-RFC patch series.

Should I prepare that patch series on top of a specific git tree? Currently
I just used the mainline tree for this.

Regards,

	Hans
Linus Walleij Sept. 5, 2018, 9:24 a.m. UTC | #4 
On Wed, Aug 29, 2018 at 10:05 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Can you reply to the question in my cover letter? I need to know
> the answer before spinning a non-RFC patch series.
>
> Should I prepare that patch series on top of a specific git tree? Currently
> I just used the mainline tree for this.

Sorry that I missed this :(

Ideally the "devel" branch in the linux-gpio git should be used,
but mostly mainine -rc1 work as well.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e8f8a1999393..0efa9ec4fec4 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -86,6 +86,8 @@  static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
 static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gpiochip);
 static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip);
+static int gpiochip_irq_lock(struct gpio_chip *chip, unsigned int offset);
+static void gpiochip_irq_unlock(struct gpio_chip *chip, unsigned int offset);
 
 static bool gpiolib_initialized;
 
@@ -1807,30 +1809,65 @@  static const struct irq_domain_ops gpiochip_domain_ops = {
 static int gpiochip_irq_reqres(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
-	int ret;
+	int ret = 0;
 
 	if (!try_module_get(chip->gpiodev->owner))
 		return -ENODEV;
 
-	ret = gpiochip_lock_as_irq(chip, d->hwirq);
-	if (ret) {
-		chip_err(chip,
-			"unable to lock HW IRQ %lu for IRQ\n",
-			d->hwirq);
+	if (chip->irq.irq_request_resources)
+		ret = chip->irq.irq_request_resources(d);
+	if (ret)
 		module_put(chip->gpiodev->owner);
-		return ret;
-	}
-	return 0;
+	return ret;
 }
 
 static void gpiochip_irq_relres(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
 
-	gpiochip_unlock_as_irq(chip, d->hwirq);
+	if (chip->irq.irq_release_resources)
+		chip->irq.irq_release_resources(d);
 	module_put(chip->gpiodev->owner);
 }
 
+static unsigned int gpiochip_irq_startup(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	WARN_ON(gpiochip_irq_lock(chip, d->hwirq));
+	return chip->irq.irq_startup(d);
+}
+
+static void gpiochip_irq_shutdown(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	gpiochip_irq_unlock(chip, d->hwirq);
+	chip->irq.irq_shutdown(d);
+}
+
+static void gpiochip_irq_enable(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	WARN_ON(gpiochip_irq_lock(chip, d->hwirq));
+	if (chip->irq.irq_enable)
+		chip->irq.irq_enable(d);
+	else
+		chip->irq.chip->irq_unmask(d);
+}
+
+static void gpiochip_irq_disable(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	gpiochip_irq_unlock(chip, d->hwirq);
+	if (chip->irq.irq_disable)
+		chip->irq.irq_disable(d);
+	else
+		chip->irq.chip->irq_mask(d);
+}
+
 static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 {
 	if (!gpiochip_irqchip_irq_valid(chip, offset))
@@ -1839,6 +1876,33 @@  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 	return irq_create_mapping(chip->irq.domain, offset);
 }
 
+static void gpiochip_set_irq_hooks(struct gpio_chip *gpiochip)
+{
+	struct irq_chip *irqchip = gpiochip->irq.chip;
+
+	if (irqchip->irq_request_resources == gpiochip_irq_reqres)
+		return;
+
+	gpiochip->irq.irq_request_resources = irqchip->irq_request_resources;
+	gpiochip->irq.irq_release_resources = irqchip->irq_release_resources;
+	gpiochip->irq.irq_enable = irqchip->irq_enable;
+	gpiochip->irq.irq_disable = irqchip->irq_disable;
+
+	irqchip->irq_request_resources = gpiochip_irq_reqres;
+	irqchip->irq_release_resources = gpiochip_irq_relres;
+	irqchip->irq_enable = gpiochip_irq_enable;
+	irqchip->irq_disable = gpiochip_irq_disable;
+
+	if (irqchip->irq_startup) {
+		gpiochip->irq.irq_startup = irqchip->irq_startup;
+		irqchip->irq_startup = gpiochip_irq_startup;
+	}
+	if (irqchip->irq_shutdown) {
+		gpiochip->irq.irq_shutdown = irqchip->irq_shutdown;
+		irqchip->irq_shutdown = gpiochip_irq_shutdown;
+	}
+}
+
 /**
  * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip
  * @gpiochip: the GPIO chip to add the IRQ chip to
@@ -1897,16 +1961,6 @@  static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 	if (!gpiochip->irq.domain)
 		return -EINVAL;
 
-	/*
-	 * It is possible for a driver to override this, but only if the
-	 * alternative functions are both implemented.
-	 */
-	if (!irqchip->irq_request_resources &&
-	    !irqchip->irq_release_resources) {
-		irqchip->irq_request_resources = gpiochip_irq_reqres;
-		irqchip->irq_release_resources = gpiochip_irq_relres;
-	}
-
 	if (gpiochip->irq.parent_handler) {
 		void *data = gpiochip->irq.parent_handler_data ?: gpiochip;
 
@@ -1922,6 +1976,8 @@  static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 		}
 	}
 
+	gpiochip_set_irq_hooks(gpiochip);
+
 	acpi_gpiochip_request_interrupts(gpiochip);
 
 	return 0;
@@ -1935,11 +1991,12 @@  static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
  */
 static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
 {
+	struct irq_chip *irqchip = gpiochip->irq.chip;
 	unsigned int offset;
 
 	acpi_gpiochip_free_interrupts(gpiochip);
 
-	if (gpiochip->irq.chip && gpiochip->irq.parent_handler) {
+	if (irqchip && gpiochip->irq.parent_handler) {
 		struct gpio_irq_chip *irq = &gpiochip->irq;
 		unsigned int i;
 
@@ -1963,11 +2020,26 @@  static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
 		irq_domain_remove(gpiochip->irq.domain);
 	}
 
-	if (gpiochip->irq.chip) {
-		gpiochip->irq.chip->irq_request_resources = NULL;
-		gpiochip->irq.chip->irq_release_resources = NULL;
-		gpiochip->irq.chip = NULL;
+	if (irqchip &&
+	    irqchip->irq_request_resources == gpiochip_irq_reqres) {
+		irqchip->irq_request_resources =
+			gpiochip->irq.irq_request_resources;
+		irqchip->irq_release_resources =
+			gpiochip->irq.irq_release_resources;
+		irqchip->irq_enable = gpiochip->irq.irq_enable;
+		irqchip->irq_disable = gpiochip->irq.irq_disable;
+		if (gpiochip->irq.irq_startup)
+			irqchip->irq_startup = gpiochip->irq.irq_startup;
+		if (gpiochip->irq.irq_shutdown)
+			irqchip->irq_shutdown = gpiochip->irq.irq_shutdown;
 	}
+	gpiochip->irq.irq_request_resources = NULL;
+	gpiochip->irq.irq_release_resources = NULL;
+	gpiochip->irq.irq_startup = NULL;
+	gpiochip->irq.irq_shutdown = NULL;
+	gpiochip->irq.irq_enable = NULL;
+	gpiochip->irq.irq_disable = NULL;
+	gpiochip->irq.chip = NULL;
 
 	gpiochip_irqchip_free_valid_mask(gpiochip);
 }
@@ -2056,15 +2128,7 @@  int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
 		return -EINVAL;
 	}
 
-	/*
-	 * It is possible for a driver to override this, but only if the
-	 * alternative functions are both implemented.
-	 */
-	if (!irqchip->irq_request_resources &&
-	    !irqchip->irq_release_resources) {
-		irqchip->irq_request_resources = gpiochip_irq_reqres;
-		irqchip->irq_release_resources = gpiochip_irq_relres;
-	}
+	gpiochip_set_irq_hooks(gpiochip);
 
 	acpi_gpiochip_request_interrupts(gpiochip);
 
@@ -3255,14 +3319,14 @@  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
+ * gpiochip_irq_lock() - 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 gpiochip_irq_lock(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpio_desc *desc;
 
@@ -3303,17 +3367,16 @@  int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq);
 
 /**
- * gpiochip_unlock_as_irq() - unlock a GPIO used as IRQ
+ * gpiochip_irq_unlock() - unlock a GPIO 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 indicate
  * that a certain GPIO is no longer used exclusively for IRQ.
  */
-void gpiochip_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
+static void gpiochip_irq_unlock(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpio_desc *desc;
 
@@ -3327,6 +3390,20 @@  void gpiochip_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
 	if (desc->label && !strcmp(desc->label, "interrupt"))
 		desc_set_label(desc, NULL);
 }
+
+/*
+ * Keep these two temporary stubs until all drivers stop calling these
+ * functions.
+ */
+int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq);
+
+void gpiochip_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
+{
+}
 EXPORT_SYMBOL_GPL(gpiochip_unlock_as_irq);
 
 bool gpiochip_line_is_irq(struct gpio_chip *chip, unsigned int offset)
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 0ea328e71ec9..0485bd339178 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -138,6 +138,13 @@  struct gpio_irq_chip {
 	 * will allocate and map all IRQs during initialization.
 	 */
 	unsigned int first;
+
+	int		(*irq_request_resources)(struct irq_data *data);
+	void		(*irq_release_resources)(struct irq_data *data);
+	unsigned int	(*irq_startup)(struct irq_data *data);
+	void		(*irq_shutdown)(struct irq_data *data);
+	void		(*irq_enable)(struct irq_data *data);
+	void		(*irq_disable)(struct irq_data *data);
 };
 
 static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip)