diff mbox

[U-Boot] gpio: Question about gpio_set_value() implementation in spear_gpio.c

Message ID 1371649453.2126.2.camel@phoenix
State Not Applicable
Headers show

Commit Message

Axel Lin June 19, 2013, 1:44 p.m. UTC 
Current code looks strange because no matter the value argument is 0 or 1
it always calls
	writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

And then gpio_get_value() always return 1.

I'm wondering if it needs to be fixed, something like below change:

Comments

Otavio Salvador June 19, 2013, 1:48 p.m. UTC | #1 
On Wed, Jun 19, 2013 at 10:44 AM, Axel Lin <axel.lin@ingics.com> wrote:
> Current code looks strange because no matter the value argument is 0 or 1
> it always calls
>         writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>
> And then gpio_get_value() always return 1.
>
> I'm wondering if it needs to be fixed, something like below change:
>
> 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;
>  }

writel(value << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

Should do no? This would avoid the if block.

--
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://projetos.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750
Marek Vasut June 19, 2013, 1:49 p.m. UTC | #2 
Dear Axel Lin,

> Current code looks strange because no matter the value argument is 0 or 1
> it always calls
> 	writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
> 
> And then gpio_get_value() always return 1.
> 
> I'm wondering if it needs to be fixed, something like below change:
> 
> 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)]);


You might want to use clrbits_le32() and setbits_le32() here, no?

>  	return 0;
>  }

Best regards,
Marek Vasut
Axel Lin June 19, 2013, 2:01 p.m. UTC | #3 
2013/6/19 Marek Vasut <marex@denx.de>:
> Dear Axel Lin,
>
>> Current code looks strange because no matter the value argument is 0 or 1
>> it always calls
>>       writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
>>
>> And then gpio_get_value() always return 1.
>>
>> I'm wondering if it needs to be fixed, something like below change:
>>
>> 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)]);
>
>
> You might want to use clrbits_le32() and setbits_le32() here, no?
>

Honestly, I ask this because I cannot find the detail datasheet for the gpio
control of this device. ( Just found the code looks strange).

Usually we can use clrbits_le32() and setbits_le32() helpers to set/clear
register bits. But if the only meaningful bit is "1 << gpio", simply use
"write 1 << gpio" and "write 0" saves a register read operation.

So maybe Stefan  or someone from ST can provide the information about gpio
control on this hardware.

Regards,
Axel
Stefan Roese June 19, 2013, 2:40 p.m. UTC | #4 
On 19.06.2013 16:01, Axel Lin wrote:
>>> -     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)]);
>>
>>
>> You might want to use clrbits_le32() and setbits_le32() here, no?
>>
> 
> Honestly, I ask this because I cannot find the detail datasheet for the gpio
> control of this device. ( Just found the code looks strange).
> 
> Usually we can use clrbits_le32() and setbits_le32() helpers to set/clear
> register bits. But if the only meaningful bit is "1 << gpio", simply use
> "write 1 << gpio" and "write 0" saves a register read operation.
> 
> So maybe Stefan  or someone from ST can provide the information about gpio
> control on this hardware.

I honestly have no idea how I tested this GPIO driver. That was too long
ago. And I don't have the time to dig out the hardware to do some
re-testing. But the code definitely looks wrong, so thanks for catching
this. Let's wait what the ST engineers have to comment here.

Thanks,
Stefan
Marek Vasut June 19, 2013, 11:51 p.m. UTC | #5 
Dear Otavio Salvador,

> On Wed, Jun 19, 2013 at 10:44 AM, Axel Lin <axel.lin@ingics.com> wrote:
> > Current code looks strange because no matter the value argument is 0 or 1
> > it always calls
> > 
> >         writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
> > 
> > And then gpio_get_value() always return 1.
> > 
> > I'm wondering if it needs to be fixed, something like below change:
> > 
> > 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;
> >  
> >  }
> 
> writel(value << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
> 
> Should do no? This would avoid the if block.

No, you need clrbits_le32() to unset the GPIO

Best regards,
Marek Vasut
Vipin Kumar June 20, 2013, 3:52 a.m. UTC | #6 
On 6/19/2013 7:14 PM, Axel Lin wrote:
> Current code looks strange because no matter the value argument is 0 or 1
> it always calls
> 	writel(1<<  gpio,&regs->gpiodata[DATA_REG_ADDR(gpio)]);
>
> And then gpio_get_value() always return 1.
>
> I'm wondering if it needs to be fixed, something like below change:
>
> 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)]);
>

Yes, this is the right way. It was a blunder. I am wondering no one ever 
tried to set a ZERO on any GPIO..

Thanks for pointing out

Regards
Vipin

>   	return 0;
>   }
>
>
>
diff mbox

Patch

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;
 }