[U-Boot] [PATCH v2 1/6] dm: gpio: hi6220: Add a hi6220 GPIO driver model driver.

[Simon Glass <sjg@chromium.org>]

Hi Peter,

On 8 July 2015 at 09:57, Peter Griffin <peter.griffin@linaro.org> wrote:
> This patch adds support for the GPIO perif found on hi6220
> SoC.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  arch/arm/include/asm/arch-hi6220/gpio.h | 29 ++++++++++
>  drivers/gpio/Makefile                   |  2 +
>  drivers/gpio/hi6220_gpio.c              | 95 +++++++++++++++++++++++++++++++++
>  3 files changed, 126 insertions(+)
>  create mode 100644 arch/arm/include/asm/arch-hi6220/gpio.h
>  create mode 100644 drivers/gpio/hi6220_gpio.c
>
> diff --git a/arch/arm/include/asm/arch-hi6220/gpio.h b/arch/arm/include/asm/arch-hi6220/gpio.h
> new file mode 100644
> index 0000000..98122a2
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-hi6220/gpio.h
> @@ -0,0 +1,29 @@
> +/*
> + * Copyright (C) 2015 Linaro
> + * Peter Griffin <peter.griffin@linaro.org>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef _HI6220_GPIO_H_
> +#define _HI6220_GPIO_H_
> +
> +#define HI6220_GPIO_BASE(bank) (((bank < 4) ? 0xf8011000 : \
> +                               0xf7020000 - 0x4000) + (0x1000 * bank))
> +
> +#define BIT(x)                 (1 << (x))

I thought we had this in U-Boot now but cannot find it.

> +
> +#define HI6220_GPIO_PER_BANK   8
> +#define HI6220_GPIO_DIR                0x400
> +
> +struct gpio_bank {
> +       u8 *base;       /* address of registers in physical memory */
> +};

We should use a C struct to access registers. See here:

http://www.denx.de/wiki/U-Boot/CodingStyle

> +
> +/* Information about a GPIO bank */
> +struct hikey_gpio_platdata {
> +       int bank_index;
> +       unsigned int base;     /* address of registers in physical memory */
> +};
> +
> +#endif /* _HI6220_GPIO_H_ */
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 5864850..b470bab 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -46,3 +46,5 @@ obj-$(CONFIG_LPC32XX_GPIO)    += lpc32xx_gpio.o
>  obj-$(CONFIG_STM32_GPIO)       += stm32_gpio.o
>  obj-$(CONFIG_ZYNQ_GPIO)                += zynq_gpio.o
>  obj-$(CONFIG_VYBRID_GPIO)      += vybrid_gpio.o
> +obj-$(CONFIG_HIKEY_GPIO)       += hi6220_gpio.o
> +
> diff --git a/drivers/gpio/hi6220_gpio.c b/drivers/gpio/hi6220_gpio.c
> new file mode 100644
> index 0000000..3f41bff
> --- /dev/null
> +++ b/drivers/gpio/hi6220_gpio.c
> @@ -0,0 +1,95 @@
> +/*
> + * Copyright (C) 2015 Linaro
> + * Peter Griffin <peter.griffin@linaro.org>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <asm/gpio.h>
> +#include <asm/io.h>
> +#include <errno.h>
> +
> +static int hi6220_gpio_direction_input(struct udevice *dev, unsigned int gpio)
> +{
> +       struct gpio_bank *bank = dev_get_priv(dev);
> +       u8 data;
> +
> +       data = readb(bank->base + HI6220_GPIO_DIR);
> +       data &= ~(1 << gpio);
> +       writeb(data, bank->base + HI6220_GPIO_DIR);

Something like this:

clrbits_8(&regs->gpio_dir, 1 << gpio)

> +
> +       return 0;
> +}
> +
> +static int hi6220_gpio_set_value(struct udevice *dev, unsigned gpio,
> +                                 int value)
> +{
> +       struct gpio_bank *bank = dev_get_priv(dev);
> +
> +       writeb(!!value << gpio, bank->base + (BIT(gpio + 2)));

Ick, use struct access.

> +       return 0;
> +}
> +
> +static int hi6220_gpio_direction_output(struct udevice *dev, unsigned gpio,
> +                                       int value)
> +{
> +       struct gpio_bank *bank = dev_get_priv(dev);
> +       u8 data;
> +
> +       data = readb(bank->base + HI6220_GPIO_DIR);
> +       data |= 1 << gpio;
> +       writeb(data, bank->base + HI6220_GPIO_DIR);
> +
> +       hi6220_gpio_set_value(dev, gpio, value);
> +
> +       return 0;
> +}
> +
> +static int hi6220_gpio_get_value(struct udevice *dev, unsigned gpio)
> +{
> +       struct gpio_bank *bank = dev_get_priv(dev);
> +
> +       return !!readb(bank->base + (BIT(gpio + 2)));
> +}
> +
> +
> +
> +static const struct dm_gpio_ops gpio_hi6220_ops = {
> +       .direction_input        = hi6220_gpio_direction_input,
> +       .direction_output       = hi6220_gpio_direction_output,
> +       .get_value              = hi6220_gpio_get_value,
> +       .set_value              = hi6220_gpio_set_value,
> +};
> +
> +static int hi6220_gpio_probe(struct udevice *dev)
> +{
> +       struct gpio_bank *bank = dev_get_priv(dev);
> +       struct hikey_gpio_platdata *plat = dev_get_platdata(dev);
> +       struct gpio_dev_priv *uc_priv = dev->uclass_priv;
> +       char name[18], *str;
> +
> +       sprintf(name, "GPIO%d_", plat->bank_index);

Could use just use 'GP' or 'P'? This seems quite long as a name. Also
I don't think you want the %d.
> +
> +       str = strdup(name);
> +       if (!str)
> +               return -ENOMEM;
> +
> +       uc_priv->bank_name = str;
> +       uc_priv->gpio_count = HI6220_GPIO_PER_BANK;
> +
> +       bank->base = (u8 *)plat->base;
> +
> +       return 0;
> +}
> +
> +U_BOOT_DRIVER(gpio_hi6220) = {
> +       .name   = "gpio_hi6220",
> +       .id     = UCLASS_GPIO,
> +       .ops    = &gpio_hi6220_ops,
> +       .probe  = hi6220_gpio_probe,
> +       .priv_auto_alloc_size = sizeof(struct gpio_bank),
> +};
> +
> +
> --
> 1.9.1
>

Regards,
Simon