[PATCH] gpio/omap: fix incorrect initialization of omap_gpio_mod_init

[DebBarma, Tarun Kanti]

On Sat, Apr 28, 2012 at 3:48 AM, Kevin Hilman <khilman at ti.com> wrote:
> Tarun Kanti DebBarma <tarun.kanti at ti.com> writes:
>
>> Breaking commit: ab985f0f7c2c0ef90b7c832f0c04f470dda0593d
>>
>> Initialization of irqenable, irqstatus registers is the common
>> operation done in this function for all OMAP platforms, viz.
>> OMAP1, OMAP2+. The latter _gpio_rmw()'s to irqenable register
>> was overwriting the correctly programmed OMAP1 value at the
>> beginning. As a result, even though it worked on OMAP2+
>> platforms it was breaking OMAP1 functionality. On close
>> observation it is found that the first _gpio_rmw() which is
>> supposedly done to take care of OMAP1 platform is generic enough
>> and takes care of OMAP2+ platform as well. Therefore remove the
>> latter _gpio_rmw() to irqenable as they are redundant now.
>>
>> Writing to ctrl and debounce_en registers for OMAP2+ platforms
>> are modified to match the original(pre-cleanup) code where the
>> registers are initialized with 0. In the cleanup series since
>> we are using _gpio_rmw(reg, 0, 1), instead of __raw_writel(),
>> we are just reading and writing the same values to ctrl and
>> debounce_en. This is not an issue for debounce_en register
>> because it has 0x0 as the default value. But in the case of
>> ctrl register the default value is 0x2 (GATINGRATIO = 0x1)
>> so that we end up writing 0x2 instead of intended 0 value.
>> Therefore correcting it to _gpio_rmw(reg, l, 0), where
>> l = 0xffffffff so that registers are initialized to 0.
>>
>> Also, changing the sequence and logic of initializing the irqstatus.
>>
>> Cc: stable at vger.kernel.org
>> Cc: Tony Lindgren <tony at atomide.com>
>> Cc: Kevin Hilman <khilman at ti.com>
>> Cc: Santosh Shilimkar <santosh.shilimkar at ti.com>
>> Cc: Grant Likely <grant.likely at secretlab.ca>
>> Reported-by: Janusz Krzysztofik <jkrzyszt at tis.icnet.pl>
>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti at ti.com>
>> ---
>> Tested on OMAP2+ platforms and bootup test on OMAP1710.
>> Janusz,
>> Please help me in validating the patch on OMAP1 platform.
>>
>>  drivers/gpio/gpio-omap.c |    9 +++------
>>  1 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 1adc2ec..910fd14 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -964,19 +964,16 @@ static void omap_gpio_mod_init(struct gpio_bank *bank)
>>               return;
>>       }
>>
>> +     _gpio_rmw(base, bank->regs->irqstatus, l, !bank->regs->irqenable_inv);
>>       _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->irqenable_inv);
>> -     _gpio_rmw(base, bank->regs->irqstatus, l,
>> -                                     bank->regs->irqenable_inv == false);
>
> Not sure I follow this change.  It seems the ending value in irqstatus
> will be the same, but it was just moved before irqenable.
Yes, it is just moved before irqenable and instead of:
bank->regs->irqenable_inv == false, I made it !bank->regs->irqenable_inv
so that it fits in single line and avoid comparison with bool.

>
> You mention changing this order in the changelog, but do not mention why.
If we consider the overall code sequence it was something like:

_gpio_rmw(base, bank->regs->irqenable, l, <condition1>);
_gpio_rmw(base, bank->regs->irqstatus, l, <condition2>);
_gpio_rmw(base, bank->regs->irqenable, l, <condition3>);
_gpio_rmw(base, bank->regs->irqenable, l, <condition4>);

So I decided to move irqstatus first (beside the changing the logic
mentioned above)
and keep all irqenable settings together. However, with just single
irqenable setting
now this does not make any difference. In short, I can just change the
logic while
keeping it in place.

>
>> -     _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->debounce_en != 0);
>> -     _gpio_rmw(base, bank->regs->irqenable, l, bank->regs->ctrl != 0);
>
> Why were these here in the first place?
These we supposedly introduced wrongly to take care of OMAP2+ platforms.
Also, I somehow made incorrect assumption that each _gpio_rmw()'s were
mutually exclusive. In fact this is what led to overwrite of initial
OMAP1 setting.

>
> The changelog is not very clear about *why* these can be removed.
I can update changelog with what I have just said above.

>
>>       if (bank->regs->debounce_en)
>> -             _gpio_rmw(base, bank->regs->debounce_en, 0, 1);
>> +             _gpio_rmw(base, bank->regs->debounce_en, l, 0);
>
> Why the read/modify/write?  Why not just go back to the __raw_write?
We can use __raw_writel() as before.

>
>>       /* Save OE default value (0xffffffff) in the context */
>>       bank->context.oe = __raw_readl(bank->base + bank->regs->direction);
>>        /* Initialize interface clk ungated, module enabled */
>>       if (bank->regs->ctrl)
>> -             _gpio_rmw(base, bank->regs->ctrl, 0, 1);
>> +             _gpio_rmw(base, bank->regs->ctrl, l, 0);
>
> same question.
Yes, here also we can use the __raw_writel().
--
Tarun
>
>>  }
>>
>>  static __devinit void
>
> Kevin