diff mbox

[v5,03/22] gpio/omap: make gpio_context part of gpio_bank structure

Message ID 1312455893-14922-4-git-send-email-tarun.kanti@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tarun Kanti DebBarma Aug. 4, 2011, 11:04 a.m. UTC
From: Charulatha V <charu@ti.com>

Currently gpio_context array used to save gpio bank's context, is used only for
OMAP3 architecture. Move gpio_context as part of gpio_bank structure so that it
can be specific to each gpio bank and can be used for any OMAP architecture

Signed-off-by: Charulatha V <charu@ti.com>
---
 drivers/gpio/gpio-omap.c |   76 ++++++++++++++++++++-------------------------
 1 files changed, 34 insertions(+), 42 deletions(-)

Comments

Santosh Shilimkar Aug. 23, 2011, 12:45 p.m. UTC | #1
On Thursday 04 August 2011 04:34 PM, Tarun Kanti DebBarma wrote:
> From: Charulatha V<charu@ti.com>
>
> Currently gpio_context array used to save gpio bank's context, is used only for
> OMAP3 architecture. Move gpio_context as part of gpio_bank structure so that it
> can be specific to each gpio bank and can be used for any OMAP architecture
>
> Signed-off-by: Charulatha V<charu@ti.com>
> ---
Few comments.

>   drivers/gpio/gpio-omap.c |   76 ++++++++++++++++++++-------------------------
>   1 files changed, 34 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index aaf07b8..2fa8b13 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -30,6 +30,19 @@
>
>   static LIST_HEAD(omap_gpio_list);
>
> +struct gpio_regs {
> +	u32 irqenable1;
> +	u32 irqenable2;
> +	u32 wake_en;
> +	u32 ctrl;
> +	u32 oe;
> +	u32 leveldetect0;
> +	u32 leveldetect1;
> +	u32 risingdetect;
> +	u32 fallingdetect;
> +	u32 dataout;
debounce and debounce_en registers are missing.
Add them please.

> +};
> +
>   struct gpio_bank {
>   	struct list_head node;
>   	unsigned long pbase;
> @@ -43,7 +56,7 @@ struct gpio_bank {
>   #endif
>   	u32 non_wakeup_gpios;
>   	u32 enabled_non_wakeup_gpios;
> -
> +	struct gpio_regs context;
>   	u32 saved_datain;
>   	u32 saved_fallingdetect;
>   	u32 saved_risingdetect;
> @@ -66,23 +79,6 @@ struct gpio_bank {
>   	struct omap_gpio_reg_offs *regs;
>   };
>
> -#ifdef CONFIG_ARCH_OMAP3
> -struct omap3_gpio_regs {
> -	u32 irqenable1;
> -	u32 irqenable2;
> -	u32 wake_en;
> -	u32 ctrl;
> -	u32 oe;
> -	u32 leveldetect0;
> -	u32 leveldetect1;
> -	u32 risingdetect;
> -	u32 fallingdetect;
> -	u32 dataout;
> -};
> -
> -static struct omap3_gpio_regs gpio_context[OMAP34XX_NR_GPIOS];
> -#endif
> -
>   #define GPIO_INDEX(bank, gpio) (gpio % bank->width)
>   #define GPIO_BIT(bank, gpio) (1<<  GPIO_INDEX(bank, gpio))
>
> @@ -1494,33 +1490,31 @@ void omap2_gpio_resume_after_idle(void)
>   void omap_gpio_save_context(void)
>   {
>   	struct gpio_bank *bank;
> -	int i = 0;
>
>   	list_for_each_entry(bank,&omap_gpio_list, node) {
> -		i++;
>
>   		if (!bank->loses_context)
>   			continue;
>
> -		gpio_context[i].irqenable1 =
> +		bank->context.irqenable1 =
>   			__raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE1);
> -		gpio_context[i].irqenable2 =
> +		bank->context.irqenable2 =
>   			__raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE2);

The context restore procedure should be done carefully. For instance
IRQ enabled register should be restored last to avoid any spurious 
interrupts.

> -		gpio_context[i].wake_en =
> +		bank->context.wake_en =
>   			__raw_readl(bank->base + OMAP24XX_GPIO_WAKE_EN);
> -		gpio_context[i].ctrl =
> +		bank->context.ctrl =
>   			__raw_readl(bank->base + OMAP24XX_GPIO_CTRL);
> -		gpio_context[i].oe =
> +		bank->context.oe =
>   			__raw_readl(bank->base + OMAP24XX_GPIO_OE);
Restore dataout register before OE restore.

> -		gpio_context[i].leveldetect0 =
> +		bank->context.leveldetect0 =
>   			__raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT0);
> -		gpio_context[i].leveldetect1 =
> +		bank->context.leveldetect1 =
>   			__raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT1);
> -		gpio_context[i].risingdetect =
> +		bank->context.risingdetect =
>   			__raw_readl(bank->base + OMAP24XX_GPIO_RISINGDETECT);
> -		gpio_context[i].fallingdetect =
> +		bank->context.fallingdetect =
>   			__raw_readl(bank->base + OMAP24XX_GPIO_FALLINGDETECT);
> -		gpio_context[i].dataout =
> +		bank->context.dataout =
>   			__raw_readl(bank->base + OMAP24XX_GPIO_DATAOUT);
>   	}
>   }

Regards
Santosh
--
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
Kevin Hilman Aug. 25, 2011, 8:23 p.m. UTC | #2
Santosh <santosh.shilimkar@ti.com> writes:

> On Thursday 04 August 2011 04:34 PM, Tarun Kanti DebBarma wrote:
>> From: Charulatha V<charu@ti.com>
>>
>> Currently gpio_context array used to save gpio bank's context, is used only for
>> OMAP3 architecture. Move gpio_context as part of gpio_bank structure so that it
>> can be specific to each gpio bank and can be used for any OMAP architecture
>>
>> Signed-off-by: Charulatha V<charu@ti.com>
>> ---
> Few comments.
>

[...]

>> @@ -1494,33 +1490,31 @@ void omap2_gpio_resume_after_idle(void)
>>   void omap_gpio_save_context(void)
>>   {
>>   	struct gpio_bank *bank;
>> -	int i = 0;
>>
>>   	list_for_each_entry(bank,&omap_gpio_list, node) {
>> -		i++;
>>
>>   		if (!bank->loses_context)
>>   			continue;
>>
>> -		gpio_context[i].irqenable1 =
>> +		bank->context.irqenable1 =
>>   			__raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE1);
>> -		gpio_context[i].irqenable2 =
>> +		bank->context.irqenable2 =
>>   			__raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE2);
>
> The context restore procedure should be done carefully. For instance
> IRQ enabled register should be restored last to avoid any spurious
> interrupts.

For the sake of clean, easy-to-review patches, this kind of functional
change should be a separate patch.

The goal of $SUBJECT patch is simply to move the context struct into the
bank struct, not change the order of the save restore.

Any changing of the order of save/restore should be in a dedicated patch
with a descriptive changelog since that is changing behavior of the code.

Kevin
--
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
Tarun Kanti DebBarma Aug. 26, 2011, 3:57 a.m. UTC | #3
[...]
> 
> >> @@ -1494,33 +1490,31 @@ void omap2_gpio_resume_after_idle(void)
> >>   void omap_gpio_save_context(void)
> >>   {
> >>   	struct gpio_bank *bank;
> >> -	int i = 0;
> >>
> >>   	list_for_each_entry(bank,&omap_gpio_list, node) {
> >> -		i++;
> >>
> >>   		if (!bank->loses_context)
> >>   			continue;
> >>
> >> -		gpio_context[i].irqenable1 =
> >> +		bank->context.irqenable1 =
> >>   			__raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE1);
> >> -		gpio_context[i].irqenable2 =
> >> +		bank->context.irqenable2 =
> >>   			__raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE2);
> >
> > The context restore procedure should be done carefully. For instance
> > IRQ enabled register should be restored last to avoid any spurious
> > interrupts.
> 
> For the sake of clean, easy-to-review patches, this kind of functional
> change should be a separate patch.
> 
> The goal of $SUBJECT patch is simply to move the context struct into the
> bank struct, not change the order of the save restore.
> 
> Any changing of the order of save/restore should be in a dedicated patch
> with a descriptive changelog since that is changing behavior of the code.
Ok.

> 
> Kevin
--
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 aaf07b8..2fa8b13 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -30,6 +30,19 @@ 
 
 static LIST_HEAD(omap_gpio_list);
 
