[U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

[Marek Vasut <marex@denx.de>]

On Wednesday, August 12, 2015 at 04:15:00 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 10 August 2015 at 09:30, Marek Vasut <marex@denx.de> wrote:
> > Add driver for the DesignWare APB GPIO IP block.
> > This driver is DM capable and probes from DT.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Simon Glass <sjg@chromium.org>
> > ---
> > 
> >  drivers/gpio/Kconfig      |   7 ++
> >  drivers/gpio/Makefile     |   1 +
> >  drivers/gpio/dwapb_gpio.c | 167
> >  ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 175
> >  insertions(+)
> >  create mode 100644 drivers/gpio/dwapb_gpio.c
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Please see nits/suggestions below.
> 
> Are you going to submit a GPIO binding change for this?

You mean for the bank-name ? Yes, I think it only makes sense to do it.

> > V2: Obtain the bank name from the "bank-name" property instead.
> > 
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 0c43777..c04388d 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -8,6 +8,13 @@ config DM_GPIO
> > 
> >           particular GPIOs that they provide. The uclass interface
> >           is defined in include/asm-generic/gpio.h.
> > 
> > +config DWAPB_GPIO
> > +       bool "DWAPB GPIO driver"
> > +       depends on DM && DM_GPIO
> > +       default n
> > +       help
> > +         Support for the Designware APB GPIO driver.
> 
> You could expand this to talk about bank naming, features supported,
> chips which use it.

Done

> > +
> > 
> >  config LPC32XX_GPIO
> >  
> >         bool "LPC32XX GPIO driver"
> >         depends on DM
> > 
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 67c6374..603c96b 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -6,6 +6,7 @@
> > 
> >  #
> >  
> >  ifndef CONFIG_SPL_BUILD
> > 
> > +obj-$(CONFIG_DWAPB_GPIO)       += dwapb_gpio.o
> > 
> >  obj-$(CONFIG_AXP_GPIO)         += axp_gpio.o
> >  endif
> >  obj-$(CONFIG_DM_GPIO)          += gpio-uclass.o
> > 
> > diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c
> > new file mode 100644
> > index 0000000..72cec48
> > --- /dev/null
> > +++ b/drivers/gpio/dwapb_gpio.c
> > @@ -0,0 +1,167 @@
> > +/*
> > + * (C) Copyright 2015 Marek Vasut <marex@denx.de>
> > + *
> > + * DesignWare APB GPIO driver
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0+
> > + */
> > +
> > +#include <common.h>
> > +#include <malloc.h>
> > +#include <asm/arch/gpio.h>
> > +#include <asm/gpio.h>
> > +#include <asm/io.h>
> > +#include <dm.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/lists.h>
> > +#include <dm/root.h>
> > +#include <errno.h>
> 
> should go just below common.h

Yup

> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +#define GPIO_SWPORTA_DR                0x00
> > +#define GPIO_SWPORTA_DDR       0x04
> > +#define GPIO_INTEN             0x30
> > +#define GPIO_INTMASK           0x34
> > +#define GPIO_INTTYPE_LEVEL     0x38
> > +#define GPIO_INT_POLARITY      0x3c
> > +#define GPIO_INTSTATUS         0x40
> > +#define GPIO_PORTA_DEBOUNCE    0x48
> > +#define GPIO_PORTA_EOI         0x4c
> > +#define GPIO_EXT_PORTA         0x50
> 
> What's the deal with C structures? Has the policy on this changed? I
> can't help thinking that your GPIO_ prefix is unnecessary.

I think we don't do that anymore. The GPIO_ stuff is copied from Linux.

> > +
> > +struct gpio_dwapb_platdata {
> > +       const char      *name;
> > +       int             bank;
> > +       int             pins;
> > +       fdt_addr_t      base;
> > +};
> > +
> > +static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin)
> > +{
> > +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> > +
> > +       clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
> > +       return 0;
> > +}
> > +
> > +static int dwapb_gpio_direction_output(struct udevice *dev, unsigned
> > pin, +                                    int val)
> > +{
> > +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> > +
> > +       setbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
> > +
> > +       if (val)
> > +               setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> > +       else
> > +               clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> > +
> > +       return 0;
> > +}
> > +
> > +static int dwapb_gpio_get_value(struct udevice *dev, unsigned pin)
> > +{
> > +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> > +       return !!(readl(plat->base + GPIO_EXT_PORTA) & (1 << pin));
> > +}
> > +
> > +
> > +static int dwapb_gpio_set_value(struct udevice *dev, unsigned pin, int
> > val) +{
> > +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> > +
> > +       if (val)
> > +               setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> > +       else
> > +               clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dm_gpio_ops gpio_dwapb_ops = {
> > +       .direction_input        = dwapb_gpio_direction_input,
> > +       .direction_output       = dwapb_gpio_direction_output,
> > +       .get_value              = dwapb_gpio_get_value,
> > +       .set_value              = dwapb_gpio_set_value,
> > +};
> > +
> > +static int gpio_dwapb_probe(struct udevice *dev)
> > +{
> > +       struct gpio_dev_priv *priv = dev_get_uclass_priv(dev);
> > +       struct gpio_dwapb_platdata *plat = dev->platdata;
> > +
> > +       if (!plat)
> > +               return 0;
> > +
> > +       priv->gpio_count = plat->pins;
> > +       priv->bank_name = plat->name;
> > +
> > +       return 0;
> > +}
> > +
> > +static int gpio_dwapb_bind(struct udevice *dev)
> > +{
> > +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> > +       const void *blob = gd->fdt_blob;
> > +       struct udevice *subdev;
> > +       fdt_addr_t base;
> > +       int ret, node, bank = 0;
> > +
> > +       /* If this is a child device, there is nothing to do here */
> > +       if (plat)
> > +               return 0;
> > +
> > +       base = fdtdec_get_addr(blob, dev->of_offset, "reg");
> > +       if (base == FDT_ADDR_T_NONE) {
> > +               debug("Can't get the GPIO register base address\n");
> > +               return -ENXIO;
> 
> -EINVAL I think, since this is an invalid parameter

OK

[...]