From: khilman@ti•com (Kevin Hilman)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_*
Date: Tue, 13 Mar 2012 09:27:21 -0700 [thread overview]
Message-ID: <8762e8v53q.fsf@ti.com> (raw)
In-Reply-To: <CAC83ZvLmaLOr4_eZZzKyP2=KvkG+LjHjeOwvxdQHhuiB8+2L0w@mail.gmail.com> (Tarun Kanti DebBarma's message of "Tue, 13 Mar 2012 12:22:28 +0530")
"DebBarma, Tarun Kanti" <tarun.kanti@ti•com> writes:
> On Tue, Mar 13, 2012 at 12:03 PM, DebBarma, Tarun Kanti
> <tarun.kanti@ti•com> wrote:
>> On Tue, Mar 13, 2012 at 11:33 AM, DebBarma, Tarun Kanti
>> <tarun.kanti@ti•com> wrote:
>>> On Tue, Mar 13, 2012 at 3:24 AM, Kevin Hilman <khilman@ti•com> wrote:
>>>> Tarun Kanti DebBarma <tarun.kanti@ti•com> writes:
>>>>
>>>>> In the existing _set_gpio_dataout_*() implementation, the dataout
>>>>> register is overwritten every time the function is called. This is
>>>>> not intended behavior because that would end up one user of a GPIO
>>>>> line overwriting what is written by another. Fix this so that previous
>>>>> value is always preserved until explicitly changed by respective
>>>>> user/driver of the GPIO line.
>>>>>
>>>>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti•com>
>>>>> ---
>>>>> ?drivers/gpio/gpio-omap.c | ? ?3 +++
>>>>> ?1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>>> index 04c2677..2e8e476 100644
>>>>> --- a/drivers/gpio/gpio-omap.c
>>>>> +++ b/drivers/gpio/gpio-omap.c
>>>>> @@ -114,6 +114,7 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)
>>>>> ? ? ? else
>>>>> ? ? ? ? ? ? ? reg += bank->regs->clr_dataout;
>>>>>
>>>>> + ? ? l |= __raw_readl(bank->base + bank->regs->set_dataout);
>>>>
>>>> minor: IMO, it's more reader-friendly if this looks like
>>>>
>>>> ? ? ? l = __raw_read(...)
>>>> ? ? ? l |= GPIO_BIT(...)
>>>> ? ? ? __raw_write(...)
>>> Agreed. I will make the change.
>> Also, the read should be: __raw_readl(bank->base + bank->regs->dataout);
>> instead of bank->regs->set_dataout.
> I see a problem with this implementation. It is not correct to write l
> |= GPIO_BIT(...).
> For example if we write to clr_dataout register we would end up
> clearing bits which
> we are not supposed to. We should just be operating on current GPIO_BIT(...).
> The l |= GPIO_BIT(...) is needed just to make sure that we have the
> right context
> stored. So the overall sequence should be something like this:
>
> void __iomem *reg = bank->base;
> u32 l = GPIO_BIT(bank, gpio);
>
> if (enable)
> reg += bank->regs->set_dataout;
> else
> reg += bank->regs->clr_dataout;
>
> __raw_writel(l, reg);
> l |= __raw_readl(bank->base + bank->regs->dataout);
> bank->context.dataout = l;
Again, you don't need the extra read-back here. Just set/clear the bit
in the context. Untested patch below.
Kevin
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 0b05629..db905c0 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -111,10 +111,13 @@ static void _set_gpio_dataout_reg(struct gpio_bank *bank, int gpio, int enable)
void __iomem *reg = bank->base;
u32 l = GPIO_BIT(bank, gpio);
- if (enable)
+ if (enable) {
reg += bank->regs->set_dataout;
- else
+ bank->context.dataout |= l;
+ } else {
reg += bank->regs->clr_dataout;
+ bank->context.dataout &= ~l;
+ }
next prev parent reply other threads:[~2012-03-13 16:27 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-03-07 11:15 [PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes Tarun Kanti DebBarma
2012-03-07 11:15 ` [PATCH v3 01/13] gpio/omap: remove saved_fallingdetect, saved_risingdetect fields Tarun Kanti DebBarma
2012-03-07 11:15 ` [PATCH v3 02/13] gpio/omap: fix wakeup_en register update in _set_gpio_wakeup() Tarun Kanti DebBarma
2012-03-07 11:59 ` Santosh Shilimkar
2012-03-07 11:15 ` [PATCH v3 03/13] gpio/omap: remove suspend_wakeup field from struct gpio_bank Tarun Kanti DebBarma
2012-03-07 11:15 ` [PATCH v3 04/13] gpio/omap: remove saved_wakeup " Tarun Kanti DebBarma
2012-03-07 11:15 ` [PATCH v3 05/13] gpio/omap: get rid of retrigger variable in gpio_irq_handler Tarun Kanti DebBarma
2012-03-09 8:46 ` DebBarma, Tarun Kanti
2012-03-12 18:53 ` Kevin Hilman
2012-03-12 18:52 ` Kevin Hilman
2012-03-07 11:15 ` [PATCH v3 06/13] gpio/omap: fix trigger type to unsigned Tarun Kanti DebBarma
2012-03-07 11:15 ` [PATCH v3 07/13] gpio/omap: fix _set_gpio_irqenable implementation Tarun Kanti DebBarma
2012-03-07 11:15 ` [PATCH v3 08/13] gpio/omap: remove redundant decoding of gpio offset Tarun Kanti DebBarma
2012-03-07 12:00 ` Santosh Shilimkar
2012-03-07 11:15 ` [PATCH v3 09/13] gpio/omap: remove suspend/resume callbacks Tarun Kanti DebBarma
2012-03-07 12:01 ` Santosh Shilimkar
2012-03-07 11:16 ` [PATCH v3 10/13] gpio/omap: fix missing dataout context save in _set_gpio_dataout_reg Tarun Kanti DebBarma
2012-03-07 12:03 ` Santosh Shilimkar
2012-03-08 3:34 ` DebBarma, Tarun Kanti
2012-03-07 11:16 ` [PATCH v3 11/13] gpio/omap: fix dataout register overwrite in _set_gpio_dataout_* Tarun Kanti DebBarma
2012-03-07 12:04 ` Santosh Shilimkar
2012-03-12 21:54 ` Kevin Hilman
2012-03-13 6:03 ` DebBarma, Tarun Kanti
2012-03-13 6:33 ` DebBarma, Tarun Kanti
2012-03-13 6:52 ` DebBarma, Tarun Kanti
2012-03-13 16:27 ` Kevin Hilman [this message]
2012-03-14 1:53 ` DebBarma, Tarun Kanti
2012-03-07 11:16 ` [PATCH v3 12/13] gpio/omap: fix incorrect context restore logic in omap_gpio_runtime_resume Tarun Kanti DebBarma
2012-03-07 12:07 ` Santosh Shilimkar
2012-03-08 3:58 ` DebBarma, Tarun Kanti
2012-03-08 7:19 ` Shilimkar, Santosh
2012-03-09 9:25 ` DebBarma, Tarun Kanti
2012-03-07 11:16 ` [PATCH v3 13/13] gpio/omap: fix incorrect update to context.irqenable1 Tarun Kanti DebBarma
2012-03-07 12:09 ` Santosh Shilimkar
2012-03-12 22:09 ` Kevin Hilman
2012-03-13 5:31 ` DebBarma, Tarun Kanti
2012-03-12 18:54 ` [PATCH v3 00/13] gpio/omap: Some more driver cleanup and fixes Kevin Hilman
2012-03-12 19:53 ` DebBarma, Tarun Kanti
2012-03-12 20:08 ` DebBarma, Tarun Kanti
2012-03-12 20:27 ` DebBarma, Tarun Kanti
2012-03-12 22:17 ` Kevin Hilman
2012-03-12 22:28 ` Kevin Hilman
2012-03-13 3:57 ` Grant Likely
2012-03-13 4:35 ` DebBarma, Tarun Kanti
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8762e8v53q.fsf@ti.com \
--to=khilman@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox