[U-Boot] [PATCH RESEND] gpio: spear_gpio: Fix gpio_set_value() implementation

[U-Boot] [PATCH RESEND] gpio: spear_gpio: Fix gpio_set_value() implementation

[Axel Lin]

In current gpio_set_value() implementation, it always sets the gpio control bit
no matter the value argument is 0 or 1. Thus the GPIOs never set to low.
This patch fixes this bug.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
Acked-by: Stefan Roese <sr@denx.de>
Reviewed-by: Vipin Kumar <vipin.kumar@st.com>
---
This patch was sent on http://lists.denx.de/pipermail/u-boot/2013-June/156861.html

Has Stefan's Ack:
http://lists.denx.de/pipermail/u-boot/2013-June/156864.html

Vipin says the code is fine, so I add Vipin's review-by.
http://lists.denx.de/pipermail/u-boot/2013-June/156966.html

Michael confirms it works:
http://lists.denx.de/pipermail/u-boot/2013-August/160652.html

No body picks up this patch, so here is a resend.
Although I think this is a bug fix, but I'll let maintainers to determinate
if this is the material for v2013.10.
Anyway, can someone at least let me know if this patch is ok for apply at some
point? I have no idea who is maintaining this file.

Regards,
Axel

 drivers/gpio/spear_gpio.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c
index 367b670..6fb4117 100644
--- a/drivers/gpio/spear_gpio.c
+++ b/drivers/gpio/spear_gpio.c
@@ -36,7 +36,10 @@ int gpio_set_value(unsigned gpio, int value)
 {
 	struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;
 
-	writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
+	if (value)
+		writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
+	else
+		writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
 
 	return 0;
 }
-- 
1.8.1.2

[U-Boot] [PATCH RESEND] gpio: spear_gpio: Fix gpio_set_value() implementation

[Albert ARIBAUD]

Hi Axel,

On Fri, 06 Sep 2013 14:22:40 +0800, Axel Lin <axel.lin@ingics.com>
wrote:

> In current gpio_set_value() implementation, it always sets the gpio control bit
> no matter the value argument is 0 or 1. Thus the GPIOs never set to low.
> This patch fixes this bug.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> Acked-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Vipin Kumar <vipin.kumar@st.com>
> ---
> This patch was sent on http://lists.denx.de/pipermail/u-boot/2013-June/156861.html
> 
> Has Stefan's Ack:
> http://lists.denx.de/pipermail/u-boot/2013-June/156864.html
> 
> Vipin says the code is fine, so I add Vipin's review-by.
> http://lists.denx.de/pipermail/u-boot/2013-June/156966.html
> 
> Michael confirms it works:
> http://lists.denx.de/pipermail/u-boot/2013-August/160652.html
> 
> No body picks up this patch, so here is a resend.
> Although I think this is a bug fix, but I'll let maintainers to determinate
> if this is the material for v2013.10.
> Anyway, can someone at least let me know if this patch is ok for apply at some
> point? I have no idea who is maintaining this file.
> 
> Regards,
> Axel
> 
>  drivers/gpio/spear_gpio.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c
> index 367b670..6fb4117 100644
> --- a/drivers/gpio/spear_gpio.c
> +++ b/drivers/gpio/spear_gpio.c
> @@ -36,7 +36,10 @@ int gpio_set_value(unsigned gpio, int value)
>  {
>  	struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;
>  
> -	writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
> +	if (value)
> +		writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
> +	else
> +		writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>  
>  	return 0;
>  }

Despite discussions in the previous thread and the confirmations that
this code is functionally equivalent to the Linux code, I still believe
this code is incorrect for both writing and reading.

From the doc, writing to GPIODATA will obviously copy each of bits 7
to 0 of the written value into the actuals GPIO mapped to bits 7 to
0 of this register (assuming they are configured as outputs, of course).
Based on this, the code above:

- when setting a single GPIO, sets *but clears up to seven other GPIOs*;
- when clearing a single GPIO, clears it *and up to seven other GPIOs*.

This code may have been tested only for a single active GPIO at a time,
for which this code would behave correctly; but as soon as two GPIOs
from the same bank must be set at the same time, it fails.

Please fix this code so that setting or clearing a GPIO does not set or
clear any other GPIO, and perform an actual test to confirm this works
before submitting V2.

BTW: if (as the previous thread seemed to imply) no one around has the
hardware to test this change, then why exactly is it needed?

Amicalement,
-- 
Albert.

[U-Boot] [PATCH RESEND] gpio: spear_gpio: Fix gpio_set_value() implementation

[Michael Trimarchi]

Hi Albert

On Sat, Sep 14, 2013 at 11:10 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Axel,
>
> On Fri, 06 Sep 2013 14:22:40 +0800, Axel Lin <axel.lin@ingics.com>
> wrote:
>
>> In current gpio_set_value() implementation, it always sets the gpio control bit
>> no matter the value argument is 0 or 1. Thus the GPIOs never set to low.
>> This patch fixes this bug.
>>
>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>> Acked-by: Stefan Roese <sr@denx.de>
>> Reviewed-by: Vipin Kumar <vipin.kumar@st.com>
>> ---
>> This patch was sent on http://lists.denx.de/pipermail/u-boot/2013-June/156861.html
>>
>> Has Stefan's Ack:
>> http://lists.denx.de/pipermail/u-boot/2013-June/156864.html
>>
>> Vipin says the code is fine, so I add Vipin's review-by.
>> http://lists.denx.de/pipermail/u-boot/2013-June/156966.html
>>
>> Michael confirms it works:
>> http://lists.denx.de/pipermail/u-boot/2013-August/160652.html
>>
>> No body picks up this patch, so here is a resend.
>> Although I think this is a bug fix, but I'll let maintainers to determinate
>> if this is the material for v2013.10.
>> Anyway, can someone at least let me know if this patch is ok for apply at some
>> point? I have no idea who is maintaining this file.
>>
>> Regards,
>> Axel
>>
>>  drivers/gpio/spear_gpio.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c
>> index 367b670..6fb4117 100644
>> --- a/drivers/gpio/spear_gpio.c
>> +++ b/drivers/gpio/spear_gpio.c
>> @@ -36,7 +36,10 @@ int gpio_set_value(unsigned gpio, int value)
>>  {
>>       struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;
>>
>> -     writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>> +     if (value)
>> +             writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>> +     else
>> +             writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>>
>>       return 0;
>>  }
>
> Despite discussions in the previous thread and the confirmations that
> this code is functionally equivalent to the Linux code, I still believe
> this code is incorrect for both writing and reading.
>
> From the doc, writing to GPIODATA will obviously copy each of bits 7
> to 0 of the written value into the actuals GPIO mapped to bits 7 to
> 0 of this register (assuming they are configured as outputs, of course).
> Based on this, the code above:
>

As I understand from the documentation (read some weeks ago),
it uses the address in clear operation not only the value

Michael


> - when setting a single GPIO, sets *but clears up to seven other GPIOs*;
> - when clearing a single GPIO, clears it *and up to seven other GPIOs*.
>
> This code may have been tested only for a single active GPIO at a time,
> for which this code would behave correctly; but as soon as two GPIOs
> from the same bank must be set at the same time, it fails.
>
> Please fix this code so that setting or clearing a GPIO does not set or
> clear any other GPIO, and perform an actual test to confirm this works
> before submitting V2.
>
> BTW: if (as the previous thread seemed to imply) no one around has the
> hardware to test this change, then why exactly is it needed?
>
> Amicalement,
> --
> Albert.

[U-Boot] [PATCH RESEND] gpio: spear_gpio: Fix gpio_set_value() implementation

[Axel Lin]

