OMAP: GPIO: Cleanup OMAP GPIO driver

From: Charulatha V <[email protected]>

Remove cpu-is checks while enabling/disabling OMAP GPIO module during a gpio request/free.

Signed-off-by Charulatha V <[email protected]>

---
 arch/arm/mach-omap1/gpio15xx.c         |    2 +
 arch/arm/mach-omap1/gpio16xx.c         |    2 +
 arch/arm/mach-omap1/gpio7xx.c          |    2 +
 arch/arm/mach-omap2/gpio.c             |    2 +
 arch/arm/plat-omap/include/plat/gpio.h |    1 +
 drivers/gpio/gpio_omap.c               |   53 ++++++++++++++------------------
 6 files changed, 32 insertions(+), 30 deletions(-)

arch/arm/mach-omap1/gpio15xx.c +2 -0

index f79c6ae..6d83e0a 100644
@@ -42,6 +42,7 @@ static struct omap_gpio_reg_offs omap15xx_mpuio_regs = {
 	.irqstatus	= OMAP_MPUIO_GPIO_INT,
 	.irqenable	= OMAP_MPUIO_GPIO_MASKIT,
 	.irqenable_inv	= true,
+	.ctrl		= USHRT_MAX,
 };
 
 static struct __initdata omap_gpio_platform_data omap15xx_mpu_gpio_config = {

@@ -83,6 +84,7 @@ static struct omap_gpio_reg_offs omap15xx_gpio_regs = {
 	.irqstatus	= OMAP1510_GPIO_INT_STATUS,
 	.irqenable	= OMAP1510_GPIO_INT_MASK,
 	.irqenable_inv	= true,
+	.ctrl		= USHRT_MAX,
 };
 
 static struct __initdata omap_gpio_platform_data omap15xx_gpio_config = {

arch/arm/mach-omap1/gpio16xx.c +2 -0

index c69b3b1..6bba196 100644
@@ -45,6 +45,7 @@ static struct omap_gpio_reg_offs omap16xx_mpuio_regs = {
 	.irqstatus	= OMAP_MPUIO_GPIO_INT,
 	.irqenable	= OMAP_MPUIO_GPIO_MASKIT,
 	.irqenable_inv	= true,
+	.ctrl		= USHRT_MAX,
 };
 
 static struct __initdata omap_gpio_platform_data omap16xx_mpu_gpio_config = {

@@ -89,6 +90,7 @@ static struct omap_gpio_reg_offs omap16xx_gpio_regs = {
 	.irqenable	= OMAP1610_GPIO_IRQENABLE1,
 	.set_irqenable	= OMAP1610_GPIO_SET_IRQENABLE1,
 	.clr_irqenable	= OMAP1610_GPIO_CLEAR_IRQENABLE1,
+	.ctrl		= USHRT_MAX,
 };
 
 static struct __initdata omap_gpio_platform_data omap16xx_gpio1_config = {

arch/arm/mach-omap1/gpio7xx.c +2 -0

index d7f2ad3..0fc2557 100644
@@ -47,6 +47,7 @@ static struct omap_gpio_reg_offs omap7xx_mpuio_regs = {
 	.irqstatus	= OMAP_MPUIO_GPIO_INT / 2,
 	.irqenable	= OMAP_MPUIO_GPIO_MASKIT / 2,
 	.irqenable_inv	= true,
+	.ctrl		= USHRT_MAX,
 };
 
 static struct __initdata omap_gpio_platform_data omap7xx_mpu_gpio_config = {

@@ -88,6 +89,7 @@ static struct omap_gpio_reg_offs omap7xx_gpio_regs = {
 	.irqstatus	= OMAP7XX_GPIO_INT_STATUS,
 	.irqenable	= OMAP7XX_GPIO_INT_MASK,
 	.irqenable_inv	= true,
+	.ctrl		= USHRT_MAX,
 };
 
 static struct __initdata omap_gpio_platform_data omap7xx_gpio1_config = {

arch/arm/mach-omap2/gpio.c +2 -0

index 9a46d77..0446bd1 100644
@@ -84,6 +84,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 		pdata->regs->clr_irqenable = OMAP24XX_GPIO_CLEARIRQENABLE1;
 		pdata->regs->debounce = OMAP24XX_GPIO_DEBOUNCE_VAL;
 		pdata->regs->debounce_en = OMAP24XX_GPIO_DEBOUNCE_EN;
+		pdata->regs->ctrl = OMAP24XX_GPIO_CTRL;
 		break;
 	case 2:
 		pdata->bank_type = METHOD_GPIO_44XX;

@@ -100,6 +101,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 		pdata->regs->clr_irqenable = OMAP4_GPIO_IRQSTATUSCLR0;
 		pdata->regs->debounce = OMAP4_GPIO_DEBOUNCINGTIME;
 		pdata->regs->debounce_en = OMAP4_GPIO_DEBOUNCENABLE;
+		pdata->regs->ctrl = OMAP4_GPIO_CTRL;
 		break;
 	default:
 		WARN(1, "Invalid gpio bank_type\n");

arch/arm/plat-omap/include/plat/gpio.h +1 -0

index 91e8de3..caf432c 100644
@@ -188,6 +188,7 @@ struct omap_gpio_reg_offs {
 	u16 clr_irqenable;
 	u16 debounce;
 	u16 debounce_en;
+	u16 ctrl;
 
 	bool irqenable_inv;
 };

drivers/gpio/gpio_omap.c +23 -30

index a490cb1..dfdc45e 100644
@@ -89,6 +89,7 @@ int gpio_bank_count;
 
 #define GPIO_INDEX(bank, gpio) (gpio % bank->width)
 #define GPIO_BIT(bank, gpio) (1 << GPIO_INDEX(bank, gpio))
+#define GPIO_MOD_CTRL_BIT	BIT(0)
 
 static void _set_gpio_direction(struct gpio_bank *bank, int gpio, int is_input)
 {

@@ -586,22 +587,18 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 		__raw_writel(__raw_readl(reg) | (1 << offset), reg);
 	}
 #endif
-	if (!cpu_class_is_omap1()) {
-		if (!bank->mod_usage) {
-			void __iomem *reg = bank->base;
-			u32 ctrl;
-
-			if (cpu_is_omap24xx() || cpu_is_omap34xx())
-				reg += OMAP24XX_GPIO_CTRL;
-			else if (cpu_is_omap44xx())
-				reg += OMAP4_GPIO_CTRL;
-			ctrl = __raw_readl(reg);
-			/* Module is enabled, clocks are not gated */
-			ctrl &= 0xFFFFFFFE;
-			__raw_writel(ctrl, reg);
-		}
-		bank->mod_usage |= 1 << offset;
+	if ((bank->regs->ctrl != USHRT_MAX) && (!bank->mod_usage)) {
+		void __iomem *reg = bank->base + bank->regs->ctrl;
+		u32 ctrl;
+
+		ctrl = __raw_readl(reg);
+		/* Module is enabled, clocks are not gated */
+		ctrl &= ~GPIO_MOD_CTRL_BIT;
+		__raw_writel(ctrl, reg);
 	}
+
+	bank->mod_usage |= 1 << offset;
+
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;

@@ -634,22 +631,18 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 		__raw_writel(1 << offset, reg);
 	}
 #endif
-	if (!cpu_class_is_omap1()) {
-		bank->mod_usage &= ~(1 << offset);
-		if (!bank->mod_usage) {
-			void __iomem *reg = bank->base;
-			u32 ctrl;
-
-			if (cpu_is_omap24xx() || cpu_is_omap34xx())
-				reg += OMAP24XX_GPIO_CTRL;
-			else if (cpu_is_omap44xx())
-				reg += OMAP4_GPIO_CTRL;
-			ctrl = __raw_readl(reg);
-			/* Module is disabled, clocks are gated */
-			ctrl |= 1;
-			__raw_writel(ctrl, reg);
-		}
+	bank->mod_usage &= ~(1 << offset);
+
+	if ((bank->regs->ctrl != USHRT_MAX) && (!bank->mod_usage)) {
+		void __iomem *reg = bank->base + bank->regs->ctrl;
+		u32 ctrl;
+
+		ctrl = __raw_readl(reg);
+		/* Module is disabled, clocks are gated */
+		ctrl |= GPIO_MOD_CTRL_BIT;
+		__raw_writel(ctrl, reg);
 	}
+
 	_reset_gpio(bank, bank->chip.base + offset);
 	spin_unlock_irqrestore(&bank->lock, flags);
 }

Quoted 5 lines · Tarun Kanti DebBarma

From: Charulatha V <[email protected]>

Remove cpu-is checks while enabling/disabling OMAP GPIO module during a gpio request/free.

Quoted 1 line

Signed-off-by Charulatha V <[email protected]>

This looks mostly OK, but one nitpick about the usage of USHRT_MAX.

For most registers, you should just test for a non-zero register offset to determine if it's present or not instead of USHRT_MAX.

I used USHRT_MAX for the revision register because it is at offset zero on most SoCs, so testing for non-zero wouldn't work there.

Other than that, the approach looks fine.

Thanks,

Kevin

Quoted 181 lines

---

?arch/arm/mach-omap1/gpio15xx.c ? ? ? ? | ? ?2 +
?arch/arm/mach-omap1/gpio16xx.c ? ? ? ? | ? ?2 +
?arch/arm/mach-omap1/gpio7xx.c ? ? ? ? ?| ? ?2 +
?arch/arm/mach-omap2/gpio.c ? ? ? ? ? ? | ? ?2 +
?arch/arm/plat-omap/include/plat/gpio.h | ? ?1 +
?drivers/gpio/gpio_omap.c ? ? ? ? ? ? ? | ? 53 ++++++++++++++------------------

?6 files changed, 32 insertions(+), 30 deletions(-)

arch/arm/mach-omap1/gpio15xx.c +2 -0

index f79c6ae..6d83e0a 100644
@@ -42,6 +42,7 @@ static struct omap_gpio_reg_offs omap15xx_mpuio_regs = {
 ? ? ? .irqstatus ? ? ?= OMAP_MPUIO_GPIO_INT,
 ? ? ? .irqenable ? ? ?= OMAP_MPUIO_GPIO_MASKIT,
 ? ? ? .irqenable_inv ?= true,
+ ? ? .ctrl ? ? ? ? ? = USHRT_MAX,
 ?};
 
 ?static struct __initdata omap_gpio_platform_data omap15xx_mpu_gpio_config = {

@@ -83,6 +84,7 @@ static struct omap_gpio_reg_offs omap15xx_gpio_regs = {
 ? ? ? .irqstatus ? ? ?= OMAP1510_GPIO_INT_STATUS,
 ? ? ? .irqenable ? ? ?= OMAP1510_GPIO_INT_MASK,
 ? ? ? .irqenable_inv ?= true,
+ ? ? .ctrl ? ? ? ? ? = USHRT_MAX,
 ?};
 
 ?static struct __initdata omap_gpio_platform_data omap15xx_gpio_config = {

arch/arm/mach-omap1/gpio16xx.c +2 -0

index c69b3b1..6bba196 100644
@@ -45,6 +45,7 @@ static struct omap_gpio_reg_offs omap16xx_mpuio_regs = {
 ? ? ? .irqstatus ? ? ?= OMAP_MPUIO_GPIO_INT,
 ? ? ? .irqenable ? ? ?= OMAP_MPUIO_GPIO_MASKIT,
 ? ? ? .irqenable_inv ?= true,
+ ? ? .ctrl ? ? ? ? ? = USHRT_MAX,
 ?};
 
 ?static struct __initdata omap_gpio_platform_data omap16xx_mpu_gpio_config = {

@@ -89,6 +90,7 @@ static struct omap_gpio_reg_offs omap16xx_gpio_regs = {
 ? ? ? .irqenable ? ? ?= OMAP1610_GPIO_IRQENABLE1,
 ? ? ? .set_irqenable ?= OMAP1610_GPIO_SET_IRQENABLE1,
 ? ? ? .clr_irqenable ?= OMAP1610_GPIO_CLEAR_IRQENABLE1,
+ ? ? .ctrl ? ? ? ? ? = USHRT_MAX,
 ?};
 
 ?static struct __initdata omap_gpio_platform_data omap16xx_gpio1_config = {

arch/arm/mach-omap1/gpio7xx.c +2 -0

index d7f2ad3..0fc2557 100644
@@ -47,6 +47,7 @@ static struct omap_gpio_reg_offs omap7xx_mpuio_regs = {
 ? ? ? .irqstatus ? ? ?= OMAP_MPUIO_GPIO_INT / 2,
 ? ? ? .irqenable ? ? ?= OMAP_MPUIO_GPIO_MASKIT / 2,
 ? ? ? .irqenable_inv ?= true,
+ ? ? .ctrl ? ? ? ? ? = USHRT_MAX,
 ?};
 
 ?static struct __initdata omap_gpio_platform_data omap7xx_mpu_gpio_config = {

@@ -88,6 +89,7 @@ static struct omap_gpio_reg_offs omap7xx_gpio_regs = {
 ? ? ? .irqstatus ? ? ?= OMAP7XX_GPIO_INT_STATUS,
 ? ? ? .irqenable ? ? ?= OMAP7XX_GPIO_INT_MASK,
 ? ? ? .irqenable_inv ?= true,
+ ? ? .ctrl ? ? ? ? ? = USHRT_MAX,
 ?};
 
 ?static struct __initdata omap_gpio_platform_data omap7xx_gpio1_config = {

arch/arm/mach-omap2/gpio.c +1 -0

index 9a46d77..0446bd1 100644
@@ -84,6 +84,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 ? ? ? ? ? ? ? pdata->regs->clr_irqenable = OMAP24XX_GPIO_CLEARIRQENABLE1;
 ? ? ? ? ? ? ? pdata->regs->debounce = OMAP24XX_GPIO_DEBOUNCE_VAL;
 ? ? ? ? ? ? ? pdata->regs->debounce_en = OMAP24XX_GPIO_DEBOUNCE_EN;
+ ? ? ? ? ? ? pdata->regs->ctrl = OMAP24XX_GPIO_CTRL;
 ? ? ? ? ? ? ? break;

? ? ? case 2:
? ? ? ? ? ? ? pdata->bank_type = METHOD_GPIO_44XX;

arch/arm/mach-omap2/gpio.c +1 -0

@@ -100,6 +101,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 ? ? ? ? ? ? ? pdata->regs->clr_irqenable = OMAP4_GPIO_IRQSTATUSCLR0;
 ? ? ? ? ? ? ? pdata->regs->debounce = OMAP4_GPIO_DEBOUNCINGTIME;
 ? ? ? ? ? ? ? pdata->regs->debounce_en = OMAP4_GPIO_DEBOUNCENABLE;
+ ? ? ? ? ? ? pdata->regs->ctrl = OMAP4_GPIO_CTRL;
 ? ? ? ? ? ? ? break;

? ? ? default:
? ? ? ? ? ? ? WARN(1, "Invalid gpio bank_type\n");

arch/arm/plat-omap/include/plat/gpio.h +1 -0

index 91e8de3..caf432c 100644
@@ -188,6 +188,7 @@ struct omap_gpio_reg_offs {
 ? ? ? u16 clr_irqenable;
 ? ? ? u16 debounce;
 ? ? ? u16 debounce_en;
+ ? ? u16 ctrl;
 
 ? ? ? bool irqenable_inv;
 ?};

drivers/gpio/gpio_omap.c +1 -0

index a490cb1..dfdc45e 100644
@@ -89,6 +89,7 @@ int gpio_bank_count;
 
 ?#define GPIO_INDEX(bank, gpio) (gpio % bank->width)
 ?#define GPIO_BIT(bank, gpio) (1 << GPIO_INDEX(bank, gpio))
+#define GPIO_MOD_CTRL_BIT ? ?BIT(0)
 
 ?static void _set_gpio_direction(struct gpio_bank *bank, int gpio, int is_input)
 ?{

@@ -586,22 +587,18 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 ? ? ? ? ? ? ? __raw_writel(__raw_readl(reg) | (1 << offset), reg);
 ? ? ? }

?#endif
- ? ? if (!cpu_class_is_omap1()) {
- ? ? ? ? ? ? if (!bank->mod_usage) {
- ? ? ? ? ? ? ? ? ? ? void __iomem *reg = bank->base;
- ? ? ? ? ? ? ? ? ? ? u32 ctrl;

drivers/gpio/gpio_omap.c +8 -11

-
- ? ? ? ? ? ? ? ? ? ? if (cpu_is_omap24xx() || cpu_is_omap34xx())
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? reg += OMAP24XX_GPIO_CTRL;
- ? ? ? ? ? ? ? ? ? ? else if (cpu_is_omap44xx())
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? reg += OMAP4_GPIO_CTRL;
- ? ? ? ? ? ? ? ? ? ? ctrl = __raw_readl(reg);
- ? ? ? ? ? ? ? ? ? ? /* Module is enabled, clocks are not gated */
- ? ? ? ? ? ? ? ? ? ? ctrl &= 0xFFFFFFFE;
- ? ? ? ? ? ? ? ? ? ? __raw_writel(ctrl, reg);
- ? ? ? ? ? ? }
- ? ? ? ? ? ? bank->mod_usage |= 1 << offset;
+ ? ? if ((bank->regs->ctrl != USHRT_MAX) && (!bank->mod_usage)) {
+ ? ? ? ? ? ? void __iomem *reg = bank->base + bank->regs->ctrl;
+ ? ? ? ? ? ? u32 ctrl;
+
+ ? ? ? ? ? ? ctrl = __raw_readl(reg);
+ ? ? ? ? ? ? /* Module is enabled, clocks are not gated */
+ ? ? ? ? ? ? ctrl &= ~GPIO_MOD_CTRL_BIT;
+ ? ? ? ? ? ? __raw_writel(ctrl, reg);

? ? ? }

drivers/gpio/gpio_omap.c +3 -0

+
+ ? ? bank->mod_usage |= 1 << offset;
+

? ? ? spin_unlock_irqrestore(&bank->lock, flags);

? ? ? return 0;

drivers/gpio/gpio_omap.c +0 -0

@@ -634,22 +631,18 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 ? ? ? ? ? ? ? __raw_writel(1 << offset, reg);
 ? ? ? }

?#endif
- ? ? if (!cpu_class_is_omap1()) {
- ? ? ? ? ? ? bank->mod_usage &= ~(1 << offset);
- ? ? ? ? ? ? if (!bank->mod_usage) {
- ? ? ? ? ? ? ? ? ? ? void __iomem *reg = bank->base;
- ? ? ? ? ? ? ? ? ? ? u32 ctrl;

drivers/gpio/gpio_omap.c +10 -10

-
- ? ? ? ? ? ? ? ? ? ? if (cpu_is_omap24xx() || cpu_is_omap34xx())
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? reg += OMAP24XX_GPIO_CTRL;
- ? ? ? ? ? ? ? ? ? ? else if (cpu_is_omap44xx())
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? reg += OMAP4_GPIO_CTRL;
- ? ? ? ? ? ? ? ? ? ? ctrl = __raw_readl(reg);
- ? ? ? ? ? ? ? ? ? ? /* Module is disabled, clocks are gated */
- ? ? ? ? ? ? ? ? ? ? ctrl |= 1;
- ? ? ? ? ? ? ? ? ? ? __raw_writel(ctrl, reg);
- ? ? ? ? ? ? }
+ ? ? bank->mod_usage &= ~(1 << offset);
+
+ ? ? if ((bank->regs->ctrl != USHRT_MAX) && (!bank->mod_usage)) {
+ ? ? ? ? ? ? void __iomem *reg = bank->base + bank->regs->ctrl;
+ ? ? ? ? ? ? u32 ctrl;
+
+ ? ? ? ? ? ? ctrl = __raw_readl(reg);
+ ? ? ? ? ? ? /* Module is disabled, clocks are gated */
+ ? ? ? ? ? ? ctrl |= GPIO_MOD_CTRL_BIT;
+ ? ? ? ? ? ? __raw_writel(ctrl, reg);

? ? ? }
+
? ? ? _reset_gpio(bank, bank->chip.base + offset); ? ? ? spin_unlock_irqrestore(&bank->lock, flags); ?}

From: Charulatha V <[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect.

Fix the above by providing non-wakeup GPIO information through pdata specific to the SoC.

The GPIO rev id provided in the hwmod database is the same for OMAP2420 and OMAP2430. Change the GPIO rev ids in hwmod database as given below so that it can be used to identify OMAP2420 and OMAP2430. OMAP2420 - 0
OMAP2430 - 1
OMAP3 - 2
OMAP4 - 3

Signed-off-by Charulatha V <[email protected]>

Cc Cousson, Benoit <[email protected]>

Cc Paul Walmsley <[email protected]>

---
 arch/arm/mach-omap2/gpio.c                 |   26 ++++++++++++++++++++++++--
 arch/arm/mach-omap2/omap_hwmod_2430_data.c |    2 +-
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    2 +-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    2 +-
 arch/arm/plat-omap/include/plat/gpio.h     |    1 +
 drivers/gpio/gpio_omap.c                   |   11 +++--------
 6 files changed, 31 insertions(+), 13 deletions(-)

arch/arm/mach-omap2/gpio.c +24 -2

index 0446bd1..6cd26b4 100644
@@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 		return -ENOMEM;
 	}
 
+	switch (oh->class->rev) {
+	case 0:
+		if (id == 1)
+			/* non-wakeup GPIO pins for OMAP2420 Bank1 */
+			pdata->non_wakeup_gpios = 0xe203ffc0;
+		else if (id == 2)
+			/* non-wakeup GPIO pins for OMAP2420 Bank2 */
+			pdata->non_wakeup_gpios = 0x08700040;
+		break;
+	case 2:
+		if (id == 2)
+			/* non-wakeup GPIO pins for OMAP3 Bank2 */
+			pdata->non_wakeup_gpios = 0x00000001;
+		else if (id == 6)
+			/* non-wakeup GPIO pins for OMAP3 Bank6 */
+			pdata->non_wakeup_gpios = 0x08000000;
+		break;
+	default:
+		/* No non-wakeup GPIO pins for other SoCs */
+		break;
+	}
+
 	dev_attr = (struct omap_gpio_dev_attr *)oh->dev_attr;
 	pdata->bank_width = dev_attr->bank_width;
 	pdata->dbck_flag = dev_attr->dbck_flag;

@@ -70,6 +92,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 	switch (oh->class->rev) {
 	case 0:
 	case 1:
+	case 2:
 		pdata->bank_type = METHOD_GPIO_24XX;
 		pdata->regs->revision = OMAP24XX_GPIO_REVISION;
 		pdata->regs->direction = OMAP24XX_GPIO_OE;

@@ -86,7 +109,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 		pdata->regs->debounce_en = OMAP24XX_GPIO_DEBOUNCE_EN;
 		pdata->regs->ctrl = OMAP24XX_GPIO_CTRL;
 		break;
-	case 2:
+	case 3:
 		pdata->bank_type = METHOD_GPIO_44XX;
 		pdata->regs->revision = OMAP4_GPIO_REVISION;
 		pdata->regs->direction = OMAP4_GPIO_OE;

@@ -108,7 +131,6 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 		kfree(pdata);
 		return -EINVAL;
 	}
-
 	od = omap_device_build(name, id - 1, oh, pdata,
 				sizeof(*pdata),	omap_gpio_latency,
 				ARRAY_SIZE(omap_gpio_latency),

arch/arm/mach-omap2/omap_hwmod_2430_data.c +1 -1

index 9682dd5..ae702b5 100644
@@ -1728,7 +1728,7 @@ static struct omap_hwmod_class_sysconfig omap243x_gpio_sysc = {
 static struct omap_hwmod_class omap243x_gpio_hwmod_class = {
 	.name = "gpio",
 	.sysc = &omap243x_gpio_sysc,
-	.rev = 0,
+	.rev = 1,
 };
 
 /* gpio1 */

arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +1 -1

index 909a84d..05e7005 100644
@@ -2117,7 +2117,7 @@ static struct omap_hwmod_class_sysconfig omap3xxx_gpio_sysc = {
 static struct omap_hwmod_class omap3xxx_gpio_hwmod_class = {
 	.name = "gpio",
 	.sysc = &omap3xxx_gpio_sysc,
-	.rev = 1,
+	.rev = 2,
 };
 
 /* gpio_dev_attr*/

arch/arm/mach-omap2/omap_hwmod_44xx_data.c +1 -1

index abc548a..ea30752 100644
@@ -1703,7 +1703,7 @@ static struct omap_hwmod_class_sysconfig omap44xx_gpio_sysc = {
 static struct omap_hwmod_class omap44xx_gpio_hwmod_class = {
 	.name	= "gpio",
 	.sysc	= &omap44xx_gpio_sysc,
-	.rev	= 2,
+	.rev	= 3,
 };
 
 /* gpio dev_attr */

arch/arm/plat-omap/include/plat/gpio.h +1 -0

index caf432c..8014a8a 100644
@@ -199,6 +199,7 @@ struct omap_gpio_platform_data {
 	int bank_width;		/* GPIO bank width */
 	int bank_stride;	/* Only needed for omap1 MPUIO */
 	bool dbck_flag;		/* dbck required or not - True for OMAP3&4 */
+	u32 non_wakeup_gpios;
 
 	struct omap_gpio_reg_offs *regs;
 };

drivers/gpio/gpio_omap.c +3 -8

index dfdc45e..0ba4cdb 100644
@@ -1008,7 +1008,7 @@ static inline int init_gpio_info(struct platform_device *pdev)
 }
 
 /* TODO: Cleanup cpu_is_* checks */
-static void omap_gpio_mod_init(struct gpio_bank *bank, int id)
+static void omap_gpio_mod_init(struct gpio_bank *bank)
 {
 	if (cpu_class_is_omap2()) {
 		if (cpu_is_omap44xx()) {

@@ -1028,12 +1028,6 @@ static void omap_gpio_mod_init(struct gpio_bank *bank, int id)
 
 			/* Initialize interface clk ungated, module enabled */
 			__raw_writel(0, bank->base + OMAP24XX_GPIO_CTRL);
-		} else if (cpu_is_omap24xx()) {
-			static const u32 non_wakeup_gpios[] = {
-				0xe203ffc0, 0x08700040
-			};
-			if (id < ARRAY_SIZE(non_wakeup_gpios))
-				bank->non_wakeup_gpios = non_wakeup_gpios[id];
 		}
 	} else if (cpu_class_is_omap1()) {
 		if (bank_is_mpuio(bank))

@@ -1179,6 +1173,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 	bank->dbck_flag = pdata->dbck_flag;
 	bank->stride = pdata->bank_stride;
 	bank->width = pdata->bank_width;
+	bank->non_wakeup_gpios = pdata->non_wakeup_gpios;
 
 	bank->regs = pdata->regs;
 

@@ -1205,7 +1200,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 	pm_runtime_enable(bank->dev);
 	pm_runtime_get_sync(bank->dev);
 
-	omap_gpio_mod_init(bank, id);
+	omap_gpio_mod_init(bank);
 	omap_gpio_chip_init(bank);
 	omap_gpio_show_rev(bank);

Quoted 5 lines · Tarun Kanti DebBarma

From: Charulatha V <[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect.

Can you cite the documentation you're using for the OMAP3 non-wakeup GPIOs?

This is a change of functionality from current code, where all OMAP3 GPIOs are considered wakeup capable.

I'd like this to be tackled in two patches. ?One for the cleanup/consolidation, and one for change in behavior.

For this cleanup/consolidation (this series), please keep existing functionality and focus on the cleanup. ?Then, in an additional patch (on top of the cleanup/conslidation), change the functionality with a detailed changelog.

Thanks,

Kevin

Quoted 174 lines

Fix the above by providing non-wakeup GPIO information through pdata specific to the SoC.

The GPIO rev id provided in the hwmod database is the same for OMAP2420 and OMAP2430. Change the GPIO rev ids in hwmod database as given below so that it can be used to identify OMAP2420 and OMAP2430. OMAP2420 - 0
OMAP2430 - 1
OMAP3 ? ?- 2
OMAP4 ? ?- 3

Signed-off-by Charulatha V <[email protected]>

Cc Cousson, Benoit <[email protected]>

Cc Paul Walmsley <[email protected]>

---

?arch/arm/mach-omap2/gpio.c ? ? ? ? ? ? ? ? | ? 26 ++++++++++++++++++++++++--
?arch/arm/mach-omap2/omap_hwmod_2430_data.c | ? ?2 +-
?arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | ? ?2 +-
?arch/arm/mach-omap2/omap_hwmod_44xx_data.c | ? ?2 +-
?arch/arm/plat-omap/include/plat/gpio.h ? ? | ? ?1 +
?drivers/gpio/gpio_omap.c ? ? ? ? ? ? ? ? ? | ? 11 +++--------

?6 files changed, 31 insertions(+), 13 deletions(-)

arch/arm/mach-omap2/gpio.c +22 -0

index 0446bd1..6cd26b4 100644
@@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 ? ? ? ? ? ? ? return -ENOMEM;
 ? ? ? }
 
+ ? ? switch (oh->class->rev) {
+ ? ? case 0:
+ ? ? ? ? ? ? if (id == 1)
+ ? ? ? ? ? ? ? ? ? ? /* non-wakeup GPIO pins for OMAP2420 Bank1 */
+ ? ? ? ? ? ? ? ? ? ? pdata->non_wakeup_gpios = 0xe203ffc0;
+ ? ? ? ? ? ? else if (id == 2)
+ ? ? ? ? ? ? ? ? ? ? /* non-wakeup GPIO pins for OMAP2420 Bank2 */
+ ? ? ? ? ? ? ? ? ? ? pdata->non_wakeup_gpios = 0x08700040;
+ ? ? ? ? ? ? break;
+ ? ? case 2:
+ ? ? ? ? ? ? if (id == 2)
+ ? ? ? ? ? ? ? ? ? ? /* non-wakeup GPIO pins for OMAP3 Bank2 */
+ ? ? ? ? ? ? ? ? ? ? pdata->non_wakeup_gpios = 0x00000001;
+ ? ? ? ? ? ? else if (id == 6)
+ ? ? ? ? ? ? ? ? ? ? /* non-wakeup GPIO pins for OMAP3 Bank6 */
+ ? ? ? ? ? ? ? ? ? ? pdata->non_wakeup_gpios = 0x08000000;
+ ? ? ? ? ? ? break;
+ ? ? default:
+ ? ? ? ? ? ? /* No non-wakeup GPIO pins for other SoCs */
+ ? ? ? ? ? ? break;
+ ? ? }
+
 ? ? ? dev_attr = (struct omap_gpio_dev_attr *)oh->dev_attr;
 ? ? ? pdata->bank_width = dev_attr->bank_width;
 ? ? ? pdata->dbck_flag = dev_attr->dbck_flag;

@@ -70,6 +92,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 ? ? ? switch (oh->class->rev) {

? ? ? case 0:
? ? ? case 1:
+ ? ? case 2:
? ? ? ? ? ? ? pdata->bank_type = METHOD_GPIO_24XX; ? ? ? ? ? ? ? pdata->regs->revision = OMAP24XX_GPIO_REVISION; ? ? ? ? ? ? ? pdata->regs->direction = OMAP24XX_GPIO_OE;

arch/arm/mach-omap2/gpio.c +1 -2

@@ -86,7 +109,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 ? ? ? ? ? ? ? pdata->regs->debounce_en = OMAP24XX_GPIO_DEBOUNCE_EN;
 ? ? ? ? ? ? ? pdata->regs->ctrl = OMAP24XX_GPIO_CTRL;
 ? ? ? ? ? ? ? break;
- ? ? case 2:
+ ? ? case 3:
 ? ? ? ? ? ? ? pdata->bank_type = METHOD_GPIO_44XX;
 ? ? ? ? ? ? ? pdata->regs->revision = OMAP4_GPIO_REVISION;
 ? ? ? ? ? ? ? pdata->regs->direction = OMAP4_GPIO_OE;

@@ -108,7 +131,6 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
 ? ? ? ? ? ? ? kfree(pdata);
 ? ? ? ? ? ? ? return -EINVAL;
 ? ? ? }
-
 ? ? ? od = omap_device_build(name, id - 1, oh, pdata,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof(*pdata), omap_gpio_latency,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ARRAY_SIZE(omap_gpio_latency),

arch/arm/mach-omap2/omap_hwmod_2430_data.c +1 -1

index 9682dd5..ae702b5 100644
@@ -1728,7 +1728,7 @@ static struct omap_hwmod_class_sysconfig omap243x_gpio_sysc = {
 ?static struct omap_hwmod_class omap243x_gpio_hwmod_class = {
 ? ? ? .name = "gpio",
 ? ? ? .sysc = &omap243x_gpio_sysc,
- ? ? .rev = 0,
+ ? ? .rev = 1,
 ?};
 

?/* gpio1 */

arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +1 -1

index 909a84d..05e7005 100644
@@ -2117,7 +2117,7 @@ static struct omap_hwmod_class_sysconfig omap3xxx_gpio_sysc = {
 ?static struct omap_hwmod_class omap3xxx_gpio_hwmod_class = {
 ? ? ? .name = "gpio",
 ? ? ? .sysc = &omap3xxx_gpio_sysc,
- ? ? .rev = 1,
+ ? ? .rev = 2,
 ?};
 

?/* gpio_dev_attr*/

arch/arm/mach-omap2/omap_hwmod_44xx_data.c +1 -1

index abc548a..ea30752 100644
@@ -1703,7 +1703,7 @@ static struct omap_hwmod_class_sysconfig omap44xx_gpio_sysc = {
 ?static struct omap_hwmod_class omap44xx_gpio_hwmod_class = {
 ? ? ? .name ? = "gpio",
 ? ? ? .sysc ? = &omap44xx_gpio_sysc,
- ? ? .rev ? ?= 2,
+ ? ? .rev ? ?= 3,
 ?};
 

?/* gpio dev_attr */

arch/arm/plat-omap/include/plat/gpio.h +0 -0

index caf432c..8014a8a 100644
@@ -199,6 +199,7 @@ struct omap_gpio_platform_data {

? ? ? int bank_width; ? ? ? ? /* GPIO bank width */
? ? ? int bank_stride; ? ? ? ?/* Only needed for omap1 MPUIO */ ? ? ? bool dbck_flag; ? ? ? ? /* dbck required or not - True for OMAP3&4 */
+ ? ? u32 non_wakeup_gpios;

? ? ? struct omap_gpio_reg_offs *regs;
?};

drivers/gpio/gpio_omap.c +0 -0

index dfdc45e..0ba4cdb 100644
@@ -1008,7 +1008,7 @@ static inline int init_gpio_info(struct platform_device *pdev)
 ?}
 

?/* TODO: Cleanup cpu_is_* checks */

drivers/gpio/gpio_omap.c +1 -1

-static void omap_gpio_mod_init(struct gpio_bank *bank, int id)
+static void omap_gpio_mod_init(struct gpio_bank *bank)

?{
? ? ? if (cpu_class_is_omap2()) {
? ? ? ? ? ? ? if (cpu_is_omap44xx()) {

drivers/gpio/gpio_omap.c +0 -0

@@ -1028,12 +1028,6 @@ static void omap_gpio_mod_init(struct gpio_bank *bank, int id)
 

? ? ? ? ? ? ? ? ? ? ? /* Initialize interface clk ungated, module enabled */ ? ? ? ? ? ? ? ? ? ? ? __raw_writel(0, bank->base + OMAP24XX_GPIO_CTRL);
- ? ? ? ? ? ? } else if (cpu_is_omap24xx()) {
- ? ? ? ? ? ? ? ? ? ? static const u32 non_wakeup_gpios[] = {
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0xe203ffc0, 0x08700040
- ? ? ? ? ? ? ? ? ? ? };
- ? ? ? ? ? ? ? ? ? ? if (id < ARRAY_SIZE(non_wakeup_gpios))
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->non_wakeup_gpios = non_wakeup_gpios[id]; ? ? ? ? ? ? ? }
? ? ? } else if (cpu_class_is_omap1()) {
? ? ? ? ? ? ? if (bank_is_mpuio(bank))

drivers/gpio/gpio_omap.c +2 -1

@@ -1179,6 +1173,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 ? ? ? bank->dbck_flag = pdata->dbck_flag;
 ? ? ? bank->stride = pdata->bank_stride;
 ? ? ? bank->width = pdata->bank_width;
+ ? ? bank->non_wakeup_gpios = pdata->non_wakeup_gpios;
 
 ? ? ? bank->regs = pdata->regs;
 

@@ -1205,7 +1200,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 ? ? ? pm_runtime_enable(bank->dev);
 ? ? ? pm_runtime_get_sync(bank->dev);
 
- ? ? omap_gpio_mod_init(bank, id);
+ ? ? omap_gpio_mod_init(bank);
 ? ? ? omap_gpio_chip_init(bank);
 ? ? ? omap_gpio_show_rev(bank);

Quoted 8 lines · Kevin Hilman

Quoted 5 lines · Tarun Kanti DebBarma

From: Charulatha V <[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect.

Quoted 2 lines

Can you cite the documentation you're using for the OMAP3 non-wakeup GPIOs?

I am referring to Table 25-5. GPIO Channel Description at page no 3354 OMAP3430 public TRM Version P.

Quoted 12 lines · Charulatha

Quoted 10 lines · Kevin Hilman

Quoted 5 lines · Tarun Kanti DebBarma

From: Charulatha V <[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect.

Can you cite the documentation you're using for the OMAP3 non-wakeup GPIOs?

Quoted 3 lines

I am referring to Table 25-5. GPIO Channel Description at page no 3354 OMAP3430 public TRM Version P.

Based on how I understand that, those non-wakeup GPIOs are non-wakeup because they're not actually mapped anywhere, and not available on external pins. ?Therefore, they should not need any special case handling for this triggering workaround.

Kevin

Kevin,

Quoted 18 lines · Kevin Hilman

Quoted 15 lines · Charulatha

Quoted 10 lines · Kevin Hilman

Quoted 5 lines · Tarun Kanti DebBarma

From: Charulatha V <[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect.

Can you cite the documentation you're using for the OMAP3 non-wakeup GPIOs?

I am referring to Table 25-5. GPIO Channel Description at page no 3354 OMAP3430 public TRM Version P.

Quoted 4 lines

Based on how I understand that, those non-wakeup GPIOs are non-wakeup because they're not actually mapped anywhere, and not available on external pins. ?Therefore, they should not need any special case handling for this triggering workaround.

Okay. Thanks for clarifying. I will not do this for OMAP3. The special case would be only for OMAP2420 against the current code which is handling for OMAP24xx.

-----Original Message-----
From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti
Sent: Tuesday, May 24, 2011 7:55 PM
To: linux-omap at vger.kernel.org
Cc: Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha; Cousson, Benoit; Paul Walmsley
Subject: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids

From: Charulatha V <[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect.

Fix the above by providing non-wakeup GPIO information through pdata specific to the SoC.

The GPIO rev id provided in the hwmod database is the same for OMAP2420
and OMAP2430. Change the GPIO rev ids in hwmod database as given below so that it can be used to identify OMAP2420 and OMAP2430. OMAP2420 - 0
OMAP2430 - 1
OMAP3 - 2
OMAP4 - 3

Signed-off-by Charulatha V <[email protected]>

Cc Cousson, Benoit <[email protected]>

Cc Paul Walmsley <[email protected]>

---
 arch/arm/mach-omap2/gpio.c                 |   26 
++++++++++++++++++++++++--
 arch/arm/mach-omap2/omap_hwmod_2430_data.c |    2 +-
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    2 +-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    2 +-
 arch/arm/plat-omap/include/plat/gpio.h     |    1 +
 drivers/gpio/gpio_omap.c                   |   11 +++--------
 6 files changed, 31 insertions(+), 13 deletions(-)

arch/arm/mach-omap2/gpio.c +7 -0

index 0446bd1..6cd26b4 100644
@@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct 
 omap_hwmod *oh, void *unused)
 		return -ENOMEM;
 	}
 
+	switch (oh->class->rev) {
+	case 0:
+		if (id == 1)
+			/* non-wakeup GPIO pins for OMAP2420 Bank1 */
+			pdata->non_wakeup_gpios = 0xe203ffc0;
+		else if (id == 2)
+			/* non-wakeup GPIO pins for OMAP2420 Bank2 */

Quoted 23 lines

-----Original Message-----
From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti
Sent: Tuesday, May 24, 2011 7:55 PM
To: linux-omap at vger.kernel.org
Cc: Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha; Cousson, Benoit; Paul Walmsley
Subject: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids

From: Charulatha V <[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect.

Fix the above by providing non-wakeup GPIO information through pdata specific to the SoC.

Quoted 8 lines

The GPIO rev id provided in the hwmod database is the same for OMAP2420
and OMAP2430. Change the GPIO rev ids in hwmod database as given below so that it can be used to identify OMAP2420 and OMAP2430. OMAP2420 - 0
OMAP2430 - 1
OMAP3 ? ?- 2
OMAP4 ? ?- 3

[sp] Magic numbers should be avoided.
? ? Suggest using something like:
? ? #define GPIO_REV_2420 ? ? ?0
? ? #define GPIO_REV_2430 ? ? ?1
? ? #define GPIO_REV_34XX ? ? ?2
? ? #define GPIO_REV_44xx ? ? ?3

? ? We don't have to refer back to this comment while reading the code.

? ? I also believed that HWMODs were auto generated.
? ? Can the changes to structures in this patch recreated using current ? ? scripts?

~sanjeev

Quoted 34 lines

Signed-off-by Charulatha V <[email protected]>

Cc Cousson, Benoit <[email protected]>

Cc Paul Walmsley <[email protected]>

---
?arch/arm/mach-omap2/gpio.c ? ? ? ? ? ? ? ? | ? 26
++++++++++++++++++++++++--

?arch/arm/mach-omap2/omap_hwmod_2430_data.c | ? ?2 +-
?arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | ? ?2 +-
?arch/arm/mach-omap2/omap_hwmod_44xx_data.c | ? ?2 +-
?arch/arm/plat-omap/include/plat/gpio.h ? ? | ? ?1 +
?drivers/gpio/gpio_omap.c ? ? ? ? ? ? ? ? ? | ? 11 +++--------

?6 files changed, 31 insertions(+), 13 deletions(-)

arch/arm/mach-omap2/gpio.c +11 -0

index 0446bd1..6cd26b4 100644
@@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct
 omap_hwmod *oh, void *unused)
 ? ? ? ? ? ? ? return -ENOMEM;
 ? ? ? }
 
+ ? ? switch (oh->class->rev) {
+ ? ? case 0:
+ ? ? ? ? ? ? if (id == 1)
+ ? ? ? ? ? ? ? ? ? ? /* non-wakeup GPIO pins for OMAP2420 Bank1 */
+ ? ? ? ? ? ? ? ? ? ? pdata->non_wakeup_gpios = 0xe203ffc0;
+ ? ? ? ? ? ? else if (id == 2)
+ ? ? ? ? ? ? ? ? ? ? /* non-wakeup GPIO pins for OMAP2420 Bank2 */
+ ? ? ? ? ? ? ? ? ? ? pdata->non_wakeup_gpios = 0x08700040;
+ ? ? ? ? ? ? break;
+ ? ? case 2:
+ ? ? ? ? ? ? if (id == 2)

arch/arm/mach-omap2/gpio.c static int omap2_gpio_dev_init(struct · line 76

Quoted 6 lines

arch/arm/mach-omap2/gpio.c +6 -0

+ ? ? ? ? ? ? ? ? ? ? /* non-wakeup GPIO pins for OMAP3 Bank2 */
+ ? ? ? ? ? ? ? ? ? ? pdata->non_wakeup_gpios = 0x00000001;
+ ? ? ? ? ? ? else if (id == 6)
+ ? ? ? ? ? ? ? ? ? ? /* non-wakeup GPIO pins for OMAP3 Bank6 */
+ ? ? ? ? ? ? ? ? ? ? pdata->non_wakeup_gpios = 0x08000000;
+ ? ? ? ? ? ? break;

[sp] Where is the description on non-wakeup GPIOs in OMAP3?

? ? Here is text from AM37x TRM:
? ? [quote ...only relevant text]
? ? Each GPIO module provides 32 dedicated general-purpose pins with input ? ? and output capabilities; .... These pins can be configured for the ? ? following applications:
? ? - Data input (capture)/output (drive)
? ? - Keyboard interface with a debounce cell
? ? - Interrupt generation in ....
? ? - Wake-up request generation in idle mode
? ? [/quote]
? ? Otherwise, what are the GPIO2_WAKEUPENABLE (0x4905 0020) and ? ? GPIO6_WAKEUPENABLE (0x4905 8020) meant for?

arch/arm/mach-omap2/gpio.c static int omap2_gpio_dev_init(struct · line 81

Quoted 5 lines

arch/arm/mach-omap2/gpio.c +5 -0

+ ? ? default:
+ ? ? ? ? ? ? /* No non-wakeup GPIO pins for other SoCs */
+ ? ? ? ? ? ? break;
+ ? ? }
+

~sanjeev

[snip]...[snip]

Quoted 14 lines

From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti
Sent: Tuesday, May 24, 2011 7:55 PM

From: Charulatha V<[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx (bothe OMAP 2420 and 2430)& not for OMAP3 which is incorrect.

Fix the above by providing non-wakeup GPIO information through pdata specific to the SoC.

Quoted 8 lines

The GPIO rev id provided in the hwmod database is the same for OMAP2420
and OMAP2430. Change the GPIO rev ids in hwmod database as given below so that it can be used to identify OMAP2420 and OMAP2430. OMAP2420 - 0
OMAP2430 - 1
OMAP3 - 2
OMAP4 - 3

[sp] Magic numbers should be avoided.

      Suggest using something like:
      #define GPIO_REV_2420	0
      #define GPIO_REV_2430	1
      #define GPIO_REV_34XX	2
      #define GPIO_REV_44xx	3

We don't have to refer back to this comment while reading the code. I also believed that HWMODs were auto generated.
Can the changes to structures in this patch recreated using current scripts?

~sanjeev

Quoted 30 lines

Signed-off-by Charulatha V<[email protected]>

Cc Cousson, Benoit<[email protected]>

Cc Paul Walmsley<[email protected]>

---
  arch/arm/mach-omap2/gpio.c                 |   26
++++++++++++++++++++++++--
  arch/arm/mach-omap2/omap_hwmod_2430_data.c |    2 +-
  arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    2 +-
  arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    2 +-
  arch/arm/plat-omap/include/plat/gpio.h     |    1 +
  drivers/gpio/gpio_omap.c                   |   11 +++--------
  6 files changed, 31 insertions(+), 13 deletions(-)

arch/arm/mach-omap2/gpio.c +7 -0

index 0446bd1..6cd26b4 100644
@@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct
 omap_hwmod *oh, void *unused)
  		return -ENOMEM;
  	}
 
+	switch (oh->class->rev) {
+	case 0:
+		if (id == 1)
+			/* non-wakeup GPIO pins for OMAP2420 Bank1 */
+			pdata->non_wakeup_gpios = 0xe203ffc0;
+		else if (id == 2)
+			/* non-wakeup GPIO pins for OMAP2420 Bank2 */

arch/arm/mach-omap2/gpio.c static int omap2_gpio_dev_init(struct · line 76

Quoted 10 lines

arch/arm/mach-omap2/gpio.c +10 -0

+			pdata->non_wakeup_gpios = 0x08700040;
+		break;
+	case 2:
+		if (id == 2)
+			/* non-wakeup GPIO pins for OMAP3 Bank2 */
+			pdata->non_wakeup_gpios = 0x00000001;
+		else if (id == 6)
+			/* non-wakeup GPIO pins for OMAP3 Bank6 */
+			pdata->non_wakeup_gpios = 0x08000000;
+		break;

[sp] Where is the description on non-wakeup GPIOs in OMAP3?
Here is text from AM37x TRM:
[quote ...only relevant text]
Each GPIO module provides 32 dedicated general-purpose pins with input and output capabilities; .... These pins can be configured for the following applications:
- Data input (capture)/output (drive)
- Keyboard interface with a debounce cell
- Interrupt generation in ....
- Wake-up request generation in idle mode
[/quote]
Otherwise, what are the GPIO2_WAKEUPENABLE (0x4905 0020) and GPIO6_WAKEUPENABLE (0x4905 8020) meant for?

arch/arm/mach-omap2/gpio.c static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) · line 80

Quoted 5 lines

arch/arm/mach-omap2/gpio.c +5 -0

+	default:
+		/* No non-wakeup GPIO pins for other SoCs */
+		break;
+	}
+

~sanjeev

[snip]...[snip]

-----Original Message-----
From: Cousson, Benoit
Sent: Thursday, May 26, 2011 3:41 PM
To: Premi, Sanjeev
Cc: DebBarma, Tarun Kanti; linux-omap at vger.kernel.org; Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha; Paul Walmsley
Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids

Quoted 5 lines · Sanjeev

Quoted 5 lines

From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti
Sent: Tuesday, May 24, 2011 7:55 PM

Quoted 7 lines

Quoted 7 lines

From: Charulatha V<[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx (bothe OMAP 2420 and 2430)& not for OMAP3 which is incorrect.

Fix the above by providing non-wakeup GPIO information

through pdata

Quoted 5 lines

Quoted 5 lines

specific to the SoC.

The GPIO rev id provided in the hwmod database is the same for OMAP2420
and OMAP2430. Change the GPIO rev ids in hwmod database as

given below

Quoted 6 lines

Quoted 5 lines

so that it can be used to identify OMAP2420 and OMAP2430. OMAP2420 - 0
OMAP2430 - 1
OMAP3 - 2
OMAP4 - 3

Quoted 6 lines

[sp] Magic numbers should be avoided.

      Suggest using something like:
      #define GPIO_REV_2420	0
      #define GPIO_REV_2430	1
      #define GPIO_REV_34XX	2
      #define GPIO_REV_44xx	3

Well, it is not a magic number, it is a revision id that is incremented
each time there is a difference in the IP.
The OMAP version -> IP version link is done at hwmod level. The driver
does not have to know it is an OMAP3 or OMAP4. In that case we did change the IP version for every revisions, but OMAP5 will use the OMAP4
revision.
I'm not even considering all the Davinci variants that are not named OMAP but do use OMAP IPs... as you already know...
That can provide a rather confusing information for my point of view.

Whereas a comment like that will give you the exhaustive information.

0: OMAP2420
1: OMAP2430
2: OMAP3, DMxxx
3: OMAP4, OMAP5, DM816x

That being said, some drivers already did that, so I'm not opposed to such change. I just think it is not the best approach.
At least it gives a pointer to the TRM that contains the IP doc.

Quoted 2 lines

We don't have to refer back to this comment while

reading the code.

Quoted 2 lines

I also believed that HWMODs were auto generated. Can the changes to structures in this patch recreated

using current

Quoted 1 line

scripts?

Only OMAP4 & 5 are generated today, and in anycase this information is
some custom data added by driver owner. So we just have to update that
information.
I cannot really use the real HW IP version because it is irrelevant most
of the time. Maybe in the future we will be able to force the HW team to
provide relevant data in that field :-)

Regards,
Benoit

Quoted 17 lines

~sanjeev

Quoted 14 lines

Signed-off-by Charulatha V<[email protected]>

Cc Cousson, Benoit<[email protected]>

Cc Paul Walmsley<[email protected]>

---
  arch/arm/mach-omap2/gpio.c                 |   26
++++++++++++++++++++++++--
  arch/arm/mach-omap2/omap_hwmod_2430_data.c |    2 +-
  arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    2 +-
  arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    2 +-
  arch/arm/plat-omap/include/plat/gpio.h     |    1 +
  drivers/gpio/gpio_omap.c                   |   11 +++--------
  6 files changed, 31 insertions(+), 13 deletions(-)

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +0 -0

b/arch/arm/mach-omap2/gpio.c

Quoted 22 lines

Quoted 22 lines

index 0446bd1..6cd26b4 100644

arch/arm/mach-omap2/gpio.c +14 -0

@@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct
 omap_hwmod *oh, void *unused)
  		return -ENOMEM;
  	}
 
+	switch (oh->class->rev) {
+	case 0:
+		if (id == 1)
+			/* non-wakeup GPIO pins for OMAP2420 Bank1 */
+			pdata->non_wakeup_gpios = 0xe203ffc0;
+		else if (id == 2)
+			/* non-wakeup GPIO pins for OMAP2420 Bank2 */
+			pdata->non_wakeup_gpios = 0x08700040;
+		break;
+	case 2:
+		if (id == 2)
+			/* non-wakeup GPIO pins for OMAP3 Bank2 */
+			pdata->non_wakeup_gpios = 0x00000001;
+		else if (id == 6)

Quoted 8 lines

arch/arm/mach-omap2/gpio.c static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) · line 75

Quoted 3 lines

arch/arm/mach-omap2/gpio.c +3 -0

+			/* non-wakeup GPIO pins for OMAP3 Bank6 */
+			pdata->non_wakeup_gpios = 0x08000000;
+		break;

[sp] Where is the description on non-wakeup GPIOs in OMAP3?
Here is text from AM37x TRM:
[quote ...only relevant text]
Each GPIO module provides 32 dedicated

general-purpose pins with input

Quoted 1 line

and output capabilities; .... These pins can be

configured for the

Quoted 18 lines

following applications:
- Data input (capture)/output (drive)
- Keyboard interface with a debounce cell
- Interrupt generation in ....
- Wake-up request generation in idle mode [/quote]
Otherwise, what are the GPIO2_WAKEUPENABLE (0x4905 0020) and GPIO6_WAKEUPENABLE (0x4905 8020) meant for?

arch/arm/mach-omap2/gpio.c static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) · line 80

Quoted 5 lines

arch/arm/mach-omap2/gpio.c +5 -0

+	default:
+		/* No non-wakeup GPIO pins for other SoCs */
+		break;
+	}
+

~sanjeev

[snip]...[snip]

Quoted 64 lines

-----Original Message-----
From: Cousson, Benoit
Sent: Thursday, May 26, 2011 3:41 PM
To: Premi, Sanjeev
Cc: DebBarma, Tarun Kanti; linux-omap at vger.kernel.org; Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha; Paul Walmsley
Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids

Quoted 5 lines · Sanjeev

Quoted 5 lines

From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti
Sent: Tuesday, May 24, 2011 7:55 PM

Quoted 7 lines

Quoted 7 lines

From: Charulatha V<[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx
(bothe OMAP 2420 and 2430)& not for OMAP3 which is incorrect.

Fix the above by providing non-wakeup GPIO information

through pdata

Quoted 5 lines

Quoted 5 lines

specific to the SoC.

The GPIO rev id provided in the hwmod database is the same for OMAP2420
and OMAP2430. Change the GPIO rev ids in hwmod database as

given below

Quoted 6 lines

Quoted 5 lines

so that it can be used to identify OMAP2420 and OMAP2430. OMAP2420 - 0
OMAP2430 - 1
OMAP3 - 2
OMAP4 - 3

Quoted 6 lines

[sp] Magic numbers should be avoided.

       Suggest using something like:
       #define GPIO_REV_2420	0
       #define GPIO_REV_2430	1
       #define GPIO_REV_34XX	2
       #define GPIO_REV_44xx	3

Well, it is not a magic number, it is a revision id that is incremented
each time there is a difference in the IP.
The OMAP version -> IP version link is done at hwmod level. The driver
does not have to know it is an OMAP3 or OMAP4. In that case we did change the IP version for every revisions, but OMAP5 will use the OMAP4
revision.
I'm not even considering all the Davinci variants that are not named OMAP but do use OMAP IPs... as you already know...
That can provide a rather confusing information for my point of view.

Whereas a comment like that will give you the exhaustive information.

0: OMAP2420
1: OMAP2430
2: OMAP3, DMxxx
3: OMAP4, OMAP5, DM816x

Quoted 3 lines

That being said, some drivers already did that, so I'm not opposed to such change. I just think it is not the best approach.
At least it gives a pointer to the TRM that contains the IP doc.

[sp] Benoit, your are right from generation perspectove where the
it is easier to increment numbers but when we use comparison

code - as in this patch, comparison against a number isn't readable.

As example:

+ switch (oh->class->rev) { ## This is auto-generated.
+ case 0: ## But this is our code.

I am recommending this to read as:

+ switch (oh->class->rev) { ## This is auto-generated.
+ case GPIO_REV_2420: ## More readable.

-----Original Message-----
From: Cousson, Benoit
Sent: Thursday, May 26, 2011 5:41 PM
To: Premi, Sanjeev
Cc: DebBarma, Tarun Kanti; linux-omap at vger.kernel.org; Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha; Paul Walmsley
Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids

Quoted 44 lines · Sanjeev

Quoted 44 lines

-----Original Message-----
From: Cousson, Benoit
Sent: Thursday, May 26, 2011 3:41 PM
To: Premi, Sanjeev
Cc: DebBarma, Tarun Kanti; linux-omap at vger.kernel.org; Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha; Paul Walmsley
Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids

Quoted 5 lines · Sanjeev

Quoted 5 lines

From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti
Sent: Tuesday, May 24, 2011 7:55 PM

Quoted 7 lines

Quoted 7 lines

From: Charulatha V<[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx
(bothe OMAP 2420 and 2430)& not for OMAP3 which is incorrect.

Fix the above by providing non-wakeup GPIO information

through pdata

Quoted 5 lines

Quoted 5 lines

specific to the SoC.

The GPIO rev id provided in the hwmod database is the same for OMAP2420
and OMAP2430. Change the GPIO rev ids in hwmod database as

given below

Quoted 12 lines

Quoted 5 lines

so that it can be used to identify OMAP2420 and OMAP2430. OMAP2420 - 0
OMAP2430 - 1
OMAP3 - 2
OMAP4 - 3

[sp] Magic numbers should be avoided.

       Suggest using something like:
       #define GPIO_REV_2420	0
       #define GPIO_REV_2430	1
       #define GPIO_REV_34XX	2
       #define GPIO_REV_44xx	3

Quoted 10 lines

Quoted 10 lines

Well, it is not a magic number, it is a revision id that is incremented
each time there is a difference in the IP.
The OMAP version -> IP version link is done at hwmod level. The driver
does not have to know it is an OMAP3 or OMAP4. In that case we did change the IP version for every revisions, but OMAP5 will use the OMAP4
revision.
I'm not even considering all the Davinci variants that are

not named

Quoted 2 lines

Quoted 2 lines

OMAP but do use OMAP IPs... as you already know... That can provide a rather confusing information for my

point of view.

Quoted 2 lines

Quoted 2 lines

Whereas a comment like that will give you the exhaustive

information.

Quoted 7 lines

Quoted 7 lines

0: OMAP2420
1: OMAP2430
2: OMAP3, DMxxx
3: OMAP4, OMAP5, DM816x

That being said, some drivers already did that, so I'm not

opposed to

Quoted 5 lines

Quoted 2 lines

such change. I just think it is not the best approach. At least it gives a pointer to the TRM that contains the IP doc.

[sp] Benoit, your are right from generation perspectove where the
it is easier to increment numbers but when we use comparison

This is not even related to generation. A version number is a number. If OMAP2 is using GPIO v1.6 and OMAP3 the version v2.2, only that IP version is relevant for the driver.
For the documentation point of view, since we do not have a per IP TRM,
then it can make sense to know where that IP is documented. But as I said a comment is enough and will allow a much more relevant level of details information than a define.

Quoted 10 lines

code - as in this patch, comparison against a number isn't readable.

As example:

+ switch (oh->class->rev) { ## This is auto-generated.
+ case 0: ## But this is our code.

I am recommending this to read as:

Quoted 2 lines

+ switch (oh->class->rev) { ## This is auto-generated.
+ case GPIO_REV_2420: ## More readable.

More readable, maybe, but not necessarily relevant nor accurate if the
same IP version is used in another chip or revision.

What IP version is used in a DM816x? GPIO_REV_3430, GPIO_REV_4430?

Benoit

Quoted 109 lines

-----Original Message-----
From: Cousson, Benoit
Sent: Thursday, May 26, 2011 5:41 PM
To: Premi, Sanjeev
Cc: DebBarma, Tarun Kanti; linux-omap at vger.kernel.org; Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha; Paul Walmsley
Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids

Quoted 44 lines · Sanjeev

Quoted 44 lines

-----Original Message-----
From: Cousson, Benoit
Sent: Thursday, May 26, 2011 3:41 PM
To: Premi, Sanjeev
Cc: DebBarma, Tarun Kanti; linux-omap at vger.kernel.org; Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha; Paul Walmsley
Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids

Quoted 5 lines · Sanjeev

Quoted 5 lines

From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti
Sent: Tuesday, May 24, 2011 7:55 PM

Quoted 7 lines

Quoted 7 lines

From: Charulatha V<[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx
(bothe OMAP 2420 and 2430)& not for OMAP3 which is incorrect.

Fix the above by providing non-wakeup GPIO information

through pdata

Quoted 5 lines

Quoted 5 lines

specific to the SoC.

The GPIO rev id provided in the hwmod database is the same for OMAP2420
and OMAP2430. Change the GPIO rev ids in hwmod database as

given below

Quoted 12 lines

Quoted 5 lines

so that it can be used to identify OMAP2420 and OMAP2430. OMAP2420 - 0
OMAP2430 - 1
OMAP3 - 2
OMAP4 - 3

[sp] Magic numbers should be avoided.

        Suggest using something like:
        #define GPIO_REV_2420	0
        #define GPIO_REV_2430	1
        #define GPIO_REV_34XX	2
        #define GPIO_REV_44xx	3

Quoted 10 lines

Quoted 10 lines

Well, it is not a magic number, it is a revision id that is incremented
each time there is a difference in the IP.
The OMAP version -> IP version link is done at hwmod level.
The driver
does not have to know it is an OMAP3 or OMAP4. In that case we did change the IP version for every revisions, but OMAP5 will use the OMAP4
revision.
I'm not even considering all the Davinci variants that are

not named

Quoted 2 lines

Quoted 2 lines

OMAP but do use OMAP IPs... as you already know... That can provide a rather confusing information for my

point of view.

Quoted 2 lines

Quoted 2 lines

Whereas a comment like that will give you the exhaustive

information.

Quoted 7 lines

Quoted 7 lines

0: OMAP2420
1: OMAP2430
2: OMAP3, DMxxx
3: OMAP4, OMAP5, DM816x

That being said, some drivers already did that, so I'm not

opposed to

Quoted 5 lines

Quoted 2 lines

such change. I just think it is not the best approach. At least it gives a pointer to the TRM that contains the IP doc.

[sp] Benoit, your are right from generation perspectove where the
it is easier to increment numbers but when we use comparison

This is not even related to generation. A version number is a number. If OMAP2 is using GPIO v1.6 and OMAP3 the version v2.2, only that IP version is relevant for the driver.
For the documentation point of view, since we do not have a per IP TRM,
then it can make sense to know where that IP is documented. But as I said a comment is enough and will allow a much more relevant level of details information than a define.

Quoted 12 lines

code - as in this patch, comparison against a number isn't readable.

As example:

   +	switch (oh->class->rev) {     ## This is auto-generated.
   +	case 0:				## But this is our code.

       I am recommending this to read as:

   +	switch (oh->class->rev) {     ## This is auto-generated.
   +	case GPIO_REV_2420:		## More readable.

Quoted 5 lines

More readable, maybe, but not necessarily relevant nor accurate if the
same IP version is used in another chip or revision.

What IP version is used in a DM816x? GPIO_REV_3430, GPIO_REV_4430?

How is value 0 accurate?

-----Original Message-----
From: Cousson, Benoit
Sent: Thursday, May 26, 2011 6:17 PM
To: Premi, Sanjeev
Cc: DebBarma, Tarun Kanti; linux-omap at vger.kernel.org; Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha; Paul Walmsley
Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids

Quoted 29 lines · Sanjeev

Quoted 29 lines

-----Original Message-----
From: Cousson, Benoit
Sent: Thursday, May 26, 2011 5:41 PM
To: Premi, Sanjeev
Cc: DebBarma, Tarun Kanti; linux-omap at vger.kernel.org; Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha; Paul Walmsley
Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids

Quoted 17 lines · Sanjeev

Quoted 17 lines

-----Original Message-----
From: Cousson, Benoit
Sent: Thursday, May 26, 2011 3:41 PM
To: Premi, Sanjeev
Cc: DebBarma, Tarun Kanti; linux-omap at vger.kernel.org; Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha; Paul Walmsley
Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids

Quoted 5 lines · Sanjeev

Quoted 5 lines

From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti
Sent: Tuesday, May 24, 2011 7:55 PM

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

From: Charulatha V<[email protected]>

Non-wakeup GPIOs are available only in OMAP2420 and

OMAP3430. But

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the GPIO driver initializes the non-wakeup GPIO bits

for OMAP24xx

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

(bothe OMAP 2420 and 2430)& not for OMAP3 which is

incorrect.

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 2 lines

Quoted 2 lines

Fix the above by providing non-wakeup GPIO information

through pdata

Quoted 5 lines

Quoted 5 lines

specific to the SoC.

The GPIO rev id provided in the hwmod database is the same for OMAP2420
and OMAP2430. Change the GPIO rev ids in hwmod database as

given below

Quoted 6 lines

Quoted 5 lines

so that it can be used to identify OMAP2420 and OMAP2430. OMAP2420 - 0
OMAP2430 - 1
OMAP3 - 2
OMAP4 - 3

Quoted 6 lines

[sp] Magic numbers should be avoided.

        Suggest using something like:
        #define GPIO_REV_2420	0
        #define GPIO_REV_2430	1
        #define GPIO_REV_34XX	2
        #define GPIO_REV_44xx	3

Well, it is not a magic number, it is a revision id that is incremented
each time there is a difference in the IP.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

The OMAP version -> IP version link is done at hwmod level.
The driver
does not have to know it is an OMAP3 or OMAP4. In that

case we did

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

change the IP version for every revisions, but OMAP5 will use the OMAP4
revision.

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

I'm not even considering all the Davinci variants that are

not named

Quoted 2 lines

Quoted 2 lines

OMAP but do use OMAP IPs... as you already know... That can provide a rather confusing information for my

point of view.

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

Whereas a comment like that will give you the exhaustive

information.

Quoted 1 line

Quoted 1 line

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

0: OMAP2420
1: OMAP2430
2: OMAP3, DMxxx
3: OMAP4, OMAP5, DM816x

That being said, some drivers already did that, so I'm not

opposed to

Quoted 1 line

Quoted 1 line

such change. I just think it is not the best approach.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

At least it gives a pointer to the TRM that contains the IP doc.

[sp] Benoit, your are right from generation perspectove where the
it is easier to increment numbers but when we use

comparison

Quoted 2 lines

Quoted 2 lines

This is not even related to generation. A version number

is a number.

Quoted 1 line

Quoted 1 line

If OMAP2 is using GPIO v1.6 and OMAP3 the version v2.2,

only that IP

Quoted 4 lines

Quoted 4 lines

version is relevant for the driver.
For the documentation point of view, since we do not have a per IP TRM,
then it can make sense to know where that IP is

documented. But as I

Quoted 1 line

Quoted 1 line

said a comment is enough and will allow a much more

relevant level of

Quoted 1 line

Quoted 1 line

details information than a define.

Quoted 18 lines

Quoted 18 lines

Quoted 6 lines

code - as in this patch, comparison against a number isn't readable.

As example:

Quoted 7 lines

   +	switch (oh->class->rev) {     ## This is auto-generated.
   +	case 0:				## But this is our code.

       I am recommending this to read as:

   +	switch (oh->class->rev) {     ## This is auto-generated.
   +	case GPIO_REV_2420:		## More readable.

More readable, maybe, but not necessarily relevant nor accurate if the
same IP version is used in another chip or revision.

Quoted 3 lines

Quoted 1 line

What IP version is used in a DM816x? GPIO_REV_3430, GPIO_REV_4430?

How is value 0 accurate?

0 is just the revision number that can give you the exact list of Soc that does use that IP. Thanks to an exhaustive comment.

0: OMAP2420
1: OMAP2430
2: OMAP3, DMxxx
3: OMAP4, OMAP5, DM816x

BTW, I'm still waiting the answer for previous question...

Regards,
Benoit

Why not do the following:

        #define OMAP_GPIO_REV_0	0
        #define OMAP_GPIO_REV_1	1
        #define OMAP_GPIO_REV_2	2
        #define OMAP_GPIO_REV_3	3
/*
	OMAP_GPIO_REV_0:	OMAP2420
	OMAP_GPIO_REV_1:	OMAP2430
	OMAP_GPIO_REV_2:	OMAP3, DMxxx
	OMAP_GPIO_REV_3:	OMAP4, OMAP5, DM816x

*/

   +	switch (oh->class->rev) {     ## This is auto-generated.
   +	case 0:				## But this is our code.

       I am recommending this to read as:

   +	switch (oh->class->rev) {     ## This is auto-generated.
   +	case OMAP_GPIO_REV_0:		## More readable.

That approach solves both issues -- Revision -> Chip mapping in comment, no magic numbers in the code, and no implied restriction of the rev number to a specific SoC. Using the defines makes it easier to search the code for a specific revision, since you would no longer get false positives for all the other '0' and '1' constants that appear in the code. It also makes it indexible by tools like LXR.

From: Charulatha V <[email protected]>

In omap3, save/restore context is implemented for GPIO banks 2-6 as GPIO bank1 is in wakeup domain. Instead of identifying bank's power domain by bank id, make use of a flag "loses_context" which is filled by pwrdm_can_ever_lose_context() during dev_init.

For getting the powerdomain pointer, omap_hwmod_get_pwrdm() is used. omap_device_get_pwrdm() could not be used as the pwrdm information needs to be filled in pdata, whereas omap_device_get_pwrdm() could be used only after omap_device_build() call.

Signed-off-by Charulatha V <[email protected]>

From: Charulatha V <[email protected]>

gpio_context array, which is 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

TODO: extend the gpio save/restore context function for OMAP4 architecture. This is done in one of the next patches in this series

Signed-off-by Charulatha V <[email protected]>

-----Original Message-----
From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti
Sent: Tuesday, May 24, 2011 7:55 PM
To: linux-omap at vger.kernel.org
Cc: Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha
Subject: [PATCH 05/15] OMAP: GPIO: Make gpio_context part of gpio_bank structure

From: Charulatha V <[email protected]>

gpio_context array, which is 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

TODO: extend the gpio save/restore context function for OMAP4 architecture. This is done in one of the next patches in this series

Quoted 20 lines

-----Original Message-----
From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti
Sent: Tuesday, May 24, 2011 7:55 PM
To: linux-omap at vger.kernel.org
Cc: Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha
Subject: [PATCH 05/15] OMAP: GPIO: Make gpio_context part of gpio_bank structure

From: Charulatha V <[email protected]>

gpio_context array, which is 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

Quoted 3 lines

TODO: extend the gpio save/restore context function for OMAP4 architecture. This is done in one of the next patches in this series

[sp] Should this TODO be part of commit?

-----Original Message-----
From: Premi, Sanjeev
Sent: Thursday, May 26, 2011 3:29 PM
To: DebBarma, Tarun Kanti; linux-omap at vger.kernel.org
Cc: Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha
Subject: RE: [PATCH 05/15] OMAP: GPIO: Make gpio_context part of gpio_bank structure

Quoted 20 lines

-----Original Message-----
From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti
Sent: Tuesday, May 24, 2011 7:55 PM
To: linux-omap at vger.kernel.org
Cc: Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; linux-arm-kernel at lists.infradead.org; Varadarajan, Charulatha
Subject: [PATCH 05/15] OMAP: GPIO: Make gpio_context part of gpio_bank structure

From: Charulatha V <[email protected]>

gpio_context array, which is 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

Quoted 3 lines

TODO: extend the gpio save/restore context function for OMAP4 architecture. This is done in one of the next patches in this series

[sp] Should this TODO be part of commit?
BTW, this is the last patch in the series.

~sanjeev

From: Charulatha V <[email protected]>

Modify the omap_gpio_save/restore_context to support OMAP4 architecture so that the OMAP GPIO driver need not be modified when OMAP4 off mode support is available.

Signed-off-by Charulatha V <[email protected]>

Quoted 6 lines · Tarun Kanti DebBarma

From: Charulatha V <[email protected]>

Modify the omap_gpio_save/restore_context to support OMAP4 architecture so that the OMAP GPIO driver need not be modified when OMAP4 off mode support is available.

Quoted 1 line

Signed-off-by Charulatha V <[email protected]>

I don't think this patch is needed.

PATCH 11/15 removes everything added in this patch when it moves to using the register offsets.

Kevin

From: Charulatha V <[email protected]>

Modify omap_gpio_prepare_for_idle() & omap_gpio_resume_after_idle() functions to handle save context & restore context respectively in the OMAP GPIO driver itself instead of calling these functions from pm specific files. For this, in gpio_prepare_for_idle(), use omap_device_get_context_loss_count() and in gpio_resume_after_idle() call it again. If the count is different, do restore context.

context lost count is modified in omap_sram_idle() path when pwrdm_post_transition() is called. But pwrdm_post_transition() is called only after omap_gpio_resume_after_idle() is called. Hence correct this so that context lost count is modified before calling omap_gpio_resume_after_idle().

omap_gpio_prepare_for_idle() & omap_gpio_resume_after_idle() do nothing if none of the GPIOs in a bank is being used.

Also remove usage of cpu_is_* checks from the above mentioned functions and fix the multi-line comment style

Signed-off-by Charulatha V <[email protected]>

---
 arch/arm/mach-omap2/pm34xx.c           |   22 +----
 arch/arm/plat-omap/include/plat/gpio.h |    2 -
 drivers/gpio/gpio_omap.c               |  138 +++++++++++++++++---------------
 3 files changed, 78 insertions(+), 84 deletions(-)

arch/arm/mach-omap2/pm34xx.c +0 -10

index 0c5e3a4..682d147 100644
@@ -91,16 +91,6 @@ static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
 static struct powerdomain *core_pwrdm, *per_pwrdm;
 static struct powerdomain *cam_pwrdm;
 
-static inline void omap3_per_save_context(void)
-{
-	omap_gpio_save_context();
-}
-
-static inline void omap3_per_restore_context(void)
-{
-	omap_gpio_restore_context();
-}
-
 static void omap3_enable_io_chain(void)
 {
 	int timeout = 0;

arch/arm/mach-omap2/pm34xx.c +2 -6

 	/* PER */
 	if (per_next_state < PWRDM_POWER_ON) {

@@ -404,8 +396,6 @@ void omap_sram_idle(void)
 		omap_uart_prepare_idle(2);
 		omap_uart_prepare_idle(3);
 		omap2_gpio_prepare_for_idle(per_going_off);
-		if (per_next_state == PWRDM_POWER_OFF)
-				omap3_per_save_context();
 	}
 
 	/* CORE */

@@ -467,12 +457,12 @@ void omap_sram_idle(void)
 	}
 	omap3_intc_resume_idle();
 
+	pwrdm_post_transition();
+
 	/* PER */
 	if (per_next_state < PWRDM_POWER_ON) {
 		per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
 		omap2_gpio_resume_after_idle();
-		if (per_prev_state == PWRDM_POWER_OFF)
-			omap3_per_restore_context();
 		omap_uart_resume_idle(2);
 		omap_uart_resume_idle(3);
 	}

@@ -490,8 +480,6 @@ console_still_active:
 		omap3_disable_io_chain();
 	}
 
-	pwrdm_post_transition();
-
 	clkdm_allow_idle(mpu_pwrdm->pwrdm_clkdms[0]);
 }
 

arch/arm/plat-omap/include/plat/gpio.h +0 -2

index 64b1ee7..5718a45 100644
@@ -209,8 +209,6 @@ extern void omap2_gpio_prepare_for_idle(int off_mode);
 extern void omap2_gpio_resume_after_idle(void);
 extern void omap_set_gpio_debounce(int gpio, int enable);
 extern void omap_set_gpio_debounce_time(int gpio, int enable);
-extern void omap_gpio_save_context(void);
-extern void omap_gpio_restore_context(void);
 /*-------------------------------------------------------------------------*/
 
 /* Wrappers for "new style" GPIO calls, using the new infrastructure

drivers/gpio/gpio_omap.c +0 -0

index 9d55b7d..bc02ec5 100644

drivers/gpio/gpio_omap.c +5 -1

 	u16 id;
 
 	void (*set_dataout)(struct gpio_bank *bank, int gpio, int enable);

@@ -1310,6 +1313,9 @@ static struct sys_device omap_gpio_device = {
 
 #ifdef CONFIG_ARCH_OMAP2PLUS
 
+static void omap_gpio_save_context(struct gpio_bank *bank);
+static void omap_gpio_restore_context(struct gpio_bank *bank);
+
 static int workaround_enabled;
 
 void omap2_gpio_prepare_for_idle(int off_mode)

@@ -1318,6 +1324,7 @@ void omap2_gpio_prepare_for_idle(int off_mode)
 	struct gpio_bank *bank;
 
 	list_for_each_entry(bank, &omap_gpio_list, node) {
+		struct platform_device *pdev;
 		u32 l1 = 0, l2 = 0;
 		int j;
 

@@ -1334,7 +1341,7 @@ void omap2_gpio_prepare_for_idle(int off_mode)
 		 * non-wakeup GPIOs.  Otherwise spurious IRQs will be
 		 * generated.  See OMAP2420 Errata item 1.101. */
 		if (!(bank->enabled_non_wakeup_gpios))
-			continue;
+			goto save_gpio_ctx;
 
 		if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
 			bank->saved_datain = __raw_readl(bank->base +

drivers/gpio/gpio_omap.c +3 -0

+		omap_gpio_save_context(bank);
 	}
 	if (!c) {
 		workaround_enabled = 0;

@@ -1385,6 +1398,8 @@ void omap2_gpio_resume_after_idle(void)
 	struct gpio_bank *bank;
 
 	list_for_each_entry(bank, &omap_gpio_list, node) {
+		u32 ctx_lost_cnt_after;
+		struct platform_device *pdev;
 		u32 l = 0, gen, gen0, gen1;
 		int j;
 

@@ -1394,11 +1409,17 @@ void omap2_gpio_resume_after_idle(void)
 		for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
 			clk_enable(bank->dbck);

Quoted 11 lines

drivers/gpio/gpio_omap.c +4 -1

@@ -1394,11 +1409,17 @@ void omap2_gpio_resume_after_idle(void)
 		for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
 			clk_enable(bank->dbck);
 
-		if (!workaround_enabled)
+		pdev = to_platform_device(bank->dev);
+		ctx_lost_cnt_after = omap_device_get_context_loss_count(pdev);
+
+		if (ctx_lost_cnt_after == bank->ctx_lost_cnt_before)
 			continue;
 

Review void omap2_gpio_resume_after_idle(void) · line 1419

Quoted 2 lines

(quoted diff) +2 -0

+		if (!workaround_enabled)
+			goto restore_gpio_ctx;

Now that these functions are all bank-specific, this 'workaround_enabled' flag should be made per-bank.

From: Charulatha V <[email protected]>

Make workaround_enabled flag bank-specific instead of using a single flag for all the banks together. This would be helpful while making use of runtime framework in OMAP GPIO driver which would make the driver handle each GPIO bank independently.

Also rename workaround_enabled flag to off_mode_wkup_wa_enabled

Quoted 7 lines · Tarun Kanti DebBarma

From: Charulatha V <[email protected]>

Make workaround_enabled flag bank-specific instead of using a single flag for all the banks together. This would be helpful while making use of runtime framework in OMAP GPIO driver which would make the driver handle each GPIO bank independently.

Quoted 1 line

Also rename workaround_enabled flag to off_mode_wkup_wa_enabled

Do we even need a dedicated flag for this?

I seems that the combination of enabled_non_wakeup_gpios and whether or not context has been lost could be used to check this condition.

Kevin

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 <[email protected]>

Signed-off-by Charulatha V <[email protected]>

---
 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(-)

arch/arm/mach-omap1/gpio15xx.c +0 -0

index f18a4a9..b0bd21e 100644

arch/arm/mach-omap1/gpio15xx.c +3 -0

 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 = {

arch/arm/mach-omap1/gpio16xx.c +6 -0

index d886b88..403437b 100644
@@ -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 = {

arch/arm/mach-omap1/gpio7xx.c +6 -0

index c7684ce..d5a4aaf 100644
@@ -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 = {

arch/arm/mach-omap2/gpio.c +6 -0

index 0782e06..7e79999 100644
@@ -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");

arch/arm/plat-omap/include/plat/gpio.h +0 -0

index 5718a45..2d1a5d6 100644

arch/arm/plat-omap/include/plat/gpio.h +0 -0

 	bool irqenable_inv;
 };

drivers/gpio/gpio_omap.c +3 -8

index fcc60be..c189416 100644
@@ -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)

drivers/gpio/gpio_omap.c +0 -0

 	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;

drivers/gpio/gpio_omap.c +3 -51

 		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:

Quoted 21 lines · Tarun Kanti DebBarma

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 <[email protected]>

Signed-off-by Charulatha V <[email protected]>

---

?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(-)

arch/arm/mach-omap1/gpio15xx.c +0 -0

index f18a4a9..b0bd21e 100644

arch/arm/mach-omap1/gpio15xx.c static struct omap_gpio_reg_offs omap15xx_mpuio_regs = { · line 49

Quoted 7 lines

arch/arm/mach-omap1/gpio15xx.c +3 -0

@@ -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.

Quoted 88 lines

arch/arm/mach-omap1/gpio15xx.c +3 -0

 ?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 = {

arch/arm/mach-omap1/gpio16xx.c +6 -0

index d886b88..403437b 100644
@@ -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 = {

arch/arm/mach-omap1/gpio7xx.c +6 -0

index c7684ce..d5a4aaf 100644
@@ -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 = {

arch/arm/mach-omap2/gpio.c +3 -0

index 0782e06..7e79999 100644
@@ -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;

arch/arm/mach-omap2/gpio.c +3 -0

@@ -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");

arch/arm/plat-omap/include/plat/gpio.h +0 -0

index 5718a45..2d1a5d6 100644
@@ -189,6 +189,9 @@ struct omap_gpio_reg_offs {

arch/arm/plat-omap/include/plat/gpio.h struct omap_gpio_reg_offs { · line 194

Quoted 6 lines

arch/arm/plat-omap/include/plat/gpio.h +3 -0

 ? ? ? u16 debounce;
 ? ? ? u16 debounce_en;
 ? ? ? u16 ctrl;
+ ? ? u16 wkupstatus;
+ ? ? u16 wkupclear;
+ ? ? u16 wkupset;

s/wkup/wkup_/

Quoted 29 lines

arch/arm/plat-omap/include/plat/gpio.h +0 -0

 ? ? ? bool irqenable_inv;
 ?};

drivers/gpio/gpio_omap.c +3 -8

index fcc60be..c189416 100644
@@ -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)

drivers/gpio/gpio_omap.c static void omap_gpio_free(struct gpio_chip *chip, unsigned offset) · line 613

Quoted 10 lines

drivers/gpio/gpio_omap.c +2 -8

- ? ? 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...

drivers/gpio/gpio_omap.c static void omap_gpio_free(struct gpio_chip *chip, unsigned offset) · line 612

Quoted 7 lines

? ? ? ? ? ? ? /* Disable wake-up during idle for dynamic tick */
- ? ? ? ? ? ? void __iomem *reg = bank->base + OMAP4_GPIO_IRQWAKEN0;
- ? ? ? ? ? ? __raw_writel(1 << offset, reg);
- ? ? }

drivers/gpio/gpio_omap.c +2 -1

-#endif
+ ? ? ? ? ? ? __raw_writel(1 << offset, bank->wake_clear);
+

...and here you write using the copy. ?Not good for readability. ?More on this below.

Quoted 6 lines

? ? ? bank->mod_usage &= ~(1 << offset);

? ? ? if ((bank->regs->ctrl != USHRT_MAX) && (!bank->mod_usage)) {

drivers/gpio/gpio_omap.c +0 -0

@@ -1189,6 +1176,15 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 ? ? ? ? ? ? ? goto err_free;

drivers/gpio/gpio_omap.c static int __devinit omap_gpio_probe(struct platform_device *pdev) · line 1186

Quoted 10 lines

drivers/gpio/gpio_omap.c +8 -0

 ? ? ? }
 
+ ? ? /*
+ ? ? ?* 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.

drivers/gpio/gpio_omap.c err_exit: · line 1206

Quoted 10 lines

drivers/gpio/gpio_omap.c +1 -1

 ? ? ? 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

Quoted 6 lines

drivers/gpio/gpio_omap.c +0 -0

 ?{
 ? ? ? struct gpio_bank *bank;
 

@@ -1215,41 +1211,12 @@ static int omap_gpio_suspend(struct sys_device *dev, pm_message_t mesg)
 ? ? ? ? ? ? ? return 0;
 

drivers/gpio/gpio_omap.c static int omap_gpio_suspend(struct sys_device *dev, pm_message_t mesg) · line 1220

Quoted 4 lines

drivers/gpio/gpio_omap.c +0 -3

 ? ? ? 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->...'

Quoted 72 lines

drivers/gpio/gpio_omap.c +3 -55

 ? ? ? ? ? ? ? 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);

drivers/gpio/gpio_omap.c static int omap_gpio_resume(struct sys_device *dev) · line 1240

Quoted 5 lines

drivers/gpio/gpio_omap.c +2 -1

- ? ? ? ? ? ? __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*

Kevin