[PATCH] gpio: pcf857x: cleanup irq_demux_work and use threaded irq

[PATCH] gpio: pcf857x: cleanup irq_demux_work and use threaded irq

[George Cherian]

This patch
	- removes the irq_demux_work
	- Uses devm_request_threaded_irq
	- Call the user handler iff gpio_to_irq is done.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/gpio/gpio-pcf857x.c | 52 +++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index 947cff4..aebbba6 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -30,7 +30,7 @@
 #include <linux/of_device.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
-#include <linux/workqueue.h>
+#include <linux/of_irq.h>
 
 
 static const struct i2c_device_id pcf857x_id[] = {
@@ -89,12 +89,12 @@ struct pcf857x {
 	struct gpio_chip	chip;
 	struct i2c_client	*client;
 	struct mutex		lock;		/* protect 'out' */
-	struct work_struct	work;		/* irq demux work */
 	struct irq_domain	*irq_domain;	/* for irq demux  */
 	spinlock_t		slock;		/* protect irq demux */
 	unsigned		out;		/* software latch */
 	unsigned		status;		/* current status */
 	int			irq;		/* real irq number */
+	int			irq_mapped;	/* mapped gpio irqs  */
 
 	int (*write)(struct i2c_client *client, unsigned data);
 	int (*read)(struct i2c_client *client);
@@ -187,38 +187,35 @@ static void pcf857x_set(struct gpio_chip *chip, unsigned offset, int value)
 static int pcf857x_to_irq(struct gpio_chip *chip, unsigned offset)
 {
 	struct pcf857x *gpio = container_of(chip, struct pcf857x, chip);
+	int ret;
 
-	return irq_create_mapping(gpio->irq_domain, offset);
+	ret = irq_create_mapping(gpio->irq_domain, offset);
+	if (ret > 0)
+		gpio->irq_mapped |= (1 << offset);
+
+	return ret;
 }
 
-static void pcf857x_irq_demux_work(struct work_struct *work)
+static irqreturn_t pcf857x_irq(int irq, void *data)
 {
-	struct pcf857x *gpio = container_of(work,
-					       struct pcf857x,
-					       work);
+	struct pcf857x	*gpio = data;
 	unsigned long change, i, status, flags;
 
 	status = gpio->read(gpio->client);
 
 	spin_lock_irqsave(&gpio->slock, flags);
+
+	/*
+	 * call the interrupt handler iff gpio is used as
+	 * interrupt source, just to avoid bad irqs
+	 */
 
-	change = gpio->status ^ status;
+	change = ((gpio->status ^ status) & gpio->irq_mapped);
 	for_each_set_bit(i, &change, gpio->chip.ngpio)
 		generic_handle_irq(irq_find_mapping(gpio->irq_domain, i));
 	gpio->status = status;
 
 	spin_unlock_irqrestore(&gpio->slock, flags);
-}
-
-static irqreturn_t pcf857x_irq_demux(int irq, void *data)
-{
-	struct pcf857x	*gpio = data;
-
-	/*
-	 * pcf857x can't read/write data here,
-	 * since i2c data access might go to sleep.
-	 */
-	schedule_work(&gpio->work);
 
 	return IRQ_HANDLED;
 }
@@ -226,9 +223,13 @@ static irqreturn_t pcf857x_irq_demux(int irq, void *data)
 static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned int virq,
 				 irq_hw_number_t hw)
 {
+	struct pcf857x  *gpio = domain->host_data;
 	irq_set_chip_and_handler(virq,
 				 &dummy_irq_chip,
 				 handle_level_irq);
+	set_irq_flags(virq, IRQF_VALID);
+	gpio->irq_mapped |= (1 << hw);
+
 	return 0;
 }
 
@@ -241,30 +242,31 @@ static void pcf857x_irq_domain_cleanup(struct pcf857x *gpio)
 	if (gpio->irq_domain)
 		irq_domain_remove(gpio->irq_domain);
 
-	if (gpio->irq)
-		free_irq(gpio->irq, gpio);
 }
 
 static int pcf857x_irq_domain_init(struct pcf857x *gpio,
 				   struct i2c_client *client)
 {
 	int status;
+	struct device_node *node;
 
+	node = client->dev.of_node;
 	gpio->irq_domain = irq_domain_add_linear(client->dev.of_node,
 						 gpio->chip.ngpio,
 						 &pcf857x_irq_domain_ops,
-						 NULL);
+						 gpio);
 	if (!gpio->irq_domain)
 		goto fail;
 
 	/* enable real irq */
