LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [patch] drivers: gpio: sx150x: Do not access I2C from mask/unmask functions
@ 2011-02-14 16:48 Thomas Gleixner
  2011-02-14 17:08 ` Thomas Gleixner
  2011-02-14 19:45 ` Gregory Bean
  0 siblings, 2 replies; 5+ messages in thread
From: Thomas Gleixner @ 2011-02-14 16:48 UTC (permalink / raw)
  To: LKML; +Cc: Andrew Morton, Gregory Bean, Jean Delvare, Lennert Buytenhek

[-- Attachment #1: drivers-gpio-sx150x.patch --]
[-- Type: text/plain, Size: 4373 bytes --]

irq_chip->irq_mask/unmask are called with interrupts disabled and
irq_desc->lock held. So we cannot access i2c from this context. That's
what irq_bus_sync_unlock() is for.

Store the masked information in the chip data structure and update the
i2c bus from the irq_bus_sync_unlock() callback.

This does not need a while(pending) loop because the update to this is
always serialized via the bus lock, so we never have more than one pin
update pending.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Gregory Bean <gbean@codeaurora.org>
Cc: Jean Delvare <khali@linux-fr.org>
Cc: Lennert Buytenhek <buytenh@secretlab.ca>
---
 drivers/gpio/sx150x.c |   54 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 17 deletions(-)

Index: linux-2.6-tip/drivers/gpio/sx150x.c
===================================================================
--- linux-2.6-tip.orig/drivers/gpio/sx150x.c
+++ linux-2.6-tip/drivers/gpio/sx150x.c
@@ -25,6 +25,8 @@
 #include <linux/workqueue.h>
 #include <linux/i2c/sx150x.h>
 
+#define NO_UPDATE_PENDING	-1
+
 struct sx150x_device_data {
 	u8 reg_pullup;
 	u8 reg_pulldn;
@@ -47,8 +49,11 @@ struct sx150x_chip {
 	const struct sx150x_device_data *dev_cfg;
 	int                              irq_summary;
 	int                              irq_base;
+	int				 irq_update;
 	u32                              irq_sense;
-	unsigned long                    irq_set_type_pending;
+	u32				 irq_masked;
+	u32				 dev_sense;
+	u32				 dev_masked;
 	struct irq_chip                  irq_chip;
 	struct mutex                     lock;
 };
@@ -312,9 +317,8 @@ static void sx150x_irq_mask(struct irq_d
 
 	chip = container_of(ic, struct sx150x_chip, irq_chip);
 	n = d->irq - chip->irq_base;
-
-	sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 1);
-	sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense, 0);
+	chip->irq_masked |= (1 << n);
+	chip->irq_update = n;
 }
 
 static void sx150x_irq_unmask(struct irq_data *d)
@@ -326,9 +330,8 @@ static void sx150x_irq_unmask(struct irq
 	chip = container_of(ic, struct sx150x_chip, irq_chip);
 	n = d->irq - chip->irq_base;
 
-	sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 0);
-	sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense,
-			 chip->irq_sense >> (n * 2));
+	chip->irq_masked &= ~(1 << n);
+	chip->irq_update = n;
 }
 
 static int sx150x_irq_set_type(struct irq_data *d, unsigned int flow_type)
@@ -350,7 +353,7 @@ static int sx150x_irq_set_type(struct ir
 
 	chip->irq_sense &= ~(3UL << (n * 2));
 	chip->irq_sense |= val << (n * 2);
-	chip->irq_set_type_pending |= BIT(n);
+	chip->update = n;
 	return 0;
 }
 
