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
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
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
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.
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
Tony Prisk Aug. 8, 2012, 7:46 p.m. UTC | #6
>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
>  * ...

Changes made as requested.


>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

Oh.. Thanks for picking that up Arnd. You are absolutely correct. I have removed
the references for ->regoff. Left over from old code.

Regards

Tony Prisk
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);