-	status = request_irq(client->irq, pcf857x_irq_demux, 0,
-			     dev_name(&client->dev), gpio);
+	status = devm_request_threaded_irq(&client->dev, client->irq,
+				NULL, pcf857x_irq, IRQF_ONESHOT |
+				IRQF_TRIGGER_FALLING,
+				dev_name(&client->dev), gpio);
 	if (status)
 		goto fail;
 
 	/* enable gpio_to_irq() */
-	INIT_WORK(&gpio->work, pcf857x_irq_demux_work);
 	gpio->chip.to_irq	= pcf857x_to_irq;
 	gpio->irq		= client->irq;
 
-- 
1.8.1.4

Re: [PATCH] gpio: pcf857x: cleanup irq_demux_work and use threaded irq

[Linus Walleij]

On Tue, Aug 27, 2013 at 12:30 PM, George Cherian <george.cherian@ti.com> wrote:

> This patch
>         - removes the irq_demux_work
>         - Uses devm_request_threaded_irq
>         - Call the user handler iff gpio_to_irq is done.
>
> Signed-off-by: George Cherian <george.cherian@ti.com>

Can you please split this up? It seems like three different patches,
and now they block each other. The threading patch is fine and
I could apply it unless this was mixed up with other stuff.

I'd like Kuninoro and/or Nikolay to have a look at this, so please
CC them on subsequent iterations.

NB: I really like that you move the irq handling to a thread, good
job.

>  static const struct i2c_device_id pcf857x_id[] = {
> @@ -89,12 +89,12 @@ struct pcf857x {
>         struct gpio_chip        chip;
>         struct i2c_client       *client;
>         struct mutex            lock;           /* protect 'out' */
> -       struct work_struct      work;           /* irq demux work */
>         struct irq_domain       *irq_domain;    /* for irq demux  */
>         spinlock_t              slock;          /* protect irq demux */
>         unsigned                out;            /* software latch */
>         unsigned                status;         /* current status */
>         int                     irq;            /* real irq number */
> +       int                     irq_mapped;     /* mapped gpio irqs  */

This seems like an u32 or atleast unsigned, and state that its one
bit flag per IRQ. How many GPIO lines are there?

> -static void pcf857x_irq_demux_work(struct work_struct *work)
> +static irqreturn_t pcf857x_irq(int irq, void *data)
>  {
> -       struct pcf857x *gpio = container_of(work,
> -                                              struct pcf857x,
> -                                              work);
> +       struct pcf857x  *gpio = data;
>         unsigned long change, i, status, flags;
>
>         status = gpio->read(gpio->client);
>
>         spin_lock_irqsave(&gpio->slock, flags);
> +
> +       /*
> +        * call the interrupt handler iff gpio is used as
> +        * interrupt source, just to avoid bad irqs
> +        */
>
> -       change = gpio->status ^ status;
> +       change = ((gpio->status ^ status) & gpio->irq_mapped);

I don't know if that is right.

If this happens you are getting what we call a "spurious IRQ"
and this shall be reported to the IRQ core by returning
IRQ_NONE and handled from there.

> @@ -226,9 +223,13 @@ static irqreturn_t pcf857x_irq_demux(int irq, void *data)
>  static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned int virq,
>                                  irq_hw_number_t hw)
>  {
> +       struct pcf857x  *gpio = domain->host_data;
>         irq_set_chip_and_handler(virq,
>                                  &dummy_irq_chip,
>                                  handle_level_irq);
> +       set_irq_flags(virq, IRQF_VALID);
> +       gpio->irq_mapped |= (1 << hw);

I'm a bit uneasy about this. It feels like its the irqdomain's
responsibility to keep track of whether an IRQ is mapped
or not.

Maybe Grant should have a look at this.

Yours,
Linus Walleij

Re: [PATCH] gpio: pcf857x: cleanup irq_demux_work and use threaded irq

[George Cherian]

Hi Linus,

Thanks for the review. I will split it and send v2.

On 8/29/2013 6:27 PM, Linus Walleij wrote:
> On Tue, Aug 27, 2013 at 12:30 PM, George Cherian <george.cherian@ti.com> wrote:
>
>> This patch
>>          - removes the irq_demux_work
>>          - Uses devm_request_threaded_irq
>>          - Call the user handler iff gpio_to_irq is done.
>>
>> Signed-off-by: George Cherian <george.cherian@ti.com>
> Can you please split this up? It seems like three different patches,
> and now they block each other. The threading patch is fine and
> I could apply it unless this was mixed up with other stuff.
>
> I'd like Kuninoro and/or Nikolay to have a look at this, so please
> CC them on subsequent iterations.
okay
> NB: I really like that you move the irq handling to a thread, good
> job.
>
>>   static const struct i2c_device_id pcf857x_id[] = {
>> @@ -89,12 +89,12 @@ struct pcf857x {
>>          struct gpio_chip        chip;
>>          struct i2c_client       *client;
>>          struct mutex            lock;           /* protect 'out' */
>> -       struct work_struct      work;           /* irq demux work */
>>          struct irq_domain       *irq_domain;    /* for irq demux  */
>>          spinlock_t              slock;          /* protect irq demux */
>>          unsigned                out;            /* software latch */
>>          unsigned                status;         /* current status */
>>          int                     irq;            /* real irq number */
>> +       int                     irq_mapped;     /* mapped gpio irqs  */
> This seems like an u32 or atleast unsigned, and state that its one
> bit flag per IRQ. How many GPIO lines are there?
pcf857x driver supports expanders with 8 and 16 gpio lines.
>> -static void pcf857x_irq_demux_work(struct work_struct *work)
>> +static irqreturn_t pcf857x_irq(int irq, void *data)
>>   {
>> -       struct pcf857x *gpio = container_of(work,
>> -                                              struct pcf857x,
>> -                                              work);
>> +       struct pcf857x  *gpio = data;
>>          unsigned long change, i, status, flags;
>>
>>          status = gpio->read(gpio->client);
>>
>>          spin_lock_irqsave(&gpio->slock, flags);
>> +
>> +       /*
>> +        * call the interrupt handler iff gpio is used as
>> +        * interrupt source, just to avoid bad irqs
>> +        */
>>
>> -       change = gpio->status ^ status;
>> +       change = ((gpio->status ^ status) & gpio->irq_mapped);
> I don't know if that is right.
>
> If this happens you are getting what we call a "spurious IRQ"
> and this shall be reported to the IRQ core by returning
> IRQ_NONE and handled from there.
While testing I got prints like bad irq and no handler installed.
Now I notice its mostly when the n_latch is not passed properly.
>
>> @@ -226,9 +223,13 @@ static irqreturn_t pcf857x_irq_demux(int irq, void *data)
>>   static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned int virq,
>>                                   irq_hw_number_t hw)
>>   {
>> +       struct pcf857x  *gpio = domain->host_data;
>>          irq_set_chip_and_handler(virq,
>>                                   &dummy_irq_chip,
>>                                   handle_level_irq);
>> +       set_irq_flags(virq, IRQF_VALID);
>> +       gpio->irq_mapped |= (1 << hw);
> I'm a bit uneasy about this. It feels like its the irqdomain's
> responsibility to keep track of whether an IRQ is mapped
> or not.
Mainly these expanders dont have an ier sort of registers and if at all 
the initial value is not set proper
then it gives bad irq prints only once per changed bit for which there 
is no handler.
Should I drop this in v2?

>
> Maybe Grant should have a look at this.
>
> Yours,
> Linus Walleij


-- 
-George

Re: [PATCH] gpio: pcf857x: cleanup irq_demux_work and use threaded irq

[Linus Walleij]

On Thu, Aug 29, 2013 at 4:11 PM, George Cherian <george.cherian@ti.com> wrote:
> On 8/29/2013 6:27 PM, Linus Walleij wrote:

>>>          int                     irq;            /* real irq number */
>>> +       int                     irq_mapped;     /* mapped gpio irqs  */
>>
>> This seems like an u32 or atleast unsigned, and state that its one
>> bit flag per IRQ. How many GPIO lines are there?
>
> pcf857x driver supports expanders with 8 and 16 gpio lines.

OK should be an u16 then?

>>>   static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned
>>> int virq,
>>>                                   irq_hw_number_t hw)
>>>   {
>>> +       struct pcf857x  *gpio = domain->host_data;
>>>          irq_set_chip_and_handler(virq,
>>>                                   &dummy_irq_chip,
>>>                                   handle_level_irq);
>>> +       set_irq_flags(virq, IRQF_VALID);
>>> +       gpio->irq_mapped |= (1 << hw);
>>
>> I'm a bit uneasy about this. It feels like its the irqdomain's
>> responsibility to keep track of whether an IRQ is mapped
>> or not.
>
> Mainly these expanders dont have an ier sort of registers and if at all the
> initial value is not set proper
> then it gives bad irq prints only once per changed bit for which there is no
> handler.
> Should I drop this in v2?

I don't know, I mean if it's causing a problem or filling the console
with error prints it is really annoying, and if there is nothing to do
about the condition then there is not much more we can do I guess.

But please take a second look.

Yours,
Linus Walleij