+struct gpio_regs {
+	u32 irqenable1;
+	u32 irqenable2;
+	u32 wake_en;
+	u32 ctrl;
+	u32 oe;
+	u32 leveldetect0;
+	u32 leveldetect1;
+	u32 risingdetect;
+	u32 fallingdetect;
+	u32 dataout;
+};
+
 struct gpio_bank {
 	struct list_head node;
 	unsigned long pbase;
@@ -43,7 +56,7 @@  struct gpio_bank {
 #endif
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
-
+	struct gpio_regs context;
 	u32 saved_datain;
 	u32 saved_fallingdetect;
 	u32 saved_risingdetect;
@@ -66,23 +79,6 @@  struct gpio_bank {
 	struct omap_gpio_reg_offs *regs;
 };
 
-#ifdef CONFIG_ARCH_OMAP3
-struct omap3_gpio_regs {
-	u32 irqenable1;
-	u32 irqenable2;
-	u32 wake_en;
-	u32 ctrl;
-	u32 oe;
-	u32 leveldetect0;
-	u32 leveldetect1;
-	u32 risingdetect;
-	u32 fallingdetect;
-	u32 dataout;
-};
-
-static struct omap3_gpio_regs gpio_context[OMAP34XX_NR_GPIOS];
-#endif
-
 #define GPIO_INDEX(bank, gpio) (gpio % bank->width)
 #define GPIO_BIT(bank, gpio) (1 << GPIO_INDEX(bank, gpio))
 
@@ -1494,33 +1490,31 @@  void omap2_gpio_resume_after_idle(void)
 void omap_gpio_save_context(void)
 {
 	struct gpio_bank *bank;
-	int i = 0;
 
 	list_for_each_entry(bank, &omap_gpio_list, node) {
-		i++;
 
 		if (!bank->loses_context)
 			continue;
 
-		gpio_context[i].irqenable1 =
+		bank->context.irqenable1 =
 			__raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE1);
-		gpio_context[i].irqenable2 =
+		bank->context.irqenable2 =
 			__raw_readl(bank->base + OMAP24XX_GPIO_IRQENABLE2);
-		gpio_context[i].wake_en =
+		bank->context.wake_en =
 			__raw_readl(bank->base + OMAP24XX_GPIO_WAKE_EN);
-		gpio_context[i].ctrl =
+		bank->context.ctrl =
 			__raw_readl(bank->base + OMAP24XX_GPIO_CTRL);
-		gpio_context[i].oe =
+		bank->context.oe =
 			__raw_readl(bank->base + OMAP24XX_GPIO_OE);
-		gpio_context[i].leveldetect0 =
+		bank->context.leveldetect0 =
 			__raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT0);
-		gpio_context[i].leveldetect1 =
+		bank->context.leveldetect1 =
 			__raw_readl(bank->base + OMAP24XX_GPIO_LEVELDETECT1);
-		gpio_context[i].risingdetect =
+		bank->context.risingdetect =
 			__raw_readl(bank->base + OMAP24XX_GPIO_RISINGDETECT);
-		gpio_context[i].fallingdetect =
+		bank->context.fallingdetect =
 			__raw_readl(bank->base + OMAP24XX_GPIO_FALLINGDETECT);
-		gpio_context[i].dataout =
+		bank->context.dataout =
 			__raw_readl(bank->base + OMAP24XX_GPIO_DATAOUT);
 	}
 }
@@ -1528,33 +1522,31 @@  void omap_gpio_save_context(void)
 void omap_gpio_restore_context(void)
 {
 	struct gpio_bank *bank;
-	int i = 0;
 
 	list_for_each_entry(bank, &omap_gpio_list, node) {
-		i++;
 
 		if (!bank->loses_context)
 			continue;
 
-		__raw_writel(gpio_context[i].irqenable1,
+		__raw_writel(bank->context.irqenable1,
 				bank->base + OMAP24XX_GPIO_IRQENABLE1);
-		__raw_writel(gpio_context[i].irqenable2,
+		__raw_writel(bank->context.irqenable2,
 				bank->base + OMAP24XX_GPIO_IRQENABLE2);
-		__raw_writel(gpio_context[i].wake_en,
+		__raw_writel(bank->context.wake_en,
 				bank->base + OMAP24XX_GPIO_WAKE_EN);
-		__raw_writel(gpio_context[i].ctrl,
+		__raw_writel(bank->context.ctrl,
 				bank->base + OMAP24XX_GPIO_CTRL);
-		__raw_writel(gpio_context[i].oe,
+		__raw_writel(bank->context.oe,
 				bank->base + OMAP24XX_GPIO_OE);
-		__raw_writel(gpio_context[i].leveldetect0,
+		__raw_writel(bank->context.leveldetect0,
 				bank->base + OMAP24XX_GPIO_LEVELDETECT0);
-		__raw_writel(gpio_context[i].leveldetect1,
+		__raw_writel(bank->context.leveldetect1,
 				bank->base + OMAP24XX_GPIO_LEVELDETECT1);
-		__raw_writel(gpio_context[i].risingdetect,
+		__raw_writel(bank->context.risingdetect,
 				bank->base + OMAP24XX_GPIO_RISINGDETECT);
-		__raw_writel(gpio_context[i].fallingdetect,
+		__raw_writel(bank->context.fallingdetect,
 				bank->base + OMAP24XX_GPIO_FALLINGDETECT);
-		__raw_writel(gpio_context[i].dataout,
+		__raw_writel(bank->context.dataout,
 				bank->base + OMAP24XX_GPIO_DATAOUT);
 	}
 }