diff mbox

[2/6] dm: gpio: hi6220: Add a hi6220 GPIO driver model driver.

Message ID 1431437912-18988-3-git-send-email-peter.griffin@linaro.org
State New
Headers show

Commit Message

Peter Griffin May 12, 2015, 1:38 p.m. UTC 
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-armv8/gpio.h | 47 +++++++++++++++++
 drivers/gpio/Makefile                  |  2 +
 drivers/gpio/hi6220_gpio.c             | 95 ++++++++++++++++++++++++++++++++++
 3 files changed, 144 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-armv8/gpio.h
 create mode 100644 drivers/gpio/hi6220_gpio.c

Comments

Peter Griffin May 13, 2015, noon UTC | #1 
Hi Marek,

Thanks for reviewing.

> 
> > 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-armv8/gpio.h | 47 +++++++++++++++++
> >  drivers/gpio/Makefile                  |  2 +
> >  drivers/gpio/hi6220_gpio.c             | 95
> > ++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+)
> >  create mode 100644 arch/arm/include/asm/arch-armv8/gpio.h
> >  create mode 100644 drivers/gpio/hi6220_gpio.c
> > 
> > diff --git a/arch/arm/include/asm/arch-armv8/gpio.h
> > b/arch/arm/include/asm/arch-armv8/gpio.h new file mode 100644
> > index 0000000..162c2d9
> > --- /dev/null
> > +++ b/arch/arm/include/asm/arch-armv8/gpio.h
> > @@ -0,0 +1,47 @@
> > +/*
> > + * 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_GPIO0_BASE	(void *)0xf8011000
> 
> You should drop the explicit cast, that's nasty.

If I drop the cast I get lots of 

include/asm/arch/gpio.h:11:32: warning: initialization makes pointer from integer
without a cast

compiler warnings.


> Also, why don't you define this as a HI6220_GPIO_BASE(bank) instead?
> That'd trim down the number of macros and from what I see, it should
> be rather easy to do ...
> 
> #define HI6220_GPIO_BASE(bank)
> 	(((bank < 4) ? 0xf8012000 : (0xf7020000 - 0x4000)) + (0x1000 * bank))

Yes good idea, I will do it like you suggest in V2. Currently I have

#define HI6220_GPIO_BASE(bank)	(void *)(((bank < 4) ? 0xf8011000 : \
				0xf7020000 - 0x4000) + (0x1000 * bank))

To avoid the warnings mentioned above.

<snip>
> > +#define HI6220_GPIO17_BASE	(void *)0xf702d000
> > +#define HI6220_GPIO18_BASE	(void *)0xf702e000
> > +#define HI6220_GPIO19_BASE	(void *)0xf702f000
> 
> But are these even used in the driver anywhere ?

They are currently used in the hikey.c board file which defines the 
hikey_gpio_platdata structure.

Although thinking about it some more this should be moved from the hikey board
file to the driver as it is SoC specific rather than board specific.
> 
> > +#define BIT(x)			(1 << (x))
> 
> This macro should be placed into common header files.
> 
> > +#define HI6220_GPIO_PER_BANK	8
> > +#define HI6220_GPIO_DIR		0x400
> > +
> > +struct gpio_bank {
> > +	u8 *base;	/* address of registers in physical memory */
> 
> Should be void __iomem *, no ?

Will fix in v2.

regards,

Peter.
diff mbox

Patch

diff --git a/arch/arm/include/asm/arch-armv8/gpio.h b/arch/arm/include/asm/arch-armv8/gpio.h
new file mode 100644
index 0000000..162c2d9
--- /dev/null
+++ b/arch/arm/include/asm/arch-armv8/gpio.h
@@ -0,0 +1,47 @@ 
+/*
+ * 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_GPIO0_BASE	(void *)0xf8011000
+#define HI6220_GPIO1_BASE	(void *)0xf8012000
+#define HI6220_GPIO2_BASE	(void *)0xf8013000
+#define HI6220_GPIO3_BASE	(void *)0xf8014000
+#define HI6220_GPIO4_BASE	(void *)0xf7020000
+#define HI6220_GPIO5_BASE	(void *)0xf7021000
+#define HI6220_GPIO6_BASE	(void *)0xf7022000
+#define HI6220_GPIO7_BASE	(void *)0xf7023000
+#define HI6220_GPIO8_BASE	(void *)0xf7024000
+#define HI6220_GPIO9_BASE	(void *)0xf7025000
+#define HI6220_GPIO10_BASE	(void *)0xf7026000
+#define HI6220_GPIO11_BASE	(void *)0xf7027000
+#define HI6220_GPIO12_BASE	(void *)0xf7028000
+#define HI6220_GPIO13_BASE	(void *)0xf7029000
+#define HI6220_GPIO14_BASE	(void *)0xf702a000
+#define HI6220_GPIO15_BASE	(void *)0xf702b000
+#define HI6220_GPIO16_BASE	(void *)0xf702c000
+#define HI6220_GPIO17_BASE	(void *)0xf702d000
+#define HI6220_GPIO18_BASE	(void *)0xf702e000
+#define HI6220_GPIO19_BASE	(void *)0xf702f000
+
+#define BIT(x)			(1 << (x))
+
+#define HI6220_GPIO_PER_BANK	8
+#define HI6220_GPIO_DIR		0x400
+
+struct gpio_bank {
+	u8 *base;	/* address of registers in physical memory */
+};
+
+/* Information about a GPIO bank */
+struct hikey_gpio_platdata {
+	int bank_index;
+	void *base;     /* address of registers in physical memory */
+};
+
+#endif /* _HI6220_GPIO_H_ */
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 85f71c5..470fabd 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -42,3 +42,5 @@  obj-$(CONFIG_TCA642X)		+= tca642x.o
 oby-$(CONFIG_SX151X)		+= sx151x.o
 obj-$(CONFIG_SUNXI_GPIO)	+= sunxi_gpio.o
 obj-$(CONFIG_LPC32XX_GPIO)	+= lpc32xx_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);
+
+	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)));
+	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);
+
+	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),
+};
+
+