2013/9/14 Albert ARIBAUD <albert.u.boot@aribaud.net>:
> Hi Axel,
>
> On Fri, 06 Sep 2013 14:22:40 +0800, Axel Lin <axel.lin@ingics.com>
> wrote:
>
>> In current gpio_set_value() implementation, it always sets the gpio control bit
>> no matter the value argument is 0 or 1. Thus the GPIOs never set to low.
>> This patch fixes this bug.
>>
>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>> Acked-by: Stefan Roese <sr@denx.de>
>> Reviewed-by: Vipin Kumar <vipin.kumar@st.com>
>> ---
>> This patch was sent on http://lists.denx.de/pipermail/u-boot/2013-June/156861.html
>>
>> Has Stefan's Ack:
>> http://lists.denx.de/pipermail/u-boot/2013-June/156864.html
>>
>> Vipin says the code is fine, so I add Vipin's review-by.
>> http://lists.denx.de/pipermail/u-boot/2013-June/156966.html
>>
>> Michael confirms it works:
>> http://lists.denx.de/pipermail/u-boot/2013-August/160652.html
>>
>> No body picks up this patch, so here is a resend.
>> Although I think this is a bug fix, but I'll let maintainers to determinate
>> if this is the material for v2013.10.
>> Anyway, can someone at least let me know if this patch is ok for apply at some
>> point? I have no idea who is maintaining this file.
>>
>> Regards,
>> Axel
>>
>>  drivers/gpio/spear_gpio.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c
>> index 367b670..6fb4117 100644
>> --- a/drivers/gpio/spear_gpio.c
>> +++ b/drivers/gpio/spear_gpio.c
>> @@ -36,7 +36,10 @@ int gpio_set_value(unsigned gpio, int value)
>>  {
>>       struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;
>>
>> -     writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>> +     if (value)
>> +             writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>> +     else
>> +             writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>>
>>       return 0;
>>  }
>
> Despite discussions in the previous thread and the confirmations that
> this code is functionally equivalent to the Linux code, I still believe
> this code is incorrect for both writing and reading.
>
> From the doc, writing to GPIODATA will obviously copy each of bits 7
> to 0 of the written value into the actuals GPIO mapped to bits 7 to
> 0 of this register (assuming they are configured as outputs, of course).
> Based on this, the code above:
>
> - when setting a single GPIO, sets *but clears up to seven other GPIOs*;
> - when clearing a single GPIO, clears it *and up to seven other GPIOs*.
>
> This code may have been tested only for a single active GPIO at a time,
> for which this code would behave correctly; but as soon as two GPIOs
> from the same bank must be set at the same time, it fails.
>
> Please fix this code so that setting or clearing a GPIO does not set or
> clear any other GPIO, and perform an actual test to confirm this works
> before submitting V2.

