[PATCH v2 0/3] gpio / pinctrl: cherryview: Fix missing events from EC

[PATCH v2 0/3] gpio / pinctrl: cherryview: Fix missing events from EC

[Mika Westerberg]

Hi,

Up until now systems having Intel Cherryview/Braswell might lose GPEs
(General Purpose Events) from EC (Embedded Controller) because the pinctrl
driver masks all interrupt sources at probe time. I tried to fix this
already in bcb48cca23ec ("pinctrl: cherryview: Do not mask all interrupts
in probe") but it resulted that the irq core masked all the interrupts
because now we pass handle_bad_irq() as default handler for the irqchip.

After reading again the hardware spec, I think I finally understand the
problem correctly. In summary for southwest and north communities only the
first 8 (or 16) wires can be used to generate interrupts. Rest are reserved
for GPEs.

We fix this by excluding these only GPE capable wires from the IRQ domain
of the gpiochip.

This first follows what LinusW suggested and adds irq_valid_mask for each
gpiochip and then provides a function which allows manipulation from
drivers. Then we switch pinctrl-cherryview to use it.

Previous version of the patches and the discussion around this issue can be
found here:

  https://lkml.org/lkml/2015/5/22/111

Changes from v1 of the series:
  - Only allocate irq_valid_mask when needed
  - Provide gpiochip_irqchip_exclude_irq() helper which allows drivers to
    select which GPIOs to exclude.
  - Use ->nrirqs in chv_gpio_irq_handler()
  - Added patch to convert the driver to use devm_gpiochip_add_data() so
    we can just return if gpiochip_irqchip_exclude_irq() fails (and also
    this simplifies the driver).

Mika Westerberg (3):
  gpiolib: Make it possible to exclude GPIOs from IRQ domain
  pinctrl: cherryview: Convert to use devm_gpiochip_add_data()
  pinctrl: cherryview: Do not add all southwest and north GPIOs to IRQ
    domain

 Documentation/gpio/driver.txt              |  5 +++
 drivers/gpio/gpiolib.c                     | 72 ++++++++++++++++++++++++++++--
 drivers/pinctrl/intel/pinctrl-cherryview.c | 57 +++++++++++++++--------
 include/linux/gpio/driver.h                |  5 +++
 4 files changed, 117 insertions(+), 22 deletions(-)

-- 
2.9.3

[PATCH v2 1/3] gpiolib: Make it possible to exclude GPIOs from IRQ domain

[Mika Westerberg]

When using GPIO irqchip helpers to setup irqchip for a gpiolib based
driver, it is not possible to select which GPIOs to add to the IRQ domain.
Instead it just adds all GPIOs which is not always desired. For example
there might be GPIOs that for some reason cannot generated normal
interrupts at all.

To support this we add a new function gpiochip_irqchip_exclude_irq() that
can be used to exclude selected GPIOs from being added to the IRQ domain of
the gpiochip in question.

Suggested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Thomas, instead of adding flag to struct gpio_chip, I decided to provide an
API function that drivers can call. It allocates irq_valid_mask on the fly.

Let me know if you prefer a flag instead.

 Documentation/gpio/driver.txt |  5 +++
 drivers/gpio/gpiolib.c        | 72 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/gpio/driver.h   |  5 +++
 3 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
index 6cb35a78eff4..d12b2ca6fa8e 100644
--- a/Documentation/gpio/driver.txt
+++ b/Documentation/gpio/driver.txt
@@ -256,6 +256,11 @@ associated irqdomain and resource allocation callbacks, the gpiolib has
 some helpers that can be enabled by selecting the GPIOLIB_IRQCHIP Kconfig
 symbol:
 
+* gpiochip_irqchip_exclude_irq(): excludes given GPIO from IRQ domain
+  created for the gpiochip. This is useful if the GPIO for some reason
+  cannot be used as IRQ at all. Note this must be called before
+  gpiochip_irqchip_add().
+
 * gpiochip_irqchip_add(): adds an irqchip to a gpiochip. It will pass
   the struct gpio_chip* for the chip to all IRQ callbacks, so the callbacks
   need to embed the gpio_chip in its state container and obtain a pointer
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 53ff25ac66d8..31e18dde0ff7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1402,6 +1402,58 @@ static struct gpio_chip *find_chip_by_name(const char *name)
  */
 
 /**
+ * gpiochip_irqchip_exclude_irq() - Exclude a GPIO from IRQ domain of gpiochip
+ * @gpiochip: the gpiochip whose IRQ to exclude
+ * @offset: GPIO number to exclude
+ *
+ * Normally all GPIOs in a gpiochip are added to the IRQ domain created for
+ * that chip. Calling this function allows driver to exclude certain GPIOs
+ * if for instance the GPIO simply cannot generate interrupts.
+ *
+ * This must be called before gpiochip_irqchip_add().
+ *
+ * Return: %0 in case of success, negative errno in case of error.
+ */
+int gpiochip_irqchip_exclude_irq(struct gpio_chip *gpiochip,
+				 unsigned int offset)
+{
+	if (WARN_ON_ONCE(gpiochip->irqdomain))
+		return -EINVAL;
+
+	if (offset >= gpiochip->ngpio)
+		return -EINVAL;
+
+	if (!gpiochip->irq_valid_mask) {
+		unsigned long *mask;
+		int i;
+
+		mask = kcalloc(BITS_TO_LONGS(gpiochip->ngpio), sizeof(long),
+			       GFP_KERNEL);
+		if (!mask)
+			return -ENOMEM;
+
+		/* Assume by default all GPIOs are valid */
+		for (i = 0; i < gpiochip->ngpio; i++)
+			set_bit(i, mask);
+
+		gpiochip->irq_valid_mask = mask;
+	}
+
+	clear_bit(offset, gpiochip->irq_valid_mask);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpiochip_irqchip_exclude_irq);
+
+static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
+				       unsigned int offset)
+{
+	/* No mask means all valid */
+	if (!gpiochip->irq_valid_mask)
+		return true;
+	return test_bit(offset, gpiochip->irq_valid_mask);
+}
+
+/**
  * gpiochip_set_chained_irqchip() - sets a chained irqchip to a gpiochip
  * @gpiochip: the gpiochip to set the irqchip chain to
  * @irqchip: the irqchip to chain to the gpiochip
@@ -1442,9 +1494,12 @@ void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip,
 	}
 
 	/* Set the parent IRQ for all affected IRQs */
-	for (offset = 0; offset < gpiochip->ngpio; offset++)
+	for (offset = 0; offset < gpiochip->ngpio; offset++) {
+		if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
+			continue;
 		irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset),
 			       parent_irq);
+	}
 }
 EXPORT_SYMBOL_GPL(gpiochip_set_chained_irqchip);
 
@@ -1551,9 +1606,12 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
 
 	/* Remove all IRQ mappings and delete the domain */
 	if (gpiochip->irqdomain) {
-		for (offset = 0; offset < gpiochip->ngpio; offset++)
+		for (offset = 0; offset < gpiochip->ngpio; offset++) {
+			if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
+				continue;
 			irq_dispose_mapping(
 				irq_find_mapping(gpiochip->irqdomain, offset));
+		}
 		irq_domain_remove(gpiochip->irqdomain);
 	}
 
@@ -1562,6 +1620,9 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
 		gpiochip->irqchip->irq_release_resources = NULL;
 		gpiochip->irqchip = NULL;
 	}
+
+	kfree(gpiochip->irq_valid_mask);
+	gpiochip->irq_valid_mask = NULL;
 }
 
 /**
@@ -1597,6 +1658,7 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip,
 			  struct lock_class_key *lock_key)
 {
 	struct device_node *of_node;
+	bool irq_base_set = false;
 	unsigned int offset;
 	unsigned irq_base = 0;
 
@@ -1646,13 +1708,17 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip,
 	 * necessary to allocate descriptors for all IRQs.
 	 */
 	for (offset = 0; offset < gpiochip->ngpio; offset++) {
+		if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
+			continue;
 		irq_base = irq_create_mapping(gpiochip->irqdomain, offset);
-		if (offset == 0)
+		if (!irq_base_set) {
 			/*
 			 * Store the base into the gpiochip to be used when
 			 * unmapping the irqs.
 			 */
 			gpiochip->irq_base = irq_base;
+			irq_base_set = true;
+		}
 	}
 
 	acpi_gpiochip_request_interrupts(gpiochip);
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 50882e09289b..c8a4f5511b37 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -112,6 +112,8 @@ enum single_ended_mode {
  *	initialization, provided by GPIO driver
  * @irq_parent: GPIO IRQ chip parent/bank linux irq number,
  *	provided by GPIO driver
+ * @irq_valid_mask: If not %NULL holds bitmask of GPIOs which are valid to
+ *	be included in IRQ domain of the chip
  * @lock_key: per GPIO IRQ chip lockdep class
  *
  * A gpio_chip can help platforms abstract various sources of GPIOs so
@@ -190,6 +192,7 @@ struct gpio_chip {
 	irq_flow_handler_t	irq_handler;
 	unsigned int		irq_default_type;
 	int			irq_parent;
+	unsigned long		*irq_valid_mask;
 	struct lock_class_key	*lock_key;
 #endif
 
@@ -260,6 +263,8 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 
 #ifdef CONFIG_GPIOLIB_IRQCHIP
 
+int gpiochip_irqchip_exclude_irq(struct gpio_chip *gpiochip,
+				 unsigned int offset);
 void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip,
 		struct irq_chip *irqchip,
 		int parent_irq,
-- 
2.9.3

Re: [PATCH v2 1/3] gpiolib: Make it possible to exclude GPIOs from IRQ domain

[Marc Zyngier]

On Fri, 16 Sep 2016 13:52:41 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

Hi Mika,

> When using GPIO irqchip helpers to setup irqchip for a gpiolib based
> driver, it is not possible to select which GPIOs to add to the IRQ domain.
> Instead it just adds all GPIOs which is not always desired. For example
> there might be GPIOs that for some reason cannot generated normal
> interrupts at all.
> 
> To support this we add a new function gpiochip_irqchip_exclude_irq() that
> can be used to exclude selected GPIOs from being added to the IRQ domain of
> the gpiochip in question.
> 
> Suggested-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Thomas, instead of adding flag to struct gpio_chip, I decided to provide an
> API function that drivers can call. It allocates irq_valid_mask on the fly.
> 
> Let me know if you prefer a flag instead.
> 
>  Documentation/gpio/driver.txt |  5 +++
>  drivers/gpio/gpiolib.c        | 72 +++++++++++++++++++++++++++++++++++++++++--
>  include/linux/gpio/driver.h   |  5 +++
>  3 files changed, 79 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
> index 6cb35a78eff4..d12b2ca6fa8e 100644
> --- a/Documentation/gpio/driver.txt
> +++ b/Documentation/gpio/driver.txt
> @@ -256,6 +256,11 @@ associated irqdomain and resource allocation callbacks, the gpiolib has
>  some helpers that can be enabled by selecting the GPIOLIB_IRQCHIP Kconfig
>  symbol:
>  
> +* gpiochip_irqchip_exclude_irq(): excludes given GPIO from IRQ domain
> +  created for the gpiochip. This is useful if the GPIO for some reason
> +  cannot be used as IRQ at all. Note this must be called before
> +  gpiochip_irqchip_add().
> +
>  * gpiochip_irqchip_add(): adds an irqchip to a gpiochip. It will pass
>    the struct gpio_chip* for the chip to all IRQ callbacks, so the callbacks
>    need to embed the gpio_chip in its state container and obtain a pointer
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 53ff25ac66d8..31e18dde0ff7 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1402,6 +1402,58 @@ static struct gpio_chip *find_chip_by_name(const char *name)
>   */
>  
>  /**
> + * gpiochip_irqchip_exclude_irq() - Exclude a GPIO from IRQ domain of gpiochip
> + * @gpiochip: the gpiochip whose IRQ to exclude
> + * @offset: GPIO number to exclude
> + *
> + * Normally all GPIOs in a gpiochip are added to the IRQ domain created for
> + * that chip. Calling this function allows driver to exclude certain GPIOs
> + * if for instance the GPIO simply cannot generate interrupts.
> + *
> + * This must be called before gpiochip_irqchip_add().
> + *
> + * Return: %0 in case of success, negative errno in case of error.
> + */
> +int gpiochip_irqchip_exclude_irq(struct gpio_chip *gpiochip,
> +				 unsigned int offset)
> +{
> +	if (WARN_ON_ONCE(gpiochip->irqdomain))
> +		return -EINVAL;
> +
> +	if (offset >= gpiochip->ngpio)
> +		return -EINVAL;
> +
> +	if (!gpiochip->irq_valid_mask) {

There is a fundamental flaw here, which is the lack of any mutual
exclusion if you get two simultaneous calls to this function (yes, this
is unlikely, which means it will happen much earlier than you
think... ;-).

tglx's solution of adding a flag to your gpiochip gives you that mutual
exclusion (because the irqdomain doesn't exist yet). The remaining
question is whether or not flagging the invalid interrupts after the
irqdomain creation is early enough for you or not. I can't see why not,
but I know nothing about your HW.

> +		unsigned long *mask;
> +		int i;
> +
> +		mask = kcalloc(BITS_TO_LONGS(gpiochip->ngpio), sizeof(long),
> +			       GFP_KERNEL);
> +		if (!mask)
> +			return -ENOMEM;
> +
> +		/* Assume by default all GPIOs are valid */
> +		for (i = 0; i < gpiochip->ngpio; i++)
> +			set_bit(i, mask);
> +
> +		gpiochip->irq_valid_mask = mask;
> +	}
> +
> +	clear_bit(offset, gpiochip->irq_valid_mask);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gpiochip_irqchip_exclude_irq);
> +
> +static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
> +				       unsigned int offset)
> +{
> +	/* No mask means all valid */
> +	if (!gpiochip->irq_valid_mask)

Could deserves a likely() annotation...

> +		return true;
> +	return test_bit(offset, gpiochip->irq_valid_mask);
> +}
> +
> +/**
>   * gpiochip_set_chained_irqchip() - sets a chained irqchip to a gpiochip
>   * @gpiochip: the gpiochip to set the irqchip chain to
>   * @irqchip: the irqchip to chain to the gpiochip
> @@ -1442,9 +1494,12 @@ void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip,
>  	}
>  
>  	/* Set the parent IRQ for all affected IRQs */
> -	for (offset = 0; offset < gpiochip->ngpio; offset++)
> +	for (offset = 0; offset < gpiochip->ngpio; offset++) {
> +		if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
> +			continue;
>  		irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset),
>  			       parent_irq);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_set_chained_irqchip);
>  
> @@ -1551,9 +1606,12 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
>  
>  	/* Remove all IRQ mappings and delete the domain */
>  	if (gpiochip->irqdomain) {
> -		for (offset = 0; offset < gpiochip->ngpio; offset++)
> +		for (offset = 0; offset < gpiochip->ngpio; offset++) {
> +			if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
> +				continue;
>  			irq_dispose_mapping(
>  				irq_find_mapping(gpiochip->irqdomain, offset));
> +		}
>  		irq_domain_remove(gpiochip->irqdomain);
>  	}
>  
> @@ -1562,6 +1620,9 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
>  		gpiochip->irqchip->irq_release_resources = NULL;
>  		gpiochip->irqchip = NULL;
>  	}
> +
> +	kfree(gpiochip->irq_valid_mask);
> +	gpiochip->irq_valid_mask = NULL;
>  }
>  
>  /**
> @@ -1597,6 +1658,7 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip,
>  			  struct lock_class_key *lock_key)
>  {
>  	struct device_node *of_node;
> +	bool irq_base_set = false;
>  	unsigned int offset;
>  	unsigned irq_base = 0;
>  
> @@ -1646,13 +1708,17 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip,
>  	 * necessary to allocate descriptors for all IRQs.
>  	 */
>  	for (offset = 0; offset < gpiochip->ngpio; offset++) {
> +		if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
> +			continue;
>  		irq_base = irq_create_mapping(gpiochip->irqdomain, offset);
> -		if (offset == 0)
> +		if (!irq_base_set) {
>  			/*
>  			 * Store the base into the gpiochip to be used when
>  			 * unmapping the irqs.
>  			 */
>  			gpiochip->irq_base = irq_base;
> +			irq_base_set = true;
> +		}
>  	}
>  
>  	acpi_gpiochip_request_interrupts(gpiochip);
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 50882e09289b..c8a4f5511b37 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -112,6 +112,8 @@ enum single_ended_mode {
>   *	initialization, provided by GPIO driver
>   * @irq_parent: GPIO IRQ chip parent/bank linux irq number,
>   *	provided by GPIO driver
> + * @irq_valid_mask: If not %NULL holds bitmask of GPIOs which are valid to
> + *	be included in IRQ domain of the chip
>   * @lock_key: per GPIO IRQ chip lockdep class
>   *
>   * A gpio_chip can help platforms abstract various sources of GPIOs so
> @@ -190,6 +192,7 @@ struct gpio_chip {
>  	irq_flow_handler_t	irq_handler;
>  	unsigned int		irq_default_type;
>  	int			irq_parent;
> +	unsigned long		*irq_valid_mask;
>  	struct lock_class_key	*lock_key;
>  #endif
>  
> @@ -260,6 +263,8 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
>  
>  #ifdef CONFIG_GPIOLIB_IRQCHIP
>  
> +int gpiochip_irqchip_exclude_irq(struct gpio_chip *gpiochip,
> +				 unsigned int offset);
>  void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip,
>  		struct irq_chip *irqchip,
>  		int parent_irq,


Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v2 1/3] gpiolib: Make it possible to exclude GPIOs from IRQ domain

[Mika Westerberg]

On Sat, Sep 17, 2016 at 02:13:18PM +0100, Marc Zyngier wrote:
> On Fri, 16 Sep 2016 13:52:41 +0300
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> Hi Mika,
> 
> > When using GPIO irqchip helpers to setup irqchip for a gpiolib based
> > driver, it is not possible to select which GPIOs to add to the IRQ domain.
> > Instead it just adds all GPIOs which is not always desired. For example
> > there might be GPIOs that for some reason cannot generated normal
> > interrupts at all.
> > 
> > To support this we add a new function gpiochip_irqchip_exclude_irq() that
> > can be used to exclude selected GPIOs from being added to the IRQ domain of
> > the gpiochip in question.
> > 
> > Suggested-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > Thomas, instead of adding flag to struct gpio_chip, I decided to provide an
> > API function that drivers can call. It allocates irq_valid_mask on the fly.
> > 
> > Let me know if you prefer a flag instead.
> > 
> >  Documentation/gpio/driver.txt |  5 +++
> >  drivers/gpio/gpiolib.c        | 72 +++++++++++++++++++++++++++++++++++++++++--
> >  include/linux/gpio/driver.h   |  5 +++
> >  3 files changed, 79 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
> > index 6cb35a78eff4..d12b2ca6fa8e 100644
> > --- a/Documentation/gpio/driver.txt
> > +++ b/Documentation/gpio/driver.txt
> > @@ -256,6 +256,11 @@ associated irqdomain and resource allocation callbacks, the gpiolib has
> >  some helpers that can be enabled by selecting the GPIOLIB_IRQCHIP Kconfig
> >  symbol:
> >  
> > +* gpiochip_irqchip_exclude_irq(): excludes given GPIO from IRQ domain
> > +  created for the gpiochip. This is useful if the GPIO for some reason
> > +  cannot be used as IRQ at all. Note this must be called before
> > +  gpiochip_irqchip_add().
> > +
> >  * gpiochip_irqchip_add(): adds an irqchip to a gpiochip. It will pass
> >    the struct gpio_chip* for the chip to all IRQ callbacks, so the callbacks
> >    need to embed the gpio_chip in its state container and obtain a pointer
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 53ff25ac66d8..31e18dde0ff7 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1402,6 +1402,58 @@ static struct gpio_chip *find_chip_by_name(const char *name)
> >   */
> >  
> >  /**
> > + * gpiochip_irqchip_exclude_irq() - Exclude a GPIO from IRQ domain of gpiochip
> > + * @gpiochip: the gpiochip whose IRQ to exclude
> > + * @offset: GPIO number to exclude
> > + *
> > + * Normally all GPIOs in a gpiochip are added to the IRQ domain created for
> > + * that chip. Calling this function allows driver to exclude certain GPIOs
> > + * if for instance the GPIO simply cannot generate interrupts.
> > + *
> > + * This must be called before gpiochip_irqchip_add().
> > + *
> > + * Return: %0 in case of success, negative errno in case of error.
> > + */
> > +int gpiochip_irqchip_exclude_irq(struct gpio_chip *gpiochip,
> > +				 unsigned int offset)
> > +{
> > +	if (WARN_ON_ONCE(gpiochip->irqdomain))
> > +		return -EINVAL;
> > +
> > +	if (offset >= gpiochip->ngpio)
> > +		return -EINVAL;
> > +
> > +	if (!gpiochip->irq_valid_mask) {
> 
> There is a fundamental flaw here, which is the lack of any mutual
> exclusion if you get two simultaneous calls to this function (yes, this
> is unlikely, which means it will happen much earlier than you
> think... ;-).

I thought about this but came to conclusion that it is not possible to
get two simultaneus calls from the same gpio_chip. Or at least I cannot
figure out how that can happen (perhaps I'm missing something). However,
to be on the safe side I think it is better to use a flag instead here
:)

> tglx's solution of adding a flag to your gpiochip gives you that mutual
> exclusion (because the irqdomain doesn't exist yet). The remaining
> question is whether or not flagging the invalid interrupts after the
> irqdomain creation is early enough for you or not. I can't see why not,
> but I know nothing about your HW.

Since gpiochip_irqchip_add() creates both the irqdomain and mapping for
each IRQ, we either need to exclude the invalid IRQs before that
function is called or dispose the mapping in
gpiochip_irqchip_exclude_irq() which can become complex.

How about following (along the lines what Thomas suggested)?

	1. Add flag 'irq_need_valid_mask' to struct gpio_chip
	2. Make gpiochip_add_data() allocate the mask if the above flag is set
	3. Let drivers manipulate that flag directly using set/clear_bit().

> > +		unsigned long *mask;
> > +		int i;
> > +
> > +		mask = kcalloc(BITS_TO_LONGS(gpiochip->ngpio), sizeof(long),
> > +			       GFP_KERNEL);
> > +		if (!mask)
> > +			return -ENOMEM;
> > +
> > +		/* Assume by default all GPIOs are valid */
> > +		for (i = 0; i < gpiochip->ngpio; i++)
> > +			set_bit(i, mask);
> > +
> > +		gpiochip->irq_valid_mask = mask;
> > +	}
> > +
> > +	clear_bit(offset, gpiochip->irq_valid_mask);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(gpiochip_irqchip_exclude_irq);
> > +
> > +static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
> > +				       unsigned int offset)
> > +{
> > +	/* No mask means all valid */
> > +	if (!gpiochip->irq_valid_mask)
> 
> Could deserves a likely() annotation...

OK, I'll add it.

> > +		return true;
> > +	return test_bit(offset, gpiochip->irq_valid_mask);
> > +}
> > +
> > +/**
> >   * gpiochip_set_chained_irqchip() - sets a chained irqchip to a gpiochip
> >   * @gpiochip: the gpiochip to set the irqchip chain to
> >   * @irqchip: the irqchip to chain to the gpiochip
> > @@ -1442,9 +1494,12 @@ void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip,
> >  	}
> >  
> >  	/* Set the parent IRQ for all affected IRQs */
> > -	for (offset = 0; offset < gpiochip->ngpio; offset++)
> > +	for (offset = 0; offset < gpiochip->ngpio; offset++) {
> > +		if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
> > +			continue;
> >  		irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset),
> >  			       parent_irq);
> > +	}
> >  }
> >  EXPORT_SYMBOL_GPL(gpiochip_set_chained_irqchip);
> >  
> > @@ -1551,9 +1606,12 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
> >  
> >  	/* Remove all IRQ mappings and delete the domain */
> >  	if (gpiochip->irqdomain) {
> > -		for (offset = 0; offset < gpiochip->ngpio; offset++)
> > +		for (offset = 0; offset < gpiochip->ngpio; offset++) {
> > +			if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
> > +				continue;
> >  			irq_dispose_mapping(
> >  				irq_find_mapping(gpiochip->irqdomain, offset));
> > +		}
> >  		irq_domain_remove(gpiochip->irqdomain);
> >  	}
> >  
> > @@ -1562,6 +1620,9 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
> >  		gpiochip->irqchip->irq_release_resources = NULL;
> >  		gpiochip->irqchip = NULL;
> >  	}
> > +
> > +	kfree(gpiochip->irq_valid_mask);
> > +	gpiochip->irq_valid_mask = NULL;
> >  }
> >  
> >  /**
> > @@ -1597,6 +1658,7 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip,
> >  			  struct lock_class_key *lock_key)
> >  {
> >  	struct device_node *of_node;
> > +	bool irq_base_set = false;
> >  	unsigned int offset;
> >  	unsigned irq_base = 0;
> >  
> > @@ -1646,13 +1708,17 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip,
> >  	 * necessary to allocate descriptors for all IRQs.
> >  	 */
> >  	for (offset = 0; offset < gpiochip->ngpio; offset++) {
> > +		if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
> > +			continue;
> >  		irq_base = irq_create_mapping(gpiochip->irqdomain, offset);
> > -		if (offset == 0)
> > +		if (!irq_base_set) {
> >  			/*
> >  			 * Store the base into the gpiochip to be used when
> >  			 * unmapping the irqs.
> >  			 */
> >  			gpiochip->irq_base = irq_base;
> > +			irq_base_set = true;
> > +		}
> >  	}
> >  
> >  	acpi_gpiochip_request_interrupts(gpiochip);
> > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> > index 50882e09289b..c8a4f5511b37 100644
> > --- a/include/linux/gpio/driver.h
> > +++ b/include/linux/gpio/driver.h
> > @@ -112,6 +112,8 @@ enum single_ended_mode {
> >   *	initialization, provided by GPIO driver
> >   * @irq_parent: GPIO IRQ chip parent/bank linux irq number,
> >   *	provided by GPIO driver
> > + * @irq_valid_mask: If not %NULL holds bitmask of GPIOs which are valid to
> > + *	be included in IRQ domain of the chip
> >   * @lock_key: per GPIO IRQ chip lockdep class
> >   *
> >   * A gpio_chip can help platforms abstract various sources of GPIOs so
> > @@ -190,6 +192,7 @@ struct gpio_chip {
> >  	irq_flow_handler_t	irq_handler;
> >  	unsigned int		irq_default_type;
> >  	int			irq_parent;
> > +	unsigned long		*irq_valid_mask;
> >  	struct lock_class_key	*lock_key;
> >  #endif
> >  
> > @@ -260,6 +263,8 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
> >  
> >  #ifdef CONFIG_GPIOLIB_IRQCHIP
> >  
> > +int gpiochip_irqchip_exclude_irq(struct gpio_chip *gpiochip,
> > +				 unsigned int offset);
> >  void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip,
> >  		struct irq_chip *irqchip,
> >  		int parent_irq,
> 
> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny.

[PATCH v2 2/3] pinctrl: cherryview: Convert to use devm_gpiochip_add_data()

[Mika Westerberg]

This simplifies the error handling and allows us to drop the whole
chv_pinctrl_remove() function.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 0fe8fad25e4d..61d87a54a305 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1521,7 +1521,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 	chip->parent = pctrl->dev;
 	chip->base = -1;
 
-	ret = gpiochip_add_data(chip, pctrl);
+	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed to register gpiochip\n");
 		return ret;
@@ -1533,7 +1533,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 					     range->base, range->npins);
 		if (ret) {
 			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
-			goto fail;
+			return ret;
 		}
 
 		offset += range->npins;
@@ -1546,17 +1546,12 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 				   handle_bad_irq, IRQ_TYPE_NONE);
 	if (ret) {
 		dev_err(pctrl->dev, "failed to add IRQ chip\n");
-		goto fail;
+		return ret;
 	}
 
 	gpiochip_set_chained_irqchip(chip, &chv_gpio_irqchip, irq,
 				     chv_gpio_irq_handler);
 	return 0;
