Re: [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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot