[PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[Linus Walleij]

It is currently often possible in many GPIO drivers to request
a GPIO line to be used as IRQ after calling gpio_to_irq() and,
as the gpiolib is not aware of this, set the same line to
output and start driving it, with undesired side effects.

As it is a bogus usage scenario to request a line flagged as
output to used as IRQ, we introduce APIs to let gpiolib track
the use of a line as IRQ, and also set this flag from the
userspace ABI.

In this RFC patch we also augment the Nomadik pinctrl driver
to use this API to give an idea of how it is to be used.
It is probably not possible to lock line as IRQ in the gpiolib
.to_irq() callback, as the line may have different use cases
over time in a system. For a certain system or driver it may
however be possible to lock the line as IRQ in the .to_irq()
path. An alternative approach is to let the irq_chip
.irq_enable() callback do this.

The API is symmetric so that lines can also be unflagged from
IRQ use by e.g. .irq_disable(). The debugfs file is altered
so that we see if a line is reserved for IRQ.

Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Cc: Enric Balletbo i Serra <eballetbo@gmail.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c            | 86 ++++++++++++++++++++++++++++++++++++++-
 drivers/pinctrl/pinctrl-nomadik.c |  8 ++++
 include/asm-generic/gpio.h        |  3 ++
 include/linux/gpio.h              | 11 +++++
 4 files changed, 106 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4fc2860..199ba51 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -60,6 +60,7 @@ struct gpio_desc {
 #define FLAG_ACTIVE_LOW	6	/* sysfs value has active low */
 #define FLAG_OPEN_DRAIN	7	/* Gpio is open drain type */
 #define FLAG_OPEN_SOURCE 8	/* Gpio is open source type */
+#define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
 
 #define ID_SHIFT	16	/* add new flags before this one */
 
@@ -163,6 +164,17 @@ static struct gpio_desc *gpio_to_desc(unsigned gpio)
 }
 
 /**
+ * Convert an offset on a certain chip to a corresponding descriptor
+ */
+static struct gpio_desc *gpiochip_offset_to_desc(struct gpio_chip *chip,
+						 unsigned int offset)
+{
+	unsigned int gpio = chip->base + offset;
+
+	return gpio_to_desc(gpio);
+}
+
+/**
  * Convert a GPIO descriptor to the integer namespace.
  * This should disappear in the future but is needed since we still
  * use GPIO numbers for error messages and sysfs nodes
@@ -466,6 +478,13 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
 	if (ret < 0)
 		goto free_id;
 
+	ret = gpiod_lock_as_irq(desc);
+	if (ret < 0) {
+		gpiod_warn(desc,
+			   "failed to flag the GPIO for IRQ\n");
+		goto free_id;
+	}
+
 	desc->flags |= gpio_flags;
 	return 0;
 
@@ -1730,6 +1749,14 @@ static int gpiod_direction_output(struct gpio_desc *desc, int value)
 		return -EINVAL;
 	}
 
+	/* GPIOs used for IRQs shall not be set as output */
+	if (test_bit(FLAG_USED_AS_IRQ, &desc->flags)) {
+		gpiod_err(desc,
+			  "%s: tried to set a GPIO tied to an IRQ as output\n",
+			  __func__);
+		return -EIO;
+	}
+
 	/* Open drain pin should not be driven to 1 */
 	if (value && test_bit(FLAG_OPEN_DRAIN,  &desc->flags))
 		return gpiod_direction_input(desc);
@@ -2050,6 +2077,58 @@ int __gpio_to_irq(unsigned gpio)
 }
 EXPORT_SYMBOL_GPL(__gpio_to_irq);
 
+/**
+ * gpiod_lock_as_irq() - lock a GPIO to be used as IRQ
+ * @gpio: the GPIO line to lock as used for IRQ
+ *
+ * This is used directly by GPIO drivers that want to lock down
+ * a certain GPIO line to be used as IRQs, for example in the
+ * .to_irq() callback of their gpio_chip, or in the .irq_enable()
+ * of its irq_chip implementation if the GPIO is known from that
+ * code.
+ */
+static int gpiod_lock_as_irq(struct gpio_desc *desc)
+{
+	if (!desc)
+		return -EINVAL;
+
+	if (test_bit(FLAG_IS_OUT, &desc->flags)) {
+		gpiod_err(desc,
+			  "%s: tried to flag a GPIO set as output for IRQ\n",
+			  __func__);
+		return -EIO;
+	}
+
+	set_bit(FLAG_USED_AS_IRQ, &desc->flags);
+	return 0;
+}
+
+int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	return gpiod_lock_as_irq(gpiochip_offset_to_desc(chip, offset));
+}
+EXPORT_SYMBOL_GPL(gpio_lock_as_irq);
+
+/**
+ * gpiod_unlock_as_irq() - unlock a GPIO used as IRQ
+ * @gpio: the GPIO line to unlock from IRQ usage
+ *
+ * This is used directly by GPIO drivers that want to indicate
+ * that a certain GPIO is no longer used exclusively for IRQ.
+ */
+static void gpiod_unlock_as_irq(struct gpio_desc *desc)
+{
+	if (!desc)
+		return;
+
+	clear_bit(FLAG_USED_AS_IRQ, &desc->flags);
+}
+
+void gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	return gpiod_unlock_as_irq(gpiochip_offset_to_desc(chip, offset));
+}
+EXPORT_SYMBOL_GPL(gpio_unlock_as_irq);
 
 /* There's no value in making it easy to inline GPIO calls that may sleep.
  * Common examples include ones connected to I2C or SPI chips.
@@ -2091,6 +2170,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	unsigned		gpio = chip->base;
 	struct gpio_desc	*gdesc = &chip->desc[0];
 	int			is_out;
+	int			is_irq;
 
 	for (i = 0; i < chip->ngpio; i++, gpio++, gdesc++) {
 		if (!test_bit(FLAG_REQUESTED, &gdesc->flags))
@@ -2098,12 +2178,14 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 
 		gpiod_get_direction(gdesc);
 		is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
-		seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
+		is_irq = test_bit(FLAG_USED_AS_IRQ, &gdesc->flags);
+		seq_printf(s, " gpio-%-3d (%-20.20s) %s %s %s",
 			gpio, gdesc->label,
 			is_out ? "out" : "in ",
 			chip->get
 				? (chip->get(chip, i) ? "hi" : "lo")
-				: "?  ");
+				: "?  ",
+			is_irq ? "IRQ" : "   ");
 		seq_printf(s, "\n");
 	}
 }
diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
index d7c3ae3..e2d9e36 100644
--- a/drivers/pinctrl/pinctrl-nomadik.c
+++ b/drivers/pinctrl/pinctrl-nomadik.c
@@ -795,6 +795,14 @@ static int nmk_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 {
 	struct nmk_gpio_chip *nmk_chip =
 		container_of(chip, struct nmk_gpio_chip, chip);
+	int ret;
+
+	ret = gpio_lock_as_irq(chip, offset);
+	if (ret) {
+		dev_err(chip->dev, "unable to lock offset %d for IRQ\n",
+			offset);
+		return ret;
+	}
 
 	return irq_create_mapping(nmk_chip->domain, offset);
 }
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index bde6469..b309a5c 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -192,6 +192,9 @@ extern int __gpio_cansleep(unsigned gpio);
 
 extern int __gpio_to_irq(unsigned gpio);
 
+extern int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset);
+extern void gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset);
+
 extern int gpio_request_one(unsigned gpio, unsigned long flags, const char *label);
 extern int gpio_request_array(const struct gpio *array, size_t num);
 extern void gpio_free_array(const struct gpio *array, size_t num);
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index 552e3f4..68e5523 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -204,6 +204,17 @@ static inline int gpio_to_irq(unsigned gpio)
 	return -EINVAL;
 }
 
+static inline int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	WARN_ON(1);
+}
+
+static inline void gpio_unlock_as_irq(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	WARN_ON(1);
+}
+
 static inline int irq_to_gpio(unsigned irq)
 {
 	/* irq can never have been returned from gpio_to_irq() */
-- 
1.8.3.1

Re: [PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[Javier Martinez Canillas]

On 09/24/2013 01:33 PM, Linus Walleij wrote:
> It is currently often possible in many GPIO drivers to request
> a GPIO line to be used as IRQ after calling gpio_to_irq() and,
> as the gpiolib is not aware of this, set the same line to
> output and start driving it, with undesired side effects.
> 
> As it is a bogus usage scenario to request a line flagged as
> output to used as IRQ, we introduce APIs to let gpiolib track
> the use of a line as IRQ, and also set this flag from the
> userspace ABI.
> 
> In this RFC patch we also augment the Nomadik pinctrl driver
> to use this API to give an idea of how it is to be used.
> It is probably not possible to lock line as IRQ in the gpiolib
> .to_irq() callback, as the line may have different use cases
> over time in a system. For a certain system or driver it may
> however be possible to lock the line as IRQ in the .to_irq()
> path. An alternative approach is to let the irq_chip
> .irq_enable() callback do this.
> 
> The API is symmetric so that lines can also be unflagged from
> IRQ use by e.g. .irq_disable(). The debugfs file is altered
> so that we see if a line is reserved for IRQ.
> 
> Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Cc: Enric Balletbo i Serra <eballetbo@gmail.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib.c            | 86 ++++++++++++++++++++++++++++++++++++++-
>  drivers/pinctrl/pinctrl-nomadik.c |  8 ++++
>  include/asm-generic/gpio.h        |  3 ++
>  include/linux/gpio.h              | 11 +++++
>  4 files changed, 106 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 4fc2860..199ba51 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -60,6 +60,7 @@ struct gpio_desc {
>  #define FLAG_ACTIVE_LOW	6	/* sysfs value has active low */
>  #define FLAG_OPEN_DRAIN	7	/* Gpio is open drain type */
>  #define FLAG_OPEN_SOURCE 8	/* Gpio is open source type */
> +#define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
>  
>  #define ID_SHIFT	16	/* add new flags before this one */
>  
> @@ -163,6 +164,17 @@ static struct gpio_desc *gpio_to_desc(unsigned gpio)
>  }
>  
>  /**
> + * Convert an offset on a certain chip to a corresponding descriptor
> + */
> +static struct gpio_desc *gpiochip_offset_to_desc(struct gpio_chip *chip,
> +						 unsigned int offset)
> +{
> +	unsigned int gpio = chip->base + offset;
> +
> +	return gpio_to_desc(gpio);
> +}
> +
> +/**
>   * Convert a GPIO descriptor to the integer namespace.
>   * This should disappear in the future but is needed since we still
>   * use GPIO numbers for error messages and sysfs nodes
> @@ -466,6 +478,13 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
>  	if (ret < 0)
>  		goto free_id;
>  
> +	ret = gpiod_lock_as_irq(desc);
> +	if (ret < 0) {
> +		gpiod_warn(desc,
> +			   "failed to flag the GPIO for IRQ\n");
> +		goto free_id;
> +	}
> +
>  	desc->flags |= gpio_flags;
>  	return 0;
>  
> @@ -1730,6 +1749,14 @@ static int gpiod_direction_output(struct gpio_desc *desc, int value)
>  		return -EINVAL;
>  	}
>  
> +	/* GPIOs used for IRQs shall not be set as output */
> +	if (test_bit(FLAG_USED_AS_IRQ, &desc->flags)) {
> +		gpiod_err(desc,
> +			  "%s: tried to set a GPIO tied to an IRQ as output\n",
> +			  __func__);
> +		return -EIO;
> +	}
> +
>  	/* Open drain pin should not be driven to 1 */
>  	if (value && test_bit(FLAG_OPEN_DRAIN,  &desc->flags))
>  		return gpiod_direction_input(desc);
> @@ -2050,6 +2077,58 @@ int __gpio_to_irq(unsigned gpio)
>  }
>  EXPORT_SYMBOL_GPL(__gpio_to_irq);
>  
> +/**
> + * gpiod_lock_as_irq() - lock a GPIO to be used as IRQ
> + * @gpio: the GPIO line to lock as used for IRQ
> + *
> + * This is used directly by GPIO drivers that want to lock down
> + * a certain GPIO line to be used as IRQs, for example in the
> + * .to_irq() callback of their gpio_chip, or in the .irq_enable()
> + * of its irq_chip implementation if the GPIO is known from that
> + * code.
> + */
> +static int gpiod_lock_as_irq(struct gpio_desc *desc)
> +{
> +	if (!desc)
> +		return -EINVAL;
> +
> +	if (test_bit(FLAG_IS_OUT, &desc->flags)) {
> +		gpiod_err(desc,
> +			  "%s: tried to flag a GPIO set as output for IRQ\n",
> +			  __func__);
> +		return -EIO;
> +	}
> +
> +	set_bit(FLAG_USED_AS_IRQ, &desc->flags);
> +	return 0;
> +}
> +
> +int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +	return gpiod_lock_as_irq(gpiochip_offset_to_desc(chip, offset));
> +}
> +EXPORT_SYMBOL_GPL(gpio_lock_as_irq);
> +
> +/**
> + * gpiod_unlock_as_irq() - unlock a GPIO used as IRQ
> + * @gpio: the GPIO line to unlock from IRQ usage
> + *
> + * This is used directly by GPIO drivers that want to indicate
> + * that a certain GPIO is no longer used exclusively for IRQ.
> + */
> +static void gpiod_unlock_as_irq(struct gpio_desc *desc)
> +{
> +	if (!desc)
> +		return;
> +
> +	clear_bit(FLAG_USED_AS_IRQ, &desc->flags);
> +}
> +
> +void gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +	return gpiod_unlock_as_irq(gpiochip_offset_to_desc(chip, offset));
> +}
> +EXPORT_SYMBOL_GPL(gpio_unlock_as_irq);
>  
>  /* There's no value in making it easy to inline GPIO calls that may sleep.
>   * Common examples include ones connected to I2C or SPI chips.
> @@ -2091,6 +2170,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>  	unsigned		gpio = chip->base;
>  	struct gpio_desc	*gdesc = &chip->desc[0];
>  	int			is_out;
> +	int			is_irq;
>  
>  	for (i = 0; i < chip->ngpio; i++, gpio++, gdesc++) {
>  		if (!test_bit(FLAG_REQUESTED, &gdesc->flags))
> @@ -2098,12 +2178,14 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>  
>  		gpiod_get_direction(gdesc);
>  		is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
> -		seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
> +		is_irq = test_bit(FLAG_USED_AS_IRQ, &gdesc->flags);
> +		seq_printf(s, " gpio-%-3d (%-20.20s) %s %s %s",
>  			gpio, gdesc->label,
>  			is_out ? "out" : "in ",
>  			chip->get
>  				? (chip->get(chip, i) ? "hi" : "lo")
> -				: "?  ");
> +				: "?  ",
> +			is_irq ? "IRQ" : "   ");
>  		seq_printf(s, "\n");
>  	}
>  }
> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
> index d7c3ae3..e2d9e36 100644
> --- a/drivers/pinctrl/pinctrl-nomadik.c
> +++ b/drivers/pinctrl/pinctrl-nomadik.c
> @@ -795,6 +795,14 @@ static int nmk_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
>  	struct nmk_gpio_chip *nmk_chip =
>  		container_of(chip, struct nmk_gpio_chip, chip);
> +	int ret;
> +
> +	ret = gpio_lock_as_irq(chip, offset);
> +	if (ret) {
> +		dev_err(chip->dev, "unable to lock offset %d for IRQ\n",
> +			offset);
> +		return ret;
> +	}
>  
>  	return irq_create_mapping(nmk_chip->domain, offset);
>  }
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index bde6469..b309a5c 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -192,6 +192,9 @@ extern int __gpio_cansleep(unsigned gpio);
>  
>  extern int __gpio_to_irq(unsigned gpio);
>  
> +extern int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset);
> +extern void gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset);
> +
>  extern int gpio_request_one(unsigned gpio, unsigned long flags, const char *label);
>  extern int gpio_request_array(const struct gpio *array, size_t num);
>  extern void gpio_free_array(const struct gpio *array, size_t num);
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index 552e3f4..68e5523 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -204,6 +204,17 @@ static inline int gpio_to_irq(unsigned gpio)
>  	return -EINVAL;
>  }
>  
> +static inline int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +	WARN_ON(1);
> +}
> +
> +static inline void gpio_unlock_as_irq(struct gpio_chip *chip,
> +				      unsigned int offset)
> +{
> +	WARN_ON(1);
> +}
> +
>  static inline int irq_to_gpio(unsigned irq)
>  {
>  	/* irq can never have been returned from gpio_to_irq() */
> 

Hi Linus,

Thanks a lot for the patch, with your changes to gpiolib I could get rid of my
custom check on gpio_chip .direction_output function handler in gpio-omap.

I call gpio_lock_as_irq() in the irq_chip .irq_set_type and unlock it on
.irq_shutdown functions handlers.

I thought about posting a new version of my RFC path with these changes but it
seems that Tony is fond to first fix the regression and then take care of this.

By the way, I had to declare the function prototypes for
gpiod_{lock,unlock}_as_irq() since at least the lock function was used in
gpio_setup_irq() before its definition.

So with the following change [1] feel free to add my Reviewed-by when you post
this as a patch.

Thanks a lot and best regards,
Javier

[1]: diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c5dfc44..f96b79c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -97,6 +97,8 @@ static int gpiod_get_value(const struct gpio_desc *desc);
 static void gpiod_set_value(struct gpio_desc *desc, int value);
 static int gpiod_cansleep(const struct gpio_desc *desc);
 static int gpiod_to_irq(const struct gpio_desc *desc);
+static int gpiod_lock_as_irq(struct gpio_desc *desc);
+static void gpiod_unlock_as_irq(struct gpio_desc *desc);
 static int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
 static int gpiod_export_link(struct device *dev, const char *name,
                             struct gpio_desc *desc);

Re: [PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[Stephen Warren]

On 09/24/2013 05:33 AM, Linus Walleij wrote:
> It is currently often possible in many GPIO drivers to request
> a GPIO line to be used as IRQ after calling gpio_to_irq() and,
> as the gpiolib is not aware of this, set the same line to
> output and start driving it, with undesired side effects.
> 
> As it is a bogus usage scenario to request a line flagged as
> output to used as IRQ, we introduce APIs to let gpiolib track
> the use of a line as IRQ, and also set this flag from the
> userspace ABI.
> 
> In this RFC patch we also augment the Nomadik pinctrl driver
> to use this API to give an idea of how it is to be used.
> It is probably not possible to lock line as IRQ in the gpiolib
> .to_irq() callback, as the line may have different use cases
> over time in a system. For a certain system or driver it may
> however be possible to lock the line as IRQ in the .to_irq()
> path. An alternative approach is to let the irq_chip
> .irq_enable() callback do this.
> 
> The API is symmetric so that lines can also be unflagged from
> IRQ use by e.g. .irq_disable(). The debugfs file is altered
> so that we see if a line is reserved for IRQ.

OK, overall this seems like a reasonable general approach. I have a
couple comments on the implementation though:

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c

> @@ -466,6 +478,13 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
>  	if (ret < 0)
>  		goto free_id;
>  
> +	ret = gpiod_lock_as_irq(desc);
> +	if (ret < 0) {
> +		gpiod_warn(desc,
> +			   "failed to flag the GPIO for IRQ\n");
> +		goto free_id;
> +	}

I believe gpio_setup_irq() can be called with flags==0 (disable IRQ
usage) or flags!=0 (set up IRQ usage). As such, I believe that block of
code needs to check flags, and either call gpiod_lock_as_irq() or
gpio_unlock_as_irq() as appropriate.

> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
> @@ -795,6 +795,14 @@ static int nmk_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
>  	struct nmk_gpio_chip *nmk_chip =
>  		container_of(chip, struct nmk_gpio_chip, chip);
> +	int ret;
> +
> +	ret = gpio_lock_as_irq(chip, offset);

I don't think that gpio_to_irq() is the correct place to call the new
function, for two reasons:

1)

Not all paths that use interrupts call gpio_to_irq(). It's perfectly
valid for a driver to receive an IRQ number, request it, and be done.
The is commmon when a driver only cares about IRQ functionality and not
GPIO functionality, and hence did not receive a GPIO and convert it to
the IRQ.

To solve this, I think irq_chip drivers should call the new gpiolib
functions when the IRQ is actually requested or set up.

Related, where does gpio_unlock_as_irq() get called in the Nomadik
driver? It should happen when free_irq() is called.

2)

(Nit):

The fact that gpio_to_irq() was called doesn't actually guarantee that
the IRQ will be requested. Admittedly it's silly to call gpio_to_irq()
if you're not going to request the IRQ, adn this can be considered a
bug, but it can be done. This might happen (at least temporarily) due to
deferred probe.

Re: [PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[Linus Walleij]

On Tue, Sep 24, 2013 at 7:51 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/24/2013 05:33 AM, Linus Walleij wrote:

>> @@ -466,6 +478,13 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
>>       if (ret < 0)
>>               goto free_id;
>>
>> +     ret = gpiod_lock_as_irq(desc);
>> +     if (ret < 0) {
>> +             gpiod_warn(desc,
>> +                        "failed to flag the GPIO for IRQ\n");
>> +             goto free_id;
>> +     }
>
> I believe gpio_setup_irq() can be called with flags==0 (disable IRQ
> usage) or flags!=0 (set up IRQ usage). As such, I believe that block of
> code needs to check flags, and either call gpiod_lock_as_irq() or
> gpio_unlock_as_irq() as appropriate.

Yep fixed this. Actually hardening the sysfs interface was just an
unexpected side effect of this, rather nice.

>> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
>> @@ -795,6 +795,14 @@ static int nmk_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>>  {
>>       struct nmk_gpio_chip *nmk_chip =
>>               container_of(chip, struct nmk_gpio_chip, chip);
>> +     int ret;
>> +
>> +     ret = gpio_lock_as_irq(chip, offset);
>
> I don't think that gpio_to_irq() is the correct place to call the new
> function, for two reasons:
>
> 1)
>
> Not all paths that use interrupts call gpio_to_irq(). It's perfectly
> valid for a driver to receive an IRQ number, request it, and be done.
> The is commmon when a driver only cares about IRQ functionality and not
> GPIO functionality, and hence did not receive a GPIO and convert it to
> the IRQ.
>
> To solve this, I think irq_chip drivers should call the new gpiolib
> functions when the IRQ is actually requested or set up.
>
> Related, where does gpio_unlock_as_irq() get called in the Nomadik
> driver? It should happen when free_irq() is called.

Yeah if we formalize the criterion that interrupts out of any GPIO
chips should be possible to request without first getting it from the
<linux/gpio.h> interface, then this holds.

However that is not the whole story, is it? We have a gazillion
drivers calling irq_create_mapping() in this function, so I would
say that things are already a mess here.

One alternative is to do what gpio-tegra.c does and call
irq_create_mapping() on every GPIO line that can do IRQ in
probe(). However that is a bit sloppy is it not? Or is this what
we always want drivers to do? This has the side effect of showing
all these IRQs in /proc/interrupts but maybe that is not such
a big deal?

> 2)
>
> (Nit):
>
> The fact that gpio_to_irq() was called doesn't actually guarantee that
> the IRQ will be requested. Admittedly it's silly to call gpio_to_irq()
> if you're not going to request the IRQ, adn this can be considered a
> bug, but it can be done. This might happen (at least temporarily) due to
> deferred probe.

Yeah well you're right it's just supposed to be a translation function.

Part of me want to add an optional irqdomain to struct gpio_chip
and have gpio_to_irq() just call irq_find_mapping() by default
unless the driver specifies its own translation callback, because
I think this is what (modern) drivers should generally do.

What do you think about this refactoring idea?

Yours,
Linus Walleij

Re: [PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[Jean-Christophe PLAGNIOL-VILLARD]

On 13:33 Tue 24 Sep     , Linus Walleij wrote:
> It is currently often possible in many GPIO drivers to request
> a GPIO line to be used as IRQ after calling gpio_to_irq() and,
> as the gpiolib is not aware of this, set the same line to
> output and start driving it, with undesired side effects.
> 
> As it is a bogus usage scenario to request a line flagged as
> output to used as IRQ, we introduce APIs to let gpiolib track
> the use of a line as IRQ, and also set this flag from the
> userspace ABI.
> 
> In this RFC patch we also augment the Nomadik pinctrl driver
> to use this API to give an idea of how it is to be used.
> It is probably not possible to lock line as IRQ in the gpiolib
> .to_irq() callback, as the line may have different use cases
> over time in a system. For a certain system or driver it may
> however be possible to lock the line as IRQ in the .to_irq()
> path. An alternative approach is to let the irq_chip
> .irq_enable() callback do this.
> 
> The API is symmetric so that lines can also be unflagged from
> IRQ use by e.g. .irq_disable(). The debugfs file is altered
> so that we see if a line is reserved for IRQ.
> 
> Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Cc: Enric Balletbo i Serra <eballetbo@gmail.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib.c            | 86 ++++++++++++++++++++++++++++++++++++++-
>  drivers/pinctrl/pinctrl-nomadik.c |  8 ++++
>  include/asm-generic/gpio.h        |  3 ++
>  include/linux/gpio.h              | 11 +++++
>  4 files changed, 106 insertions(+), 2 deletions(-)

looks goods to me and will simplify my work to clean the drop of gpio_to_irq
in the drivers to only use resources

Best Regards,
J.
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 4fc2860..199ba51 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -60,6 +60,7 @@ struct gpio_desc {
>  #define FLAG_ACTIVE_LOW	6	/* sysfs value has active low */
>  #define FLAG_OPEN_DRAIN	7	/* Gpio is open drain type */
>  #define FLAG_OPEN_SOURCE 8	/* Gpio is open source type */
> +#define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
>  
>  #define ID_SHIFT	16	/* add new flags before this one */
>  
> @@ -163,6 +164,17 @@ static struct gpio_desc *gpio_to_desc(unsigned gpio)
>  }
>  
>  /**
> + * Convert an offset on a certain chip to a corresponding descriptor
> + */
> +static struct gpio_desc *gpiochip_offset_to_desc(struct gpio_chip *chip,
> +						 unsigned int offset)
> +{
> +	unsigned int gpio = chip->base + offset;
> +
> +	return gpio_to_desc(gpio);
> +}
> +
> +/**
>   * Convert a GPIO descriptor to the integer namespace.
>   * This should disappear in the future but is needed since we still
>   * use GPIO numbers for error messages and sysfs nodes
> @@ -466,6 +478,13 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
>  	if (ret < 0)
>  		goto free_id;
>  
> +	ret = gpiod_lock_as_irq(desc);
> +	if (ret < 0) {
> +		gpiod_warn(desc,
> +			   "failed to flag the GPIO for IRQ\n");
> +		goto free_id;
> +	}
> +
>  	desc->flags |= gpio_flags;
>  	return 0;
>  
> @@ -1730,6 +1749,14 @@ static int gpiod_direction_output(struct gpio_desc *desc, int value)
>  		return -EINVAL;
>  	}
>  
> +	/* GPIOs used for IRQs shall not be set as output */
> +	if (test_bit(FLAG_USED_AS_IRQ, &desc->flags)) {
> +		gpiod_err(desc,
> +			  "%s: tried to set a GPIO tied to an IRQ as output\n",
> +			  __func__);
> +		return -EIO;
> +	}
> +
>  	/* Open drain pin should not be driven to 1 */
>  	if (value && test_bit(FLAG_OPEN_DRAIN,  &desc->flags))
>  		return gpiod_direction_input(desc);
> @@ -2050,6 +2077,58 @@ int __gpio_to_irq(unsigned gpio)
>  }
>  EXPORT_SYMBOL_GPL(__gpio_to_irq);
>  
> +/**
> + * gpiod_lock_as_irq() - lock a GPIO to be used as IRQ
> + * @gpio: the GPIO line to lock as used for IRQ
> + *
> + * This is used directly by GPIO drivers that want to lock down
> + * a certain GPIO line to be used as IRQs, for example in the
> + * .to_irq() callback of their gpio_chip, or in the .irq_enable()
> + * of its irq_chip implementation if the GPIO is known from that
> + * code.
> + */
> +static int gpiod_lock_as_irq(struct gpio_desc *desc)
> +{
> +	if (!desc)
> +		return -EINVAL;
> +
> +	if (test_bit(FLAG_IS_OUT, &desc->flags)) {
> +		gpiod_err(desc,
> +			  "%s: tried to flag a GPIO set as output for IRQ\n",
> +			  __func__);
> +		return -EIO;
> +	}
> +
> +	set_bit(FLAG_USED_AS_IRQ, &desc->flags);
> +	return 0;
> +}
> +
> +int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +	return gpiod_lock_as_irq(gpiochip_offset_to_desc(chip, offset));
> +}
> +EXPORT_SYMBOL_GPL(gpio_lock_as_irq);
> +
> +/**
> + * gpiod_unlock_as_irq() - unlock a GPIO used as IRQ
> + * @gpio: the GPIO line to unlock from IRQ usage
> + *
> + * This is used directly by GPIO drivers that want to indicate
> + * that a certain GPIO is no longer used exclusively for IRQ.
> + */
> +static void gpiod_unlock_as_irq(struct gpio_desc *desc)
> +{
> +	if (!desc)
> +		return;
> +
> +	clear_bit(FLAG_USED_AS_IRQ, &desc->flags);
> +}
> +
> +void gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +	return gpiod_unlock_as_irq(gpiochip_offset_to_desc(chip, offset));
> +}
> +EXPORT_SYMBOL_GPL(gpio_unlock_as_irq);
>  
>  /* There's no value in making it easy to inline GPIO calls that may sleep.
>   * Common examples include ones connected to I2C or SPI chips.
> @@ -2091,6 +2170,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>  	unsigned		gpio = chip->base;
>  	struct gpio_desc	*gdesc = &chip->desc[0];
>  	int			is_out;
> +	int			is_irq;
>  
>  	for (i = 0; i < chip->ngpio; i++, gpio++, gdesc++) {
>  		if (!test_bit(FLAG_REQUESTED, &gdesc->flags))
> @@ -2098,12 +2178,14 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>  
>  		gpiod_get_direction(gdesc);
>  		is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
> -		seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
> +		is_irq = test_bit(FLAG_USED_AS_IRQ, &gdesc->flags);
> +		seq_printf(s, " gpio-%-3d (%-20.20s) %s %s %s",
>  			gpio, gdesc->label,
>  			is_out ? "out" : "in ",
>  			chip->get
>  				? (chip->get(chip, i) ? "hi" : "lo")
> -				: "?  ");
> +				: "?  ",
> +			is_irq ? "IRQ" : "   ");
>  		seq_printf(s, "\n");
>  	}
>  }
> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
> index d7c3ae3..e2d9e36 100644
> --- a/drivers/pinctrl/pinctrl-nomadik.c
> +++ b/drivers/pinctrl/pinctrl-nomadik.c
> @@ -795,6 +795,14 @@ static int nmk_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
>  	struct nmk_gpio_chip *nmk_chip =
>  		container_of(chip, struct nmk_gpio_chip, chip);
> +	int ret;
> +
> +	ret = gpio_lock_as_irq(chip, offset);
> +	if (ret) {
> +		dev_err(chip->dev, "unable to lock offset %d for IRQ\n",
> +			offset);
> +		return ret;
> +	}
>  
>  	return irq_create_mapping(nmk_chip->domain, offset);
>  }
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index bde6469..b309a5c 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -192,6 +192,9 @@ extern int __gpio_cansleep(unsigned gpio);
>  
>  extern int __gpio_to_irq(unsigned gpio);
>  
> +extern int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset);
> +extern void gpio_unlock_as_irq(struct gpio_chip *chip, unsigned int offset);
> +
>  extern int gpio_request_one(unsigned gpio, unsigned long flags, const char *label);
>  extern int gpio_request_array(const struct gpio *array, size_t num);
>  extern void gpio_free_array(const struct gpio *array, size_t num);
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index 552e3f4..68e5523 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -204,6 +204,17 @@ static inline int gpio_to_irq(unsigned gpio)
>  	return -EINVAL;
>  }
>  
> +static inline int gpio_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +	WARN_ON(1);
> +}
> +
> +static inline void gpio_unlock_as_irq(struct gpio_chip *chip,
> +				      unsigned int offset)
> +{
> +	WARN_ON(1);
> +}
> +
>  static inline int irq_to_gpio(unsigned irq)
>  {
>  	/* irq can never have been returned from gpio_to_irq() */
> -- 
> 1.8.3.1
> 

Re: [PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[Stephen Warren]

On 10/11/2013 02:39 AM, Linus Walleij wrote:
> On Tue, Sep 24, 2013 at 7:51 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 09/24/2013 05:33 AM, Linus Walleij wrote:

>>> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
>>> @@ -795,6 +795,14 @@ static int nmk_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>>>  {
>>>       struct nmk_gpio_chip *nmk_chip =
>>>               container_of(chip, struct nmk_gpio_chip, chip);
>>> +     int ret;
>>> +
>>> +     ret = gpio_lock_as_irq(chip, offset);
>>
>> I don't think that gpio_to_irq() is the correct place to call the new
>> function, for two reasons:
>>
>> 1)
>>
>> Not all paths that use interrupts call gpio_to_irq(). It's perfectly
>> valid for a driver to receive an IRQ number, request it, and be done.
>> The is commmon when a driver only cares about IRQ functionality and not
>> GPIO functionality, and hence did not receive a GPIO and convert it to
>> the IRQ.
>>
>> To solve this, I think irq_chip drivers should call the new gpiolib
>> functions when the IRQ is actually requested or set up.
>>
>> Related, where does gpio_unlock_as_irq() get called in the Nomadik
>> driver? It should happen when free_irq() is called.
> 
> Yeah if we formalize the criterion that interrupts out of any GPIO
> chips should be possible to request without first getting it from the
> <linux/gpio.h> interface, then this holds.
> 
> However that is not the whole story, is it? We have a gazillion
> drivers calling irq_create_mapping() in this function, so I would
> say that things are already a mess here.

I expect things are a mess indeed:-)

I believe that if a driver is only calling irq_create_mapping() inside
gpio_to_irq(), it's a bug. I think things can operate correctly in one
of two cases, at least with DT:

1) irq_create_mapping() is called from both gpio_to_irq() and the
of_xlate callback for IRQs.

(I don't think this method would work in a board-file-based system where
of_xlate isn't called for IRQs...)

or:

2) irq_create_mapping() is called for all IRQs when registering the IRQ
controller/domain.

To me, (2) is much simpler, and avoids the issue (1) probably has with
only supporting direct IRQ usage (without something calling gpio_to_irq()).

> One alternative is to do what gpio-tegra.c does and call
> irq_create_mapping() on every GPIO line that can do IRQ in
> probe(). However that is a bit sloppy is it not? Or is this what
> we always want drivers to do?

I tend to think it's a nice simple approach that should support any
higher-level usage-model (direct IRQ usage, or "mapped" via gpio_to_irq()).

> This has the side effect of showing
> all these IRQs in /proc/interrupts but maybe that is not such
> a big deal?

I think that's actually a benefit; you can see all the possible IRQ
sources in the system, and whether each is handled, or not.

>> 2)
>>
>> (Nit):
>>
>> The fact that gpio_to_irq() was called doesn't actually guarantee that
>> the IRQ will be requested. Admittedly it's silly to call gpio_to_irq()
>> if you're not going to request the IRQ, adn this can be considered a
>> bug, but it can be done. This might happen (at least temporarily) due to
>> deferred probe.
> 
> Yeah well you're right it's just supposed to be a translation function.
> 
> Part of me want to add an optional irqdomain to struct gpio_chip
> and have gpio_to_irq() just call irq_find_mapping() by default
> unless the driver specifies its own translation callback, because
> I think this is what (modern) drivers should generally do.
> 
> What do you think about this refactoring idea?

That sounds reasonable.

Re: [PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[Jean-Christophe PLAGNIOL-VILLARD]

On 13:10 Fri 11 Oct     , Stephen Warren wrote:
> On 10/11/2013 02:39 AM, Linus Walleij wrote:
> > On Tue, Sep 24, 2013 at 7:51 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >> On 09/24/2013 05:33 AM, Linus Walleij wrote:
> 
> >>> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
> >>> @@ -795,6 +795,14 @@ static int nmk_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> >>>  {
> >>>       struct nmk_gpio_chip *nmk_chip =
> >>>               container_of(chip, struct nmk_gpio_chip, chip);
> >>> +     int ret;
> >>> +
> >>> +     ret = gpio_lock_as_irq(chip, offset);
> >>
> >> I don't think that gpio_to_irq() is the correct place to call the new
> >> function, for two reasons:
> >>
> >> 1)
> >>
> >> Not all paths that use interrupts call gpio_to_irq(). It's perfectly
> >> valid for a driver to receive an IRQ number, request it, and be done.
> >> The is commmon when a driver only cares about IRQ functionality and not
> >> GPIO functionality, and hence did not receive a GPIO and convert it to
> >> the IRQ.
> >>
> >> To solve this, I think irq_chip drivers should call the new gpiolib
> >> functions when the IRQ is actually requested or set up.
> >>
> >> Related, where does gpio_unlock_as_irq() get called in the Nomadik
> >> driver? It should happen when free_irq() is called.
> > 
> > Yeah if we formalize the criterion that interrupts out of any GPIO
> > chips should be possible to request without first getting it from the
> > <linux/gpio.h> interface, then this holds.
> > 
> > However that is not the whole story, is it? We have a gazillion
> > drivers calling irq_create_mapping() in this function, so I would
> > say that things are already a mess here.
> 
> I expect things are a mess indeed:-)
> 
> I believe that if a driver is only calling irq_create_mapping() inside
> gpio_to_irq(), it's a bug. I think things can operate correctly in one
> of two cases, at least with DT:
> 
> 1) irq_create_mapping() is called from both gpio_to_irq() and the
> of_xlate callback for IRQs.
> 
> (I don't think this method would work in a board-file-based system where
> of_xlate isn't called for IRQs...)

this is what do the at91 driver today
> 
> or:
> 
> 2) irq_create_mapping() is called for all IRQs when registering the IRQ
> controller/domain.
> 
> To me, (2) is much simpler, and avoids the issue (1) probably has with
> only supporting direct IRQ usage (without something calling gpio_to_irq()).

no as you can not track which gpio is an activer IRQ
as if an gpio is an active IRQ you need to forbiden gpio_directio & co

Best Regards,
J.
> 
> > One alternative is to do what gpio-tegra.c does and call
> > irq_create_mapping() on every GPIO line that can do IRQ in
> > probe(). However that is a bit sloppy is it not? Or is this what
> > we always want drivers to do?
> 
> I tend to think it's a nice simple approach that should support any
> higher-level usage-model (direct IRQ usage, or "mapped" via gpio_to_irq()).
> 
> > This has the side effect of showing
> > all these IRQs in /proc/interrupts but maybe that is not such
> > a big deal?
> 
> I think that's actually a benefit; you can see all the possible IRQ
> sources in the system, and whether each is handled, or not.
> 
> >> 2)
> >>
> >> (Nit):
> >>
> >> The fact that gpio_to_irq() was called doesn't actually guarantee that
> >> the IRQ will be requested. Admittedly it's silly to call gpio_to_irq()
> >> if you're not going to request the IRQ, adn this can be considered a
> >> bug, but it can be done. This might happen (at least temporarily) due to
> >> deferred probe.
> > 
> > Yeah well you're right it's just supposed to be a translation function.
> > 
> > Part of me want to add an optional irqdomain to struct gpio_chip
> > and have gpio_to_irq() just call irq_find_mapping() by default
> > unless the driver specifies its own translation callback, because
> > I think this is what (modern) drivers should generally do.
> > 
> > What do you think about this refactoring idea?
> 
> That sounds reasonable.

Re: [PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[Linus Walleij]

On Fri, Oct 11, 2013 at 9:10 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/11/2013 02:39 AM, Linus Walleij wrote:

>> However that is not the whole story, is it? We have a gazillion
>> drivers calling irq_create_mapping() in this function, so I would
>> say that things are already a mess here.
>
> I expect things are a mess indeed:-)
>
> I believe that if a driver is only calling irq_create_mapping() inside
> gpio_to_irq(), it's a bug. I think things can operate correctly in one
> of two cases, at least with DT:
>
> 1) irq_create_mapping() is called from both gpio_to_irq() and the
> of_xlate callback for IRQs.
>
> (I don't think this method would work in a board-file-based system where
> of_xlate isn't called for IRQs...)
>
> or:
>
> 2) irq_create_mapping() is called for all IRQs when registering the IRQ
> controller/domain.
>
> To me, (2) is much simpler, and avoids the issue (1) probably has with
> only supporting direct IRQ usage (without something calling gpio_to_irq()).

Hm. Jean-Christophe says (1) is the way to go ... This is
arch/arm/mach-at91/gpio.c I guess.

But this seems to call irq_create_mapping() for all IRQs on
all gpiochips in at91_gpio_irq_setup() which is called by the board
init code?

Maybe I'm not following this :-/

I'll take a look over "my" GPIO drivers and see how I can make
those a bit more elegant so as to set a good example.

Yours,
Linus Walleij

Re: [PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:03 Mon 14 Oct     , Linus Walleij wrote:
> On Fri, Oct 11, 2013 at 9:10 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> > On 10/11/2013 02:39 AM, Linus Walleij wrote:
> 
> >> However that is not the whole story, is it? We have a gazillion
> >> drivers calling irq_create_mapping() in this function, so I would
> >> say that things are already a mess here.
> >
> > I expect things are a mess indeed:-)
> >
> > I believe that if a driver is only calling irq_create_mapping() inside
> > gpio_to_irq(), it's a bug. I think things can operate correctly in one
> > of two cases, at least with DT:
> >
> > 1) irq_create_mapping() is called from both gpio_to_irq() and the
> > of_xlate callback for IRQs.
> >
> > (I don't think this method would work in a board-file-based system where
> > of_xlate isn't called for IRQs...)
> >
> > or:
> >
> > 2) irq_create_mapping() is called for all IRQs when registering the IRQ
> > controller/domain.
> >
> > To me, (2) is much simpler, and avoids the issue (1) probably has with
> > only supporting direct IRQ usage (without something calling gpio_to_irq()).
> 
> Hm. Jean-Christophe says (1) is the way to go ... This is
> arch/arm/mach-at91/gpio.c I guess.

no this is in the new pinctrl+ gpio driver
in drivers/pinctrl/pinctrl-at91.c

the arch/arm/mach-at91/gpio.c is the old one for non-dt support that is going
to be drop
> 
> But this seems to call irq_create_mapping() for all IRQs on
> all gpiochips in at91_gpio_irq_setup() which is called by the board
> init code?
> 
> Maybe I'm not following this :-/

it's in at91_gpio_irq_domain_xlate
> 
> I'll take a look over "my" GPIO drivers and see how I can make
> those a bit more elegant so as to set a good example.
> 
> Yours,
> Linus Walleij

Re: [PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[Stephen Warren]

On 10/12/2013 12:06 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 13:10 Fri 11 Oct     , Stephen Warren wrote:
>> On 10/11/2013 02:39 AM, Linus Walleij wrote:
>>> On Tue, Sep 24, 2013 at 7:51 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On 09/24/2013 05:33 AM, Linus Walleij wrote:
>>
>>>>> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
>>>>> @@ -795,6 +795,14 @@ static int nmk_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>>>>>  {
>>>>>       struct nmk_gpio_chip *nmk_chip =
>>>>>               container_of(chip, struct nmk_gpio_chip, chip);
>>>>> +     int ret;
>>>>> +
>>>>> +     ret = gpio_lock_as_irq(chip, offset);
>>>>
>>>> I don't think that gpio_to_irq() is the correct place to call the new
>>>> function, for two reasons:
>>>>
>>>> 1)
>>>>
>>>> Not all paths that use interrupts call gpio_to_irq(). It's perfectly
>>>> valid for a driver to receive an IRQ number, request it, and be done.
>>>> The is commmon when a driver only cares about IRQ functionality and not
>>>> GPIO functionality, and hence did not receive a GPIO and convert it to
>>>> the IRQ.
>>>>
>>>> To solve this, I think irq_chip drivers should call the new gpiolib
>>>> functions when the IRQ is actually requested or set up.
>>>>
>>>> Related, where does gpio_unlock_as_irq() get called in the Nomadik
>>>> driver? It should happen when free_irq() is called.
>>>
>>> Yeah if we formalize the criterion that interrupts out of any GPIO
>>> chips should be possible to request without first getting it from the
>>> <linux/gpio.h> interface, then this holds.
>>>
>>> However that is not the whole story, is it? We have a gazillion
>>> drivers calling irq_create_mapping() in this function, so I would
>>> say that things are already a mess here.
>>
>> I expect things are a mess indeed:-)
>>
>> I believe that if a driver is only calling irq_create_mapping() inside
>> gpio_to_irq(), it's a bug. I think things can operate correctly in one
>> of two cases, at least with DT:
>>
>> 1) irq_create_mapping() is called from both gpio_to_irq() and the
>> of_xlate callback for IRQs.
>>
>> (I don't think this method would work in a board-file-based system where
>> of_xlate isn't called for IRQs...)
> 
> this is what do the at91 driver today
>
>> or:
>>
>> 2) irq_create_mapping() is called for all IRQs when registering the IRQ
>> controller/domain.
>>
>> To me, (2) is much simpler, and avoids the issue (1) probably has with
>> only supporting direct IRQ usage (without something calling gpio_to_irq()).
> 
> no as you can not track which gpio is an activer IRQ
> as if an gpio is an active IRQ you need to forbiden gpio_directio & co

I think you're confusing two issues. The existance of an IRQ mapping is
not related to whether a GPIO is used as an IRQ. In that patch that
Linus sent for the nomadik GPIO driver, whether a GPIO is used as an IRQ
is configured in the irq_chip startup/shutdown functions, and has no
impact on, nor is impacted by, the presence/absence of IRQ mappings.

Re: [PATCH] RFC: gpio: add API to be strict about GPIO IRQ usage

[Jean-Christophe PLAGNIOL-VILLARD]

On 12:00 Mon 14 Oct     , Stephen Warren wrote:
> On 10/12/2013 12:06 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 13:10 Fri 11 Oct     , Stephen Warren wrote:
> >> On 10/11/2013 02:39 AM, Linus Walleij wrote:
> >>> On Tue, Sep 24, 2013 at 7:51 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >>>> On 09/24/2013 05:33 AM, Linus Walleij wrote:
> >>
> >>>>> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
> >>>>> @@ -795,6 +795,14 @@ static int nmk_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> >>>>>  {
> >>>>>       struct nmk_gpio_chip *nmk_chip =
> >>>>>               container_of(chip, struct nmk_gpio_chip, chip);
> >>>>> +     int ret;
> >>>>> +
> >>>>> +     ret = gpio_lock_as_irq(chip, offset);
> >>>>
> >>>> I don't think that gpio_to_irq() is the correct place to call the new
> >>>> function, for two reasons:
> >>>>
> >>>> 1)
> >>>>
> >>>> Not all paths that use interrupts call gpio_to_irq(). It's perfectly
> >>>> valid for a driver to receive an IRQ number, request it, and be done.
> >>>> The is commmon when a driver only cares about IRQ functionality and not
> >>>> GPIO functionality, and hence did not receive a GPIO and convert it to
> >>>> the IRQ.
> >>>>
> >>>> To solve this, I think irq_chip drivers should call the new gpiolib
> >>>> functions when the IRQ is actually requested or set up.
> >>>>
> >>>> Related, where does gpio_unlock_as_irq() get called in the Nomadik
> >>>> driver? It should happen when free_irq() is called.
> >>>
> >>> Yeah if we formalize the criterion that interrupts out of any GPIO
> >>> chips should be possible to request without first getting it from the
> >>> <linux/gpio.h> interface, then this holds.
> >>>
> >>> However that is not the whole story, is it? We have a gazillion
> >>> drivers calling irq_create_mapping() in this function, so I would
> >>> say that things are already a mess here.
> >>
> >> I expect things are a mess indeed:-)
> >>
> >> I believe that if a driver is only calling irq_create_mapping() inside
> >> gpio_to_irq(), it's a bug. I think things can operate correctly in one
> >> of two cases, at least with DT:
> >>
> >> 1) irq_create_mapping() is called from both gpio_to_irq() and the
> >> of_xlate callback for IRQs.
> >>
> >> (I don't think this method would work in a board-file-based system where
> >> of_xlate isn't called for IRQs...)
> > 
> > this is what do the at91 driver today
> >
> >> or:
> >>
> >> 2) irq_create_mapping() is called for all IRQs when registering the IRQ
> >> controller/domain.
> >>
> >> To me, (2) is much simpler, and avoids the issue (1) probably has with
> >> only supporting direct IRQ usage (without something calling gpio_to_irq()).
> > 
> > no as you can not track which gpio is an activer IRQ
> > as if an gpio is an active IRQ you need to forbiden gpio_directio & co
> 
> I think you're confusing two issues. The existance of an IRQ mapping is
> not related to whether a GPIO is used as an IRQ. In that patch that
> Linus sent for the nomadik GPIO driver, whether a GPIO is used as an IRQ
> is configured in the irq_chip startup/shutdown functions, and has no
> impact on, nor is impacted by, the presence/absence of IRQ mappings.

yes it's as for me the existence of an mapping means you want to use it as an
IRQ and need only to append if so.
You does not need to create an IRQ mapping if not.

That's why I said on at91 we configure the GPIO at xlate

Best Regards,
J.