LinuxLists.cc - [PATCH 0/6] gpio/omap: Some more driver cleanup and fixes

[permalink] [raw]

Subject: [PATCH 4/6] gpio/omap: get rid of retrigger variable in gpio_irq_handler

This local variable is just assigned zero and then OR'ed
with isr. It does not appear to serve any purpose and so
removing it.

Signed-off-by: Tarun Kanti DebBarma <[email protected]>
---
drivers/gpio/gpio-omap.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index b62e861..3dd4b3a 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -623,7 +623,6 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
u32 isr;
unsigned int gpio_irq, gpio_index;
struct gpio_bank *bank;
- u32 retrigger = 0;
int unmasked = 0;
struct irq_chip *chip = irq_desc_get_chip(desc);

@@ -660,8 +659,6 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
chained_irq_exit(chip, desc);
}

- isr |= retrigger;
- retrigger = 0;
if (!isr)
break;

--
1.7.0.4

2012-02-23 12:10:45

[permalink] [raw]

Subject: [PATCH 6/6] gpio/omap: fix _set_gpio_irqenable implementation

This function should be capable of both enabling and disabling interrupts
based upon the *enable* parameter. Right now the function only enables
the interrupt and *enable* is not used at all. So add the interrupt
disable capability also using the parameter.

Signed-off-by: Tarun Kanti DebBarma <[email protected]>
---
drivers/gpio/gpio-omap.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 67535c8..acc71a0 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -473,7 +473,10 @@ static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)

static inline void _set_gpio_irqenable(struct gpio_bank *bank, int gpio, int enable)
{
- _enable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
+ if (enable)
+ _enable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
+ else
+ _disable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
}

/*
--
1.7.0.4

2012-02-23 12:10:56

[permalink] [raw]

Subject: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

Since we already have bank->context.wake_en to keep track
of gpios which are wakeup enabled, there is no need to have
this field any more.

Signed-off-by: Tarun Kanti DebBarma <[email protected]>
---
drivers/gpio/gpio-omap.c | 11 +++++------
1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 64f15d5..b62e861 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -53,7 +53,6 @@ struct gpio_bank {
void __iomem *base;
u16 irq;
u16 virtual_irq_start;
- u32 suspend_wakeup;
u32 non_wakeup_gpios;
u32 enabled_non_wakeup_gpios;
struct gpio_regs context;
@@ -497,9 +496,9 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)

spin_lock_irqsave(&bank->lock, flags);
if (enable)
- bank->suspend_wakeup |= gpio_bit;
+ bank->context.wake_en |= gpio_bit;
else
- bank->suspend_wakeup &= ~gpio_bit;
+ bank->context.wake_en &= ~gpio_bit;

spin_unlock_irqrestore(&bank->lock, flags);

@@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)

spin_lock_irqsave(&bank->lock, flags);
bank->context.wake_en = __raw_readl(mask_reg);
- __raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
+ __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
spin_unlock_irqrestore(&bank->lock, flags);

return 0;
@@ -1137,12 +1136,12 @@ static int omap_gpio_suspend(struct device *dev)
if (!bank->mod_usage || !bank->loses_context)
return 0;

- if (!bank->regs->wkup_en || !bank->suspend_wakeup)
+ if (!bank->regs->wkup_en || !bank->context.wake_en)
return 0;

spin_lock_irqsave(&bank->lock, flags);
_gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
- _gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
+ _gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
spin_unlock_irqrestore(&bank->lock, flags);

return 0;
--
1.7.0.4

2012-02-23 12:11:16

[permalink] [raw]

Subject: [PATCH 5/6] gpio/omap: fix trigger type to unsigned

The GPIO trigger parameter is of type unsigned.
enum {
IRQ_TYPE_NONE = 0x00000000,
IRQ_TYPE_EDGE_RISING = 0x00000001,
IRQ_TYPE_EDGE_FALLING = 0x00000002,
IRQ_TYPE_EDGE_BOTH = (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING),
IRQ_TYPE_LEVEL_HIGH = 0x00000004,
IRQ_TYPE_LEVEL_LOW = 0x00000008,
IRQ_TYPE_LEVEL_MASK = (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH),
IRQ_TYPE_SENSE_MASK = 0x0000000f,

IRQ_TYPE_PROBE = 0x00000010,
...
};
Even though gpio_irq_type(struct irq_data *d, unsigned type) has the right type
of parameter, the subsequent called functions set_gpio_triggering() and
set_gpio_trigger() wrongly makes it signed integer. Fix this.

Signed-off-by: Tarun Kanti DebBarma <[email protected]>
---
drivers/gpio/gpio-omap.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 3dd4b3a..67535c8 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -232,7 +232,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
}

static inline void set_gpio_trigger(struct gpio_bank *bank, int gpio,
- int trigger)
+ unsigned trigger)
{
void __iomem *base = bank->base;
u32 gpio_bit = 1 << gpio;
@@ -314,7 +314,8 @@ static void _toggle_gpio_edge_triggering(struct gpio_bank *bank, int gpio)
static void _toggle_gpio_edge_triggering(struct gpio_bank *bank, int gpio) {}
#endif

-static int _set_gpio_triggering(struct gpio_bank *bank, int gpio, int trigger)
+static int _set_gpio_triggering(struct gpio_bank *bank, int gpio,
+ unsigned trigger)
{
void __iomem *reg = bank->base;
void __iomem *base = bank->base;
--
1.7.0.4

2012-02-23 12:11:15

[permalink] [raw]

Subject: [PATCH 2/6] gpio/omap: remove saved_wakeup field from struct gpio_bank

There is no more need to have saved_wakeup. Instead we can use
context.wake_en which holds the current wakeup enable register
context. This also means that the read from wakeup enable register
is not needed.

Signed-off-by: Tarun Kanti DebBarma <[email protected]>
---
drivers/gpio/gpio-omap.c | 13 ++++---------
1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 40a1fb2..64f15d5 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -54,7 +54,6 @@ struct gpio_bank {
u16 irq;
u16 virtual_irq_start;
u32 suspend_wakeup;
- u32 saved_wakeup;
u32 non_wakeup_gpios;
u32 enabled_non_wakeup_gpios;
struct gpio_regs context;
@@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
unsigned long flags;

spin_lock_irqsave(&bank->lock, flags);
- bank->saved_wakeup = __raw_readl(mask_reg);
+ bank->context.wake_en = __raw_readl(mask_reg);
__raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
spin_unlock_irqrestore(&bank->lock, flags);

@@ -788,7 +787,7 @@ static int omap_mpuio_resume_noirq(struct device *dev)
unsigned long flags;

spin_lock_irqsave(&bank->lock, flags);
- __raw_writel(bank->saved_wakeup, mask_reg);
+ __raw_writel(bank->context.wake_en, mask_reg);
spin_unlock_irqrestore(&bank->lock, flags);

return 0;
@@ -1133,7 +1132,6 @@ static int omap_gpio_suspend(struct device *dev)
struct platform_device *pdev = to_platform_device(dev);
struct gpio_bank *bank = platform_get_drvdata(pdev);
void __iomem *base = bank->base;
- void __iomem *wakeup_enable;
unsigned long flags;

if (!bank->mod_usage || !bank->loses_context)
@@ -1142,10 +1140,7 @@ static int omap_gpio_suspend(struct device *dev)
if (!bank->regs->wkup_en || !bank->suspend_wakeup)
return 0;

- wakeup_enable = bank->base + bank->regs->wkup_en;
-
spin_lock_irqsave(&bank->lock, flags);
- bank->saved_wakeup = __raw_readl(wakeup_enable);
_gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
_gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
spin_unlock_irqrestore(&bank->lock, flags);
@@ -1163,12 +1158,12 @@ static int omap_gpio_resume(struct device *dev)
if (!bank->mod_usage || !bank->loses_context)
return 0;

- if (!bank->regs->wkup_en || !bank->saved_wakeup)
+ if (!bank->regs->wkup_en || !bank->context.wake_en)
return 0;

spin_lock_irqsave(&bank->lock, flags);
_gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
- _gpio_rmw(base, bank->regs->wkup_en, bank->saved_wakeup, 1);
+ _gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
spin_unlock_irqrestore(&bank->lock, flags);

return 0;
--
1.7.0.4

2012-02-23 12:11:14

[permalink] [raw]

Subject: [PATCH 1/6] gpio/omap: remove saved_fallingdetect, saved_risingdetect fields

Since we already have context.fallingdetect and context.risingdetect
there is no more need to have these additional fields. Also, getting
rid of extra reads associated with them.

Signed-off-by: Tarun Kanti DebBarma <[email protected]>
---
drivers/gpio/gpio-omap.c | 19 ++++++++-----------
1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index f29252f..40a1fb2 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -59,8 +59,6 @@ struct gpio_bank {
u32 enabled_non_wakeup_gpios;
struct gpio_regs context;
u32 saved_datain;
- u32 saved_fallingdetect;
- u32 saved_risingdetect;
u32 level_mask;
u32 toggle_mask;
spinlock_t lock;
@@ -1224,11 +1222,9 @@ static int omap_gpio_runtime_suspend(struct device *dev)

bank->saved_datain = __raw_readl(bank->base +
bank->regs->datain);
- l1 = __raw_readl(bank->base + bank->regs->fallingdetect);
- l2 = __raw_readl(bank->base + bank->regs->risingdetect);
+ l1 = bank->context.fallingdetect;
+ l2 = bank->context.risingdetect;

- bank->saved_fallingdetect = l1;
- bank->saved_risingdetect = l2;
l1 &= ~bank->enabled_non_wakeup_gpios;
l2 &= ~bank->enabled_non_wakeup_gpios;

@@ -1287,9 +1283,9 @@ static int omap_gpio_runtime_resume(struct device *dev)
}
}

- __raw_writel(bank->saved_fallingdetect,
+ __raw_writel(bank->context.fallingdetect,
bank->base + bank->regs->fallingdetect);
- __raw_writel(bank->saved_risingdetect,
+ __raw_writel(bank->context.risingdetect,
bank->base + bank->regs->risingdetect);
l = __raw_readl(bank->base + bank->regs->datain);

@@ -1306,14 +1302,15 @@ static int omap_gpio_runtime_resume(struct device *dev)
* No need to generate IRQs for the rising edge for gpio IRQs
* configured with falling edge only; and vice versa.
*/
- gen0 = l & bank->saved_fallingdetect;
+ gen0 = l & bank->context.fallingdetect;
gen0 &= bank->saved_datain;

- gen1 = l & bank->saved_risingdetect;
+ gen1 = l & bank->context.risingdetect;
gen1 &= ~(bank->saved_datain);

/* FIXME: Consider GPIO IRQs with level detections properly! */
- gen = l & (~(bank->saved_fallingdetect) & ~(bank->saved_risingdetect));
+ gen = l & (~(bank->context.fallingdetect) &
+ ~(bank->context.risingdetect));
/* Consider all GPIO IRQs needed to be updated */
gen |= gen0 | gen1;

--
1.7.0.4

2012-02-23 12:26:55

by Santosh Shilimkar

[permalink] [raw]

Subject: Re: [PATCH 0/6] gpio/omap: Some more driver cleanup and fixes

On Thu, Feb 23, 2012 at 5:40 PM, Tarun Kanti DebBarma
<[email protected]> wrote:
> The cleanup is mostly getting rid of redundant fields in struct gpio_bank{}
> as we already have them as part of bank->context now. Also, remove un-used
> variable from gpio_irq_handler.
>
> The fix include correction of _set_gpio_irqenable() implementation and fix
> type mismatch of gpio trigger parameter.
>
> It is baselined on top of Kevin's following series:
> gpio/omap: cleanup and runtime PM conversion for v3.4
> git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.4/gpio/runtime-pm-cleanup
>
> I have applied Benoit's GPIO patches in following series on top of Kevin's
> before applying my changes.
> gpio/omap: Cleanup and adaptation to Device Tree
> git://git.kernel.org/pub/scm/linux/kernel/git/bcousson/linux-omap-dt.git for_3.4/dt_gpio
>
> Series is available here for reference:
> git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev for_3.4/gpio_further_cleanup_fixes
>
> Power Test: Off-mode and Retention on OMAP3430 (Suspend and Idle paths).
> Functional Test: OMAP2430, OMAP3430SDP, ZOOM3, OMAP4430, OMAP4-BLAZE
>
> Tarun Kanti DebBarma (6):
> ?gpio/omap: remove saved_fallingdetect, saved_risingdetect fields
> ?gpio/omap: remove saved_wakeup field from struct gpio_bank
> ?gpio/omap: remove suspend_wakeup field from struct gpio_bank
> ?gpio/omap: get rid of retrigger variable in gpio_irq_handler
> ?gpio/omap: fix trigger type to unsigned
> ?gpio/omap: fix _set_gpio_irqenable implementation
>
> ?drivers/gpio/gpio-omap.c | ? 56 +++++++++++++++++++--------------------------
> ?1 files changed, 24 insertions(+), 32 deletions(-)
>
Nice clean-up series. I have gone through this series one more time
Thanks for updating change-logs. I noticed you dropped the edge triggered
irq wakeup fix....I see on the list now.... Kevin has fixed that already.

Series looks good to me. You can add:
Reviewed-by: Santosh Shilimkar <[email protected]>

Regards
Santosh

2012-02-23 12:28:21

[permalink] [raw]

Subject: Re: [PATCH 1/6] gpio/omap: remove saved_fallingdetect, saved_risingdetect fields

On Thu, Feb 23, 2012 at 05:40:26PM +0530, Tarun Kanti DebBarma wrote:
> Since we already have context.fallingdetect and context.risingdetect
> there is no more need to have these additional fields. Also, getting
> rid of extra reads associated with them.
>
> Signed-off-by: Tarun Kanti DebBarma <[email protected]>

Looks trivial and correct:

Acked-by: Felipe Balbi <[email protected]>

--
balbi

Attachments:

(No filename) (395.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-02-23 12:28:52

[permalink] [raw]

Subject: Re: [PATCH 2/6] gpio/omap: remove saved_wakeup field from struct gpio_bank

On Thu, Feb 23, 2012 at 05:40:27PM +0530, Tarun Kanti DebBarma wrote:
> There is no more need to have saved_wakeup. Instead we can use
> context.wake_en which holds the current wakeup enable register
> context. This also means that the read from wakeup enable register
> is not needed.
>
> Signed-off-by: Tarun Kanti DebBarma <[email protected]>

Likewise:

Acked-by: Felipe Balbi <[email protected]>

--
balbi

Attachments:

(No filename) (409.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-02-23 12:29:49

[permalink] [raw]

Subject: Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

On Thu, Feb 23, 2012 at 05:40:28PM +0530, Tarun Kanti DebBarma wrote:
> Since we already have bank->context.wake_en to keep track
> of gpios which are wakeup enabled, there is no need to have
> this field any more.
>
> Signed-off-by: Tarun Kanti DebBarma <[email protected]>

Likewise:

Acked-by: Felipe Balbi <[email protected]>

--
balbi

Attachments:

(No filename) (338.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-02-23 12:30:26

[permalink] [raw]

Subject: Re: [PATCH 4/6] gpio/omap: get rid of retrigger variable in gpio_irq_handler

On Thu, Feb 23, 2012 at 05:40:29PM +0530, Tarun Kanti DebBarma wrote:
> This local variable is just assigned zero and then OR'ed
> with isr. It does not appear to serve any purpose and so
> removing it.
>
> Signed-off-by: Tarun Kanti DebBarma <[email protected]>

hehe, makes a lot of sense :-)

Acked-by: Felipe Balbi <[email protected]>

--
balbi

Attachments:

(No filename) (347.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-02-23 12:31:00

[permalink] [raw]

Subject: Re: [PATCH 5/6] gpio/omap: fix trigger type to unsigned

On Thu, Feb 23, 2012 at 05:40:30PM +0530, Tarun Kanti DebBarma wrote:
> The GPIO trigger parameter is of type unsigned.
> enum {
> IRQ_TYPE_NONE = 0x00000000,
> IRQ_TYPE_EDGE_RISING = 0x00000001,
> IRQ_TYPE_EDGE_FALLING = 0x00000002,
> IRQ_TYPE_EDGE_BOTH = (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING),
> IRQ_TYPE_LEVEL_HIGH = 0x00000004,
> IRQ_TYPE_LEVEL_LOW = 0x00000008,
> IRQ_TYPE_LEVEL_MASK = (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH),
> IRQ_TYPE_SENSE_MASK = 0x0000000f,
>
> IRQ_TYPE_PROBE = 0x00000010,
> ...
> };
> Even though gpio_irq_type(struct irq_data *d, unsigned type) has the right type
> of parameter, the subsequent called functions set_gpio_triggering() and
> set_gpio_trigger() wrongly makes it signed integer. Fix this.
>
> Signed-off-by: Tarun Kanti DebBarma <[email protected]>

Cool, I guess sparse would also have caught that error :-)

Acked-by: Felipe Balbi <[email protected]>

--
balbi

Attachments:

(No filename) (1.01 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-02-23 12:31:39

[permalink] [raw]

Subject: Re: [PATCH 6/6] gpio/omap: fix _set_gpio_irqenable implementation

On Thu, Feb 23, 2012 at 05:40:31PM +0530, Tarun Kanti DebBarma wrote:
> This function should be capable of both enabling and disabling interrupts
> based upon the *enable* parameter. Right now the function only enables
> the interrupt and *enable* is not used at all. So add the interrupt
> disable capability also using the parameter.
>
> Signed-off-by: Tarun Kanti DebBarma <[email protected]>

This one should probably be ported to stable releases, adding
stable@vger to the cc list

Acked-by: Felipe Balbi <[email protected]>

> ---
> drivers/gpio/gpio-omap.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 67535c8..acc71a0 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -473,7 +473,10 @@ static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
>
> static inline void _set_gpio_irqenable(struct gpio_bank *bank, int gpio, int enable)
> {
> - _enable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
> + if (enable)
> + _enable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
> + else
> + _disable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
> }
>
> /*
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
balbi

Attachments:

(No filename) (1.44 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-02-23 12:39:26

by Shubhrajyoti D

[permalink] [raw]

Subject: Re: [PATCH 5/6] gpio/omap: fix trigger type to unsigned

On Thursday 23 February 2012 05:40 PM, Tarun Kanti DebBarma wrote:
> The GPIO trigger parameter is of type unsigned.
> enum {
> IRQ_TYPE_NONE = 0x00000000,
> IRQ_TYPE_EDGE_RISING = 0x00000001,
> IRQ_TYPE_EDGE_FALLING = 0x00000002,
> IRQ_TYPE_EDGE_BOTH = (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING),
> IRQ_TYPE_LEVEL_HIGH = 0x00000004,
> IRQ_TYPE_LEVEL_LOW = 0x00000008,
> IRQ_TYPE_LEVEL_MASK = (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH),
> IRQ_TYPE_SENSE_MASK = 0x0000000f,
>
> IRQ_TYPE_PROBE = 0x00000010,
> ...
> };
> Even though gpio_irq_type(struct irq_data *d, unsigned type) has the right type
> of parameter, the subsequent called functions set_gpio_triggering() and
> set_gpio_trigger() wrongly makes it signed integer. Fix this.
>
> Signed-off-by: Tarun Kanti DebBarma <[email protected]>
> ---
> drivers/gpio/gpio-omap.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 3dd4b3a..67535c8 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -232,7 +232,7 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
> }
>
> static inline void set_gpio_trigger(struct gpio_bank *bank, int gpio,
> - int trigger)
> + unsigned trigger)
> {
> void __iomem *base = bank->base;
> u32 gpio_bit = 1 << gpio;
> @@ -314,7 +314,8 @@ static void _toggle_gpio_edge_triggering(struct gpio_bank *bank, int gpio)
> static void _toggle_gpio_edge_triggering(struct gpio_bank *bank, int gpio) {}
> #endif
>
> -static int _set_gpio_triggering(struct gpio_bank *bank, int gpio, int trigger)
> +static int _set_gpio_triggering(struct gpio_bank *bank, int gpio,
> + unsigned trigger)
you meant unsigned int ? could it be made u32.
> {
> void __iomem *reg = bank->base;
> void __iomem *base = bank->base;

2012-02-23 12:46:39

by Russell King - ARM Linux

[permalink] [raw]

Subject: Re: [PATCH 5/6] gpio/omap: fix trigger type to unsigned

On Thu, Feb 23, 2012 at 06:09:16PM +0530, Shubhrajyoti wrote:
> On Thursday 23 February 2012 05:40 PM, Tarun Kanti DebBarma wrote:
> > -static int _set_gpio_triggering(struct gpio_bank *bank, int gpio, int trigger)
> > +static int _set_gpio_triggering(struct gpio_bank *bank, int gpio,
> > + unsigned trigger)
> you meant unsigned int ? could it be made u32.

In C, unsigned is the same as unsigned int.

2012-02-27 23:50:28

[permalink] [raw]

Subject: Re: [PATCH 2/6] gpio/omap: remove saved_wakeup field from struct gpio_bank

Tarun Kanti DebBarma <[email protected]> writes:

> There is no more need to have saved_wakeup. Instead we can use
> context.wake_en which holds the current wakeup enable register
> context. This also means that the read from wakeup enable register
> is not needed.
>
> Signed-off-by: Tarun Kanti DebBarma <[email protected]>

Looks right, but one question below...

> ---
> drivers/gpio/gpio-omap.c | 13 ++++---------
> 1 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 40a1fb2..64f15d5 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -54,7 +54,6 @@ struct gpio_bank {
> u16 irq;
> u16 virtual_irq_start;
> u32 suspend_wakeup;
> - u32 saved_wakeup;
> u32 non_wakeup_gpios;
> u32 enabled_non_wakeup_gpios;
> struct gpio_regs context;
> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
> unsigned long flags;
>
> spin_lock_irqsave(&bank->lock, flags);
> - bank->saved_wakeup = __raw_readl(mask_reg);
> + bank->context.wake_en = __raw_readl(mask_reg);

Why is this read needed?

Kevin

> __raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
> spin_unlock_irqrestore(&bank->lock, flags);
>

2012-02-27 23:54:40

[permalink] [raw]

Subject: Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

Tarun Kanti DebBarma <[email protected]> writes:

> Since we already have bank->context.wake_en to keep track
> of gpios which are wakeup enabled, there is no need to have
> this field any more.
>
> Signed-off-by: Tarun Kanti DebBarma <[email protected]>

I'm not crazy about this change...

> ---
> drivers/gpio/gpio-omap.c | 11 +++++------
> 1 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 64f15d5..b62e861 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -53,7 +53,6 @@ struct gpio_bank {
> void __iomem *base;
> u16 irq;
> u16 virtual_irq_start;
> - u32 suspend_wakeup;
> u32 non_wakeup_gpios;
> u32 enabled_non_wakeup_gpios;
> struct gpio_regs context;
> @@ -497,9 +496,9 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
>
> spin_lock_irqsave(&bank->lock, flags);
> if (enable)
> - bank->suspend_wakeup |= gpio_bit;
> + bank->context.wake_en |= gpio_bit;
> else
> - bank->suspend_wakeup &= ~gpio_bit;
> + bank->context.wake_en &= ~gpio_bit;

The bank->context values are expected to be copies of the actual
register contents, and here that is clearly not the case.

With this change, you're using the context register to track changes
that you *might* eventually write to the register.

IMO, this is more confusing than having a separate field to track this.

Kevin

> spin_unlock_irqrestore(&bank->lock, flags);
>
> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>
> spin_lock_irqsave(&bank->lock, flags);
> bank->context.wake_en = __raw_readl(mask_reg);
> - __raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
> + __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
> spin_unlock_irqrestore(&bank->lock, flags);
>
> return 0;
> @@ -1137,12 +1136,12 @@ static int omap_gpio_suspend(struct device *dev)
> if (!bank->mod_usage || !bank->loses_context)
> return 0;
>
> - if (!bank->regs->wkup_en || !bank->suspend_wakeup)
> + if (!bank->regs->wkup_en || !bank->context.wake_en)
> return 0;
>
> spin_lock_irqsave(&bank->lock, flags);
> _gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
> - _gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
> + _gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
> spin_unlock_irqrestore(&bank->lock, flags);
>
> return 0;

2012-02-28 00:02:35

[permalink] [raw]

Subject: Re: [PATCH 4/6] gpio/omap: get rid of retrigger variable in gpio_irq_handler

Tarun Kanti DebBarma <[email protected]> writes:

> This local variable is just assigned zero and then OR'ed
> with isr. It does not appear to serve any purpose and so
> removing it.
>
> Signed-off-by: Tarun Kanti DebBarma <[email protected]>

Looks like the use of this was removed when I moved things over to using
the generic IRQ framework, but I didn't fully clean up.

Can you update the changelog to something along the lines of:

"commit 672e302e3c (ARM: OMAP: use edge/level handlers from generic IRQ
framework) removed retrigger support in favor of using generic IRQ
framework. This patch cleans up some unused remnants of that removal.

Thanks,

Kevin

> ---
> drivers/gpio/gpio-omap.c | 3 ---
> 1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index b62e861..3dd4b3a 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -623,7 +623,6 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> u32 isr;
> unsigned int gpio_irq, gpio_index;
> struct gpio_bank *bank;
> - u32 retrigger = 0;
> int unmasked = 0;
> struct irq_chip *chip = irq_desc_get_chip(desc);
>
> @@ -660,8 +659,6 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> - isr |= retrigger;
> - retrigger = 0;
> if (!isr)
> break;

2012-02-28 00:11:35

[permalink] [raw]

Subject: Re: [PATCH 6/6] gpio/omap: fix _set_gpio_irqenable implementation

Tarun Kanti DebBarma <[email protected]> writes:

> This function should be capable of both enabling and disabling interrupts
> based upon the *enable* parameter. Right now the function only enables
> the interrupt and *enable* is not used at all. So add the interrupt
> disable capability also using the parameter.
>
> Signed-off-by: Tarun Kanti DebBarma <[email protected]>

Hmm, interesting.

This means that the IRQ mask/unmask stuff is not actually doing anything
since it's always leaving the IRQ enabled. Curious that we haven't seen
side effects of that. Maybe since the trigger type is none, the
interrupts won't fire.

In any case, this is a good fix.

Kevin

> ---
> drivers/gpio/gpio-omap.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 67535c8..acc71a0 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -473,7 +473,10 @@ static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
>
> static inline void _set_gpio_irqenable(struct gpio_bank *bank, int gpio, int enable)
> {
> - _enable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
> + if (enable)
> + _enable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
> + else
> + _disable_gpio_irqbank(bank, GPIO_BIT(bank, gpio));
> }
>
> /*

2012-02-28 05:08:38

[permalink] [raw]

Subject: Re: [PATCH 2/6] gpio/omap: remove saved_wakeup field from struct gpio_bank

On Tue, Feb 28, 2012 at 5:20 AM, Kevin Hilman <[email protected]> wrote:
> Tarun Kanti DebBarma <[email protected]> writes:
>
>> There is no more need to have saved_wakeup. Instead we can use
>> context.wake_en which holds the current wakeup enable register
>> context. This also means that the read from wakeup enable register
>> is not needed.
>>
>> Signed-off-by: Tarun Kanti DebBarma <[email protected]>
>
> Looks right, but one question below...
>
>> ---
>> ?drivers/gpio/gpio-omap.c | ? 13 ++++---------
>> ?1 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 40a1fb2..64f15d5 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -54,7 +54,6 @@ struct gpio_bank {
>> ? ? ? u16 irq;
>> ? ? ? u16 virtual_irq_start;
>> ? ? ? u32 suspend_wakeup;
>> - ? ? u32 saved_wakeup;
>> ? ? ? u32 non_wakeup_gpios;
>> ? ? ? u32 enabled_non_wakeup_gpios;
>> ? ? ? struct gpio_regs context;
>> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>> ? ? ? unsigned long ? ? ? ? ? flags;
>>
>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>> - ? ? bank->saved_wakeup = __raw_readl(mask_reg);
>> + ? ? bank->context.wake_en = __raw_readl(mask_reg);
>
> Why is this read needed?
Well, we don't really need as we already have context.wake_en updated elsewhere.
I will update this. Thanks.
--
Tarun
>
> Kevin
>
>> ? ? ? __raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
>> ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>

2012-02-28 05:11:49

[permalink] [raw]

Subject: Re: [PATCH 4/6] gpio/omap: get rid of retrigger variable in gpio_irq_handler

On Tue, Feb 28, 2012 at 5:32 AM, Kevin Hilman <[email protected]> wrote:
> Tarun Kanti DebBarma <[email protected]> writes:
>
>> This local variable is just assigned zero and then OR'ed
>> with isr. It does not appear to serve any purpose and so
>> removing it.
>>
>> Signed-off-by: Tarun Kanti DebBarma <[email protected]>
>
> Looks like the use of this was removed when I moved things over to using
> the generic IRQ framework, but I didn't fully clean up.
>
> Can you update the changelog to something along the lines of:
>
> "commit 672e302e3c (ARM: OMAP: use edge/level handlers from generic IRQ
> framework) removed retrigger support in favor of using generic IRQ
> framework. ?This patch cleans up some unused remnants of that removal.
Yes, sure.
--
Tarun
>
> Thanks,
>
> Kevin
>
>> ---
>> ?drivers/gpio/gpio-omap.c | ? ?3 ---
>> ?1 files changed, 0 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index b62e861..3dd4b3a 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -623,7 +623,6 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>> ? ? ? u32 isr;
>> ? ? ? unsigned int gpio_irq, gpio_index;
>> ? ? ? struct gpio_bank *bank;
>> - ? ? u32 retrigger = 0;
>> ? ? ? int unmasked = 0;
>> ? ? ? struct irq_chip *chip = irq_desc_get_chip(desc);
>>
>> @@ -660,8 +659,6 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>> ? ? ? ? ? ? ? ? ? ? ? chained_irq_exit(chip, desc);
>> ? ? ? ? ? ? ? }
>>
>> - ? ? ? ? ? ? isr |= retrigger;
>> - ? ? ? ? ? ? retrigger = 0;
>> ? ? ? ? ? ? ? if (!isr)
>> ? ? ? ? ? ? ? ? ? ? ? break;

2012-02-28 09:39:09

[permalink] [raw]

Subject: Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

On Tue, Feb 28, 2012 at 5:24 AM, Kevin Hilman <[email protected]> wrote:
> Tarun Kanti DebBarma <[email protected]> writes:
>
>> Since we already have bank->context.wake_en to keep track
>> of gpios which are wakeup enabled, there is no need to have
>> this field any more.
>>
>> Signed-off-by: Tarun Kanti DebBarma <[email protected]>
>
> I'm not crazy about this change...
>
>> ---
>> ?drivers/gpio/gpio-omap.c | ? 11 +++++------
>> ?1 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 64f15d5..b62e861 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -53,7 +53,6 @@ struct gpio_bank {
>> ? ? ? void __iomem *base;
>> ? ? ? u16 irq;
>> ? ? ? u16 virtual_irq_start;
>> - ? ? u32 suspend_wakeup;
>> ? ? ? u32 non_wakeup_gpios;
>> ? ? ? u32 enabled_non_wakeup_gpios;
>> ? ? ? struct gpio_regs context;
>> @@ -497,9 +496,9 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
>>
>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>> ? ? ? if (enable)
>> - ? ? ? ? ? ? bank->suspend_wakeup |= gpio_bit;
>> + ? ? ? ? ? ? bank->context.wake_en |= gpio_bit;
>> ? ? ? else
>> - ? ? ? ? ? ? bank->suspend_wakeup &= ~gpio_bit;
>> + ? ? ? ? ? ? bank->context.wake_en &= ~gpio_bit;
>
> The bank->context values are expected to be copies of the actual
> register contents, and here that is clearly not the case.
Right, it should have been this:

if (enable)
- bank->suspend_wakeup |= gpio_bit;
+ bank->context.wake_en |= gpio_bit;
else
- bank->suspend_wakeup &= ~gpio_bit;
+ bank->context.wake_en &= ~gpio_bit;
+
+ __raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en);

>
> With this change, you're using the context register to track changes
> that you *might* eventually write to the register.
The above change ensures that bank->context.wake_en reflects the
latest register value.
There are two distinct paths through which bank->context.wake_en is
updated now, viz:
Path1:-
chip.irq_set_type() --> gpio_irq_type() --> _set_gpio_triggering() -->
set_gpio_trigger()

Path2:-
chip.irq_set_wake() --> gpio_wake_enable() --> irq_set_wake()

>
> IMO, this is more confusing than having a separate field to track this.
So, there is no need have a separate field to keep track of this.
I hope my understanding is right.
--
Tarun

>
> Kevin
>
>> ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>
>> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>
>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>> ? ? ? bank->context.wake_en = __raw_readl(mask_reg);
>> - ? ? __raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
>> + ? ? __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
>> ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>
>> ? ? ? return 0;
>> @@ -1137,12 +1136,12 @@ static int omap_gpio_suspend(struct device *dev)
>> ? ? ? if (!bank->mod_usage || !bank->loses_context)
>> ? ? ? ? ? ? ? return 0;
>>
>> - ? ? if (!bank->regs->wkup_en || !bank->suspend_wakeup)
>> + ? ? if (!bank->regs->wkup_en || !bank->context.wake_en)
>> ? ? ? ? ? ? ? return 0;
>>
>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>> ? ? ? _gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
>> - ? ? _gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
>> + ? ? _gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
>> ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>
>> ? ? ? return 0;

2012-02-28 11:15:24

[permalink] [raw]

Subject: Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

On Tue, Feb 28, 2012 at 3:09 PM, DebBarma, Tarun Kanti
<[email protected]> wrote:
> On Tue, Feb 28, 2012 at 5:24 AM, Kevin Hilman <[email protected]> wrote:
>> Tarun Kanti DebBarma <[email protected]> writes:
>>
>>> Since we already have bank->context.wake_en to keep track
>>> of gpios which are wakeup enabled, there is no need to have
>>> this field any more.
>>>
>>> Signed-off-by: Tarun Kanti DebBarma <[email protected]>
>>
>> I'm not crazy about this change...
>>
>>> ---
>>> ?drivers/gpio/gpio-omap.c | ? 11 +++++------
>>> ?1 files changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 64f15d5..b62e861 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -53,7 +53,6 @@ struct gpio_bank {
>>> ? ? ? void __iomem *base;
>>> ? ? ? u16 irq;
>>> ? ? ? u16 virtual_irq_start;
>>> - ? ? u32 suspend_wakeup;
>>> ? ? ? u32 non_wakeup_gpios;
>>> ? ? ? u32 enabled_non_wakeup_gpios;
>>> ? ? ? struct gpio_regs context;
>>> @@ -497,9 +496,9 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
>>>
>>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>>> ? ? ? if (enable)
>>> - ? ? ? ? ? ? bank->suspend_wakeup |= gpio_bit;
>>> + ? ? ? ? ? ? bank->context.wake_en |= gpio_bit;
>>> ? ? ? else
>>> - ? ? ? ? ? ? bank->suspend_wakeup &= ~gpio_bit;
>>> + ? ? ? ? ? ? bank->context.wake_en &= ~gpio_bit;
>>
>> The bank->context values are expected to be copies of the actual
>> register contents, and here that is clearly not the case.
> Right, it should have been this:
>
> ? ? ? ?if (enable)
> - ? ? ? ? ? ? ? bank->suspend_wakeup |= gpio_bit;
> + ? ? ? ? ? ? ? bank->context.wake_en |= gpio_bit;
> ? ? ? ?else
> - ? ? ? ? ? ? ? bank->suspend_wakeup &= ~gpio_bit;
> + ? ? ? ? ? ? ? bank->context.wake_en &= ~gpio_bit;
> +
> + ? ? ? __raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en);
>
>>
>> With this change, you're using the context register to track changes
>> that you *might* eventually write to the register.
> The above change ensures that bank->context.wake_en reflects the
> latest register value.
> There are two distinct paths through which bank->context.wake_en is
> updated now, viz:
> Path1:-
> chip.irq_set_type() --> gpio_irq_type() --> _set_gpio_triggering() -->
> set_gpio_trigger()
>
> Path2:-
> chip.irq_set_wake() --> gpio_wake_enable() --> irq_set_wake()
Sorry, it should have been:
chip.irq_set_wake() --> gpio_wake_enable() --> _set_gpio_wakeup()

>
>>
>> IMO, this is more confusing than having a separate field to track this.
> So, there is no need have a separate field to keep track of this.
> I hope my understanding is right.
> --
> Tarun
>
>>
>> Kevin
>>
>>> ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>>
>>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>>> ? ? ? bank->context.wake_en = __raw_readl(mask_reg);
>>> - ? ? __raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
>>> + ? ? __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
>>> ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>> ? ? ? return 0;
>>> @@ -1137,12 +1136,12 @@ static int omap_gpio_suspend(struct device *dev)
>>> ? ? ? if (!bank->mod_usage || !bank->loses_context)
>>> ? ? ? ? ? ? ? return 0;
>>>
>>> - ? ? if (!bank->regs->wkup_en || !bank->suspend_wakeup)
>>> + ? ? if (!bank->regs->wkup_en || !bank->context.wake_en)
>>> ? ? ? ? ? ? ? return 0;
>>>
>>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>>> ? ? ? _gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
>>> - ? ? _gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
>>> + ? ? _gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
>>> ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>> ? ? ? return 0;

2012-02-28 18:46:03

[permalink] [raw]

Subject: Re: [PATCH 3/6] gpio/omap: remove suspend_wakeup field from struct gpio_bank

"DebBarma, Tarun Kanti" <[email protected]> writes:

> On Tue, Feb 28, 2012 at 5:24 AM, Kevin Hilman <[email protected]> wrote:
>> Tarun Kanti DebBarma <[email protected]> writes:
>>
>>> Since we already have bank->context.wake_en to keep track
>>> of gpios which are wakeup enabled, there is no need to have
>>> this field any more.
>>>
>>> Signed-off-by: Tarun Kanti DebBarma <[email protected]>
>>
>> I'm not crazy about this change...
>>
>>> ---
>>> drivers/gpio/gpio-omap.c | 11 +++++------
>>> 1 files changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 64f15d5..b62e861 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -53,7 +53,6 @@ struct gpio_bank {
>>> void __iomem *base;
>>> u16 irq;
>>> u16 virtual_irq_start;
>>> - u32 suspend_wakeup;
>>> u32 non_wakeup_gpios;
>>> u32 enabled_non_wakeup_gpios;
>>> struct gpio_regs context;
>>> @@ -497,9 +496,9 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
>>>
>>> spin_lock_irqsave(&bank->lock, flags);
>>> if (enable)
>>> - bank->suspend_wakeup |= gpio_bit;
>>> + bank->context.wake_en |= gpio_bit;
>>> else
>>> - bank->suspend_wakeup &= ~gpio_bit;
>>> + bank->context.wake_en &= ~gpio_bit;
>>
>> The bank->context values are expected to be copies of the actual
>> register contents, and here that is clearly not the case.
> Right, it should have been this:
>
> if (enable)
> - bank->suspend_wakeup |= gpio_bit;
> + bank->context.wake_en |= gpio_bit;
> else
> - bank->suspend_wakeup &= ~gpio_bit;
> + bank->context.wake_en &= ~gpio_bit;
> +
> + __raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en);
>
>>
>> With this change, you're using the context register to track changes
>> that you *might* eventually write to the register.
> The above change ensures that bank->context.wake_en reflects the
> latest register value.

OK, but that changes the behavior of the current code.

The current code *only* writes this register in suspend and resume.
_set_gpio_wakeup() just records the value that is going to be written in
suspend.

Now, I'm not saying we shouldn't make the changes you propose above. We
probably should be updating the wake-enable register whenever
_set_gpio_wakeup() is run so that GPIO wakeups work across runtime
suspend/resume as well.

However, you should probably make that functional change a separate
patch *before* you do $SUBJECT patch which just changes the variable
used to cache the register contents.

Kevin

> There are two distinct paths through which bank->context.wake_en is
> updated now, viz:
> Path1:-
> chip.irq_set_type() --> gpio_irq_type() --> _set_gpio_triggering() -->
> set_gpio_trigger()
>
> Path2:-
> chip.irq_set_wake() --> gpio_wake_enable() --> irq_set_wake()
>
>>
>> IMO, this is more confusing than having a separate field to track this.
> So, there is no need have a separate field to keep track of this.
> I hope my understanding is right.
> --
> Tarun
>
>>
>> Kevin
>>
>>> spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>> @@ -772,7 +771,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
>>>
>>> spin_lock_irqsave(&bank->lock, flags);
>>> bank->context.wake_en = __raw_readl(mask_reg);
>>> - __raw_writel(0xffff & ~bank->suspend_wakeup, mask_reg);
>>> + __raw_writel(0xffff & ~bank->context.wake_en, mask_reg);
>>> spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>> return 0;
>>> @@ -1137,12 +1136,12 @@ static int omap_gpio_suspend(struct device *dev)
>>> if (!bank->mod_usage || !bank->loses_context)
>>> return 0;
>>>
>>> - if (!bank->regs->wkup_en || !bank->suspend_wakeup)
>>> + if (!bank->regs->wkup_en || !bank->context.wake_en)
>>> return 0;
>>>
>>> spin_lock_irqsave(&bank->lock, flags);
>>> _gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
>>> - _gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
>>> + _gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
>>> spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>> return 0;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2012-02-29 04:19:00