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

[Santosh <santosh.shilimkar@ti.com>]

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