This patch series fixes IRQ support and cleans up OF device-tree support for
the PCA953X gpio driver.
This is version 4 of the patch series, after receiving feedback from
Grant Likely and Thomas Gleixner.
Changes in this version:
- Call irq_alloc_descs unconditionally to allocate IRQ descriptors.
- Instead of adding linux,irq-base to OF bindings, remove all of them, since
they are not needed. IRQ's are allocated dynamically by irq_alloc_descs
(pdata->irq_base may still be specified by platform setup code, to set a
base for searching).
- For CONFIG_OF_GPIO, pdata->irq_base is set to -1 to disable GPIO-interrupt
support, if the "interrupts" property is not specified in the I2C
device-node.
- On suggestion from Grant Likely, changed IRQF_TRIGGER_FALLING to
IRQF_TRIGGER_LOW to enable the driver to share the physical interrupt line,
and better reflect the actual working of the pin (active-low interrupt).
- Split-out the patches a bit more.
David Jander (6):
GPIO: pca953x.c: Fix IRQ support.
GPIO: pca953x.c: Set device platform_data pointer after allocating it
GPIO: pca953x.c: Remove meaningless device-tree bindings
GPIO: pca953x.c: Interrupt pin is active-low
GPIO: pca953x.c: Fix warning of enabled interrupts in handler
GPIO: pca953x.c: Add missing irq_mask_ack handler in struct irq_chip
drivers/gpio/pca953x.c | 79 +++++++++++++----------------------------------
1 files changed, 22 insertions(+), 57 deletions(-)
--
1.7.4.1
It seems that in the normal case, IRQ_NOREQUEST needs to be explicitly
cleared, otherwise claiming the interrupt fails.
In the case of sparse interrupts, the descriptor needs to be allocated
first.
Signed-off-by: David Jander <[email protected]>
---
drivers/gpio/pca953x.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index 0451d7a..b31f881 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -474,12 +474,16 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
* this purpose.
*/
chip->irq_stat &= chip->reg_direction;
- chip->irq_base = pdata->irq_base;
mutex_init(&chip->irq_lock);
+ chip->irq_base = irq_alloc_descs(-1, pdata->irq_base, chip->gpio_chip.ngpio, -1);
+ if (chip->irq_base < 0)
+ goto out_failed;
+
for (lvl = 0; lvl < chip->gpio_chip.ngpio; lvl++) {
int irq = lvl + chip->irq_base;
+ irq_clear_status_flags(irq, IRQ_NOREQUEST);
irq_set_chip_data(irq, chip);
irq_set_chip_and_handler(irq, &pca953x_irq_chip,
handle_simple_irq);
--
1.7.4.1
In the case that we obtain device-tree data to fill in platform_data,
the dev.platform_data pointer needs to be set, since it is used by
functions such as pca953x_irq_setup() later on.
Signed-off-by: David Jander <[email protected]>
---
drivers/gpio/pca953x.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index b31f881..2dff562 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -660,6 +660,7 @@ static int __devinit pca953x_probe(struct i2c_client *client,
* dynamically and must be freed in the driver
*/
chip->dyn_pdata = pdata;
+ client->dev.platform_data = pdata;
}
if (pdata == NULL) {
--
1.7.4.1
The property 'polarity' is handled by the GPIO core, and the 'gpio-base'
should be assigned automatically. It is meaningless in the device-tree,
since GPIO's are identified by the "chip-name"/offset pair.
This way, the whole pca953x_get_alt_pdata() can go away.
We still need to check whether we really want GPIO-interrupt functionality
by simply looking if the I2C node has an interrupts property defined, since
this property is not used for anything else.
Signed-off-by: David Jander <[email protected]>
---
drivers/gpio/pca953x.c | 62 ++++++++---------------------------------------
1 files changed, 11 insertions(+), 51 deletions(-)
diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index 2dff562..ae9fe61 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -21,7 +21,6 @@
#include <linux/slab.h>
#ifdef CONFIG_OF_GPIO
#include <linux/of_platform.h>
-#include <linux/of_gpio.h>
#endif
#define PCA953X_INPUT 0
@@ -539,55 +538,6 @@ static void pca953x_irq_teardown(struct pca953x_chip *chip)
}
#endif
-/*
- * Handlers for alternative sources of platform_data
- */
-#ifdef CONFIG_OF_GPIO
-/*
- * Translate OpenFirmware node properties into platform_data
- */
-static struct pca953x_platform_data *
-pca953x_get_alt_pdata(struct i2c_client *client)
-{
- struct pca953x_platform_data *pdata;
- struct device_node *node;
- const __be32 *val;
- int size;
-
- node = client->dev.of_node;
- if (node == NULL)
- return NULL;
-
- pdata = kzalloc(sizeof(struct pca953x_platform_data), GFP_KERNEL);
- if (pdata == NULL) {
- dev_err(&client->dev, "Unable to allocate platform_data\n");
- return NULL;
- }
-
- pdata->gpio_base = -1;
- val = of_get_property(node, "linux,gpio-base", &size);
- if (val) {
- if (size != sizeof(*val))
- dev_warn(&client->dev, "%s: wrong linux,gpio-base\n",
- node->full_name);
- else
- pdata->gpio_base = be32_to_cpup(val);
- }
-
- val = of_get_property(node, "polarity", NULL);
- if (val)
- pdata->invert = *val;
-
- return pdata;
-}
-#else
-static struct pca953x_platform_data *
-pca953x_get_alt_pdata(struct i2c_client *client)
-{
- return NULL;
-}
-#endif
-
static int __devinit device_pca953x_init(struct pca953x_chip *chip, int invert)
{
int ret;
@@ -654,7 +604,17 @@ static int __devinit pca953x_probe(struct i2c_client *client,
pdata = client->dev.platform_data;
if (pdata == NULL) {
- pdata = pca953x_get_alt_pdata(client);
+ pdata = kzalloc(sizeof(struct pca953x_platform_data), GFP_KERNEL);
+ if (pdata == NULL) {
+ dev_err(&client->dev, "Unable to allocate platform_data\n");
+ goto out_failed;
+ }
+ pdata->gpio_base = -1;
+#ifdef CONFIG_OF_GPIO
+ /* If I2C node has no interrupts property, disable GPIO interrupts */
+ if (of_find_property(client->dev.of_node, "interrupts", NULL) == NULL)
+ pdata->irq_base = -1;
+#endif
/*
* Unlike normal platform_data, this is allocated
* dynamically and must be freed in the driver
--
1.7.4.1
The interrupt pin of the PCA953x is active low, and on the rising edge
no interrupt should be produced.
Signed-off-by: David Jander <[email protected]>
---
drivers/gpio/pca953x.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index ae9fe61..97e0d32 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -496,8 +496,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
ret = request_threaded_irq(client->irq,
NULL,
pca953x_irq_handler,
- IRQF_TRIGGER_RISING |
- IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT,
dev_name(&client->dev), chip);
if (ret) {
dev_err(&client->dev, "failed to request irq %d\n",
--
1.7.4.1
When using nested threaded irqs, use handle_nested_irq(). This function
does not call the chip handler, so no handler is set.
Signed-off-by: David Jander <[email protected]>
---
drivers/gpio/pca953x.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index 97e0d32..dfed81f 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -436,7 +436,7 @@ static irqreturn_t pca953x_irq_handler(int irq, void *devid)
do {
level = __ffs(pending);
- generic_handle_irq(level + chip->irq_base);
+ handle_nested_irq(level + chip->irq_base);
pending &= ~(1 << level);
} while (pending);
@@ -484,8 +484,8 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
irq_clear_status_flags(irq, IRQ_NOREQUEST);
irq_set_chip_data(irq, chip);
- irq_set_chip_and_handler(irq, &pca953x_irq_chip,
- handle_simple_irq);
+ irq_set_chip(irq, &pca953x_irq_chip);
+ irq_set_nested_thread(irq, true);
#ifdef CONFIG_ARM
set_irq_flags(irq, IRQF_VALID);
#else
--
1.7.4.1
Either .irq_mask_ack or .irq_ack need to be filled in, otherwise in
kernel/irq/chip.c: mask_ack_irq() a non-initialized handler is called.
Signed-off-by: David Jander <[email protected]>
---
drivers/gpio/pca953x.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index dfed81f..b476c1c 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -379,6 +379,7 @@ static int pca953x_irq_set_type(struct irq_data *d, unsigned int type)
static struct irq_chip pca953x_irq_chip = {
.name = "pca953x",
.irq_mask = pca953x_irq_mask,
+ .irq_mask_ack = pca953x_irq_mask,
.irq_unmask = pca953x_irq_unmask,
.irq_bus_lock = pca953x_irq_bus_lock,
.irq_bus_sync_unlock = pca953x_irq_bus_sync_unlock,
--
1.7.4.1
On Wed, Jun 08, 2011 at 02:48:34PM +0200, David Jander wrote:
> Either .irq_mask_ack or .irq_ack need to be filled in, otherwise in
> kernel/irq/chip.c: mask_ack_irq() a non-initialized handler is called.
>
> Signed-off-by: David Jander <[email protected]>
Does this matter anymore given that the driver no longer sets a handler?
g.
> ---
> drivers/gpio/pca953x.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
> index dfed81f..b476c1c 100644
> --- a/drivers/gpio/pca953x.c
> +++ b/drivers/gpio/pca953x.c
> @@ -379,6 +379,7 @@ static int pca953x_irq_set_type(struct irq_data *d, unsigned int type)
> static struct irq_chip pca953x_irq_chip = {
> .name = "pca953x",
> .irq_mask = pca953x_irq_mask,
> + .irq_mask_ack = pca953x_irq_mask,
> .irq_unmask = pca953x_irq_unmask,
> .irq_bus_lock = pca953x_irq_bus_lock,
> .irq_bus_sync_unlock = pca953x_irq_bus_sync_unlock,
> --
> 1.7.4.1
>
On Wed, Jun 08, 2011 at 02:48:31PM +0200, David Jander wrote:
> The property 'polarity' is handled by the GPIO core, and the 'gpio-base'
> should be assigned automatically. It is meaningless in the device-tree,
> since GPIO's are identified by the "chip-name"/offset pair.
> This way, the whole pca953x_get_alt_pdata() can go away.
> We still need to check whether we really want GPIO-interrupt functionality
> by simply looking if the I2C node has an interrupts property defined, since
> this property is not used for anything else.
>
> Signed-off-by: David Jander <[email protected]>
> ---
> drivers/gpio/pca953x.c | 62 ++++++++---------------------------------------
> 1 files changed, 11 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
> index 2dff562..ae9fe61 100644
> --- a/drivers/gpio/pca953x.c
> +++ b/drivers/gpio/pca953x.c
> @@ -21,7 +21,6 @@
> #include <linux/slab.h>
> #ifdef CONFIG_OF_GPIO
> #include <linux/of_platform.h>
> -#include <linux/of_gpio.h>
> #endif
>
> #define PCA953X_INPUT 0
> @@ -539,55 +538,6 @@ static void pca953x_irq_teardown(struct pca953x_chip *chip)
> }
> #endif
>
> -/*
> - * Handlers for alternative sources of platform_data
> - */
> -#ifdef CONFIG_OF_GPIO
> -/*
> - * Translate OpenFirmware node properties into platform_data
> - */
> -static struct pca953x_platform_data *
> -pca953x_get_alt_pdata(struct i2c_client *client)
> -{
> - struct pca953x_platform_data *pdata;
> - struct device_node *node;
> - const __be32 *val;
> - int size;
> -
> - node = client->dev.of_node;
> - if (node == NULL)
> - return NULL;
> -
> - pdata = kzalloc(sizeof(struct pca953x_platform_data), GFP_KERNEL);
> - if (pdata == NULL) {
> - dev_err(&client->dev, "Unable to allocate platform_data\n");
> - return NULL;
> - }
> -
> - pdata->gpio_base = -1;
> - val = of_get_property(node, "linux,gpio-base", &size);
> - if (val) {
> - if (size != sizeof(*val))
> - dev_warn(&client->dev, "%s: wrong linux,gpio-base\n",
> - node->full_name);
> - else
> - pdata->gpio_base = be32_to_cpup(val);
> - }
> -
> - val = of_get_property(node, "polarity", NULL);
> - if (val)
> - pdata->invert = *val;
> -
> - return pdata;
> -}
> -#else
> -static struct pca953x_platform_data *
> -pca953x_get_alt_pdata(struct i2c_client *client)
> -{
> - return NULL;
> -}
> -#endif
> -
Ah, hmm. I missed that you were adding documentation for properties
that were already implemented. I try really hard not to break things
that others are already relying on, so I don't think this code can be
removed. However, bonus points if you make it deprecated and spit
out a scary warning when these properties are used.
> static int __devinit device_pca953x_init(struct pca953x_chip *chip, int invert)
> {
> int ret;
> @@ -654,7 +604,17 @@ static int __devinit pca953x_probe(struct i2c_client *client,
>
> pdata = client->dev.platform_data;
> if (pdata == NULL) {
> - pdata = pca953x_get_alt_pdata(client);
> + pdata = kzalloc(sizeof(struct pca953x_platform_data), GFP_KERNEL);
> + if (pdata == NULL) {
> + dev_err(&client->dev, "Unable to allocate platform_data\n");
> + goto out_failed;
> + }
> + pdata->gpio_base = -1;
> +#ifdef CONFIG_OF_GPIO
> + /* If I2C node has no interrupts property, disable GPIO interrupts */
> + if (of_find_property(client->dev.of_node, "interrupts", NULL) == NULL)
> + pdata->irq_base = -1;
> +#endif
Interesting fact: pdata is only used during setup of this driver. All
the goofing around to kzalloc a pdata for the dt use case is really
kind of pointless, and the driver could be simpler if it were removed.
It would be nice if somebody could investigate this.
g.
On Wed, Jun 08, 2011 at 02:48:29PM +0200, David Jander wrote:
> It seems that in the normal case, IRQ_NOREQUEST needs to be explicitly
> cleared, otherwise claiming the interrupt fails.
> In the case of sparse interrupts, the descriptor needs to be allocated
> first.
>
> Signed-off-by: David Jander <[email protected]>
Merged, thanks
g.
On Wed, Jun 08, 2011 at 02:48:30PM +0200, David Jander wrote:
> In the case that we obtain device-tree data to fill in platform_data,
> the dev.platform_data pointer needs to be set, since it is used by
> functions such as pca953x_irq_setup() later on.
>
> Signed-off-by: David Jander <[email protected]>
> ---
> drivers/gpio/pca953x.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
> index b31f881..2dff562 100644
> --- a/drivers/gpio/pca953x.c
> +++ b/drivers/gpio/pca953x.c
> @@ -660,6 +660,7 @@ static int __devinit pca953x_probe(struct i2c_client *client,
> * dynamically and must be freed in the driver
> */
> chip->dyn_pdata = pdata;
> + client->dev.platform_data = pdata;
No, this is illegal. The dev->platform_data pointer must be
considered immutable by the device driver. Otherwise you risk
breaking driver unbind/rebind use case.
Instead, the driver should be fixed so that it doesn't look at the
client->dev.platform_data pointer after .probe() has returned. You
can do this by copying the relevant data into the driver's private
data structure.
g.
On Wed, Jun 08, 2011 at 02:48:32PM +0200, David Jander wrote:
> The interrupt pin of the PCA953x is active low, and on the rising edge
> no interrupt should be produced.
>
> Signed-off-by: David Jander <[email protected]>
Merged, thanks.
g.
On Wed, Jun 08, 2011 at 02:48:33PM +0200, David Jander wrote:
> When using nested threaded irqs, use handle_nested_irq(). This function
> does not call the chip handler, so no handler is set.
>
> Signed-off-by: David Jander <[email protected]>
Merged, thanks.
g.
> ---
> drivers/gpio/pca953x.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
> index 97e0d32..dfed81f 100644
> --- a/drivers/gpio/pca953x.c
> +++ b/drivers/gpio/pca953x.c
> @@ -436,7 +436,7 @@ static irqreturn_t pca953x_irq_handler(int irq, void *devid)
>
> do {
> level = __ffs(pending);
> - generic_handle_irq(level + chip->irq_base);
> + handle_nested_irq(level + chip->irq_base);
>
> pending &= ~(1 << level);
> } while (pending);
> @@ -484,8 +484,8 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>
> irq_clear_status_flags(irq, IRQ_NOREQUEST);
> irq_set_chip_data(irq, chip);
> - irq_set_chip_and_handler(irq, &pca953x_irq_chip,
> - handle_simple_irq);
> + irq_set_chip(irq, &pca953x_irq_chip);
> + irq_set_nested_thread(irq, true);
> #ifdef CONFIG_ARM
> set_irq_flags(irq, IRQF_VALID);
> #else
> --
> 1.7.4.1
>
On Wed, 8 Jun 2011 11:08:57 -0600
Grant Likely <[email protected]> wrote:
> On Wed, Jun 08, 2011 at 02:48:31PM +0200, David Jander wrote:
> > The property 'polarity' is handled by the GPIO core, and the 'gpio-base'
> > should be assigned automatically. It is meaningless in the device-tree,
> > since GPIO's are identified by the "chip-name"/offset pair.
> > This way, the whole pca953x_get_alt_pdata() can go away.
> > We still need to check whether we really want GPIO-interrupt functionality
> > by simply looking if the I2C node has an interrupts property defined, since
> > this property is not used for anything else.
> >
> > Signed-off-by: David Jander <[email protected]>
> > ---
> > drivers/gpio/pca953x.c | 62
> > ++++++++--------------------------------------- 1 files changed, 11
> > insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
> > index 2dff562..ae9fe61 100644
> > --- a/drivers/gpio/pca953x.c
> > +++ b/drivers/gpio/pca953x.c
> > @@ -21,7 +21,6 @@
> > #include <linux/slab.h>
> > #ifdef CONFIG_OF_GPIO
> > #include <linux/of_platform.h>
> > -#include <linux/of_gpio.h>
> > #endif
> >
> > #define PCA953X_INPUT 0
> > @@ -539,55 +538,6 @@ static void pca953x_irq_teardown(struct pca953x_chip
> > *chip) }
> > #endif
> >
> > -/*
> > - * Handlers for alternative sources of platform_data
> > - */
> > -#ifdef CONFIG_OF_GPIO
> > -/*
> > - * Translate OpenFirmware node properties into platform_data
> > - */
> > -static struct pca953x_platform_data *
> > -pca953x_get_alt_pdata(struct i2c_client *client)
> > -{
> > - struct pca953x_platform_data *pdata;
> > - struct device_node *node;
> > - const __be32 *val;
> > - int size;
> > -
> > - node = client->dev.of_node;
> > - if (node == NULL)
> > - return NULL;
> > -
> > - pdata = kzalloc(sizeof(struct pca953x_platform_data), GFP_KERNEL);
> > - if (pdata == NULL) {
> > - dev_err(&client->dev, "Unable to allocate
> > platform_data\n");
> > - return NULL;
> > - }
> > -
> > - pdata->gpio_base = -1;
> > - val = of_get_property(node, "linux,gpio-base", &size);
> > - if (val) {
> > - if (size != sizeof(*val))
> > - dev_warn(&client->dev, "%s: wrong
> > linux,gpio-base\n",
> > - node->full_name);
> > - else
> > - pdata->gpio_base = be32_to_cpup(val);
> > - }
> > -
> > - val = of_get_property(node, "polarity", NULL);
> > - if (val)
> > - pdata->invert = *val;
> > -
> > - return pdata;
> > -}
> > -#else
> > -static struct pca953x_platform_data *
> > -pca953x_get_alt_pdata(struct i2c_client *client)
> > -{
> > - return NULL;
> > -}
> > -#endif
> > -
>
> Ah, hmm. I missed that you were adding documentation for properties
> that were already implemented. I try really hard not to break things
> that others are already relying on, so I don't think this code can be
> removed. However, bonus points if you make it deprecated and spit
> out a scary warning when these properties are used.
Ok :-)
Is something like a WARN() a good thing to do here?
> > static int __devinit device_pca953x_init(struct pca953x_chip *chip, int
> > invert) {
> > int ret;
> > @@ -654,7 +604,17 @@ static int __devinit pca953x_probe(struct i2c_client
> > *client,
> > pdata = client->dev.platform_data;
> > if (pdata == NULL) {
> > - pdata = pca953x_get_alt_pdata(client);
> > + pdata = kzalloc(sizeof(struct pca953x_platform_data),
> > GFP_KERNEL);
> > + if (pdata == NULL) {
> > + dev_err(&client->dev, "Unable to allocate
> > platform_data\n");
> > + goto out_failed;
> > + }
> > + pdata->gpio_base = -1;
> > +#ifdef CONFIG_OF_GPIO
> > + /* If I2C node has no interrupts property, disable GPIO
> > interrupts */
> > + if (of_find_property(client->dev.of_node, "interrupts",
> > NULL) == NULL)
> > + pdata->irq_base = -1;
> > +#endif
>
> Interesting fact: pdata is only used during setup of this driver. All
> the goofing around to kzalloc a pdata for the dt use case is really
> kind of pointless, and the driver could be simpler if it were removed.
> It would be nice if somebody could investigate this.
I agree. Before I fix that, would it have been ok to just look up the
"interrupts" property like this to decide whether to activate GPIO-interrupt
support?
Unfortunately of_platform code leaves the .irq member as 0 if this property is
not specified, and AFAIK, "0" is a valid IRQ number on some platforms, so I
can't check on the value.
Should I post a complete new patch set, or can I just reply on the not yet
accepted parts (2 and 3, while 6 could be dismissed)?
Also, I just realized that I do need to free the alloc'd irq descriptors in
case the driver fails in gpiochip_add(). You already accepted that patch,
though. Should I post a separate patch adding just the missing
irq_free_descs(), or should I modify the patch and repost it?
Best regards,
--
David Jander
Protonic Holland.