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

[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.

> 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
> .
>