No.
Some people (Marek, and *Michael*) asked this question in original
discussion thread.
The datasheet says each GPIO is controlled by *different* register.
(Well, it's unusual.)
And that is why we don't need a read-write-update operation.
Simply write 0 to the register does work. ( *Michael* replied it works )

>
> BTW: if (as the previous thread seemed to imply) no one around has the
> hardware to test this change, then why exactly is it needed?
>
> Amicalement,
> --
> Albert.

[U-Boot] [PATCH RESEND] gpio: spear_gpio: Fix gpio_set_value() implementation

[Michael Trimarchi]

Hi Axel

On Sat, Sep 14, 2013 at 11:34 AM, Axel Lin <axel.lin@ingics.com> wrote:
> 2013/9/14 Albert ARIBAUD <albert.u.boot@aribaud.net>:
>> Hi Axel,
>>
>> On Fri, 06 Sep 2013 14:22:40 +0800, Axel Lin <axel.lin@ingics.com>
>> wrote:
>>
>>> In current gpio_set_value() implementation, it always sets the gpio control bit
>>> no matter the value argument is 0 or 1. Thus the GPIOs never set to low.
>>> This patch fixes this bug.
>>>
>>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>>> Acked-by: Stefan Roese <sr@denx.de>
>>> Reviewed-by: Vipin Kumar <vipin.kumar@st.com>
>>> ---
>>> This patch was sent on http://lists.denx.de/pipermail/u-boot/2013-June/156861.html
>>>
>>> Has Stefan's Ack:
>>> http://lists.denx.de/pipermail/u-boot/2013-June/156864.html
>>>
>>> Vipin says the code is fine, so I add Vipin's review-by.
>>> http://lists.denx.de/pipermail/u-boot/2013-June/156966.html
>>>
>>> Michael confirms it works:
>>> http://lists.denx.de/pipermail/u-boot/2013-August/160652.html
>>>
>>> No body picks up this patch, so here is a resend.
>>> Although I think this is a bug fix, but I'll let maintainers to determinate
>>> if this is the material for v2013.10.
>>> Anyway, can someone at least let me know if this patch is ok for apply at some
>>> point? I have no idea who is maintaining this file.
>>>
>>> Regards,
>>> Axel
>>>
>>>  drivers/gpio/spear_gpio.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c
>>> index 367b670..6fb4117 100644
>>> --- a/drivers/gpio/spear_gpio.c
>>> +++ b/drivers/gpio/spear_gpio.c
>>> @@ -36,7 +36,10 @@ int gpio_set_value(unsigned gpio, int value)
>>>  {
>>>       struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;
>>>
>>> -     writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>>> +     if (value)
>>> +             writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>>> +     else
>>> +             writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>>>
>>>       return 0;
>>>  }
>>
>> Despite discussions in the previous thread and the confirmations that
>> this code is functionally equivalent to the Linux code, I still believe
>> this code is incorrect for both writing and reading.
>>
>> From the doc, writing to GPIODATA will obviously copy each of bits 7
>> to 0 of the written value into the actuals GPIO mapped to bits 7 to
>> 0 of this register (assuming they are configured as outputs, of course).
>> Based on this, the code above:
>>
>> - when setting a single GPIO, sets *but clears up to seven other GPIOs*;
>> - when clearing a single GPIO, clears it *and up to seven other GPIOs*.
>>
>> This code may have been tested only for a single active GPIO at a time,
>> for which this code would behave correctly; but as soon as two GPIOs
>> from the same bank must be set at the same time, it fails.
>>
>> Please fix this code so that setting or clearing a GPIO does not set or
>> clear any other GPIO, and perform an actual test to confirm this works
>> before submitting V2.
>
> No.
> Some people (Marek, and *Michael*) asked this question in original
> discussion thread.
> The datasheet says each GPIO is controlled by *different* register.
> (Well, it's unusual.)
> And that is why we don't need a read-write-update operation.
> Simply write 0 to the register does work. ( *Michael* replied it works )
>
>>
>> BTW: if (as the previous thread seemed to imply) no one around has the
>> hardware to test this change, then why exactly is it needed?
>>

Yes it's a strange implementation but looking at the documentation seems correct

Michael

>> Amicalement,
>> --
>> Albert.

[U-Boot] [PATCH RESEND] gpio: spear_gpio: Fix gpio_set_value() implementation

[Albert ARIBAUD]

Hi Michael,

On Sat, 14 Sep 2013 11:38:02 +0200, Michael Trimarchi
<michael@amarulasolutions.com> wrote:

> Hi Axel
> 
> On Sat, Sep 14, 2013 at 11:34 AM, Axel Lin <axel.lin@ingics.com> wrote:
> > 2013/9/14 Albert ARIBAUD <albert.u.boot@aribaud.net>:
> >> Hi Axel,
> >>
> >> On Fri, 06 Sep 2013 14:22:40 +0800, Axel Lin <axel.lin@ingics.com>
> >> wrote:
> >>
> >>> In current gpio_set_value() implementation, it always sets the gpio control bit
> >>> no matter the value argument is 0 or 1. Thus the GPIOs never set to low.
> >>> This patch fixes this bug.
> >>>
> >>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> >>> Acked-by: Stefan Roese <sr@denx.de>
> >>> Reviewed-by: Vipin Kumar <vipin.kumar@st.com>
> >>> ---
> >>> This patch was sent on http://lists.denx.de/pipermail/u-boot/2013-June/156861.html
> >>>
> >>> Has Stefan's Ack:
> >>> http://lists.denx.de/pipermail/u-boot/2013-June/156864.html
> >>>
> >>> Vipin says the code is fine, so I add Vipin's review-by.
> >>> http://lists.denx.de/pipermail/u-boot/2013-June/156966.html
> >>>
> >>> Michael confirms it works:
> >>> http://lists.denx.de/pipermail/u-boot/2013-August/160652.html
> >>>
> >>> No body picks up this patch, so here is a resend.
> >>> Although I think this is a bug fix, but I'll let maintainers to determinate
> >>> if this is the material for v2013.10.
> >>> Anyway, can someone at least let me know if this patch is ok for apply at some
> >>> point? I have no idea who is maintaining this file.
> >>>
> >>> Regards,
> >>> Axel
> >>>
> >>>  drivers/gpio/spear_gpio.c | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c
> >>> index 367b670..6fb4117 100644
> >>> --- a/drivers/gpio/spear_gpio.c
> >>> +++ b/drivers/gpio/spear_gpio.c
> >>> @@ -36,7 +36,10 @@ int gpio_set_value(unsigned gpio, int value)
> >>>  {
> >>>       struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;
> >>>
> >>> -     writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
> >>> +     if (value)
> >>> +             writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
> >>> +     else
> >>> +             writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
> >>>
> >>>       return 0;
> >>>  }
> >>
> >> Despite discussions in the previous thread and the confirmations that
> >> this code is functionally equivalent to the Linux code, I still believe
> >> this code is incorrect for both writing and reading.
> >>
> >> From the doc, writing to GPIODATA will obviously copy each of bits 7
> >> to 0 of the written value into the actuals GPIO mapped to bits 7 to
> >> 0 of this register (assuming they are configured as outputs, of course).
> >> Based on this, the code above:
> >>
> >> - when setting a single GPIO, sets *but clears up to seven other GPIOs*;
> >> - when clearing a single GPIO, clears it *and up to seven other GPIOs*.
> >>
> >> This code may have been tested only for a single active GPIO at a time,
> >> for which this code would behave correctly; but as soon as two GPIOs
> >> from the same bank must be set at the same time, it fails.
> >>
> >> Please fix this code so that setting or clearing a GPIO does not set or
> >> clear any other GPIO, and perform an actual test to confirm this works
> >> before submitting V2.
> >
> > No.
> > Some people (Marek, and *Michael*) asked this question in original
> > discussion thread.
> > The datasheet says each GPIO is controlled by *different* register.
> > (Well, it's unusual.)
> > And that is why we don't need a read-write-update operation.
> > Simply write 0 to the register does work. ( *Michael* replied it works )
> >
> >>
> >> BTW: if (as the previous thread seemed to imply) no one around has the
> >> hardware to test this change, then why exactly is it needed?
> >>
> 
> Yes it's a strange implementation but looking at the documentation seems correct

Ok-- I got the "masking address" trick this time, thanks. It is indeed
unusual, and the code is indeed correct. However, I would like the
patch to include a few lines of comment to explain how and why it
works, for the benefit of its future readers who might find it weird
too.

> Michael

Amicalement,
-- 
Albert.

[U-Boot] [PATCH RESEND] gpio: spear_gpio: Fix gpio_set_value() implementation

[Michael Trimarchi]

Hi

On Sun, Sep 15, 2013 at 5:20 PM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Michael,
>
> On Sat, 14 Sep 2013 11:38:02 +0200, Michael Trimarchi
> <michael@amarulasolutions.com> wrote:
>
>> Hi Axel
>>
>> On Sat, Sep 14, 2013 at 11:34 AM, Axel Lin <axel.lin@ingics.com> wrote:
>> > 2013/9/14 Albert ARIBAUD <albert.u.boot@aribaud.net>:
>> >> Hi Axel,
>> >>
>> >> On Fri, 06 Sep 2013 14:22:40 +0800, Axel Lin <axel.lin@ingics.com>
>> >> wrote:
>> >>
>> >>> In current gpio_set_value() implementation, it always sets the gpio control bit
>> >>> no matter the value argument is 0 or 1. Thus the GPIOs never set to low.
>> >>> This patch fixes this bug.
>> >>>
>> >>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>> >>> Acked-by: Stefan Roese <sr@denx.de>
>> >>> Reviewed-by: Vipin Kumar <vipin.kumar@st.com>
>> >>> ---
>> >>> This patch was sent on http://lists.denx.de/pipermail/u-boot/2013-June/156861.html
>> >>>
>> >>> Has Stefan's Ack:
>> >>> http://lists.denx.de/pipermail/u-boot/2013-June/156864.html
>> >>>
>> >>> Vipin says the code is fine, so I add Vipin's review-by.
>> >>> http://lists.denx.de/pipermail/u-boot/2013-June/156966.html
>> >>>
>> >>> Michael confirms it works:
>> >>> http://lists.denx.de/pipermail/u-boot/2013-August/160652.html
>> >>>
>> >>> No body picks up this patch, so here is a resend.
>> >>> Although I think this is a bug fix, but I'll let maintainers to determinate
>> >>> if this is the material for v2013.10.
>> >>> Anyway, can someone at least let me know if this patch is ok for apply at some
>> >>> point? I have no idea who is maintaining this file.
>> >>>
>> >>> Regards,
>> >>> Axel
>> >>>
>> >>>  drivers/gpio/spear_gpio.c | 5 ++++-
>> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c
>> >>> index 367b670..6fb4117 100644
>> >>> --- a/drivers/gpio/spear_gpio.c
>> >>> +++ b/drivers/gpio/spear_gpio.c
>> >>> @@ -36,7 +36,10 @@ int gpio_set_value(unsigned gpio, int value)
>> >>>  {
>> >>>       struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;
>> >>>
>> >>> -     writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>> >>> +     if (value)
>> >>> +             writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>> >>> +     else
>> >>> +             writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>> >>>
>> >>>       return 0;
>> >>>  }
>> >>
>> >> Despite discussions in the previous thread and the confirmations that
>> >> this code is functionally equivalent to the Linux code, I still believe
>> >> this code is incorrect for both writing and reading.
>> >>
>> >> From the doc, writing to GPIODATA will obviously copy each of bits 7
>> >> to 0 of the written value into the actuals GPIO mapped to bits 7 to
>> >> 0 of this register (assuming they are configured as outputs, of course).
>> >> Based on this, the code above:
>> >>
>> >> - when setting a single GPIO, sets *but clears up to seven other GPIOs*;
>> >> - when clearing a single GPIO, clears it *and up to seven other GPIOs*.
>> >>
>> >> This code may have been tested only for a single active GPIO at a time,
>> >> for which this code would behave correctly; but as soon as two GPIOs
>> >> from the same bank must be set at the same time, it fails.
>> >>
>> >> Please fix this code so that setting or clearing a GPIO does not set or
>> >> clear any other GPIO, and perform an actual test to confirm this works
>> >> before submitting V2.
>> >
>> > No.
>> > Some people (Marek, and *Michael*) asked this question in original
>> > discussion thread.
>> > The datasheet says each GPIO is controlled by *different* register.
>> > (Well, it's unusual.)
>> > And that is why we don't need a read-write-update operation.
>> > Simply write 0 to the register does work. ( *Michael* replied it works )
>> >
>> >>
>> >> BTW: if (as the previous thread seemed to imply) no one around has the
>> >> hardware to test this change, then why exactly is it needed?
>> >>
>>
>> Yes it's a strange implementation but looking at the documentation seems correct
>
> Ok-- I got the "masking address" trick this time, thanks. It is indeed
> unusual, and the code is indeed correct. However, I would like the
> patch to include a few lines of comment to explain how and why it
> works, for the benefit of its future readers who might find it weird
> too.
>

I think that patch need to have a better description

Michael

>> Michael
>
> Amicalement,
> --
> Albert.