[RFC PATCH] gpiolib: chain irq_request/release_resources() and irq_dis/enable()

[Hans Verkuil <hverkuil@xxxxxxxxx>]

Hi all,

Here is yet another attempt to allow drivers to disable the irq and drive
the gpio as an output.

Please be gentle with me: I am neither an expert on the gpio internals, nor on
the irq internals.

This patch lets gpiolib override the irq_chip's irq_request/release_resources and
irq_en/disable hooks.

The old hooks are stored and called by gpiolib.

As a result, the gpiochip_(un)lock_as_irq functions can become static (not yet
implemented in this patch) and these calls should be removed from all drivers.

I haven't done that since I first want to know if what I am doing here even
makes sense.

Reviewing the removal of those calls in drivers should be fairly easy.

Regards,

	Hans

Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
---
 drivers/gpio/gpiolib.c      | 78 +++++++++++++++++++++++--------------
 include/linux/gpio/driver.h |  5 +++
 2 files changed, 53 insertions(+), 30 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e11a3bb03820..20429197756f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1800,28 +1800,48 @@ 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 res = 0;

 	if (!try_module_get(chip->gpiodev->owner))
 		return -ENODEV;
-
-	if (gpiochip_lock_as_irq(chip, d->hwirq)) {
-		chip_err(chip,
-			"unable to lock HW IRQ %lu for IRQ\n",
-			d->hwirq);
+	if (chip->irq.chip->irq_request_resources)
+		res = chip->irq.chip->irq_request_resources(d);
+	if (res)
 		module_put(chip->gpiodev->owner);
-		return -EINVAL;
-	}
-	return 0;
+	return res;
 }

 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.chip->irq_release_resources)
+		chip->irq.chip->irq_release_resources(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));
+	if (chip->irq.chip->irq_enable)
+		chip->irq.chip->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_unlock_as_irq(chip, d->hwirq);
+	if (chip->irq.chip->irq_disable)
+		chip->irq.chip->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))
@@ -1889,16 +1909,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;

@@ -1956,8 +1966,16 @@ 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_request_resources =
+			gpiochip->irq.irq_request_resources;
+		gpiochip->irq.chip->irq_release_resources =
+			gpiochip->irq.irq_release_resources;
+		gpiochip->irq.chip->irq_enable = gpiochip->irq.irq_enable;
+		gpiochip->irq.chip->irq_disable = gpiochip->irq.irq_disable;
+		gpiochip->irq.irq_request_resources = NULL;
+		gpiochip->irq.irq_release_resources = NULL;
+		gpiochip->irq.irq_enable = NULL;
+		gpiochip->irq.irq_disable = NULL;
 		gpiochip->irq.chip = NULL;
 	}

@@ -2048,15 +2066,15 @@ 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->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;

 	acpi_gpiochip_request_interrupts(gpiochip);

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 5382b5183b7e..e352dd160424 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -138,6 +138,11 @@ 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);
+	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)
-- 
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