[PATCH v4 0/3] cleanup of gpio_pcf857x.c

[PATCH v4 0/3] cleanup of gpio_pcf857x.c

[George Cherian]

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

v1 --> v2
        Split v1 to 3 patches
v2 --> v3
	Remove the unnecessary dts patches.
v3 --> v4
	Remove gpio->irq (in patch 2)

Note: these patches were made after applying [1].
[1] - [PATCH v5] gpio: pcf857x: Add OF support - https://lkml.org/lkml/2013/8/27/70

George Cherian (3):
  gpio: pcf857x: change to devm_request_threaded_irq
  gpio: pcf857x: remove the irq_demux_work and gpio->irq
  gpio: pcf857x: call the gpio user handler iff gpio_to_irq is done

 drivers/gpio/gpio-pcf857x.c | 53 ++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

-- 
1.8.1.4

[PATCH v3 1/3] gpio: pcf857x: change to devm_request_threaded_irq

[George Cherian]

Remove the request_irq and use devm_request_threaded_irq
also cleanup free_irq. devm_* takes care of that.

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

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index 947cff4..4d0d28c 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -191,6 +191,25 @@ static int pcf857x_to_irq(struct gpio_chip *chip, unsigned offset)
 	return irq_create_mapping(gpio->irq_domain, offset);
 }
 
