diff mbox series

[2/6] gpiolib: override irq_request/release_resources hooks

Message ID 20180907102053.3221-3-hverkuil@xs4all.nl
State New
Headers show
Series gpiolib: track irq enabled/disabled state | expand

Commit Message

Hans Verkuil Sept. 7, 2018, 10:20 a.m. UTC 
From: Hans Verkuil <hans.verkuil@cisco.com>

Rather than completely replacing any existing irq_request/release_resources
irq_chip callbacks, remember the driver's version and call it from our
override.

Two drivers that select GPIOLIB_IRQCHIP and set these callbacks had to
be changed, otherwise you would get into an infinite loop.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib.c                | 60 ++++++++++++++++-----------
 drivers/pinctrl/intel/pinctrl-intel.c | 32 --------------
 drivers/pinctrl/pinctrl-st.c          | 11 +----
 include/linux/gpio/driver.h           | 14 +++++++
 4 files changed, 50 insertions(+), 67 deletions(-)

Comments

Mika Westerberg Sept. 7, 2018, 10:33 a.m. UTC | #1 
Hi,

On Fri, Sep 07, 2018 at 12:20:49PM +0200, Hans Verkuil wrote:
> -static int intel_gpio_irq_reqres(struct irq_data *d)
> -{
> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> -	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
> -	int pin;
> -	int ret;
> -
> -	pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);

I don't think you can just remove this hook because we depend on the
above translation.

> -	if (pin >= 0) {
> -		ret = gpiochip_lock_as_irq(gc, pin);
> -		if (ret) {
> -			dev_err(pctrl->dev, "unable to lock HW IRQ %d for IRQ\n",
> -				pin);
> -			return ret;
> -		}
> -	}
> -	return 0;
> -}
> -
> -static void intel_gpio_irq_relres(struct irq_data *d)
> -{
> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> -	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
> -	int pin;
> -
> -	pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);

Same here.

> -	if (pin >= 0)
> -		gpiochip_unlock_as_irq(gc, pin);
> -}
Hans Verkuil Sept. 7, 2018, 10:51 a.m. UTC | #2 
On 09/07/2018 12:33 PM, Mika Westerberg wrote:
> Hi,
> 
> On Fri, Sep 07, 2018 at 12:20:49PM +0200, Hans Verkuil wrote:
>> -static int intel_gpio_irq_reqres(struct irq_data *d)
>> -{
>> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> -	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
>> -	int pin;
>> -	int ret;
>> -
>> -	pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
> 
> I don't think you can just remove this hook because we depend on the
> above translation.
> 
>> -	if (pin >= 0) {
>> -		ret = gpiochip_lock_as_irq(gc, pin);
>> -		if (ret) {
>> -			dev_err(pctrl->dev, "unable to lock HW IRQ %d for IRQ\n",
>> -				pin);
>> -			return ret;
>> -		}
>> -	}
>> -	return 0;
>> -}
>> -
>> -static void intel_gpio_irq_relres(struct irq_data *d)
>> -{
>> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> -	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
>> -	int pin;
>> -
>> -	pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
> 
> Same here.
> 
>> -	if (pin >= 0)
>> -		gpiochip_unlock_as_irq(gc, pin);
>> -}

Oops, I didn't see this.

Linus, you said here https://www.spinics.net/lists/linux-gpio/msg32297.html:

"Actually the latter method, to use local copies of the enable/disable
function pointers in gpiochip->irq is what we should use also
for the request/release_resources callbacks."

I did that in this series, but in the original code gpiolib never overrides
the irq_chip callbacks, only if both request and release callbacks are NULL
will gpiolib fill in its own callback.

And that's no doubt for this reason (irq translation).

Unless you have a better idea, then I will just revert this and only set
these callbacks if the driver doesn't specify them (i.e. pretty much what I
did in my RFCv3 series).

The advantage is that I no longer need to modify this driver and the
pinctrl-st.c driver.

Regards,

	Hans
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cbab0e744de0..d2c1e2cccc65 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1815,17 +1815,40 @@  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 static int gpiochip_irq_reqres(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	int ret;
 
-	return gpiochip_reqres_irq(chip, d->hwirq);
+	ret = gpiochip_reqres_irq(chip, d->hwirq);
+	if (ret || !chip->irq.irq_reqres)
+		return ret;
+	ret = chip->irq.irq_reqres(d);
+	if (ret)
+		gpiochip_relres_irq(chip, d->hwirq);
+	return ret;
 }
 
 static void gpiochip_irq_relres(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
 
+	if (chip->irq.irq_relres)
+		chip->irq.irq_relres(d);
 	gpiochip_relres_irq(chip, d->hwirq);
 }
 
