Kevin,
On Thu, May 26, 2011 at 04:27, Kevin Hilman <khilman@xxxxxx> wrote:
> Tarun Kanti DebBarma <tarun.kanti@xxxxxx> writes:
>
>> Since wake_status, wake_clear, wake_set is common for all banks on a given
>> OMAP version it is enough to get their values once during probe().
>> Also, register offsets are already initialzed according to OMAP versions
>> during device registration. We no longer need these explicit checks.
>>
>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@xxxxxx>
>> Signed-off-by: Charulatha V <charu@xxxxxx>
>> ---
>> arch/arm/mach-omap1/gpio15xx.c | 6 ++
>> arch/arm/mach-omap1/gpio16xx.c | 6 ++
>> arch/arm/mach-omap1/gpio7xx.c | 6 ++
>> arch/arm/mach-omap2/gpio.c | 6 ++
>> arch/arm/plat-omap/include/plat/gpio.h | 3 +
>> drivers/gpio/gpio_omap.c | 102 +++++++-------------------------
>> 6 files changed, 49 insertions(+), 80 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap1/gpio15xx.c b/arch/arm/mach-omap1/gpio15xx.c
>> index f18a4a9..b0bd21e 100644
>> --- a/arch/arm/mach-omap1/gpio15xx.c
>> +++ b/arch/arm/mach-omap1/gpio15xx.c
>> @@ -43,6 +43,9 @@ static struct omap_gpio_reg_offs omap15xx_mpuio_regs = {
>> .irqenable = OMAP_MPUIO_GPIO_MASKIT,
>> .irqenable_inv = true,
>> .ctrl = USHRT_MAX,
>> + .wkupstatus = USHRT_MAX,
>> + .wkupclear = USHRT_MAX,
>> + .wkupset = USHRT_MAX,
>> };
>
> Same comment as earlier about USHRT_MAX.
>
> Just use zero to indicate no register present.
>
>> static struct __initdata omap_gpio_platform_data omap15xx_mpu_gpio_config = {
>> @@ -85,6 +88,9 @@ static struct omap_gpio_reg_offs omap15xx_gpio_regs = {
>> .irqenable = OMAP1510_GPIO_INT_MASK,
>> .irqenable_inv = true,
>> .ctrl = USHRT_MAX,
>> + .wkupstatus = USHRT_MAX,
>> + .wkupclear = USHRT_MAX,
>> + .wkupset = USHRT_MAX,
>> };
>>
>> static struct __initdata omap_gpio_platform_data omap15xx_gpio_config = {
>> diff --git a/arch/arm/mach-omap1/gpio16xx.c b/arch/arm/mach-omap1/gpio16xx.c
>> index d886b88..403437b 100644
>> --- a/arch/arm/mach-omap1/gpio16xx.c
>> +++ b/arch/arm/mach-omap1/gpio16xx.c
>> @@ -46,6 +46,9 @@ static struct omap_gpio_reg_offs omap16xx_mpuio_regs = {
>> .irqenable = OMAP_MPUIO_GPIO_MASKIT,
>> .irqenable_inv = true,
>> .ctrl = USHRT_MAX,
>> + .wkupstatus = USHRT_MAX,
>> + .wkupclear = USHRT_MAX,
>> + .wkupset = USHRT_MAX,
>> };
>>
>> static struct __initdata omap_gpio_platform_data omap16xx_mpu_gpio_config = {
>> @@ -91,6 +94,9 @@ static struct omap_gpio_reg_offs omap16xx_gpio_regs = {
>> .set_irqenable = OMAP1610_GPIO_SET_IRQENABLE1,
>> .clr_irqenable = OMAP1610_GPIO_CLEAR_IRQENABLE1,
>> .ctrl = USHRT_MAX,
>> + .wkupstatus = OMAP1610_GPIO_WAKEUPENABLE,
>> + .wkupclear = OMAP1610_GPIO_CLEAR_WAKEUPENA,
>> + .wkupset = OMAP1610_GPIO_SET_WAKEUPENA,
>> };
>>
>> static struct __initdata omap_gpio_platform_data omap16xx_gpio1_config = {
>> diff --git a/arch/arm/mach-omap1/gpio7xx.c b/arch/arm/mach-omap1/gpio7xx.c
>> index c7684ce..d5a4aaf 100644
>> --- a/arch/arm/mach-omap1/gpio7xx.c
>> +++ b/arch/arm/mach-omap1/gpio7xx.c
>> @@ -48,6 +48,9 @@ static struct omap_gpio_reg_offs omap7xx_mpuio_regs = {
>> .irqenable = OMAP_MPUIO_GPIO_MASKIT / 2,
>> .irqenable_inv = true,
>> .ctrl = USHRT_MAX,
>> + .wkupstatus = USHRT_MAX,
>> + .wkupclear = USHRT_MAX,
>> + .wkupset = USHRT_MAX,
>> };
>>
>> static struct __initdata omap_gpio_platform_data omap7xx_mpu_gpio_config = {
>> @@ -90,6 +93,9 @@ static struct omap_gpio_reg_offs omap7xx_gpio_regs = {
>> .irqenable = OMAP7XX_GPIO_INT_MASK,
>> .irqenable_inv = true,
>> .ctrl = USHRT_MAX,
>> + .wkupstatus = USHRT_MAX,
>> + .wkupclear = USHRT_MAX,
>> + .wkupset = USHRT_MAX,
>> };
>>
>> static struct __initdata omap_gpio_platform_data omap7xx_gpio1_config = {
>> diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c
>> index 0782e06..7e79999 100644
>> --- a/arch/arm/mach-omap2/gpio.c
>> +++ b/arch/arm/mach-omap2/gpio.c
>> @@ -111,6 +111,9 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
>> pdata->regs->debounce = OMAP24XX_GPIO_DEBOUNCE_VAL;
>> pdata->regs->debounce_en = OMAP24XX_GPIO_DEBOUNCE_EN;
>> pdata->regs->ctrl = OMAP24XX_GPIO_CTRL;
>> + pdata->regs->wkupstatus = OMAP24XX_GPIO_WAKE_EN;
>> + pdata->regs->wkupclear = OMAP24XX_GPIO_CLEARWKUENA;
>> + pdata->regs->wkupset = OMAP24XX_GPIO_SETWKUENA;
>> break;
>> case 3:
>> pdata->bank_type = METHOD_GPIO_44XX;
>> @@ -128,6 +131,9 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
>> pdata->regs->debounce = OMAP4_GPIO_DEBOUNCINGTIME;
>> pdata->regs->debounce_en = OMAP4_GPIO_DEBOUNCENABLE;
>> pdata->regs->ctrl = OMAP4_GPIO_CTRL;
>> + pdata->regs->wkupstatus = OMAP4_GPIO_IRQWAKEN0;
>> + pdata->regs->wkupclear = OMAP4_GPIO_IRQWAKEN0;
>> + pdata->regs->wkupset = OMAP4_GPIO_IRQWAKEN0;
>> break;
>> default:
>> WARN(1, "Invalid gpio bank_type\n");
>> diff --git a/arch/arm/plat-omap/include/plat/gpio.h b/arch/arm/plat-omap/include/plat/gpio.h
>> index 5718a45..2d1a5d6 100644
>> --- a/arch/arm/plat-omap/include/plat/gpio.h
>> +++ b/arch/arm/plat-omap/include/plat/gpio.h
>> @@ -189,6 +189,9 @@ struct omap_gpio_reg_offs {
>> u16 debounce;
>> u16 debounce_en;
>> u16 ctrl;
>> + u16 wkupstatus;
>> + u16 wkupclear;
>> + u16 wkupset;
>
> s/wkup/wkup_/
Okay.
>
>> bool irqenable_inv;
>> };
>> diff --git a/drivers/gpio/gpio_omap.c b/drivers/gpio/gpio_omap.c
>> index fcc60be..c189416 100644
>> --- a/drivers/gpio/gpio_omap.c
>> +++ b/drivers/gpio/gpio_omap.c
>> @@ -77,6 +77,9 @@ struct gpio_bank {
>> u32 width;
>> u32 ctx_lost_cnt_before;
>> u16 id;
>> + void __iomem *wake_status;
>> + void __iomem *wake_clear;
>> + void __iomem *wake_set;
>>
>> void (*set_dataout)(struct gpio_bank *bank, int gpio, int enable);
>>
>> @@ -606,27 +609,11 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
>> unsigned long flags;
>>
>> spin_lock_irqsave(&bank->lock, flags);
>> -#ifdef CONFIG_ARCH_OMAP16XX
>> - if (bank->method == METHOD_GPIO_1610) {
>> - /* Disable wake-up during idle for dynamic tick */
>> - void __iomem *reg = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
>> - __raw_writel(1 << offset, reg);
>> - }
>> -#endif
>> -#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
>> - if (bank->method == METHOD_GPIO_24XX) {
>> - /* Disable wake-up during idle for dynamic tick */
>> - void __iomem *reg = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
>> - __raw_writel(1 << offset, reg);
>> - }
>> -#endif
>> -#ifdef CONFIG_ARCH_OMAP4
>> - if (bank->method == METHOD_GPIO_44XX) {
>> +
>> + if (bank->regs->wkupclear != USHRT_MAX)
>
> Here you check the 'regs' version...
>
>> /* Disable wake-up during idle for dynamic tick */
>> - void __iomem *reg = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - __raw_writel(1 << offset, reg);
>> - }
>> -#endif
>> + __raw_writel(1 << offset, bank->wake_clear);
>> +
>
> ...and here you write using the copy. Not good for readability. More
> on this below.
Agreed. Will do the needful.
>
>> bank->mod_usage &= ~(1 << offset);
>>
>> if ((bank->regs->ctrl != USHRT_MAX) && (!bank->mod_usage)) {
>> @@ -1189,6 +1176,15 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>> goto err_free;
>> }
>>
>> + /*
>> + * Storing these addresses avoids redundant computation of these
>> + * values every time in suspend/resume functions and for all the
>> + * gpio banks.
>> + */
>> + bank->wake_status = bank->base + bank->regs->wkupstatus;
>> + bank->wake_clear = bank->base + bank->regs->wkupclear;
>> + bank->wake_set = bank->base + bank->regs->wkupset;
>
> Well, it's not really redundant since these are only used in the suspend
> and resume functions. I'd rather have an extra add in the
> suspend/resume functions than have 3 extra words in every struct gpio_bank.
>
> Also, Using 'bank + reg offset' in the functions that use them is
> consistent with the pattern of all the other changes in the cleanup
> series, so lets not start something new.
Agreed.
>
>> pm_runtime_enable(bank->dev);
>> pm_runtime_get_sync(bank->dev);
>>
>> @@ -1207,7 +1203,7 @@ err_exit:
>> }
>>
>> #if defined(CONFIG_ARCH_OMAP16XX) || defined(CONFIG_ARCH_OMAP2PLUS)
>> -static int omap_gpio_suspend(struct sys_device *dev, pm_message_t mesg)
>> +static int omap_gpio_suspend(struct sys_device *dev, pm_message_t unused)
>
> change not related to $SUBJECT patch
>
>> {
>> struct gpio_bank *bank;
>>
>> @@ -1215,41 +1211,12 @@ static int omap_gpio_suspend(struct sys_device *dev, pm_message_t mesg)
>> return 0;
>>
>> list_for_each_entry(bank, &omap_gpio_list, node) {
>> - void __iomem *wake_status;
>> - void __iomem *wake_clear;
>> - void __iomem *wake_set;
>
> IMO, these should stay here and should just be assigned 'bank->base +
> bank->regs->...'
Okay.
>
>> unsigned long flags;
>>
>> - switch (bank->method) {
>> -#ifdef CONFIG_ARCH_OMAP16XX
>> - case METHOD_GPIO_1610:
>> - wake_status = bank->base + OMAP1610_GPIO_WAKEUPENABLE;
>> - wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
>> - wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
>> - break;
>> -#endif
>> -#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
>> - case METHOD_GPIO_24XX:
>> - wake_status = bank->base + OMAP24XX_GPIO_WAKE_EN;
>> - wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
>> - wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
>> - break;
>> -#endif
>> -#ifdef CONFIG_ARCH_OMAP4
>> - case METHOD_GPIO_44XX:
>> - wake_status = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - break;
>> -#endif
>> - default:
>> - continue;
>> - }
>> -
>> spin_lock_irqsave(&bank->lock, flags);
>> - bank->saved_wakeup = __raw_readl(wake_status);
>> - __raw_writel(0xffffffff, wake_clear);
>> - __raw_writel(bank->suspend_wakeup, wake_set);
>> + bank->saved_wakeup = __raw_readl(bank->wake_status);
>> + __raw_writel(0xffffffff, bank->wake_clear);
>> + __raw_writel(bank->suspend_wakeup, bank->wake_set);
>> spin_unlock_irqrestore(&bank->lock, flags);
>> }
>
>> @@ -1264,36 +1231,11 @@ static int omap_gpio_resume(struct sys_device *dev)
>> return 0;
>>
>> list_for_each_entry(bank, &omap_gpio_list, node) {
>> - void __iomem *wake_clear;
>> - void __iomem *wake_set;
>> unsigned long flags;
>>
>> - switch (bank->method) {
>> -#ifdef CONFIG_ARCH_OMAP16XX
>> - case METHOD_GPIO_1610:
>> - wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
>> - wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
>> - break;
>> -#endif
>> -#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
>> - case METHOD_GPIO_24XX:
>> - wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
>> - wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
>> - break;
>> -#endif
>> -#ifdef CONFIG_ARCH_OMAP4
>> - case METHOD_GPIO_44XX:
>> - wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - break;
>> -#endif
>> - default:
>> - continue;
>> - }
>> -
>> spin_lock_irqsave(&bank->lock, flags);
>> - __raw_writel(0xffffffff, wake_clear);
>> - __raw_writel(bank->saved_wakeup, wake_set);
>> + __raw_writel(0xffffffff, bank->wake_clear);
>> + __raw_writel(bank->saved_wakeup, bank->wake_set);
>> spin_unlock_irqrestore(&bank->lock, flags);
>> }
>
> In addition, the cpu_is_* checks in the suspend/resume functions could
> be replaced by checking for non-zero values in bank->regs->wkup*
This is taken care in a later patch. In our next series, we will take care
about the patch order too with cleanup taken care in a separately and
later any functionality change/fixes.
-V Charulatha
>
> Kevin
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