+static irqreturn_t pcf857x_irq(int irq, void *data)
+{
+	struct pcf857x  *gpio = data;
+	unsigned long change, i, status, flags;
+
+	status = gpio->read(gpio->client);
+
+	spin_lock_irqsave(&gpio->slock, flags);
+
+	change = gpio->status ^ status;
+	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);
+
+	return IRQ_HANDLED;
+}
+
 static void pcf857x_irq_demux_work(struct work_struct *work)
 {
 	struct pcf857x *gpio = container_of(work,
@@ -241,8 +260,6 @@ 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,
@@ -258,8 +275,11 @@ static int pcf857x_irq_domain_init(struct pcf857x *gpio,
 		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;
 
-- 
1.8.1.4

[PATCH v3 2/3] gpio: pcf857x: remove the irq_demux_work and gpio->irq

[George Cherian]

Now that we are using devm_request_threaded_irq no need for
irq_demux_work and gpio->irq. Remove all its references.

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

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index 4d0d28c..ef52ee3 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -30,7 +30,6 @@
 #include <linux/of_device.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
-#include <linux/workqueue.h>
 
 
 static const struct i2c_device_id pcf857x_id[] = {
@@ -89,12 +88,10 @@ 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 (*write)(struct i2c_client *client, unsigned data);
 	int (*read)(struct i2c_client *client);
@@ -210,38 +207,6 @@ static irqreturn_t pcf857x_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void pcf857x_irq_demux_work(struct work_struct *work)
-{
-	struct pcf857x *gpio = container_of(work,
-					       struct pcf857x,
-					       work);
-	unsigned long change, i, status, flags;
-
-	status = gpio->read(gpio->client);
-
-	spin_lock_irqsave(&gpio->slock, flags);
-
-	change = gpio->status ^ status;
-	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;
-}
-
 static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned int virq,
 				 irq_hw_number_t hw)
 {
@@ -284,9 +249,7 @@ static int pcf857x_irq_domain_init(struct pcf857x *gpio,
 		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;
 
 	return 0;
 
-- 
1.8.1.4

[PATCH v3 3/3] gpio: pcf857x: call the gpio user handler iff gpio_to_irq is done

[George Cherian]

For pcf857x driver if the initial state is not set properly (proper
n_latch is not passed), we get bad irq prints on console.
We get this only for the first interrupt and doesnot repeat for further
interrupts unles and until there are other gpio pins which are not flipping
continously.

following prints are seen on console.

[   40.983924] irq 0, desc: ce004080, depth: 1, count: 0, unhandled: 0
[   40.990511] ->handle_irq():  c00aa538, handle_bad_irq+0x0/0x260
[   40.996768] ->irq_data.chip(): c080b6ec, no_irq_chip+0x0/0x60
[   41.002842] ->action():   (null)
[   41.006242]    IRQ_NOPROBE set
[   41.009465]  IRQ_NOREQUEST set

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

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index ef52ee3..4887b8c 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -92,6 +92,7 @@ struct pcf857x {
 	spinlock_t		slock;		/* protect irq demux */
 	unsigned		out;		/* software latch */
 	unsigned		status;		/* current status */
+	unsigned		irq_mapped;	/* mapped gpio irqs */
 
 	int (*write)(struct i2c_client *client, unsigned data);
 	int (*read)(struct i2c_client *client);
@@ -184,8 +185,13 @@ 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 irqreturn_t pcf857x_irq(int irq, void *data)
@@ -197,7 +203,12 @@ static irqreturn_t pcf857x_irq(int irq, void *data)
 
 	spin_lock_irqsave(&gpio->slock, flags);
 
-	change = gpio->status ^ status;
+	/*
+	 * call the interrupt handler iff gpio is used as
+	 * interrupt source, just to avoid bad irqs
+	 */
+
+	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;
@@ -210,9 +221,14 @@ static irqreturn_t pcf857x_irq(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;
 }
 
@@ -235,7 +251,7 @@ static int pcf857x_irq_domain_init(struct pcf857x *gpio,
 	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;
 
-- 
1.8.1.4

Re: [PATCH v4 0/3] cleanup of gpio_pcf857x.c

[Kuninori Morimoto]

Hi

> This patch series
>         - removes the irq_demux_work
>         - Uses devm_request_threaded_irq
>         - Call the user handler iff gpio_to_irq is done.
> 
> v1 --> v2
>         Split v1 to 3 patches
> v2 --> v3
> 	Remove the unnecessary dts patches.
> v3 --> v4
> 	Remove gpio->irq (in patch 2)
> 
> Note: these patches were made after applying [1].
> [1] - [PATCH v5] gpio: pcf857x: Add OF support - https://lkml.org/lkml/2013/8/27/70
> 
> George Cherian (3):
>   gpio: pcf857x: change to devm_request_threaded_irq
>   gpio: pcf857x: remove the irq_demux_work and gpio->irq
>   gpio: pcf857x: call the gpio user handler iff gpio_to_irq is done
> 
>  drivers/gpio/gpio-pcf857x.c | 53 ++++++++++++++++++++++-----------------------
>  1 file changed, 26 insertions(+), 27 deletions(-)

For all patches

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Best regards
---
Kuninori Morimoto

Re: [PATCH v4 0/3] cleanup of gpio_pcf857x.c

[Linus Walleij]

On Wed, Sep 4, 2013 at 9:03 AM, George Cherian <george.cherian@ti.com> wrote:

> This patch series
>         - removes the irq_demux_work
>         - Uses devm_request_threaded_irq
>         - Call the user handler iff gpio_to_irq is done.
>
> v1 --> v2
>         Split v1 to 3 patches
> v2 --> v3
>         Remove the unnecessary dts patches.
> v3 --> v4
>         Remove gpio->irq (in patch 2)

Applied all three patches with Kuninoro's ACK.

However your last iteration was still labeled "v3" in the subject
so I was a bit confused, but I took all those sent on sep 4.

Please check the result in my GPIO tree or linux-next and
make sure it works.

> Note: these patches were made after applying [1].
> [1] - [PATCH v5] gpio: pcf857x: Add OF support - https://lkml.org/lkml/2013/8/27/70

Well that patch is not finished, but by rebasing patch 2
with patch -p1 < patch2.patch it applied anyway.

Now Laurent has to finalize his DT bindings on top
of your patches instead.

Yours,
Linus Walleij

Re: [PATCH v4 0/3] cleanup of gpio_pcf857x.c

[Laurent Pinchart]

Hi Linus,

On Tuesday 17 September 2013 14:51:48 Linus Walleij wrote:
> On Wed, Sep 4, 2013 at 9:03 AM, George Cherian wrote:
> > This patch series
> > 
> >         - removes the irq_demux_work
> >         - Uses devm_request_threaded_irq
> >         - Call the user handler iff gpio_to_irq is done.
> > 
> > v1 --> v2
> > 
> >         Split v1 to 3 patches
> > 
> > v2 --> v3
> > 
> >         Remove the unnecessary dts patches.
> > 
> > v3 --> v4
> > 
> >         Remove gpio->irq (in patch 2)
> 
> Applied all three patches with Kuninoro's ACK.
> 
> However your last iteration was still labeled "v3" in the subject
> so I was a bit confused, but I took all those sent on sep 4.
> 
> Please check the result in my GPIO tree or linux-next and
> make sure it works.
> 
> > Note: these patches were made after applying [1].
> > [1] - [PATCH v5] gpio: pcf857x: Add OF support -
> > https://lkml.org/lkml/2013/8/27/70
>
> Well that patch is not finished, but by rebasing patch 2 with
> patch -p1 < patch2.patch it applied anyway.
> 
> Now Laurent has to finalize his DT bindings on top of your patches instead.

I will do so. At first sight I don't see any conflict issue.

-- 
Regards,

Laurent Pinchart

[PATCH v3 0/3] cleanup of gpio_pcf857x.c

[George Cherian]

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

v1 --> v2
        Split v1 to 3 patches
v2 --> v3
	Remove the unnecessary dts patches.


George Cherian (3):
  gpio: pcf857x: change to devm_request_threaded_irq
  gpio: pcf857x: remove the irq_demux_work
  gpio: pcf857x: call the gpio user handler iff gpio_to_irq is done

 drivers/gpio/gpio-pcf857x.c | 51 +++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

-- 
1.8.1.4

[PATCH v3 1/3] gpio: pcf857x: change to devm_request_threaded_irq

[George Cherian]

Remove the request_irq and use devm_request_threaded_irq
also cleanup free_irq. devm_* takes care of that.

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

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index 947cff4..4d0d28c 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -191,6 +191,25 @@ static int pcf857x_to_irq(struct gpio_chip *chip, unsigned offset)
 	return irq_create_mapping(gpio->irq_domain, offset);
 }
 
+static irqreturn_t pcf857x_irq(int irq, void *data)
+{
+	struct pcf857x  *gpio = data;
+	unsigned long change, i, status, flags;
+
+	status = gpio->read(gpio->client);
+
+	spin_lock_irqsave(&gpio->slock, flags);
+
+	change = gpio->status ^ status;
+	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);
+
+	return IRQ_HANDLED;
+}
+
 static void pcf857x_irq_demux_work(struct work_struct *work)
 {
 	struct pcf857x *gpio = container_of(work,
@@ -241,8 +260,6 @@ 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,
@@ -258,8 +275,11 @@ static int pcf857x_irq_domain_init(struct pcf857x *gpio,
 		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;
 
-- 
1.8.1.4