+static void gpiochip_set_irq_hooks(struct gpio_chip *gpiochip)
+{
+	struct irq_chip *irqchip = gpiochip->irq.chip;
+
+	if (WARN_ON(irqchip->irq_request_resources == gpiochip_irq_reqres))
+		return;
+
+	gpiochip->irq.irq_reqres = irqchip->irq_request_resources;
+	gpiochip->irq.irq_relres = irqchip->irq_release_resources;
+
+	irqchip->irq_request_resources = gpiochip_irq_reqres;
+	irqchip->irq_release_resources = gpiochip_irq_relres;
+}
+
 /**
  * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip
  * @gpiochip: the GPIO chip to add the IRQ chip to
@@ -1884,16 +1907,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;
 
@@ -1909,6 +1922,8 @@  static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 		}
 	}
 
+	gpiochip_set_irq_hooks(gpiochip);
+
 	acpi_gpiochip_request_interrupts(gpiochip);
 
 	return 0;
@@ -1922,11 +1937,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;
 
@@ -1950,11 +1966,13 @@  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_reqres;
+		irqchip->irq_release_resources = gpiochip->irq.irq_relres;
 	}
+	gpiochip->irq.irq_reqres = NULL;
+	gpiochip->irq.irq_relres = NULL;
+	gpiochip->irq.chip = NULL;
 
 	gpiochip_irqchip_free_valid_mask(gpiochip);
 }
@@ -2043,15 +2061,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);
 
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 62b009b27eda..3d0bd7b99725 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -872,36 +872,6 @@  static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned offset,
 	return -EINVAL;
 }
 
-static int intel_gpio_irq_reqres(struct irq_data *d)
-{
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
-	int pin;
-	int ret;
-
-	pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
-	if (pin >= 0) {
-		ret = gpiochip_lock_as_irq(gc, pin);
-		if (ret) {
-			dev_err(pctrl->dev, "unable to lock HW IRQ %d for IRQ\n",
-				pin);
-			return ret;
-		}
-	}
-	return 0;
-}
-
-static void intel_gpio_irq_relres(struct irq_data *d)
-{
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
-	int pin;
-
-	pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
-	if (pin >= 0)
-		gpiochip_unlock_as_irq(gc, pin);
-}
-
 static void intel_gpio_irq_ack(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
@@ -1117,8 +1087,6 @@  static irqreturn_t intel_gpio_irq(int irq, void *data)
 
 static struct irq_chip intel_gpio_irqchip = {
 	.name = "intel-gpio",
-	.irq_request_resources = intel_gpio_irq_reqres,
-	.irq_release_resources = intel_gpio_irq_relres,
 	.irq_enable = intel_gpio_irq_enable,
 	.irq_ack = intel_gpio_irq_ack,
 	.irq_mask = intel_gpio_irq_mask,
diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
index 0966bb0bf71f..02a45bcd79c1 100644
--- a/drivers/pinctrl/pinctrl-st.c
+++ b/drivers/pinctrl/pinctrl-st.c
@@ -1290,15 +1290,7 @@  static int st_gpio_irq_request_resources(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 
 	st_gpio_direction_input(gc, d->hwirq);
-
-	return gpiochip_lock_as_irq(gc, d->hwirq);
-}
-
-static void st_gpio_irq_release_resources(struct irq_data *d)
-{
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-
-	gpiochip_unlock_as_irq(gc, d->hwirq);
+	return 0;
 }
 
 static int st_gpio_irq_set_type(struct irq_data *d, unsigned type)
@@ -1456,7 +1448,6 @@  static const struct gpio_chip st_gpio_template = {
 static struct irq_chip st_gpio_irqchip = {
 	.name			= "GPIO",
 	.irq_request_resources	= st_gpio_irq_request_resources,
-	.irq_release_resources	= st_gpio_irq_release_resources,
 	.irq_disable		= st_gpio_irq_mask,
 	.irq_mask		= st_gpio_irq_mask,
 	.irq_unmask		= st_gpio_irq_unmask,
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 380251955ad1..b445b083e568 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -138,6 +138,20 @@  struct gpio_irq_chip {
 	 * will allocate and map all IRQs during initialization.
 	 */
 	unsigned int first;
+
+	/**
+	 * @irq_reqres:
+	 *
+	 * Store old irq_chip irq_request_resources callback
+	 */
+	int		(*irq_reqres)(struct irq_data *d);
+
+	/**
+	 * @irq_relres:
+	 *
+	 * Store old irq_chip irq_release_resources callback
+	 */
+	void		(*irq_relres)(struct irq_data *d);
 };
 
 static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip)