From: Tarun Kanti DebBarma <hidden> Date: 2012-02-29 22:44:20
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
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
v2:
- Added a new patch to update wakeup_en register in _set_gpio_wakeup()
in addition to updating bank->context.wake_en.
- Added a new patch to remove redundant decoding of gpio offset in
gpio_get(), _get_gpio_datain() and _get_gpio_dataout().
- Added a new patch to remove suspend/resume callbacks because the
operations performed with the callbacks are redundant.
Tarun Kanti DebBarma (9):
gpio/omap: remove saved_fallingdetect, saved_risingdetect fields
gpio/omap: fix wakeup_en register update in _set_gpio_wakeup()
gpio/omap: remove suspend_wakeup field from struct gpio_bank
gpio/omap: remove saved_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
gpio/omap: remove redundant decoding of gpio offset
gpio/omap: remove suspend/resume callbacks
drivers/gpio/gpio-omap.c | 113 +++++++++++-----------------------------------
1 files changed, 27 insertions(+), 86 deletions(-)
From: Tarun Kanti DebBarma <hidden> Date: 2012-02-29 22:44:21
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 <redacted>
Reviewed-by: Santosh Shilimkar <redacted>
Acked-by: Felipe Balbi <redacted>
---
drivers/gpio/gpio-omap.c | 19 ++++++++-----------
1 files changed, 8 insertions(+), 11 deletions(-)
From: Tarun Kanti DebBarma <hidden> Date: 2012-02-29 22:44:22
There are two ways through which wakeup_en register can be programmed
using gpiolib APIs as shown below. It is seen that in the second case
in _set_gpio_wakeup(), even though bank->suspend_wakeup is updated
correctly, its value is not programmed in wakeup_en register. Fix this.
chip.irq_set_type()->gpio_irq_type()->_set_gpio_triggering()->set_gpio_trigger()
chip.irq_set_wake()->gpio_wake_enable()->_set_gpio_wakeup()
Signed-off-by: Tarun Kanti DebBarma <redacted>
---
drivers/gpio/gpio-omap.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
@@ -504,6 +504,7 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)elsebank->suspend_wakeup&=~gpio_bit;+__raw_writel(bank->suspend_wakeup,bank->base+bank->regs->wkup_en);spin_unlock_irqrestore(&bank->lock,flags);return0;
From: Tarun Kanti DebBarma <hidden> Date: 2012-02-29 22:44:23
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 <redacted>
Reviewed-by: Santosh Shilimkar <redacted>
Acked-by: Felipe Balbi <redacted>
---
drivers/gpio/gpio-omap.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
@@ -500,11 +499,11 @@ 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;-__raw_writel(bank->suspend_wakeup,bank->base+bank->regs->wkup_en);+__raw_writel(bank->context.wake_en,bank->base+bank->regs->wkup_en);spin_unlock_irqrestore(&bank->lock,flags);return0;
@@ -776,7 +775,7 @@ static int omap_mpuio_suspend_noirq(struct device *dev)spin_lock_irqsave(&bank->lock,flags);bank->saved_wakeup=__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);return0;
@@ -1150,7 +1149,7 @@ static int omap_gpio_suspend(struct device *dev)if(!bank->mod_usage||!bank->loses_context)return0;-if(!bank->regs->wkup_en||!bank->suspend_wakeup)+if(!bank->regs->wkup_en||!bank->context.wake_en)return0;wakeup_enable=bank->base+bank->regs->wkup_en;
@@ -1158,7 +1157,7 @@ static int omap_gpio_suspend(struct device *dev)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);+_gpio_rmw(base,bank->regs->wkup_en,bank->context.wake_en,1);spin_unlock_irqrestore(&bank->lock,flags);return0;
From: Tarun Kanti DebBarma <hidden> Date: 2012-02-29 22:44:24
There is no more need to have saved_wakeup because bank->context.wake_en
already holds that value. So getting rid of read/write operation associated
with this field.
Signed-off-by: Tarun Kanti DebBarma <redacted>
Reviewed-by: Santosh Shilimkar <redacted>
Acked-by: Felipe Balbi <redacted>
---
drivers/gpio/gpio-omap.c | 12 +++---------
1 files changed, 3 insertions(+), 9 deletions(-)
From: Tarun Kanti DebBarma <hidden> Date: 2012-02-29 22:44:25
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 <redacted>
Reviewed-by: Santosh Shilimkar <redacted>
Acked-by: Felipe Balbi <redacted>
---
drivers/gpio/gpio-omap.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
From: Tarun Kanti DebBarma <hidden> Date: 2012-02-29 22:44:26
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 <redacted>
Reviewed-by: Santosh Shilimkar <redacted>
Acked-by: Felipe Balbi <redacted>
---
drivers/gpio/gpio-omap.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
From: Tarun Kanti DebBarma <hidden> Date: 2012-02-29 22:44:27
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 <redacted>
Reviewed-by: Santosh Shilimkar <redacted>
Acked-by: Felipe Balbi <redacted>
---
drivers/gpio/gpio-omap.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
From: Tarun Kanti DebBarma <hidden> Date: 2012-02-29 22:44:28
In gpio_get(), _get_gpio_datain() and _get_gpio_dataout() get rid of
un-necessary operation to compute gpio mask. The gpio offset passed
to gpio_get() is sufficient to do that.
Here is Russell's original comment:
Can someone explain to me this:
#define GPIO_INDEX(bank, gpio) (gpio % bank->width)
#define GPIO_BIT(bank, gpio) (1 << GPIO_INDEX(bank, gpio))
static int _get_gpio_datain(struct gpio_bank *bank, int gpio)
{
void __iomem *reg = bank->base + bank->regs->datain;
return (__raw_readl(reg) & GPIO_BIT(bank, gpio)) != 0;
}
static int gpio_get(struct gpio_chip *chip, unsigned offset)
{
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
void __iomem *reg = bank->base;
int gpio = chip->base + offset;
u32 mask = GPIO_BIT(bank, gpio);
if (gpio_is_input(bank, mask))
return _get_gpio_datain(bank, gpio);
else
return _get_gpio_dataout(bank, gpio);
}
Given that bank->width on OMAP is either 32 or 16, and GPIO numbers for
any GPIO chip are always aligned to 32 or 16, why does this code bother
adding the chips base gpio number and then modulo the width?
Surely this means if - for argument sake - you registered a GPIO chip
with 8 lines followed by one with 16 lines, GPIO0..7 would be chip 0
bit 0..7, GPIO8..15 would be chip 1 bit 8..15, GPIO16..23 would be
chip 1 bit 0..7.
However, if you registered a GPIO chip with 16 lines first, it would
mean GPIO0..15 would be chip 0 bit 0..15, and GPIO16..31 would be
chip 1 bit 0..15.
Surely this kind of behaviour is not intended?
Is there a reason why the bitmask can't just be (1 << offset) where
offset is passed into these functions as GPIO number - chip->base ?
Reported-by: Russell King - ARM Linux <redacted>
Signed-off-by: Tarun Kanti DebBarma <redacted>
---
drivers/gpio/gpio-omap.c | 18 +++++++-----------
1 files changed, 7 insertions(+), 11 deletions(-)
From: Tarun Kanti DebBarma <hidden> Date: 2012-02-29 22:44:29
Both omap_gpio_suspend() and omap_gpio_resume() does programming
of wakeup_en register.
_gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0);
_gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1);
This is redundant in omap_gpio_suspend() because wakeup_en
register automatically gets initialized in _set_gpio_wakeup()
and set_gpio_trigger() while being called either from
chip.irq_set_wake() or chip.irq_set_type().
This is redundant in omap_gpio_resume() because wakeup_en
register is programmed in omap_gpio_restore_context() called
which is called from runtime resume callback.
Signed-off-by: Tarun Kanti DebBarma <redacted>
---
drivers/gpio/gpio-omap.c | 47 ----------------------------------------------
1 files changed, 0 insertions(+), 47 deletions(-)