On Wednesday, August 12, 2015 at 04:15:00 PM, Simon Glass wrote:
> Hi Marek,
>
> On 10 August 2015 at 09:30, Marek Vasut <ma...@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 <ma...@denx.de>
> > Cc: Simon Glass <s...@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 <s...@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 <ma...@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
[...]
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
