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

[U-Boot] [PATCH] 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>
---
 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 d3c728e..8878608 100644
--- a/drivers/gpio/spear_gpio.c
+++ b/drivers/gpio/spear_gpio.c
@@ -52,7 +52,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] gpio: spear_gpio: Fix gpio_set_value() implementation

[Stefan Roese]

On 20.06.2013 09:13, Axel Lin 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>

Thanks,
Stefan

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

[Michael Trimarchi]

Hi
Il giorno 20/giu/2013 09:14, "Axel Lin" <axel.lin@ingics.com> ha scritto:
>
> 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>
> ---
>  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 d3c728e..8878608 100644
> --- a/drivers/gpio/spear_gpio.c
> +++ b/drivers/gpio/spear_gpio.c
> @@ -52,7 +52,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;
>  }

Does it work the clear? Seems that it sets to 0 all the bank. I'm using the
mobile

Michael

> --
> 1.8.1.2
>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

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

[Marek Vasut]

Dear Michael Trimarchi,

> Hi
> 
> Il giorno 20/giu/2013 09:14, "Axel Lin" <axel.lin@ingics.com> ha scritto:
> > 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>
> > ---
> > 
> >  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 d3c728e..8878608 100644
> > --- a/drivers/gpio/spear_gpio.c
> > +++ b/drivers/gpio/spear_gpio.c
> > @@ -52,7 +52,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;
> >  
> >  }
> 
> Does it work the clear? Seems that it sets to 0 all the bank. I'm using the
> mobile

I don't think I speak language of your tribe (lol) ;-)

What's "the mobile" please ?

Best regards,
Marek Vasut

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

[Michael Trimarchi]

Hi
Il giorno 20/giu/2013 15:57, "Marek Vasut" <marex@denx.de> ha scritto:
>
> Dear Michael Trimarchi,
>
> > Hi
> >
> > Il giorno 20/giu/2013 09:14, "Axel Lin" <axel.lin@ingics.com> ha
scritto:
> > > 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>
> > > ---
> > >
> > >  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 d3c728e..8878608 100644
> > > --- a/drivers/gpio/spear_gpio.c
> > > +++ b/drivers/gpio/spear_gpio.c
> > > @@ -52,7 +52,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;
> > >
> > >  }
> >
> > Does it work the clear? Seems that it sets to 0 all the bank. I'm using
the
> > mobile
>
> I don't think I speak language of your tribe (lol) ;-)
>
> What's "the mobile" please ?
>
> Best regards,
> Marek Vasut
Android mobile, italian dictinary and same comment of you

:D

Michael

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

[Marek Vasut]

Dear Michael Trimarchi,

> Hi
> 
> Il giorno 20/giu/2013 15:57, "Marek Vasut" <marex@denx.de> ha scritto:
> > Dear Michael Trimarchi,
> > 
> > > Hi
> > > 
> > > Il giorno 20/giu/2013 09:14, "Axel Lin" <axel.lin@ingics.com> ha
> 
> scritto:
> > > > 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>
> > > > ---
> > > > 
> > > >  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 d3c728e..8878608 100644
> > > > --- a/drivers/gpio/spear_gpio.c
> > > > +++ b/drivers/gpio/spear_gpio.c
> > > > @@ -52,7 +52,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;
> > > >  
> > > >  }
> > > 
> > > Does it work the clear? Seems that it sets to 0 all the bank. I'm using
> 
> the
> 
> > > mobile
> > 
> > I don't think I speak language of your tribe (lol) ;-)
> > 
> > What's "the mobile" please ?
> > 
> > Best regards,
> > Marek Vasut
> 
> Android mobile, italian dictinary and same comment of you

Poor you, the android {keyboard,mailer,} is such a crap ;-(

Best regards,
Marek Vasut

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

[Marek Vasut]

Dear 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>
> ---
>  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 d3c728e..8878608 100644
> --- a/drivers/gpio/spear_gpio.c
> +++ b/drivers/gpio/spear_gpio.c
> @@ -52,7 +52,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)]);

How can this possibly work? Writing 0 to the whole bank will unset all the 
GPIOs, no ?

Best regards,
Marek Vasut

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

[Axel Lin]

2013/6/20 Marek Vasut <marex@denx.de>
>
> Dear 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>
> > ---
> >  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 d3c728e..8878608 100644
> > --- a/drivers/gpio/spear_gpio.c
> > +++ b/drivers/gpio/spear_gpio.c
> > @@ -52,7 +52,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)]);
>
> How can this possibly work? Writing 0 to the whole bank will unset all the
> GPIOs, no ?


Because each GPIO is controlled by a register.
And only one bit will be set when set gpio to high.

So it's safe to write 0 for clearing the bit.

Note, the gpio_get_value() implementation also assumes there is only one bit
will be set. ( If this is not true, both gpio_get_value() and gpio_set_value()
need fix.)

Vipin, can you review this patch and confirm this behavior?

Thanks,
Axel

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

[Michael Trimarchi]

Hi

On 06/20/2013 03:56 PM, Axel Lin wrote:
> 2013/6/20 Marek Vasut <marex@denx.de>
>>
>> Dear 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>
>>> ---
>>>  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 d3c728e..8878608 100644
>>> --- a/drivers/gpio/spear_gpio.c
>>> +++ b/drivers/gpio/spear_gpio.c
>>> @@ -52,7 +52,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)]);
>>
>> How can this possibly work? Writing 0 to the whole bank will unset all the
>> GPIOs, no ?
> 
> 
> Because each GPIO is controlled by a register.
> And only one bit will be set when set gpio to high.
> 

Yes, but how to reset just one bit if you use the same register offset?

I don't know this core but I know two possibilities:

1) one set register and one clear register
	if (enable)
		writel(1 << gpio, REGSET_BANK(gpio));
	else
		writel(1 << gpio, REGCLEAR_BANK(gpio));
2) or 
set operation
	reg = readl(REG(gpio);
	if (enable)
		reg |= 1 << gpio;
	else
		reg &= ~(1 << gpio);
	writel(reg, REG(GPIO));

Any other way?

Michael	
	

> So it's safe to write 0 for clearing the bit.
> 
> Note, the gpio_get_value() implementation also assumes there is only one bit
> will be set. ( If this is not true, both gpio_get_value() and gpio_set_value()
> need fix.)
> 
> Vipin, can you review this patch and confirm this behavior?
> 
> Thanks,
> Axel
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 


-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |

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

[Marek Vasut]

Dear Michael Trimarchi,

> Hi
> 
> On 06/20/2013 03:56 PM, Axel Lin wrote:
> > 2013/6/20 Marek Vasut <marex@denx.de>
> > 
> >> Dear 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>
> >>> ---
> >>> 
> >>>  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 d3c728e..8878608 100644
> >>> --- a/drivers/gpio/spear_gpio.c
> >>> +++ b/drivers/gpio/spear_gpio.c
> >>> @@ -52,7 +52,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)]);
> >> 
> >> How can this possibly work? Writing 0 to the whole bank will unset all
> >> the GPIOs, no ?
> > 
> > Because each GPIO is controlled by a register.
> > And only one bit will be set when set gpio to high.
> 
> Yes, but how to reset just one bit if you use the same register offset?
> 
> I don't know this core but I know two possibilities:
> 
> 1) one set register and one clear register
> 	if (enable)
> 		writel(1 << gpio, REGSET_BANK(gpio));
> 	else
> 		writel(1 << gpio, REGCLEAR_BANK(gpio));
> 2) or
> set operation
> 	reg = readl(REG(gpio);
> 	if (enable)
> 		reg |= 1 << gpio;
> 	else
> 		reg &= ~(1 << gpio);
> 	writel(reg, REG(GPIO));
> 
> Any other way?

I think it's about time to read the datasheet :b

Best regards,
Marek Vasut

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

[Vipin Kumar]

On 6/20/2013 7:26 PM, Axel Lin wrote:
> 2013/6/20 Marek Vasut<marex@denx.de>
>>
>> Dear 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>
>>> ---
>>>   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 d3c728e..8878608 100644
>>> --- a/drivers/gpio/spear_gpio.c
>>> +++ b/drivers/gpio/spear_gpio.c
>>> @@ -52,7 +52,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)]);
>>
>> How can this possibly work? Writing 0 to the whole bank will unset all the
>> GPIOs, no ?
>
>
> Because each GPIO is controlled by a register.
> And only one bit will be set when set gpio to high.
>
> So it's safe to write 0 for clearing the bit.
>
> Note, the gpio_get_value() implementation also assumes there is only one bit
> will be set. ( If this is not true, both gpio_get_value() and gpio_set_value()
> need fix.)
>
> Vipin, can you review this patch and confirm this behavior?
>

Yes this is right. and the code is fine

Regards
Vipin

> Thanks,
> Axel
> .
>

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

[Michael Trimarchi]

On 06/21/2013 06:40 AM, Vipin Kumar wrote:
> On 6/20/2013 7:26 PM, Axel Lin wrote:
>> 2013/6/20 Marek Vasut<marex@denx.de>
>>>
>>> Dear 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>
>>>> ---
>>>>   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 d3c728e..8878608 100644
>>>> --- a/drivers/gpio/spear_gpio.c
>>>> +++ b/drivers/gpio/spear_gpio.c
>>>> @@ -52,7 +52,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)]);
>>>
>>> How can this possibly work? Writing 0 to the whole bank will unset all the
>>> GPIOs, no ?
>>
>>
>> Because each GPIO is controlled by a register.
>> And only one bit will be set when set gpio to high.
>>
>> So it's safe to write 0 for clearing the bit.
>>
>> Note, the gpio_get_value() implementation also assumes there is only one bit
>> will be set. ( If this is not true, both gpio_get_value() and gpio_set_value()
>> need fix.)
>>
>> Vipin, can you review this patch and confirm this behavior?
>>
> 
> Yes this is right. and the code is fine
> 

The problem is not in set one bit but in reset one bit. Can you check
the else path?


Michael


> Regards
> Vipin
> 
>> Thanks,
>> Axel
>> .
>>
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

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

[Axel Lin]

2013/6/21 Michael Trimarchi <michael@amarulasolutions.com>:
> On 06/21/2013 06:40 AM, Vipin Kumar wrote:
>> On 6/20/2013 7:26 PM, Axel Lin wrote:
>>> 2013/6/20 Marek Vasut<marex@denx.de>
>>>>
>>>> Dear 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>
>>>>> ---
>>>>>   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 d3c728e..8878608 100644
>>>>> --- a/drivers/gpio/spear_gpio.c
>>>>> +++ b/drivers/gpio/spear_gpio.c
>>>>> @@ -52,7 +52,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)]);
>>>>
>>>> How can this possibly work? Writing 0 to the whole bank will unset all the
>>>> GPIOs, no ?
>>>
>>>
>>> Because each GPIO is controlled by a register.
>>> And only one bit will be set when set gpio to high.
>>>
>>> So it's safe to write 0 for clearing the bit.
>>>
>>> Note, the gpio_get_value() implementation also assumes there is only one bit
>>> will be set. ( If this is not true, both gpio_get_value() and gpio_set_value()
>>> need fix.)
>>>
>>> Vipin, can you review this patch and confirm this behavior?
>>>
>>
>> Yes this is right. and the code is fine
>>
>
> The problem is not in set one bit but in reset one bit. Can you check
> the else path?

Hi,
I'm not the best person to answer this question because I don't have the
hardware and datasheet.
In the case only one bit is meaningful and the reset bits are 0,
it looks ok for me to write 0 for clearing the bit.
( note, each gpio pin is controlled by different register.)

This patch is acked and reviewed by Stefan Roese and Vipin Kumar.
I'm wondering if this patch is acceptable?
Or maybe a test-by can help to make this patch acceptable?

Regards,
Axel

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

[Michael Trimarchi]

