diff mbox

[8/8] ARM: vt8500: gpio: Devicetree support for arch-vt8500

Message ID 1344389967-8465-9-git-send-email-linux@prisktech.co.nz (mailing list archive)
State New, archived
Headers show

Commit Message

Tony Prisk Aug. 8, 2012, 1:39 a.m. UTC
Converted the existing arch-vt8500 gpio to a platform_device.
Added support for WM8505 and WM8650 GPIO controllers.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
---
 drivers/gpio/Kconfig       |    6 +
 drivers/gpio/Makefile      |    1 +
 drivers/gpio/gpio-vt8500.c |  318 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 325 insertions(+)
 create mode 100644 drivers/gpio/gpio-vt8500.c

Comments

Linus Walleij Aug. 8, 2012, 9:11 a.m. UTC | #1
On Wed, Aug 8, 2012 at 3:39 AM, Tony Prisk <linux@prisktech.co.nz> wrote:

> Converted the existing arch-vt8500 gpio to a platform_device.
> Added support for WM8505 and WM8650 GPIO controllers.
(...)
> +++ b/drivers/gpio/gpio-vt8500.c

This driver looks very one-bit-per-gpio typed. Are you sure you cannot
just reuse drivers/gpio/gpio-generic.c? Make a compelling case please...

> +struct vt8500_gpio_bank_regs {
> +       int     en;
> +       int     dir;
> +       int     data_out;
> +       int     data_in;

Why are all these members int? They should be u8 from reading your code.

> +       int     ngpio;
> +};


> +static struct vt8500_gpio_data vt8500_data = {
> +       .num_banks      = 7,
> +       .banks  = {
> +               VT8500_BANK(0x00, 0x20, 0x40, 0x60, 26),
> +               VT8500_BANK(0x04, 0x24, 0x44, 0x64, 28),
> +               VT8500_BANK(0x08, 0x28, 0x48, 0x68, 31),
> +               VT8500_BANK(0x0C, 0x2C, 0x4C, 0x6C, 19),
> +               VT8500_BANK(0x10, 0x30, 0x50, 0x70, 19),
> +               VT8500_BANK(0x14, 0x34, 0x54, 0x74, 23),
> +               VT8500_BANK(-1, 0x3C, 0x5C, 0x7C, 9),    /* external gpio */

What on earth are all those magic numbers?

I *guess* they're enabling some default GPIO settings etc.

But it really needs better structure, #defines for each one or
atleast include <linux/bitops.h> and say:

= BIT(4) | /* Enable GPIO pin 5 on this bank */
   BIT(5); /* Enable GPIO pin 6 on this bank */

However I suspect this is board specific and should
be taken from device tree. Please elaborate on this...

Ditto for the different instances.

(...)
> +       unsigned val;

Looks like all of these should be u8.

> +       val = readl(vt8500_chip->base + vt8500_chip->regs->en +
> +                                                       vt8500_chip->regoff);

val = (u8) readl(...);

usw

> +       val |= (1 << offset);

Use <linux/bitops.h>

val |= BIT(offset);

Apart from these remarks it's looking good...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Aug. 8, 2012, 9:19 a.m. UTC | #2
On Wednesday 08 August 2012, Linus Walleij wrote:
> On Wed, Aug 8, 2012 at 3:39 AM, Tony Prisk <linux@prisktech.co.nz> wrote:
> 
> > Converted the existing arch-vt8500 gpio to a platform_device.
> > Added support for WM8505 and WM8650 GPIO controllers.
> (...)
> > +++ b/drivers/gpio/gpio-vt8500.c
> 
> This driver looks very one-bit-per-gpio typed. Are you sure you cannot
> just reuse drivers/gpio/gpio-generic.c? Make a compelling case please...
> 
> > +struct vt8500_gpio_bank_regs {
> > +       int     en;
> > +       int     dir;
> > +       int     data_out;
> > +       int     data_in;
> 
> Why are all these members int? They should be u8 from reading your code.
> 
> > +       int     ngpio;
> > +};

Not necessarily 8 bit, but definitely unsigned.

> > +static struct vt8500_gpio_data vt8500_data = {
> > +       .num_banks      = 7,
> > +       .banks  = {
> > +               VT8500_BANK(0x00, 0x20, 0x40, 0x60, 26),
> > +               VT8500_BANK(0x04, 0x24, 0x44, 0x64, 28),
> > +               VT8500_BANK(0x08, 0x28, 0x48, 0x68, 31),
> > +               VT8500_BANK(0x0C, 0x2C, 0x4C, 0x6C, 19),
> > +               VT8500_BANK(0x10, 0x30, 0x50, 0x70, 19),
> > +               VT8500_BANK(0x14, 0x34, 0x54, 0x74, 23),
> > +               VT8500_BANK(-1, 0x3C, 0x5C, 0x7C, 9),    /* external gpio */
> 
> What on earth are all those magic numbers?
> 
> I *guess* they're enabling some default GPIO settings etc.

No, they are the register offsets you quoted above, per bank. There
is no easy way to abstract these, and I suggested putting the
values into the source code rather than describing each bank
separately in the .dtsi file.

My feeling however is that the "vt8500_chip->regoff" is wrong, which
would mean only the first bank works. The code adds the same offsets
per bank once more that it sets in this bank table.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Aug. 8, 2012, 2:28 p.m. UTC | #3
On Wed, Aug 8, 2012 at 11:19 AM, Arnd Bergmann <arnd@arndb.de> wrote:

>> What on earth are all those magic numbers?
>>
>> I *guess* they're enabling some default GPIO settings etc.
>
> No, they are the register offsets you quoted above, per bank.

Aha I was fooled by this:

+struct vt8500_gpio_bank_regs {
+       int     en;
+       int     dir;
+       int     data_out;
+       int     data_in;
+       int     ngpio;
+};

This needs to be named something intuitive like "vt8500_gpio_bank_regoffsets"

Some kerneldoc intsead of the opaque comment above will also improve
readability a lot:

/**
  * struct vt8500_gpio_bank_regoffsets
  * @en: offset to enable register in the bank
  * ...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Aug. 8, 2012, 6:38 p.m. UTC | #4
On 08/07/2012 07:39 PM, Tony Prisk wrote:
> Converted the existing arch-vt8500 gpio to a platform_device.
> Added support for WM8505 and WM8650 GPIO controllers.

> diff --git a/drivers/gpio/gpio-vt8500.c b/drivers/gpio/gpio-vt8500.c

> +static struct of_device_id vt8500_gpio_dt_ids[] = {
> +	{ .compatible = "via,vt8500-gpio", .data = &vt8500_data, },
> +	{ .compatible = "wm,wm8505-gpio", .data = &wm8505_data, },
> +	{ .compatible = "wm,wm8650-gpio", .data = &wm8650_data, },
> +	{ /* Sentinel */ },
> +};
> +
> +static int __devinit vt8500_gpio_probe(struct platform_device *pdev)
> +{
> +	void __iomem *gpio_base;
> +	struct device_node *np;
> +	const struct of_device_id *of_id =
> +				of_match_device(vt8500_gpio_dt_ids, &pdev->dev);
> +
> +	if (!of_id) {
> +		dev_err(&pdev->dev, "Failed to find gpio controller\n");
> +		return -ENODEV;
> +	}
> +
> +	np = of_find_matching_node(NULL, vt8500_gpio_dt_ids);

Can't you use pdev->dev.of_node instead of searching for it again?

...
> +	of_node_put(np);

If so, you could also remove that.

> +static int __init vt8500_gpio_init(void)
> +{
> +	return platform_driver_probe(&vt8500_gpio_driver, &vt8500_gpio_probe);
> +}
> +
> +static void __exit vt8500_gpio_exit(void)
> +{
> +	return platform_driver_unregister(&vt8500_gpio_driver);
> +}
> +
> +module_init(vt8500_gpio_init);
> +module_exit(vt8500_gpio_exit);

I think that's all just:

module_platform_driver(vt8500_gpio_driver);

(except that _init uses platform_driver_probe() rather than
platform_driver_register(), which seems unusual. I guess that explains
the of_find_matching_node() above too.)

> +MODULE_LICENSE("GPL");

That should be "GPL v2" given the license header.

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Aug. 8, 2012, 7:17 p.m. UTC | #5
On Wednesday 08 August 2012, Stephen Warren wrote:
> I think that's all just:
> 
> module_platform_driver(vt8500_gpio_driver);
> 
> (except that _init uses platform_driver_probe() rather than
> platform_driver_register(), which seems unusual. I guess that explains
> the of_find_matching_node() above too.)

Ah, I totally missed both of these. Using platform_driver_register
is definitely preferred over platform_driver_probe in cases like
this, so using module_platform_driver is the right simplification.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 542f0c0..3c8897a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -183,6 +183,12 @@  config GPIO_STA2X11
 	  Say yes here to support the STA2x11/ConneXt GPIO device.
 	  The GPIO module has 128 GPIO pins with alternate functions.
 
+config GPIO_VT8500
+	bool "VIA/Wondermedia SoC GPIO Support"
+	depends on ARCH_VT8500
+	help
+	  Say yes here to support the VT8500/WM8505/WM8650 GPIO controller.
+
 config GPIO_XILINX
 	bool "Xilinx GPIO support"
 	depends on PPC_OF || MICROBLAZE
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 0f55662..2c014b9 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -66,6 +66,7 @@  obj-$(CONFIG_GPIO_TPS65912)	+= gpio-tps65912.o
 obj-$(CONFIG_GPIO_TWL4030)	+= gpio-twl4030.o
 obj-$(CONFIG_GPIO_UCB1400)	+= gpio-ucb1400.o
 obj-$(CONFIG_GPIO_VR41XX)	+= gpio-vr41xx.o
+obj-$(CONFIG_GPIO_VT8500)	+= gpio-vt8500.o
 obj-$(CONFIG_GPIO_VX855)	+= gpio-vx855.o
 obj-$(CONFIG_GPIO_WM831X)	+= gpio-wm831x.o
 obj-$(CONFIG_GPIO_WM8350)	+= gpio-wm8350.o
diff --git a/drivers/gpio/gpio-vt8500.c b/drivers/gpio/gpio-vt8500.c
new file mode 100644
index 0000000..3306634
--- /dev/null
+++ b/drivers/gpio/gpio-vt8500.c
@@ -0,0 +1,318 @@ 
+/* linux/arch/arm/mach-vt8500/gpio.c
+ *
+ * Copyright (C) 2012 Tony Prisk <linux@prisktech.co.nz>
+ * Based on gpio.c:
+ * - Copyright (C) 2010 Alexey Charkov <alchark@gmail.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_device.h>
+
+/*
+	We handle GPIOs by bank, each bank containing up to 32 GPIOs covered
+	by one set of registers (although not all may be valid).
+
+	Because different SoC's have different register offsets, we pass the
+	register offsets as data in vt8500_gpio_dt_ids[].
+*/
+
+struct vt8500_gpio_bank_regs {
+	int	en;
+	int	dir;
+	int	data_out;
+	int	data_in;
+	int	ngpio;
+};
+
+struct vt8500_gpio_data {
+	unsigned int			num_banks;
+	struct vt8500_gpio_bank_regs	banks[];
+};
+
+#define VT8500_BANK(__en, __dir, __out, __in, __ngpio)		\
+{								\
+	.en = __en,						\
+	.dir = __dir,						\
+	.data_out = __out,					\
+	.data_in = __in,					\
+	.ngpio = __ngpio,					\
+}
+
+static struct vt8500_gpio_data vt8500_data = {
+	.num_banks	= 7,
+	.banks	= {
+		VT8500_BANK(0x00, 0x20, 0x40, 0x60, 26),
+		VT8500_BANK(0x04, 0x24, 0x44, 0x64, 28),
+		VT8500_BANK(0x08, 0x28, 0x48, 0x68, 31),
+		VT8500_BANK(0x0C, 0x2C, 0x4C, 0x6C, 19),
+		VT8500_BANK(0x10, 0x30, 0x50, 0x70, 19),
+		VT8500_BANK(0x14, 0x34, 0x54, 0x74, 23),
+		VT8500_BANK(-1, 0x3C, 0x5C, 0x7C, 9),	 /* external gpio */
+	},
+};
+
+static struct vt8500_gpio_data wm8505_data = {
+	.num_banks	= 10,
+	.banks	= {
+		VT8500_BANK(0x40, 0x68, 0x90, 0xB8, 8),
+		VT8500_BANK(0x44, 0x6C, 0x94, 0xBC, 32),
+		VT8500_BANK(0x48, 0x70, 0x98, 0xC0, 6),
+		VT8500_BANK(0x4C, 0x74, 0x9C, 0xC4, 16),
+		VT8500_BANK(0x50, 0x78, 0xA0, 0xC8, 25),
+		VT8500_BANK(0x54, 0x7C, 0xA4, 0xCC, 5),
+		VT8500_BANK(0x58, 0x80, 0xA8, 0xD0, 5),
+		VT8500_BANK(0x5C, 0x84, 0xAC, 0xD4, 12),
+		VT8500_BANK(0x60, 0x88, 0xB0, 0xD8, 16),
+		VT8500_BANK(0x64, 0x8C, 0xB4, 0xDC, 22),
+	},
+};
+
+/*
+ * No information about which bits are valid so we just make
+ * them all available until its figured out.
+ */
+static struct vt8500_gpio_data wm8650_data = {
+	.num_banks	= 9,
+	.banks	= {
+		VT8500_BANK(0x40, 0x80, 0xC0, 0x00, 32),
+		VT8500_BANK(0x44, 0x84, 0xC4, 0x04, 32),
+		VT8500_BANK(0x48, 0x88, 0xC8, 0x08, 32),
+		VT8500_BANK(0x4C, 0x8C, 0xCC, 0x0C, 32),
+		VT8500_BANK(0x50, 0x90, 0xD0, 0x10, 32),
+		VT8500_BANK(0x54, 0x94, 0xD4, 0x14, 32),
+		VT8500_BANK(0x58, 0x98, 0xD8, 0x18, 32),
+		VT8500_BANK(0x5C, 0x9C, 0xDC, 0x1C, 32),
+		VT8500_BANK(0x7C, 0xBC, 0xFC, 0x3C, 32),
+	},
+};
+
+struct vt8500_gpio_chip {
+	struct gpio_chip		chip;
+
+	const struct vt8500_gpio_bank_regs *regs;
+	void __iomem	*base;
+	unsigned int	regoff;
+};
+
+
+#define to_vt8500(__chip) container_of(__chip, struct vt8500_gpio_chip, chip)
+
+static int vt8500_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	unsigned val;
+	struct vt8500_gpio_chip *vt8500_chip = to_vt8500(chip);
+
+	val = readl(vt8500_chip->base + vt8500_chip->regs->en +
+							vt8500_chip->regoff);
+	val |= (1 << offset);
+	writel(val, vt8500_chip->base + vt8500_chip->regs->en +
+							vt8500_chip->regoff);
+
+	return 0;
+}
+
+static void vt8500_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	struct vt8500_gpio_chip *vt8500_chip = to_vt8500(chip);
+
+	unsigned val = readl(vt8500_chip->base + vt8500_chip->regs->en +
+							vt8500_chip->regoff);
+	val &= ~(1 << offset);
+	writel(val, vt8500_chip->base + vt8500_chip->regs->en +
+							vt8500_chip->regoff);
+}
+
+static int vt8500_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+	struct vt8500_gpio_chip *vt8500_chip = to_vt8500(chip);
+
+	unsigned val = readl(vt8500_chip->base + vt8500_chip->regs->dir +
+							vt8500_chip->regoff);
+	val &= ~(1 << offset);
+	writel(val, vt8500_chip->base + vt8500_chip->regs->dir +
+							vt8500_chip->regoff);
+
+	return 0;
+}
+
+static int vt8500_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
+								int value)
+{
+	struct vt8500_gpio_chip *vt8500_chip = to_vt8500(chip);
+
+	unsigned val = readl(vt8500_chip->base + vt8500_chip->regs->dir +
+							vt8500_chip->regoff);
+	val |= (1 << offset);
+	writel(val, vt8500_chip->base + vt8500_chip->regs->dir +
+							vt8500_chip->regoff);
+
+	if (value) {
+		val = readl(vt8500_chip->base + vt8500_chip->regs->data_out +
+							vt8500_chip->regoff);
+		val |= (1 << offset);
+		writel(val, vt8500_chip->base + vt8500_chip->regs->data_out +
+							vt8500_chip->regoff);
+	}
+	return 0;
+}
+
+static int vt8500_gpio_get_value(struct gpio_chip *chip, unsigned offset)
+{
+	struct vt8500_gpio_chip *vt8500_chip = to_vt8500(chip);
+
+	return (readl(vt8500_chip->base + vt8500_chip->regoff) >> offset) & 1;
+}
+
+static void vt8500_gpio_set_value(struct gpio_chip *chip, unsigned offset,
+								int value)
+{
+	struct vt8500_gpio_chip *vt8500_chip = to_vt8500(chip);
+
+	unsigned val = readl(vt8500_chip->base + vt8500_chip->regs->data_out +
+							vt8500_chip->regoff);
+	if (value)
+		val |= (1 << offset);
+	else
+		val &= ~(1 << offset);
+
+	writel(val, vt8500_chip->base + vt8500_chip->regs->data_out + 
+							vt8500_chip->regoff);
+}
+
+static int vt8500_of_xlate(struct gpio_chip *gc,
+			    const struct of_phandle_args *gpiospec, u32 *flags)
+{
+	/* bank if specificed in gpiospec->args[0] */
+	if (flags)
+		*flags = gpiospec->args[2];
+
+	return gpiospec->args[1];
+}
+
+static int vt8500_add_chips(struct platform_device *pdev, void __iomem *base,
+				const struct vt8500_gpio_data *data)
+{
+	struct vt8500_gpio_chip *vtchip;
+	struct gpio_chip *chip;
+	int i;
+	int pin_cnt = 0;
+
+	vtchip = devm_kzalloc(&pdev->dev,
+			sizeof(struct vt8500_gpio_chip) * data->num_banks,
+			GFP_KERNEL);
+	if (!vtchip) {
+		pr_err("%s: failed to allocate chip memory\n", __func__);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < data->num_banks; i++)
+	{
+		vtchip[i].base = base;
+		vtchip[i].regs = &data->banks[i];
+		vtchip[i].regoff = i << 2;
+
+		chip = &vtchip[i].chip;
+
+		chip->of_xlate = vt8500_of_xlate;
+		chip->of_gpio_n_cells = 3;
+		chip->of_node = pdev->dev.of_node;
+
+		chip->request = vt8500_gpio_request;
+		chip->free = vt8500_gpio_free;
+		chip->direction_input = vt8500_gpio_direction_input;
+		chip->direction_output = vt8500_gpio_direction_output;
+		chip->get = vt8500_gpio_get_value;
+		chip->set = vt8500_gpio_set_value;
+		chip->can_sleep = 0;
+		chip->base = pin_cnt;
+		chip->ngpio = data->banks[i].ngpio;
+
+		pin_cnt += data->banks[i].ngpio;
+
+		gpiochip_add(chip);
+	}
+	return 0;
+}
+
+static struct of_device_id vt8500_gpio_dt_ids[] = {
+	{ .compatible = "via,vt8500-gpio", .data = &vt8500_data, },
+	{ .compatible = "wm,wm8505-gpio", .data = &wm8505_data, },
+	{ .compatible = "wm,wm8650-gpio", .data = &wm8650_data, },
+	{ /* Sentinel */ },
+};
+
+static int __devinit vt8500_gpio_probe(struct platform_device *pdev)
+{
+	void __iomem *gpio_base;
+	struct device_node *np;
+	const struct of_device_id *of_id =
+				of_match_device(vt8500_gpio_dt_ids, &pdev->dev);
+
+	if (!of_id) {
+		dev_err(&pdev->dev, "Failed to find gpio controller\n");
+		return -ENODEV;
+	}
+
+	np = of_find_matching_node(NULL, vt8500_gpio_dt_ids);
+	if (!np) {
+		dev_err(&pdev->dev, "Missing GPIO description in devicetree\n");
+		return -EFAULT;
+	}
+
+	gpio_base = of_iomap(np, 0);
+	if (!gpio_base) {
+		dev_err(&pdev->dev, "Unable to map GPIO registers\n");
+		of_node_put(np);
+		return -ENOMEM;
+	}
+
+	of_node_put(np);
+
+	vt8500_add_chips(pdev, gpio_base, of_id->data);
+
+	return 0;
+}
+
+static struct platform_driver vt8500_gpio_driver = {
+	.probe		= vt8500_gpio_probe,
+	.driver		= {
+		.name	= "vt8500-gpio",
+		.owner	= THIS_MODULE,
+		.of_match_table = vt8500_gpio_dt_ids,
+	},
+};
+
+static int __init vt8500_gpio_init(void)
+{
+	return platform_driver_probe(&vt8500_gpio_driver, &vt8500_gpio_probe);
+}
+
+static void __exit vt8500_gpio_exit(void)
+{
+	return platform_driver_unregister(&vt8500_gpio_driver);
+}
+
+module_init(vt8500_gpio_init);
+module_exit(vt8500_gpio_exit);
+
+MODULE_DESCRIPTION("VT8500 GPIO Driver");
+MODULE_AUTHOR("Tony Prisk <linux@prisktech.co.nz>");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, vt8500_gpio_dt_ids);