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