RE: [PATCH] [OMAP] GPIO Module disable if all pins inactive

2009-10-26 Thread Varadarajan, Charu Latha 
  #endif
 + if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
 + if (!bank-gpio_status) {
 + ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
 + /* Module is enabled, clocks are not gated */
 + ctrl = 0xFFFE;
 + __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
 + }
 + bank-gpio_status |= 1  offset;
 + }
 why do this every time a gpio is enabled? why not do this iff gpio was
 never used before.. how about the following:
 The module is enabled only when gpio_status indicates that no GPIO
 in that  module is currently active and the GPIO being requested is the 1st 
 one
 to be active in that module.
 Each module would be disabled in free() API when all GPIOs in a particular 
 module
 becomes inactive. The module is re-enabled in request() API when a GPIO is
 requested from the module that was previously disabled.
Thanks.
Welcome
 if (!bank-gpio_status  (cpu_is_omap24xx() || cpu_is_omap34xx() ||
 cpu_is_omap44xx())) {
u32 ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
/* Module is enabled, clocks are not gated */
ctrl = 0xFFFE;
__raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
 }
 bank-gpio_status |= 1  offset;
 Why to touch gpio_status if not used (for other than 34xx/24xx/44xx cases)?
either the gpio_status should be under a #ifdef for 34xx when defining
or it should be usable by all. what it does now is do both.
 gpio_status is not used under  #ifdef for 34xx. It is used only with 
cpu_is_omap
(34xx/24xx/44xx). Should we use both #ifdef and cpu_is_omap together? Why? 
But I don't see that approach in LO. For eg., usage of dbck_enable_mask is 
used only with cpu_is_omap and is not declared under  #ifdef.

my proposal is to allow gpio_status to be usable by ALL OMAPs - maybe
OMAP1 also could also use it.. I cannot comment - but it does look to
have scope of usage beyond omap2/3/4 series?
Even though OMAP1 supports the same feature, I am not including it as I
cannot test it and I am not sure about it in OMAP1. For 24xx, 34xx  44xx, 
the registers used and offsets are all the same. So the same code is reused. --
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] [OMAP] GPIO Module disable if all pins inactive

2009-10-26 Thread Menon, Nishanth 
 From: Varadarajan, Charu Latha
 Sent: Monday, October 26, 2009 4:07 AM
 
   #endif
  + if (cpu_is_omap24xx() || cpu_is_omap34xx() ||
 cpu_is_omap44xx()) {
  + if (!bank-gpio_status) {
  + ctrl = __raw_readl(bank-base +
 OMAP24XX_GPIO_CTRL);
  + /* Module is enabled, clocks are not gated */
  + ctrl = 0xFFFE;
  + __raw_writel(ctrl, bank-base +
 OMAP24XX_GPIO_CTRL);
  + }
  + bank-gpio_status |= 1  offset;
  + }
  why do this every time a gpio is enabled? why not do this iff gpio was
  never used before.. how about the following:
  The module is enabled only when gpio_status indicates that no GPIO
  in that  module is currently active and the GPIO being requested is the
 1st one
  to be active in that module.
  Each module would be disabled in free() API when all GPIOs in a
 particular module
  becomes inactive. The module is re-enabled in request() API when a GPIO
 is
  requested from the module that was previously disabled.
 Thanks.
 Welcome
  if (!bank-gpio_status  (cpu_is_omap24xx() || cpu_is_omap34xx() ||
  cpu_is_omap44xx())) {
 u32 ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
 /* Module is enabled, clocks are not gated */
 ctrl = 0xFFFE;
 __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
  }
  bank-gpio_status |= 1  offset;
  Why to touch gpio_status if not used (for other than 34xx/24xx/44xx
 cases)?
 either the gpio_status should be under a #ifdef for 34xx when defining
 or it should be usable by all. what it does now is do both.
  gpio_status is not used under  #ifdef for 34xx. It is used only with
 cpu_is_omap
 (34xx/24xx/44xx). Should we use both #ifdef and cpu_is_omap together? Why?
 But I don't see that approach in LO. For eg., usage of dbck_enable_mask is
 used only with cpu_is_omap and is not declared under  #ifdef.

Please see [1] saved_datain is an example of why I think gpio.c could go thru a 
cleanup ;) already in #ifdef in line 1908, in line 1925, we add a new #ifdef 
under a #ifdef :D.. err...

Ok this piece of code is not perfect.. 

Regards,
Nishanth Menon
[1] 
http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=blob;f=arch/arm/plat-omap/gpio.c;h=487bea7b5605fe366064d792d0c9cc8aed1eac63;hb=0bbf5337f2f2957775051a3caf60b66d3306c815#l174
 
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] [OMAP] GPIO Module disable if all pins inactive

2009-10-26 Thread Varadarajan, Charu Latha 
sinipped
 bank-gpio_status |= 1  offset;
 Why to touch gpio_status if not used (for other than 34xx/24xx/44xx
 cases)?
either the gpio_status should be under a #ifdef for 34xx when defining
or it should be usable by all. what it does now is do both.
  gpio_status is not used under  #ifdef for 34xx. It is used only with
 cpu_is_omap
 (34xx/24xx/44xx). Should we use both #ifdef and cpu_is_omap together? Why?
 But I don't see that approach in LO. For eg., usage of dbck_enable_mask is
 used only with cpu_is_omap and is not declared under  #ifdef.

Please see [1] saved_datain is an example of why I think gpio.c could go thru 
a cleanup ;) already in #ifdef in line 1908, in line 1925, we add a new #ifdef 
under a #ifdef :D.. err...

Ok this piece of code is not perfect..
Looking onto lines 1908  1925, I accept that the gpio.c code is not perfect. 
But they 
are nothing to do with my patch. I guess this is the same at many places in most
of the drivers and someone has to take up the job of cleaning up. 
Regards,
Nishanth Menon
.[1] 
http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=blob;f=arch/arm/plat-omap/gpio.c;h=487bea7b5605fe366064d792d0c9cc8aed1eac63;hb=0bbf5337f2f2957775051a3caf60b66d3306c815#l174
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [OMAP] GPIO Module disable if all pins inactive

2009-10-23 Thread charu 
From: Charulatha V ch...@ti.com

This patch disables a GPIO module when all the pins of GPIO
module are inactive (clock gating forced at module level) and
enables the module when any gpio in the module is requested.

Signed-off-by: Charulatha V ch...@ti.com
---
 arch/arm/plat-omap/gpio.c |   22 ++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index cdc2a58..2304a5d 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -194,6 +194,7 @@ struct gpio_bank {
spinlock_t lock;
struct gpio_chip chip;
struct clk *dbck;
+   u32 gpio_status;
 };
 
 #define METHOD_MPUIO   0
@@ -1080,6 +1081,7 @@ static int omap_gpio_request(struct gpio_chip *chip, 
unsigned offset)
 {
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
+   u32 ctrl = 0;
 
spin_lock_irqsave(bank-lock, flags);
 
@@ -1097,6 +1099,15 @@ static int omap_gpio_request(struct gpio_chip *chip, 
unsigned offset)
__raw_writel(__raw_readl(reg) | (1  offset), reg);
}
 #endif
+   if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
+   if (!bank-gpio_status) {
+   ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
+   /* Module is enabled, clocks are not gated */
+   ctrl = 0xFFFE;
+   __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
+   }
+   bank-gpio_status |= 1  offset;
+   }
spin_unlock_irqrestore(bank-lock, flags);
 
return 0;
@@ -1106,6 +1117,7 @@ static void omap_gpio_free(struct gpio_chip *chip, 
unsigned offset)
 {
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
+   u32 ctrl = 0;
 
spin_lock_irqsave(bank-lock, flags);
 #ifdef CONFIG_ARCH_OMAP16XX
@@ -1123,6 +1135,15 @@ static void omap_gpio_free(struct gpio_chip *chip, 
unsigned offset)
__raw_writel(1  offset, reg);
}
 #endif
+   if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
+   bank-gpio_status = ~(1  offset);
+   if (!bank-gpio_status) {
+   ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
+   /* Module is disabled, clocks are gated */
+   ctrl |= 1;
+   __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
+   }
+   }
_reset_gpio(bank, bank-chip.base + offset);
spin_unlock_irqrestore(bank-lock, flags);
 }
@@ -1700,6 +1721,7 @@ static int __init _omap_gpio_init(void)
gpio_count = 32;
}
 #endif
+   bank-gpio_status = 0;
/* REVISIT eventually switch from OMAP-specific gpio structs
 * over to the generic ones
 */
-- 
1.6.0.4

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

Re: [PATCH] [OMAP] GPIO Module disable if all pins inactive

2009-10-23 Thread Nishanth Menon 