Hi
Il giorno 30/giu/2013 06:18, "Axel Lin" <axel.lin@ingics.com> ha scritto:
>
> 2013/6/21 Michael Trimarchi <michael@amarulasolutions.com>:
> > On 06/21/2013 06:40 AM, Vipin Kumar wrote:
> >> On 6/20/2013 7:26 PM, Axel Lin wrote:
> >>> 2013/6/20 Marek Vasut<marex@denx.de>
> >>>>
> >>>> Dear 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>
> >>>>> ---
> >>>>>   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 d3c728e..8878608 100644
> >>>>> --- a/drivers/gpio/spear_gpio.c
> >>>>> +++ b/drivers/gpio/spear_gpio.c
> >>>>> @@ -52,7 +52,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)]);
> >>>>
> >>>> How can this possibly work? Writing 0 to the whole bank will unset
all the
> >>>> GPIOs, no ?
> >>>
> >>>
> >>> Because each GPIO is controlled by a register.
> >>> And only one bit will be set when set gpio to high.
> >>>
> >>> So it's safe to write 0 for clearing the bit.
> >>>
> >>> Note, the gpio_get_value() implementation also assumes there is only
one bit
> >>> will be set. ( If this is not true, both gpio_get_value() and
gpio_set_value()
> >>> need fix.)
> >>>
> >>> Vipin, can you review this patch and confirm this behavior?
> >>>
> >>
> >> Yes this is right. and the code is fine
> >>
> >
> > The problem is not in set one bit but in reset one bit. Can you check
> > the else path?
>
> Hi,
> I'm not the best person to answer this question because I don't have the
> hardware and datasheet.
> In the case only one bit is meaningful and the reset bits are 0,
> it looks ok for me to write 0 for clearing the bit.
> ( note, each gpio pin is controlled by different register.)
>
> This patch is acked and reviewed by Stefan Roese and Vipin Kumar.
> I'm wondering if this patch is acceptable?
> Or maybe a test-by can help to make this patch acceptable?
>

If each pin is controlled by a different register why you need to 1<<gpio
in set path?

And how it works for gpio 33?

Michael

> Regards,
> Axel

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

[Axel Lin]

2013/6/30 Michael Trimarchi <michael@amarulasolutions.com>:
> Hi
> Il giorno 30/giu/2013 06:18, "Axel Lin" <axel.lin@ingics.com> ha scritto:
>
>
>>
>> 2013/6/21 Michael Trimarchi <michael@amarulasolutions.com>:
>> > On 06/21/2013 06:40 AM, Vipin Kumar wrote:
>> >> On 6/20/2013 7:26 PM, Axel Lin wrote:
>> >>> 2013/6/20 Marek Vasut<marex@denx.de>
>> >>>>
>> >>>> Dear 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>
>> >>>>> ---
>> >>>>>   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 d3c728e..8878608 100644
>> >>>>> --- a/drivers/gpio/spear_gpio.c
>> >>>>> +++ b/drivers/gpio/spear_gpio.c
>> >>>>> @@ -52,7 +52,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)]);
>> >>>>
>> >>>> How can this possibly work? Writing 0 to the whole bank will unset
>> >>>> all the
>> >>>> GPIOs, no ?
>> >>>
>> >>>
>> >>> Because each GPIO is controlled by a register.
>> >>> And only one bit will be set when set gpio to high.
>> >>>
>> >>> So it's safe to write 0 for clearing the bit.
>> >>>
>> >>> Note, the gpio_get_value() implementation also assumes there is only
>> >>> one bit
>> >>> will be set. ( If this is not true, both gpio_get_value() and
>> >>> gpio_set_value()
>> >>> need fix.)
>> >>>
>> >>> Vipin, can you review this patch and confirm this behavior?
>> >>>
>> >>
>> >> Yes this is right. and the code is fine
>> >>
>> >
>> > The problem is not in set one bit but in reset one bit. Can you check
>> > the else path?
>>
>> Hi,
>> I'm not the best person to answer this question because I don't have the
>> hardware and datasheet.
>> In the case only one bit is meaningful and the reset bits are 0,
>> it looks ok for me to write 0 for clearing the bit.
>> ( note, each gpio pin is controlled by different register.)
>>
>> This patch is acked and reviewed by Stefan Roese and Vipin Kumar.
>> I'm wondering if this patch is acceptable?
>> Or maybe a test-by can help to make this patch acceptable?
>>
>
> If each pin is controlled by a different register why you need to 1<<gpio in
> set path?

Because the meaningful bit for different register is different.

>
> And how it works for gpio 33?

SPEAR_GPIO_COUNT is 8, so this driver only allows setting gpio0 ~ gpio7.

Vipin, any chance to double check the datasheet and confirm if this patch is ok?

Regards,
Axel

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

[Vipin Kumar]

On 6/30/2013 2:27 PM, Axel Lin wrote:
> 2013/6/30 Michael Trimarchi<michael@amarulasolutions.com>:
>> Hi
>> Il giorno 30/giu/2013 06:18, "Axel Lin"<axel.lin@ingics.com>  ha scritto:
>>
>>
>>>
>>> 2013/6/21 Michael Trimarchi<michael@amarulasolutions.com>:
>>>> On 06/21/2013 06:40 AM, Vipin Kumar wrote:
>>>>> On 6/20/2013 7:26 PM, Axel Lin wrote:
>>>>>> 2013/6/20 Marek Vasut<marex@denx.de>
>>>>>>>
>>>>>>> Dear 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>
>>>>>>>> ---
>>>>>>>>    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 d3c728e..8878608 100644
>>>>>>>> --- a/drivers/gpio/spear_gpio.c
>>>>>>>> +++ b/drivers/gpio/spear_gpio.c
>>>>>>>> @@ -52,7 +52,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)]);
>>>>>>>
>>>>>>> How can this possibly work? Writing 0 to the whole bank will unset
>>>>>>> all the
>>>>>>> GPIOs, no ?
>>>>>>
>>>>>>
>>>>>> Because each GPIO is controlled by a register.
>>>>>> And only one bit will be set when set gpio to high.
>>>>>>
>>>>>> So it's safe to write 0 for clearing the bit.
>>>>>>
>>>>>> Note, the gpio_get_value() implementation also assumes there is only
>>>>>> one bit
>>>>>> will be set. ( If this is not true, both gpio_get_value() and
>>>>>> gpio_set_value()
>>>>>> need fix.)
>>>>>>
>>>>>> Vipin, can you review this patch and confirm this behavior?
>>>>>>
>>>>>
>>>>> Yes this is right. and the code is fine
>>>>>
>>>>
>>>> The problem is not in set one bit but in reset one bit. Can you check
>>>> the else path?
>>>
>>> Hi,
>>> I'm not the best person to answer this question because I don't have the
>>> hardware and datasheet.
>>> In the case only one bit is meaningful and the reset bits are 0,
>>> it looks ok for me to write 0 for clearing the bit.
>>> ( note, each gpio pin is controlled by different register.)
>>>
>>> This patch is acked and reviewed by Stefan Roese and Vipin Kumar.
>>> I'm wondering if this patch is acceptable?
>>> Or maybe a test-by can help to make this patch acceptable?
>>>
>>
>> If each pin is controlled by a different register why you need to 1<<gpio in
>> set path?
>
> Because the meaningful bit for different register is different.
>
>>
>> And how it works for gpio 33?
>
> SPEAR_GPIO_COUNT is 8, so this driver only allows setting gpio0 ~ gpio7.
>
> Vipin, any chance to double check the datasheet and confirm if this patch is ok?
>

The questions raised here are valid and it forced me to re-read the 
datasheet. For your convenience, I must tell you that the device is 
actually pl061 from ARM, so the driver can also be named so.

The datasheet is here
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0190b/I1002697.html

Quoting from the datasheet
"The GPIODATA register is the data register. In software control mode, 
values written in the GPIODATA register are transferred onto the GPOUT 
pins if the respective pins have been configured as outputs through the 
GPIODIR register.

In order to write to GPIODATA, the corresponding bits in the mask, 
resulting from the address bus, PADDR[9:2], must be HIGH. Otherwise the 
bit values remain unchanged by the write.

Similarly, the values read from this register are determined for each 
bit, by the mask bit derived from the address used to access the data 
register, PADDR[9:2]. Bits that are 1 in the address mask cause the 
corresponding bits in GPIODATA to be read, and bits that are 0 in the 
address mask cause the corresponding bits in GPIODATA to be read as 0, 
regardless of their value.

A read from GPIODATA returns the last bit value written if the 
respective pins are configured as output, or it returns the value on the 
corresponding input GPIN bit when these are configured as inputs. All 
bits are cleared by a reset."

After reading all this I am confused about numbering of the gpio's. I 
think the numbering should be from 1 to 8 for a device. And this would 
mean that we should write to *&regs->datareg[1 << (gpio - 1)]* instead 
of the present code which is _&regs->datareg[1 << (gpio + 2)]_

Moreover, One GPIO device can control only 8 pins, so there is no 
question of having GPIO33. In an SoC design, GPIO33 may  actually map to 
GPIO1 of device 4. I hope I am clear on this

Regards
Vipin

> Regards,
> Axel
> .
>

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

[Axel Lin]

>
> The questions raised here are valid and it forced me to re-read the
> datasheet. For your convenience, I must tell you that the device is actually
> pl061 from ARM, so the driver can also be named so.
>
> The datasheet is here
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0190b/I1002697.html
>
> Quoting from the datasheet
> "The GPIODATA register is the data register. In software control mode,
> values written in the GPIODATA register are transferred onto the GPOUT pins
> if the respective pins have been configured as outputs through the GPIODIR
> register.
>
> In order to write to GPIODATA, the corresponding bits in the mask, resulting
> from the address bus, PADDR[9:2], must be HIGH. Otherwise the bit values
> remain unchanged by the write.
>
> Similarly, the values read from this register are determined for each bit,
> by the mask bit derived from the address used to access the data register,
> PADDR[9:2]. Bits that are 1 in the address mask cause the corresponding bits
> in GPIODATA to be read, and bits that are 0 in the address mask cause the
> corresponding bits in GPIODATA to be read as 0, regardless of their value.
>
> A read from GPIODATA returns the last bit value written if the respective
> pins are configured as output, or it returns the value on the corresponding
> input GPIN bit when these are configured as inputs. All bits are cleared by
> a reset."
>
> After reading all this I am confused about numbering of the gpio's. I think
> the numbering should be from 1 to 8 for a device. And this would mean that
> we should write to *&regs->datareg[1 << (gpio - 1)]* instead of the present
> code which is _&regs->datareg[1 << (gpio + 2)]_

Hi Vipin,
Thanks for the review and providing the datasheet information.
You mentioned that this is PL061.
So... I just checked the gpio-pl061 driver in linux kernel.
It's writing to _&regs->datareg[1 << (gpio + 2)]. and seems no bug
report for this.

And the gpio_get/set implementation in linux kernel has the same behavior as
this patch does:

( below is from linux/drivers/gpio/gpio-pl061.c )

static int pl061_get_value(struct gpio_chip *gc, unsigned offset)
{
        struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);

        return !!readb(chip->base + (1 << (offset + 2)));
}

static void pl061_set_value(struct gpio_chip *gc, unsigned offset, int value)
{
        struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);

        writeb(!!value << offset, chip->base + (1 << (offset + 2)));
}

BTW, it would be great if you have the hardware to test.

Regards,
Axel

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

[Vipin Kumar]