-
-fail:
-	gpiochip_remove(chip);
-
-	return ret;
 }
 
 static int chv_pinctrl_probe(struct platform_device *pdev)
@@ -1624,15 +1619,6 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int chv_pinctrl_remove(struct platform_device *pdev)
-{
-	struct chv_pinctrl *pctrl = platform_get_drvdata(pdev);
-
-	gpiochip_remove(&pctrl->chip);
-
-	return 0;
-}
-
 #ifdef CONFIG_PM_SLEEP
 static int chv_pinctrl_suspend(struct device *dev)
 {
@@ -1729,7 +1715,6 @@ MODULE_DEVICE_TABLE(acpi, chv_pinctrl_acpi_match);
 
 static struct platform_driver chv_pinctrl_driver = {
 	.probe = chv_pinctrl_probe,
-	.remove = chv_pinctrl_remove,
 	.driver = {
 		.name = "cherryview-pinctrl",
 		.pm = &chv_pinctrl_pm_ops,
-- 
2.9.3

[PATCH v2 3/3] pinctrl: cherryview: Do not add all southwest and north GPIOs to IRQ domain

[Mika Westerberg]

It turns out that for north and southwest communities, they can only
generate GPIO interrupts for lower 8 interrupts (IntSel value). The upper
part (8-15) can only generate GPEs (General Purpose Events).

Now the reason why EC events such as pressing hotkeys does not work if we
mask all the interrupts is that in order to generate either interrupts or
GPEs the INTMASK register must have that particular interrupt unmasked. In
case of GPEs the CPU does not trigger normal interrupt (and thus the GPIO
driver does not see it) but instead it causes SCI (System Control
Interrupt) to be triggered with the GPE in question set.

To make this all work as expected we only add those GPIOs to the IRQ domain
that can actually generate interrupts (IntSel value 0-7) and skip others.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 36 +++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 61d87a54a305..6fab68c0c6fa 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -134,6 +134,7 @@ struct chv_gpio_pinrange {
  * @gpio_ranges: An array of GPIO ranges in this community
  * @ngpio_ranges: Number of GPIO ranges
  * @ngpios: Total number of GPIOs in this community
+ * @nirqs: Total number of IRQs this community can generate
  */
 struct chv_community {
 	const char *uid;
@@ -146,6 +147,7 @@ struct chv_community {
 	const struct chv_gpio_pinrange *gpio_ranges;
 	size_t ngpio_ranges;
 	size_t ngpios;
+	size_t nirqs;
 };
 
 struct chv_pin_context {
@@ -396,6 +398,12 @@ static const struct chv_community southwest_community = {
 	.gpio_ranges = southwest_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(southwest_gpio_ranges),
 	.ngpios = ARRAY_SIZE(southwest_pins),
+	/*
+	 * Southwest community can benerate GPIO interrupts only for the
+	 * first 8 interrupts. The upper half (8-15) can only be used to
+	 * trigger GPEs.
+	 */
+	.nirqs = 8,
 };
 
 static const struct pinctrl_pin_desc north_pins[] = {
@@ -479,6 +487,12 @@ static const struct chv_community north_community = {
 	.gpio_ranges = north_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(north_gpio_ranges),
 	.ngpios = ARRAY_SIZE(north_pins),
+	/*
+	 * North community can benerate GPIO interrupts only for the first
+	 * 8 interrupts. The upper half (8-15) can only be used to trigger
+	 * GPEs.
+	 */
+	.nirqs = 8,
 };
 
 static const struct pinctrl_pin_desc east_pins[] = {
@@ -521,6 +535,7 @@ static const struct chv_community east_community = {
 	.gpio_ranges = east_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(east_gpio_ranges),
 	.ngpios = ARRAY_SIZE(east_pins),
+	.nirqs = 16,
 };
 
 static const struct pinctrl_pin_desc southeast_pins[] = {
@@ -646,6 +661,7 @@ static const struct chv_community southeast_community = {
 	.gpio_ranges = southeast_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(southeast_gpio_ranges),
 	.ngpios = ARRAY_SIZE(southeast_pins),
+	.nirqs = 16,
 };
 
 static const struct chv_community *chv_communities[] = {
@@ -1497,7 +1513,7 @@ static void chv_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_enter(chip, desc);
 
 	pending = readl(pctrl->regs + CHV_INTSTAT);
-	for_each_set_bit(intr_line, &pending, 16) {
+	for_each_set_bit(intr_line, &pending, pctrl->community->nirqs) {
 		unsigned irq, offset;
 
 		offset = pctrl->intr_lines[intr_line];
@@ -1539,6 +1555,24 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		offset += range->npins;
 	}
 
+	/* Do not add GPIOs that can only generate GPEs to the IRQ domain */
+	for (i = 0; i < pctrl->community->npins; i++) {
+		const struct pinctrl_pin_desc *desc;
+		u32 intsel;
+
+		desc = &pctrl->community->pins[i];
+
+		intsel = readl(chv_padreg(pctrl, desc->number, CHV_PADCTRL0));
+		intsel &= CHV_PADCTRL0_INTSEL_MASK;
+		intsel >>= CHV_PADCTRL0_INTSEL_SHIFT;
+
+		if (intsel >= pctrl->community->nirqs) {
+			ret = gpiochip_irqchip_exclude_irq(chip, i);
+			if (ret)
+				return ret;
+		}
+	}
+
 	/* Clear all interrupts */
 	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
 
-- 
2.9.3