@@ -404,15 +407,29 @@ static void sx150x_irq_bus_sync_unlock(s
 
 	chip = container_of(ic, struct sx150x_chip, irq_chip);
 
-	while (chip->irq_set_type_pending) {
-		n = __ffs(chip->irq_set_type_pending);
-		chip->irq_set_type_pending &= ~BIT(n);
-		if (!(irq_to_desc(n + chip->irq_base)->status & IRQ_MASKED))
-			sx150x_write_cfg(chip, n, 2,
-					chip->dev_cfg->reg_sense,
-					chip->irq_sense >> (n * 2));
-	}
+	if (chip->irq_update == NO_UPDATE_PENDING)
+		goto out;
 
+	n = chip->irq_update;
+	chip->irq_update = NO_UPDATE_PENDING;
+
+	/* Avoid updates if nothing changed */
+	if (chip->dev_sense == chip->irq_sense &&
+	    chip->dev_sense == chip->irq_masked)
+		goto out;
+
+	chip->dev_sense = chip->irq_sense;
+	chip->dev_masked = chip->irq_masked;
+
+	if (irq_masked & (1 << n)) {
+		sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 1);
+		sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense, 0);
+	} else {
+		sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 0);
+		sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense,
+				 chip->irq_sense >> (n * 2));
+	}
+out:
 	mutex_unlock(&chip->lock);
 }
 
@@ -445,8 +462,11 @@ static void sx150x_init_chip(struct sx15
 	chip->irq_chip.irq_bus_sync_unlock = sx150x_irq_bus_sync_unlock;
 	chip->irq_summary                  = -1;
 	chip->irq_base                     = -1;
+	chip->irq_masked                   = ~0;
 	chip->irq_sense                    = 0;
-	chip->irq_set_type_pending         = 0;
+	chip->dev_masked                   = ~0;
+	chip->dev_sense                    = 0;
+	chip->irq_update		   = NO_UPDATE_PENDING;
 }
 
 static int sx150x_init_io(struct sx150x_chip *chip, u8 base, u16 cfg)



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [patch] drivers: gpio: sx150x: Do not access I2C from mask/unmask functions
  2011-02-14 16:48 [patch] drivers: gpio: sx150x: Do not access I2C from mask/unmask functions Thomas Gleixner
@ 2011-02-14 17:08 ` Thomas Gleixner
  2011-02-16  2:48   ` Grant Likely
  2011-02-14 19:45 ` Gregory Bean
  1 sibling, 1 reply; 5+ messages in thread
From: Thomas Gleixner @ 2011-02-14 17:08 UTC (permalink / raw)
  To: LKML; +Cc: Andrew Morton, Gregory Bean, Jean Delvare, Lennert Buytenhek

On Mon, 14 Feb 2011, Thomas Gleixner wrote:

Ooops. quilt refresh should be mandatory for quilt mail. One day I'll fix that.

------->
Subject: drivers: gpio: sx150x: Do not access I2C from mask/unmask functions
From: Thomas Gleixner <tglx@linutronix.de>
Date: Mon, 14 Feb 2011 16:58:11 +0100

irq_chip->irq_mask/unmask are called with interrupts disabled and
irq_desc->lock held. So we cannot access i2c from this context. That's
what irq_bus_sync_unlock() is for.
 
Store the masked information in the chip data structure and update the
i2c bus from the irq_bus_sync_unlock() callback.

This does not need a while(pending) loop because the update to this is
always serialized via the bus lock, so we never have more than one pin
update pending.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Gregory Bean <gbean@codeaurora.org>
Cc: Jean Delvare <khali@linux-fr.org>
Cc: Lennert Buytenhek <buytenh@secretlab.ca>
---
 drivers/gpio/sx150x.c |   54 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 17 deletions(-)

Index: linux-2.6-tip/drivers/gpio/sx150x.c
===================================================================
--- linux-2.6-tip.orig/drivers/gpio/sx150x.c
+++ linux-2.6-tip/drivers/gpio/sx150x.c
@@ -25,6 +25,8 @@
 #include <linux/workqueue.h>
 #include <linux/i2c/sx150x.h>
 
+#define NO_UPDATE_PENDING	-1
+
 struct sx150x_device_data {
 	u8 reg_pullup;
 	u8 reg_pulldn;
@@ -47,8 +49,11 @@ struct sx150x_chip {
 	const struct sx150x_device_data *dev_cfg;
 	int                              irq_summary;
 	int                              irq_base;
+	int				 irq_update;
 	u32                              irq_sense;
-	unsigned long                    irq_set_type_pending;
+	u32				 irq_masked;
+	u32				 dev_sense;
+	u32				 dev_masked;
 	struct irq_chip                  irq_chip;
 	struct mutex                     lock;
 };
@@ -312,9 +317,8 @@ static void sx150x_irq_mask(struct irq_d
 
 	chip = container_of(ic, struct sx150x_chip, irq_chip);
 	n = d->irq - chip->irq_base;
-
-	sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 1);
-	sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense, 0);
+	chip->irq_masked |= (1 << n);
+	chip->irq_update = n;
 }
 
 static void sx150x_irq_unmask(struct irq_data *d)
@@ -326,9 +330,8 @@ static void sx150x_irq_unmask(struct irq
 	chip = container_of(ic, struct sx150x_chip, irq_chip);
 	n = d->irq - chip->irq_base;
 
-	sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 0);
-	sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense,
-			 chip->irq_sense >> (n * 2));
+	chip->irq_masked &= ~(1 << n);
+	chip->irq_update = n;
 }
 
 static int sx150x_irq_set_type(struct irq_data *d, unsigned int flow_type)
@@ -350,7 +353,7 @@ static int sx150x_irq_set_type(struct ir
 
 	chip->irq_sense &= ~(3UL << (n * 2));
 	chip->irq_sense |= val << (n * 2);
-	chip->irq_set_type_pending |= BIT(n);
+	chip->irq_update = n;
 	return 0;
 }
 
@@ -404,15 +407,29 @@ static void sx150x_irq_bus_sync_unlock(s
 
 	chip = container_of(ic, struct sx150x_chip, irq_chip);
 
-	while (chip->irq_set_type_pending) {
-		n = __ffs(chip->irq_set_type_pending);
-		chip->irq_set_type_pending &= ~BIT(n);
-		if (!(irq_to_desc(n + chip->irq_base)->status & IRQ_MASKED))
-			sx150x_write_cfg(chip, n, 2,
-					chip->dev_cfg->reg_sense,
-					chip->irq_sense >> (n * 2));
-	}
+	if (chip->irq_update == NO_UPDATE_PENDING)
+		goto out;
 
+	n = chip->irq_update;
+	chip->irq_update = NO_UPDATE_PENDING;
+
+	/* Avoid updates if nothing changed */
+	if (chip->dev_sense == chip->irq_sense &&
+	    chip->dev_sense == chip->irq_masked)
+		goto out;
+
+	chip->dev_sense = chip->irq_sense;
+	chip->dev_masked = chip->irq_masked;
+
+	if (chip->irq_masked & (1 << n)) {
+		sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 1);
+		sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense, 0);
+	} else {
+		sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 0);
+		sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense,
+				 chip->irq_sense >> (n * 2));
+	}
+out:
 	mutex_unlock(&chip->lock);
 }
 
@@ -445,8 +462,11 @@ static void sx150x_init_chip(struct sx15
 	chip->irq_chip.irq_bus_sync_unlock = sx150x_irq_bus_sync_unlock;
 	chip->irq_summary                  = -1;
 	chip->irq_base                     = -1;
+	chip->irq_masked                   = ~0;
 	chip->irq_sense                    = 0;
-	chip->irq_set_type_pending         = 0;
+	chip->dev_masked                   = ~0;
+	chip->dev_sense                    = 0;
+	chip->irq_update		   = NO_UPDATE_PENDING;
 }
 
 static int sx150x_init_io(struct sx150x_chip *chip, u8 base, u16 cfg)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [patch] drivers: gpio: sx150x: Do not access I2C from mask/unmask functions
  2011-02-14 16:48 [patch] drivers: gpio: sx150x: Do not access I2C from mask/unmask functions Thomas Gleixner
  2011-02-14 17:08 ` Thomas Gleixner
@ 2011-02-14 19:45 ` Gregory Bean
  2011-02-14 19:58   ` Thomas Gleixner
  1 sibling, 1 reply; 5+ messages in thread
From: Gregory Bean @ 2011-02-14 19:45 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Andrew Morton, Jean Delvare, Lennert Buytenhek, rvaswani

>  static int sx150x_irq_set_type(struct irq_data *d, unsigned int flow_type)
> @@ -350,7 +353,7 @@ static int sx150x_irq_set_type(struct ir
>  
>  	chip->irq_sense &= ~(3UL << (n * 2));
>  	chip->irq_sense |= val << (n * 2);
> -	chip->irq_set_type_pending |= BIT(n);
> +	chip->update = n;

Should this be irq_update?

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [patch] drivers: gpio: sx150x: Do not access I2C from mask/unmask functions
  2011-02-14 19:45 ` Gregory Bean
@ 2011-02-14 19:58   ` Thomas Gleixner
  0 siblings, 0 replies; 5+ messages in thread
From: Thomas Gleixner @ 2011-02-14 19:58 UTC (permalink / raw)
  To: Gregory Bean
  Cc: LKML, Andrew Morton, Jean Delvare, Lennert Buytenhek, rvaswani

On Mon, 14 Feb 2011, Gregory Bean wrote:

> >  static int sx150x_irq_set_type(struct irq_data *d, unsigned int flow_type)
> > @@ -350,7 +353,7 @@ static int sx150x_irq_set_type(struct ir
> >  
> >  	chip->irq_sense &= ~(3UL << (n * 2));
> >  	chip->irq_sense |= val << (n * 2);
> > -	chip->irq_set_type_pending |= BIT(n);
> > +	chip->update = n;
> 
> Should this be irq_update?

See reply to myself

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [patch] drivers: gpio: sx150x: Do not access I2C from mask/unmask functions
  2011-02-14 17:08 ` Thomas Gleixner
@ 2011-02-16  2:48   ` Grant Likely
  0 siblings, 0 replies; 5+ messages in thread
From: Grant Likely @ 2011-02-16  2:48 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Andrew Morton, Gregory Bean, Jean Delvare, Lennert Buytenhek

On Mon, Feb 14, 2011 at 06:08:51PM +0100, Thomas Gleixner wrote:
> On Mon, 14 Feb 2011, Thomas Gleixner wrote:
> 
> Ooops. quilt refresh should be mandatory for quilt mail. One day I'll fix that.
> 
> ------->
> Subject: drivers: gpio: sx150x: Do not access I2C from mask/unmask functions
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Mon, 14 Feb 2011 16:58:11 +0100
> 
> irq_chip->irq_mask/unmask are called with interrupts disabled and
> irq_desc->lock held. So we cannot access i2c from this context. That's
> what irq_bus_sync_unlock() is for.
>  
> Store the masked information in the chip data structure and update the
> i2c bus from the irq_bus_sync_unlock() callback.
> 
> This does not need a while(pending) loop because the update to this is
> always serialized via the bus lock, so we never have more than one pin
> update pending.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Gregory Bean <gbean@codeaurora.org>
> Cc: Jean Delvare <khali@linux-fr.org>
> Cc: Lennert Buytenhek <buytenh@secretlab.ca>

Applied, thanks.

g.

> ---
>  drivers/gpio/sx150x.c |   54 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 37 insertions(+), 17 deletions(-)
> 
> Index: linux-2.6-tip/drivers/gpio/sx150x.c
> ===================================================================
> --- linux-2.6-tip.orig/drivers/gpio/sx150x.c
> +++ linux-2.6-tip/drivers/gpio/sx150x.c
> @@ -25,6 +25,8 @@
>  #include <linux/workqueue.h>
>  #include <linux/i2c/sx150x.h>
>  
> +#define NO_UPDATE_PENDING	-1
> +
>  struct sx150x_device_data {
>  	u8 reg_pullup;
>  	u8 reg_pulldn;
> @@ -47,8 +49,11 @@ struct sx150x_chip {
>  	const struct sx150x_device_data *dev_cfg;
>  	int                              irq_summary;
>  	int                              irq_base;
> +	int				 irq_update;
>  	u32                              irq_sense;
> -	unsigned long                    irq_set_type_pending;
> +	u32				 irq_masked;
> +	u32				 dev_sense;
> +	u32				 dev_masked;
>  	struct irq_chip                  irq_chip;
>  	struct mutex                     lock;
>  };
> @@ -312,9 +317,8 @@ static void sx150x_irq_mask(struct irq_d
>  
>  	chip = container_of(ic, struct sx150x_chip, irq_chip);
>  	n = d->irq - chip->irq_base;
> -
> -	sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 1);
> -	sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense, 0);
> +	chip->irq_masked |= (1 << n);
> +	chip->irq_update = n;
>  }
>  
>  static void sx150x_irq_unmask(struct irq_data *d)
> @@ -326,9 +330,8 @@ static void sx150x_irq_unmask(struct irq
>  	chip = container_of(ic, struct sx150x_chip, irq_chip);
>  	n = d->irq - chip->irq_base;
>  
> -	sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 0);
> -	sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense,
> -			 chip->irq_sense >> (n * 2));
> +	chip->irq_masked &= ~(1 << n);
> +	chip->irq_update = n;
>  }
>  
>  static int sx150x_irq_set_type(struct irq_data *d, unsigned int flow_type)
> @@ -350,7 +353,7 @@ static int sx150x_irq_set_type(struct ir
>  
>  	chip->irq_sense &= ~(3UL << (n * 2));
>  	chip->irq_sense |= val << (n * 2);
> -	chip->irq_set_type_pending |= BIT(n);
> +	chip->irq_update = n;
>  	return 0;
>  }
>  
> @@ -404,15 +407,29 @@ static void sx150x_irq_bus_sync_unlock(s
>  
>  	chip = container_of(ic, struct sx150x_chip, irq_chip);
>  
> -	while (chip->irq_set_type_pending) {
> -		n = __ffs(chip->irq_set_type_pending);
> -		chip->irq_set_type_pending &= ~BIT(n);
> -		if (!(irq_to_desc(n + chip->irq_base)->status & IRQ_MASKED))
> -			sx150x_write_cfg(chip, n, 2,
> -					chip->dev_cfg->reg_sense,
> -					chip->irq_sense >> (n * 2));
> -	}
> +	if (chip->irq_update == NO_UPDATE_PENDING)
> +		goto out;
>  
> +	n = chip->irq_update;
> +	chip->irq_update = NO_UPDATE_PENDING;
> +
> +	/* Avoid updates if nothing changed */
> +	if (chip->dev_sense == chip->irq_sense &&
> +	    chip->dev_sense == chip->irq_masked)
> +		goto out;
> +
> +	chip->dev_sense = chip->irq_sense;
> +	chip->dev_masked = chip->irq_masked;
> +
> +	if (chip->irq_masked & (1 << n)) {
> +		sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 1);
> +		sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense, 0);
> +	} else {
> +		sx150x_write_cfg(chip, n, 1, chip->dev_cfg->reg_irq_mask, 0);
> +		sx150x_write_cfg(chip, n, 2, chip->dev_cfg->reg_sense,
> +				 chip->irq_sense >> (n * 2));
> +	}
> +out:
>  	mutex_unlock(&chip->lock);
>  }
>  
> @@ -445,8 +462,11 @@ static void sx150x_init_chip(struct sx15
>  	chip->irq_chip.irq_bus_sync_unlock = sx150x_irq_bus_sync_unlock;
>  	chip->irq_summary                  = -1;
>  	chip->irq_base                     = -1;
> +	chip->irq_masked                   = ~0;
>  	chip->irq_sense                    = 0;
> -	chip->irq_set_type_pending         = 0;
> +	chip->dev_masked                   = ~0;
> +	chip->dev_sense                    = 0;
> +	chip->irq_update		   = NO_UPDATE_PENDING;
>  }
>  
>  static int sx150x_init_io(struct sx150x_chip *chip, u8 base, u16 cfg)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2011-02-16  2:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-14 16:48 [patch] drivers: gpio: sx150x: Do not access I2C from mask/unmask functions Thomas Gleixner
2011-02-14 17:08 ` Thomas Gleixner
2011-02-16  2:48   ` Grant Likely
2011-02-14 19:45 ` Gregory Bean
2011-02-14 19:58   ` Thomas Gleixner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).