On 7/1/2013 11:02 AM, Axel Lin wrote:
>>
>> The questions raised here are valid and it forced me to re-read the
>> datasheet. For your convenience, I must tell you that the device is actually
>> pl061 from ARM, so the driver can also be named so.
>>
>> The datasheet is here
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0190b/I1002697.html
>>
>> Quoting from the datasheet
>> "The GPIODATA register is the data register. In software control mode,
>> values written in the GPIODATA register are transferred onto the GPOUT pins
>> if the respective pins have been configured as outputs through the GPIODIR
>> register.
>>
>> In order to write to GPIODATA, the corresponding bits in the mask, resulting
>> from the address bus, PADDR[9:2], must be HIGH. Otherwise the bit values
>> remain unchanged by the write.
>>
>> Similarly, the values read from this register are determined for each bit,
>> by the mask bit derived from the address used to access the data register,
>> PADDR[9:2]. Bits that are 1 in the address mask cause the corresponding bits
>> in GPIODATA to be read, and bits that are 0 in the address mask cause the
>> corresponding bits in GPIODATA to be read as 0, regardless of their value.
>>
>> A read from GPIODATA returns the last bit value written if the respective
>> pins are configured as output, or it returns the value on the corresponding
>> input GPIN bit when these are configured as inputs. All bits are cleared by
>> a reset."
>>
>> After reading all this I am confused about numbering of the gpio's. I think
>> the numbering should be from 1 to 8 for a device. And this would mean that
>> we should write to *&regs->datareg[1<<  (gpio - 1)]* instead of the present
>> code which is _&regs->datareg[1<<  (gpio + 2)]_
>
> Hi Vipin,

Hello Alex,

> Thanks for the review and providing the datasheet information.
> You mentioned that this is PL061.
> So... I just checked the gpio-pl061 driver in linux kernel.
> It's writing to _&regs->datareg[1<<  (gpio + 2)]. and seems no bug
> report for this.
>

Yes, I see it now. The difference is that we are using a writel and the 
datareg is a u32 array.

> And the gpio_get/set implementation in linux kernel has the same behavior as
> this patch does:
>
> ( below is from linux/drivers/gpio/gpio-pl061.c )
>
> static int pl061_get_value(struct gpio_chip *gc, unsigned offset)
> {
>          struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
>
>          return !!readb(chip->base + (1<<  (offset + 2)));
> }
>
> static void pl061_set_value(struct gpio_chip *gc, unsigned offset, int value)
> {
>          struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
>
>          writeb(!!value<<  offset, chip->base + (1<<  (offset + 2)));
> }
>
> BTW, it would be great if you have the hardware to test.
>

I am sorry about this. I have now moved to a different group and I have 
no access to the hardware

Regards
Vipin

> Regards,
> Axel
> .
>

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

[Axel Lin]

2013/7/1 Vipin Kumar <vipin.kumar@st.com>:
> On 7/1/2013 11:02 AM, Axel Lin wrote:
>>>
>>>
>>> The questions raised here are valid and it forced me to re-read the
>>> datasheet. For your convenience, I must tell you that the device is
>>> actually
>>> pl061 from ARM, so the driver can also be named so.
>>>
>>> The datasheet is here
>>>
>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0190b/I1002697.html
>>>
>>> Quoting from the datasheet
>>> "The GPIODATA register is the data register. In software control mode,
>>> values written in the GPIODATA register are transferred onto the GPOUT
>>> pins
>>> if the respective pins have been configured as outputs through the
>>> GPIODIR
>>> register.
>>>
>>> In order to write to GPIODATA, the corresponding bits in the mask,
>>> resulting
>>> from the address bus, PADDR[9:2], must be HIGH. Otherwise the bit values
>>> remain unchanged by the write.
>>>
>>> Similarly, the values read from this register are determined for each
>>> bit,
>>> by the mask bit derived from the address used to access the data
>>> register,
>>> PADDR[9:2]. Bits that are 1 in the address mask cause the corresponding
>>> bits
>>> in GPIODATA to be read, and bits that are 0 in the address mask cause the
>>> corresponding bits in GPIODATA to be read as 0, regardless of their
>>> value.
>>>
>>> A read from GPIODATA returns the last bit value written if the respective
>>> pins are configured as output, or it returns the value on the
>>> corresponding
>>> input GPIN bit when these are configured as inputs. All bits are cleared
>>> by
>>> a reset."
>>>
>>> After reading all this I am confused about numbering of the gpio's. I
>>> think
>>> the numbering should be from 1 to 8 for a device. And this would mean
>>> that
>>> we should write to *&regs->datareg[1<<  (gpio - 1)]* instead of the
>>> present
>>> code which is _&regs->datareg[1<<  (gpio + 2)]_
>>
>>
>> Hi Vipin,
>
>
> Hello Alex,
>
>
>> Thanks for the review and providing the datasheet information.
>> You mentioned that this is PL061.
>> So... I just checked the gpio-pl061 driver in linux kernel.
>> It's writing to _&regs->datareg[1<<  (gpio + 2)]. and seems no bug
>> report for this.
>>
>
> Yes, I see it now. The difference is that we are using a writel and the
> datareg is a u32 array.

Hi,
The merge window is going to close.
I'm wondering if this patch is ok or not (I think it's a bug fix).
I think the only issue is nobody has this hardware to test.
[Sorry to back to this topic so late... just too busy recently.]

Regards,
Axel

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

[Michael Trimarchi]

Hi

On Mon, Aug 12, 2013 at 6:57 PM, Axel Lin <axel.lin@ingics.com> wrote:
> 2013/7/1 Vipin Kumar <vipin.kumar@st.com>:
>> On 7/1/2013 11:02 AM, Axel Lin wrote:
>>>>
>>>>
>>>> The questions raised here are valid and it forced me to re-read the
>>>> datasheet. For your convenience, I must tell you that the device is
>>>> actually
>>>> pl061 from ARM, so the driver can also be named so.
>>>>
>>>> The datasheet is here
>>>>
>>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0190b/I1002697.html
>>>>
>>>> Quoting from the datasheet
>>>> "The GPIODATA register is the data register. In software control mode,
>>>> values written in the GPIODATA register are transferred onto the GPOUT
>>>> pins
>>>> if the respective pins have been configured as outputs through the
>>>> GPIODIR
>>>> register.
>>>>
>>>> In order to write to GPIODATA, the corresponding bits in the mask,
>>>> resulting
>>>> from the address bus, PADDR[9:2], must be HIGH. Otherwise the bit values
>>>> remain unchanged by the write.
>>>>
>>>> Similarly, the values read from this register are determined for each
>>>> bit,
>>>> by the mask bit derived from the address used to access the data
>>>> register,
>>>> PADDR[9:2]. Bits that are 1 in the address mask cause the corresponding
>>>> bits
>>>> in GPIODATA to be read, and bits that are 0 in the address mask cause the
>>>> corresponding bits in GPIODATA to be read as 0, regardless of their
>>>> value.
>>>>
>>>> A read from GPIODATA returns the last bit value written if the respective
>>>> pins are configured as output, or it returns the value on the
>>>> corresponding
>>>> input GPIN bit when these are configured as inputs. All bits are cleared
>>>> by
>>>> a reset."
>>>>
>>>> After reading all this I am confused about numbering of the gpio's. I
>>>> think
>>>> the numbering should be from 1 to 8 for a device. And this would mean
>>>> that
>>>> we should write to *&regs->datareg[1<<  (gpio - 1)]* instead of the
>>>> present
>>>> code which is _&regs->datareg[1<<  (gpio + 2)]_
>>>
>>>
>>> Hi Vipin,
>>
>>
>> Hello Alex,
>>
>>
>>> Thanks for the review and providing the datasheet information.
>>> You mentioned that this is PL061.
>>> So... I just checked the gpio-pl061 driver in linux kernel.
>>> It's writing to _&regs->datareg[1<<  (gpio + 2)]. and seems no bug
>>> report for this.
>>>
>>
>> Yes, I see it now. The difference is that we are using a writel and the
>> datareg is a u32 array.
>
> Hi,
> The merge window is going to close.
> I'm wondering if this patch is ok or not (I think it's a bug fix).
> I think the only issue is nobody has this hardware to test.
> [Sorry to back to this topic so late... just too busy recently.]
>

You are right. I read the documentation and it works

Michael

> Regards,
> Axel