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_GPIOdefault nhelpSupport 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 DMdiff --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);elseclrbits_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);elseclrbits_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
[...]