diff mbox

[v3,12/20] GPIO: OMAP: Use wkup_status for all SoCs

Message ID 87hb6rtt2f.fsf@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kevin Hilman July 12, 2011, 3:30 p.m. UTC 
"DebBarma, Tarun Kanti" <tarun.kanti@ti.com> writes:

> Kevin,
> [...]
>> 
>> >  		set_gpio_trigger(bank, gpio, trigger);
>> >  	} else if (bank->regs->irqctrl) {
>> >  		reg += bank->regs->irqctrl;
>> > @@ -295,13 +296,7 @@ static int _set_gpio_triggering(struct gpio_bank
>> *bank, int gpio, int trigger)
>> >  		if (trigger & IRQ_TYPE_EDGE_FALLING)
>> >  			l |= 1 << (gpio << 1);
>> >
>> > -		if (trigger)
>> > -			/* Enable wake-up during idle for dynamic tick */
>> > -			__raw_writel(1 << gpio, bank->base
>> > -						+ bank->regs->wkup_set);
>> > -		else
>> > -			__raw_writel(1 << gpio, bank->base
>> > -						+ bank->regs->wkup_clear);
>> > +		MOD_REG_BIT(bank->regs->wkup_status, 1 << gpio, trigger != 0);
>> 
>> This isn't right, so I'm not sure how this is working. At this point:
>> 
>>      base = bank->base;
>>      reg = bank->base + bank->regs->edgectrl[12];
> Right so far...
>
>> 
>> but if you look at MOD_REG_BIT(), it does register access using 'base +
>> reg', but here that will be 'bank->base + bank->base + reg offset',
>> which will be doing a read/modify/write to who-knows-where.
> My understanding was MOD_REG_BIT(bank->regs->wkup_status, ...) translate to:
> bank->base + bank->regs->wkup_status, for the following reason:
>
> In the following macro, reg is equivalent to bank->regs->wkup_status.
> But this is not the case with base, which remains as bank->base.
> Please correct me.

You're right.  

This is a good example why using macros like this (with some arguments
passed and some arguments implied) leads to confusion.

> #define MOD_REG_BIT(reg, bit_mask, set) \
> do {    \
>         int l = __raw_readl(base + reg); \
>         if (set) l |= bit_mask; \
>         else l &= ~bit_mask; \
>         __raw_writel(l, base + reg); \
> } while(0)
>
>> 
>> >  		__raw_writel(l, reg);
>> 
>> Oh, now I see why it works: because you have an additional write here,
>> which isn't right either because MOD_REG_BIT() already does the read
>> *and* write.
> This should be writing to following still.
> reg = bank->base + bank->regs->edgectrl[12];

Yes, but it is a duplicate write since the MOD_REG_BIT() already does
the write.

A better solution is to just get rid of this macro and use a static inline.

The patch below does just this, and I'm including it into my
gpio-cleanup series (branch: for_3.1/gpio-cleanup-2.)  Please rebase
your updated series on that branch.

Kevin

From 0c3da6533061f51236743ebaf4bae23561e315fe Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@ti.com>
Date: Tue, 12 Jul 2011 08:18:15 -0700
Subject: [PATCH] gpio/omap: replace MOD_REG_BIT macro with static inline

This macro is ugly and confusing, especially since it passes in most
arguments, but uses an implied 'base' from the caller.

Replace it with an equivalent static inline.

Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 drivers/gpio/gpio-omap.c |   54 ++++++++++++++++++++++++---------------------
 1 files changed, 29 insertions(+), 25 deletions(-)

Comments

Tarun Kanti DebBarma July 13, 2011, 3:55 a.m. UTC | #1 
[...] 
> A better solution is to just get rid of this macro and use a static
> inline.
> 
> The patch below does just this, and I'm including it into my
> gpio-cleanup series (branch: for_3.1/gpio-cleanup-2.)  Please rebase
> your updated series on that branch.
Alright! Thanks.
--
Tarun

> 
> Kevin
> 
> From 0c3da6533061f51236743ebaf4bae23561e315fe Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@ti.com>
> Date: Tue, 12 Jul 2011 08:18:15 -0700
> Subject: [PATCH] gpio/omap: replace MOD_REG_BIT macro with static inline
> 
> This macro is ugly and confusing, especially since it passes in most
> arguments, but uses an implied 'base' from the caller.
> 
> Replace it with an equivalent static inline.
> 
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
>  drivers/gpio/gpio-omap.c |   54 ++++++++++++++++++++++++-----------------
> ----
>  1 files changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 501ca3d..6d616ee 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -148,13 +148,17 @@ static int _get_gpio_dataout(struct gpio_bank *bank,
> int gpio)
>  	return (__raw_readl(reg) & GPIO_BIT(bank, gpio)) != 0;
>  }
> 
> -#define MOD_REG_BIT(reg, bit_mask, set)	\
> -do {	\
> -	int l = __raw_readl(base + reg); \
> -	if (set) l |= bit_mask; \
> -	else l &= ~bit_mask; \
> -	__raw_writel(l, base + reg); \
> -} while(0)
> +static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool
> set)
> +{
> +	int l = __raw_readl(base + reg);
> +
> +	if (set)
> +		l |= mask;
> +	else
> +		l &= ~mask;
> +
> +	__raw_writel(l, base + reg);
> +}
> 
>  /**
>   * _set_gpio_debounce - low level gpio debounce time
> @@ -210,28 +214,28 @@ static inline void set_24xx_gpio_triggering(struct
> gpio_bank *bank, int gpio,
>  	u32 gpio_bit = 1 << gpio;
> 
>  	if (cpu_is_omap44xx()) {
> -		MOD_REG_BIT(OMAP4_GPIO_LEVELDETECT0, gpio_bit,
> -			trigger & IRQ_TYPE_LEVEL_LOW);
> -		MOD_REG_BIT(OMAP4_GPIO_LEVELDETECT1, gpio_bit,
> -			trigger & IRQ_TYPE_LEVEL_HIGH);
> -		MOD_REG_BIT(OMAP4_GPIO_RISINGDETECT, gpio_bit,
> -			trigger & IRQ_TYPE_EDGE_RISING);
> -		MOD_REG_BIT(OMAP4_GPIO_FALLINGDETECT, gpio_bit,
> -			trigger & IRQ_TYPE_EDGE_FALLING);
> +		_gpio_rmw(base, OMAP4_GPIO_LEVELDETECT0, gpio_bit,
> +			  trigger & IRQ_TYPE_LEVEL_LOW);
> +		_gpio_rmw(base, OMAP4_GPIO_LEVELDETECT1, gpio_bit,
> +			  trigger & IRQ_TYPE_LEVEL_HIGH);
> +		_gpio_rmw(base, OMAP4_GPIO_RISINGDETECT, gpio_bit,
> +			  trigger & IRQ_TYPE_EDGE_RISING);
> +		_gpio_rmw(base, OMAP4_GPIO_FALLINGDETECT, gpio_bit,
> +			  trigger & IRQ_TYPE_EDGE_FALLING);
>  	} else {
> -		MOD_REG_BIT(OMAP24XX_GPIO_LEVELDETECT0, gpio_bit,
> -			trigger & IRQ_TYPE_LEVEL_LOW);
> -		MOD_REG_BIT(OMAP24XX_GPIO_LEVELDETECT1, gpio_bit,
> -			trigger & IRQ_TYPE_LEVEL_HIGH);
> -		MOD_REG_BIT(OMAP24XX_GPIO_RISINGDETECT, gpio_bit,
> -			trigger & IRQ_TYPE_EDGE_RISING);
> -		MOD_REG_BIT(OMAP24XX_GPIO_FALLINGDETECT, gpio_bit,
> -			trigger & IRQ_TYPE_EDGE_FALLING);
> +		_gpio_rmw(base, OMAP24XX_GPIO_LEVELDETECT0, gpio_bit,
> +			  trigger & IRQ_TYPE_LEVEL_LOW);
> +		_gpio_rmw(base, OMAP24XX_GPIO_LEVELDETECT1, gpio_bit,
> +			  trigger & IRQ_TYPE_LEVEL_HIGH);
> +		_gpio_rmw(base, OMAP24XX_GPIO_RISINGDETECT, gpio_bit,
> +			  trigger & IRQ_TYPE_EDGE_RISING);
> +		_gpio_rmw(base, OMAP24XX_GPIO_FALLINGDETECT, gpio_bit,
> +			  trigger & IRQ_TYPE_EDGE_FALLING);
>  	}
>  	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
>  		if (cpu_is_omap44xx()) {
> -			MOD_REG_BIT(OMAP4_GPIO_IRQWAKEN0, gpio_bit,
> -				trigger != 0);
> +			_gpio_rmw(base, OMAP4_GPIO_IRQWAKEN0, gpio_bit,
> +				  trigger != 0);
>  		} else {
>  			/*
>  			 * GPIO wakeup request can only be generated on edge
> --
> 1.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 501ca3d..6d616ee 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -148,13 +148,17 @@  static int _get_gpio_dataout(struct gpio_bank *bank, int gpio)
 	return (__raw_readl(reg) & GPIO_BIT(bank, gpio)) != 0;
 }
 
-#define MOD_REG_BIT(reg, bit_mask, set)	\
-do {	\
-	int l = __raw_readl(base + reg); \
-	if (set) l |= bit_mask; \
-	else l &= ~bit_mask; \
-	__raw_writel(l, base + reg); \
-} while(0)
+static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
+{
+	int l = __raw_readl(base + reg);
+
+	if (set) 
+		l |= mask;
+	else
+		l &= ~mask;
+
+	__raw_writel(l, base + reg);
+}
 
 /**
  * _set_gpio_debounce - low level gpio debounce time
@@ -210,28 +214,28 @@  static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
 	u32 gpio_bit = 1 << gpio;
 
 	if (cpu_is_omap44xx()) {
-		MOD_REG_BIT(OMAP4_GPIO_LEVELDETECT0, gpio_bit,
-			trigger & IRQ_TYPE_LEVEL_LOW);
-		MOD_REG_BIT(OMAP4_GPIO_LEVELDETECT1, gpio_bit,
-			trigger & IRQ_TYPE_LEVEL_HIGH);
-		MOD_REG_BIT(OMAP4_GPIO_RISINGDETECT, gpio_bit,
-			trigger & IRQ_TYPE_EDGE_RISING);
-		MOD_REG_BIT(OMAP4_GPIO_FALLINGDETECT, gpio_bit,
-			trigger & IRQ_TYPE_EDGE_FALLING);
+		_gpio_rmw(base, OMAP4_GPIO_LEVELDETECT0, gpio_bit,
+			  trigger & IRQ_TYPE_LEVEL_LOW);
+		_gpio_rmw(base, OMAP4_GPIO_LEVELDETECT1, gpio_bit,
+			  trigger & IRQ_TYPE_LEVEL_HIGH);
+		_gpio_rmw(base, OMAP4_GPIO_RISINGDETECT, gpio_bit,
+			  trigger & IRQ_TYPE_EDGE_RISING);
+		_gpio_rmw(base, OMAP4_GPIO_FALLINGDETECT, gpio_bit,
+			  trigger & IRQ_TYPE_EDGE_FALLING);
 	} else {
-		MOD_REG_BIT(OMAP24XX_GPIO_LEVELDETECT0, gpio_bit,
-			trigger & IRQ_TYPE_LEVEL_LOW);
-		MOD_REG_BIT(OMAP24XX_GPIO_LEVELDETECT1, gpio_bit,
-			trigger & IRQ_TYPE_LEVEL_HIGH);
-		MOD_REG_BIT(OMAP24XX_GPIO_RISINGDETECT, gpio_bit,
-			trigger & IRQ_TYPE_EDGE_RISING);
-		MOD_REG_BIT(OMAP24XX_GPIO_FALLINGDETECT, gpio_bit,
-			trigger & IRQ_TYPE_EDGE_FALLING);
+		_gpio_rmw(base, OMAP24XX_GPIO_LEVELDETECT0, gpio_bit,
+			  trigger & IRQ_TYPE_LEVEL_LOW);
+		_gpio_rmw(base, OMAP24XX_GPIO_LEVELDETECT1, gpio_bit,
+			  trigger & IRQ_TYPE_LEVEL_HIGH);
+		_gpio_rmw(base, OMAP24XX_GPIO_RISINGDETECT, gpio_bit,
+			  trigger & IRQ_TYPE_EDGE_RISING);
+		_gpio_rmw(base, OMAP24XX_GPIO_FALLINGDETECT, gpio_bit,
+			  trigger & IRQ_TYPE_EDGE_FALLING);
 	}
 	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
 		if (cpu_is_omap44xx()) {
-			MOD_REG_BIT(OMAP4_GPIO_IRQWAKEN0, gpio_bit,
-				trigger != 0);
+			_gpio_rmw(base, OMAP4_GPIO_IRQWAKEN0, gpio_bit,
+				  trigger != 0);
 		} else {
 			/*
 			 * GPIO wakeup request can only be generated on edge