Varadarajan, Charu Latha had written, on 10/23/2009 10:55 AM, the following:

From: Charulatha V ch...@ti.com

This patch disables a GPIO module when all the pins of GPIO
module are inactive (clock gating forced at module level) and
enables the module when any gpio in the module is requested.

Signed-off-by: Charulatha V ch...@ti.com
---
 arch/arm/plat-omap/gpio.c |   22 ++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index cdc2a58..2304a5d 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -194,6 +194,7 @@ struct gpio_bank {
spinlock_t lock;
struct gpio_chip chip;
struct clk *dbck;
+   u32 gpio_status;

please rename this as gpio_usage?

maybe OMAP1 could also benefit out of this..

 };
 
 #define METHOD_MPUIO		0

@@ -1080,6 +1081,7 @@ static int omap_gpio_request(struct gpio_chip *chip, 
unsigned offset)
 {
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
+   u32 ctrl = 0;
Remove this to the {} no point in wasting stack space when you dont need 
to + you will generate warning for OMAP1 platforms.
 
 	spin_lock_irqsave(bank-lock, flags);
 
@@ -1097,6 +1099,15 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)

__raw_writel(__raw_readl(reg) | (1  offset), reg);
}
 #endif
+   if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
+   if (!bank-gpio_status) {
+   ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
+   /* Module is enabled, clocks are not gated */
+   ctrl = 0xFFFE;
+   __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
+   }
+   bank-gpio_status |= 1  offset;
+   }

why do this every time a gpio is enabled? why not do this iff gpio was
never used before.. how about the following:
if (!bank-gpio_status  (cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx())) {
u32 ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
/* Module is enabled, clocks are not gated */
ctrl = 0xFFFE;
__raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
}
bank-gpio_status |= 1  offset;

spin_unlock_irqrestore(bank-lock, flags);
 
 	return 0;

@@ -1106,6 +1117,7 @@ static void omap_gpio_free(struct gpio_chip *chip, 
unsigned offset)
 {
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
+   u32 ctrl = 0;

used just once - move it to the {} + warning to OMAP1
 
 	spin_lock_irqsave(bank-lock, flags);

 #ifdef CONFIG_ARCH_OMAP16XX
@@ -1123,6 +1135,15 @@ static void omap_gpio_free(struct gpio_chip *chip, 
unsigned offset)
__raw_writel(1  offset, reg);
}
 #endif
+   if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
+   bank-gpio_status = ~(1  offset);
+   if (!bank-gpio_status) {
+   ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
+   /* Module is disabled, clocks are gated */
+   ctrl |= 1;
+   __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
+   }
+   }

how about this:
bank-gpio_status = ~(1  offset);
if (!bank-gpio_status  (cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx())) {
u32 ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
/* Module is disabled, clocks are gated */
ctrl |= 1;
__raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
}

_reset_gpio(bank, bank-chip.base + offset);
spin_unlock_irqrestore(bank-lock, flags);
 }
@@ -1700,6 +1721,7 @@ static int __init _omap_gpio_init(void)
gpio_count = 32;
}
 #endif
+   bank-gpio_status = 0;
/* REVISIT eventually switch from OMAP-specific gpio structs
 * over to the generic ones
 */


Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] [OMAP] GPIO Module disable if all pins inactive

2009-10-23 Thread Varadarajan, Charu Latha 
Varadarajan, Charu Latha had written, on 10/23/2009 10:55 AM, the following:
 From: Charulatha V ch...@ti.com

 This patch disables a GPIO module when all the pins of GPIO
 module are inactive (clock gating forced at module level) and
 enables the module when any gpio in the module is requested.

 Signed-off-by: Charulatha V ch...@ti.com
 ---
  arch/arm/plat-omap/gpio.c |   22 ++
  1 files changed, 22 insertions(+), 0 deletions(-)

 diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
 index cdc2a58..2304a5d 100644
 --- a/arch/arm/plat-omap/gpio.c
 +++ b/arch/arm/plat-omap/gpio.c
 @@ -194,6 +194,7 @@ struct gpio_bank {
spinlock_t lock;
struct gpio_chip chip;
struct clk *dbck;
 + u32 gpio_status;
please rename this as gpio_usage?
okay

maybe OMAP1 could also benefit out of this..
  };
 
  #define METHOD_MPUIO 0
 @@ -1080,6 +1081,7 @@ static int omap_gpio_request(struct gpio_chip *chip, 
 unsigned offset)
  {
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
 + u32 ctrl = 0;
Remove this to the {} no point in wasting stack space when you dont need
to + you will generate warning for OMAP1 platforms.
 
spin_lock_irqsave(bank-lock, flags);
 
 @@ -1097,6 +1099,15 @@ static int omap_gpio_request(struct gpio_chip *chip, 
 unsigned offset)
__raw_writel(__raw_readl(reg) | (1  offset), reg);
}
  #endif
 + if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
 + if (!bank-gpio_status) {
 + ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
 + /* Module is enabled, clocks are not gated */
 + ctrl = 0xFFFE;
 + __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
 + }
 + bank-gpio_status |= 1  offset;
 + }
why do this every time a gpio is enabled? why not do this iff gpio was
never used before.. how about the following:
The module is enabled only when gpio_status indicates that no GPIO 
in that  module is currently active and the GPIO being requested is the 1st one 
to be active in that module.
Each module would be disabled in free() API when all GPIOs in a particular 
module 
becomes inactive. The module is re-enabled in request() API when a GPIO is 
requested from the module that was previously disabled.
if (!bank-gpio_status  (cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx())) {
u32 ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
/* Module is enabled, clocks are not gated */
ctrl = 0xFFFE;
__raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
}
bank-gpio_status |= 1  offset;
Why to touch gpio_status if not used (for other than 34xx/24xx/44xx cases)? 
spin_unlock_irqrestore(bank-lock, flags);
 
return 0;
 @@ -1106,6 +1117,7 @@ static void omap_gpio_free(struct gpio_chip *chip, 
 unsigned offset)
  {
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
 + u32 ctrl = 0;
used just once - move it to the {} + warning to OMAP1
 
spin_lock_irqsave(bank-lock, flags);
  #ifdef CONFIG_ARCH_OMAP16XX
 @@ -1123,6 +1135,15 @@ static void omap_gpio_free(struct gpio_chip *chip, 
 unsigned offset)
__raw_writel(1  offset, reg);
}
  #endif
 + if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
 + bank-gpio_status = ~(1  offset);
 + if (!bank-gpio_status) {
 + ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
 + /* Module is disabled, clocks are gated */
 + ctrl |= 1;
 + __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
 + }
 + }
how about this:
bank-gpio_status = ~(1  offset);
if (!bank-gpio_status  (cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx())) {
u32 ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
/* Module is disabled, clocks are gated */
ctrl |= 1;
__raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
}
Why to touch gpio_status if not used (for other than 24xx/34xx/44xx cases)? 
_reset_gpio(bank, bank-chip.base + offset);
spin_unlock_irqrestore(bank-lock, flags);
  }
 @@ -1700,6 +1721,7 @@ static int __init _omap_gpio_init(void)
gpio_count = 32;
}
  #endif
 + bank-gpio_status = 0;
/* REVISIT eventually switch from OMAP-specific gpio structs
 * over to the generic ones
 */
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [OMAP] GPIO Module disable if all pins inactive

2009-10-23 Thread Nishanth Menon 

Varadarajan, Charu Latha had written, on 10/23/2009 11:05 PM, the following:

 #endif
+ if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
+ if (!bank-gpio_status) {
+ ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
+ /* Module is enabled, clocks are not gated */
+ ctrl = 0xFFFE;
+ __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
+ }
+ bank-gpio_status |= 1  offset;
+ }

why do this every time a gpio is enabled? why not do this iff gpio was
never used before.. how about the following:
The module is enabled only when gpio_status indicates that no GPIO 
in that  module is currently active and the GPIO being requested is the 1st one 
to be active in that module.
Each module would be disabled in free() API when all GPIOs in a particular module 
becomes inactive. The module is re-enabled in request() API when a GPIO is 
requested from the module that was previously disabled.

Thanks.


if (!bank-gpio_status  (cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx())) {
   u32 ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
   /* Module is enabled, clocks are not gated */
   ctrl = 0xFFFE;
   __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
}
bank-gpio_status |= 1  offset;
Why to touch gpio_status if not used (for other than 34xx/24xx/44xx cases)? 
either the gpio_status should be under a #ifdef for 34xx when defining 
or it should be usable by all. what it does now is do both.


my proposal is to allow gpio_status to be usable by ALL OMAPs - maybe 
OMAP1 also could also use it.. I cannot comment - but it does look to 
have scope of usage beyond omap2/3/4 series?


--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html