[PATCH 0/4] PowerPC: implement GPIO API

[PATCH 0/4] PowerPC: implement GPIO API

[Anton Vorontsov]

Hi all,

OF device tree GPIOs bindings are similar to IRQs:

pario0: gpio-controller@0 {
	#gpio-cells = <2>;
	num-ports = <7>;
};

device@0 {
	gpios = <bank pin bank pin bank pin>;
	gpio-parent = <&pario0>;
};

"bank pin" scheme is controller specific, so controllers that want
to implement flat mappings or any other could do so.

So far I implemented GPIO API for QE and CPM2 chips. GPIO API for
CPM1 implemented by Jochen Friedrich <jochen@scram.de>, included in
this patchset.

- QE GPIO API tested to work with FSL UPM NAND driver and
  MPC8360E-RDK (STMicro NAND512W3A2BN6E);
- CPM2 GPIO API tested to work with MPC8555E+Samsung HY27UF081G6 (LP);
- CPM1 GPIO API untested.

GPIO API is described in Documentation/gpio.txt, and these
patches are tend to support most of it.

As an additional bonus, PowerPC now gets access to few pleasing
drivers:

- drivers/leds/leds-gpio.c (we could use it to play with
  on-board LEDs);
- drivers/i2c/busses/i2c-gpio.c (generic I2C bit-banging driver);
- drivers/input/keyboard/gpio_keys.c - gpio keys (requires
  gpio_to_irq, so far not implemented);
- Could be more (I named the ones I knew about).

Also, in the upcoming kernels, there will be GPIOLIB[1] addition to
the generic GPIO API, to support off-chip GPIO expanders (like MFDs
on I2C/LBC). But so far we support on-chip GPIOs only, with single
controller built-in.

Changes since RFC:
- Implemented #gpio-cells handling;
- Per-bank spinlocks removed;
- Added a patch which implements GPIO API for CPM1;
- Few minor fixes.

Thanks,

[1] http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.24-rc4/2.6.24-rc4-mm1/broken-out/generic-gpio-gpio_chip-support.patch

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

[PATCH 1/4] [POWERPC] Implement GPIO API embryo

[Anton Vorontsov]

This patch implements GPIO API as described in Documentation/gpio.txt.
Two calls unimplemented though: irq_to_gpio and gpio_to_irq.

This patch also provides OF helpers.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/Kconfig             |    3 ++
 arch/powerpc/kernel/prom_parse.c |   64 ++++++++++++++++++++++++++++++++++++++
 include/asm-powerpc/gpio.h       |   47 ++++++++++++++++++++++++++++
 include/asm-powerpc/prom.h       |   23 +++++++++++++
 4 files changed, 137 insertions(+), 0 deletions(-)
 create mode 100644 include/asm-powerpc/gpio.h

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 232c298..a4fa173 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -73,6 +73,9 @@ config GENERIC_FIND_NEXT_BIT
 	bool
 	default y
 
+config GENERIC_GPIO
+	bool
+
 config ARCH_NO_VIRT_TO_BUS
 	def_bool PPC64
 
diff --git a/arch/powerpc/kernel/prom_parse.c b/arch/powerpc/kernel/prom_parse.c
index 90eb3a3..9875598 100644
--- a/arch/powerpc/kernel/prom_parse.c
+++ b/arch/powerpc/kernel/prom_parse.c
@@ -7,6 +7,7 @@
 #include <linux/ioport.h>
 #include <linux/etherdevice.h>
 #include <asm/prom.h>
+#include <asm/gpio.h>
 #include <asm/pci-bridge.h>
 
 #ifdef DEBUG
@@ -1079,3 +1080,66 @@ void __iomem *of_iomap(struct device_node *np, int index)
 	return ioremap(res.start, 1 + res.end - res.start);
 }
 EXPORT_SYMBOL(of_iomap);
+
+int __of_parse_gpio_bank_pin(struct device_node *np, int index,
+			     int bank_width, int max_bank)
+{
+	int bank;
+	int pin;
+	const u32 *gpios;
+
+	/*
+	 * We can get there only if of_get_gpio() succeeded, thus
+	 * no need checking for "gpios" existence.
+	 */
+	gpios = of_get_property(np, "gpios", NULL);
+	bank = gpios[index * 2];
+	pin = gpios[index * 2 + 1];
+
+	if (bank >= max_bank || pin >= bank_width)
+		return -EINVAL;
+
+	return bank * bank_width + pin;
+}
+EXPORT_SYMBOL_GPL(__of_parse_gpio_bank_pin);
+
+int of_get_gpio(struct device_node *np, int index)
+{
+	int ret = -EINVAL;
+	const phandle *gc_ph;
+	struct device_node *gc;
+	struct of_gpio_chip *of_gpio_chip;
+	int size;
+	const u32 *gpio_cells;
+	const u32 *gpios;
+	u32 nr_cells;
+
+	gc_ph = of_get_property(np, "gpio-parent", NULL);
+	if (!gc_ph)
+		return ret;
+
+	gc = of_find_node_by_phandle(*gc_ph);
+	if (!gc || !gc->data)
+		return ret;
+
+	gpio_cells = of_get_property(gc, "#gpio-cells", &size);
+	if (!gpio_cells || size != sizeof(*gpio_cells) || *gpio_cells == 0)
+		goto err;
+
+	gpios = of_get_property(np, "gpios", &size);
+	if (!gpios)
+		goto err;
+	nr_cells = size / sizeof(u32);
+
+	if (nr_cells < *gpio_cells || nr_cells % *gpio_cells ||
+			index > nr_cells / *gpio_cells - 1)
+		goto err;
+
+	of_gpio_chip = gc->data;
+
+	ret = of_gpio_chip->xlate(np, index);
+err:
+	of_node_put(gc);
+	return ret;
+}
+EXPORT_SYMBOL(of_get_gpio);
diff --git a/include/asm-powerpc/gpio.h b/include/asm-powerpc/gpio.h
new file mode 100644
index 0000000..e0a4f85
--- /dev/null
+++ b/include/asm-powerpc/gpio.h
@@ -0,0 +1,47 @@
+/*
+ * Generic GPIO API implementation for PowerPC.
+ *
+ * Copyright (c) 2007  MontaVista Software, Inc.
+ * Copyright (c) 2007  Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __ASM_POWERPC_GPIO_H
+#define __ASM_POWERPC_GPIO_H
+
+extern int gpio_request(unsigned int gpio, const char *label);
+static inline void gpio_free(unsigned int gpio) {}
+
+extern int gpio_direction_input(unsigned int gpio);
+extern int gpio_direction_output(unsigned int gpio, int value);
+
+extern int gpio_get_value(unsigned int gpio);
+extern int gpio_set_value(unsigned int gpio, int value);
+
+/*
+ * Not implemented, yet.
+ */
+static inline int gpio_to_irq(unsigned int gpio)
+{
+	return -ENOSYS;
+}
+
+static inline int irq_to_gpio(unsigned int irq)
+{
+	return -ENOSYS;
+}
+
+/*
+ * OF specific gpio_chip handler.
+ */
+struct of_gpio_chip {
+	int (*xlate)(struct device_node *np, int index);
+};
+
+#include <asm-generic/gpio.h>
+
+#endif /* __ASM_POWERPC_GPIO_H */
diff --git a/include/asm-powerpc/prom.h b/include/asm-powerpc/prom.h
index 78b7b0d..f882efc 100644
--- a/include/asm-powerpc/prom.h
+++ b/include/asm-powerpc/prom.h
@@ -330,6 +330,29 @@ extern int of_irq_to_resource(struct device_node *dev, int index,
  */
 extern void __iomem *of_iomap(struct device_node *device, int index);
 
+/**
+ * __of_parse_gpio_bank_pin - Helper function to translate "bank pin" GPIOs
+ * @np:		device node to get GPIO from
+ * @index:	index of the GPIO
+ * @bank_width:	pins per bank
+ * @max_bank:	maximum value to represent a bank
+ *
+ * Returns GPIO number to use with Linux generic GPIO api, or
+ * one of the errno value on the error condition.
+ */
+extern int __of_parse_gpio_bank_pin(struct device_node *np, int index,
+				    int bank_width, int max_bank);
+
+/**
+ * of_get_gpio - Translate GPIO number given in the device tree
+ * @np:		device node to get GPIO from
+ * @index:	index of the GPIO
+ *
+ * Returns GPIO number to use with Linux generic GPIO API, or
+ * one of the errno value on the error condition.
+ */
+extern int of_get_gpio(struct device_node *np, int index);
+
 /*
  * NB:  This is here while we transition from using asm/prom.h
  * to linux/of.h
-- 
1.5.2.2

Re: [PATCH 1/4] [POWERPC] Implement GPIO API embryo

[Segher Boessenkool]

> +int __of_parse_gpio_bank_pin(struct device_node *np, int index,
> +			     int bank_width, int max_bank)
> +{
> +	int bank;
> +	int pin;
> +	const u32 *gpios;
> +
> +	/*
> +	 * We can get there only if of_get_gpio() succeeded, thus
> +	 * no need checking for "gpios" existence.
> +	 */
> +	gpios = of_get_property(np, "gpios", NULL);
> +	bank = gpios[index * 2];
> +	pin = gpios[index * 2 + 1];

If you stick with the #gpio-cells plan, here is where you should use it.


Segher

Re: [PATCH 1/4] [POWERPC] Implement GPIO API embryo

[David Gibson]

On Sun, Dec 23, 2007 at 03:49:30AM +0100, Segher Boessenkool wrote:
> > +int __of_parse_gpio_bank_pin(struct device_node *np, int index,
> > +			     int bank_width, int max_bank)
> > +{
> > +	int bank;
> > +	int pin;
> > +	const u32 *gpios;
> > +
> > +	/*
> > +	 * We can get there only if of_get_gpio() succeeded, thus
> > +	 * no need checking for "gpios" existence.
> > +	 */
> > +	gpios = of_get_property(np, "gpios", NULL);
> > +	bank = gpios[index * 2];
> > +	pin = gpios[index * 2 + 1];
> 
> If you stick with the #gpio-cells plan, here is where you should use it.

I think part of what's happening here is due to the patch's history.
The "bank pin" information was always of a format local to the
controller, but originally the size wasn't explicitly stated in the
tree - it was just implicit in the type of gpio controller.  I
suggested that #gpio-cells be added, which it has been, but it looks
like the code hasn't been updated to use it everywhere.  Obviously a
driver for a particular gpio controller would generally need to assert
that the controller's #gpio-cells has the correct value for this
controller type, after which code like the above would be acceptable.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 1/4] [POWERPC] Implement GPIO API embryo

[Segher Boessenkool]

> I think part of what's happening here is due to the patch's history.

That doesn't make it right though ;-)

> Obviously a
> driver for a particular gpio controller would generally need to assert
> that the controller's #gpio-cells has the correct value for this
> controller type, after which code like the above would be acceptable.

Well to nitpick...  _a_ correct value.  But sure, if this is device-
specific code, it is okay.  OTOH, this really should be a helper
function that each GPIO controller driver can use.


Segher

[PATCH 2/4] [POWERPC] QE: implement GPIO API

[Anton Vorontsov]

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/platforms/Kconfig     |    1 +
 arch/powerpc/sysdev/qe_lib/qe_io.c |   82 +++++++++++++++++++++++++++++++++++-
 2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index ea22cad..3d9ff27 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -265,6 +265,7 @@ config TAU_AVERAGE
 config QUICC_ENGINE
 	bool
 	select PPC_LIB_RHEAP
+	select GENERIC_GPIO
 	help
 	  The QUICC Engine (QE) is a new generation of communications
 	  coprocessors on Freescale embedded CPUs (akin to CPM in older chips).
diff --git a/arch/powerpc/sysdev/qe_lib/qe_io.c b/arch/powerpc/sysdev/qe_lib/qe_io.c
index e53ea4d..3c9e5fe 100644
--- a/arch/powerpc/sysdev/qe_lib/qe_io.c
+++ b/arch/powerpc/sysdev/qe_lib/qe_io.c
@@ -23,6 +23,7 @@
 
 #include <asm/io.h>
 #include <asm/prom.h>
+#include <asm/gpio.h>
 #include <sysdev/fsl_soc.h>
 
 #undef DEBUG
@@ -43,6 +44,16 @@ struct port_regs {
 
 static struct port_regs *par_io = NULL;
 static int num_par_io_ports = 0;
+static spinlock_t qe_pio_lock = __SPIN_LOCK_UNLOCKED(qe_pio_lock);
+
+static int par_io_xlate(struct device_node *np, int index)
+{
+	return __of_parse_gpio_bank_pin(np, index, 32, num_par_io_ports);
+}
+
+static struct of_gpio_chip of_gpio_chip = {
+	.xlate = par_io_xlate,
+};
 
 int par_io_init(struct device_node *np)
 {
@@ -60,6 +71,8 @@ int par_io_init(struct device_node *np)
 	if (num_ports)
 		num_par_io_ports = *num_ports;
 
+	np->data = &of_gpio_chip;
+
 	return 0;
 }
 
@@ -67,9 +80,12 @@ int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
 		      int assignment, int has_irq)
 {
 	u32 pin_mask1bit, pin_mask2bits, new_mask2bits, tmp_val;
+	unsigned long flags;
 
-	if (!par_io)
-		return -1;
+	if (!par_io || port >= num_par_io_ports)
+		return -EINVAL;
+
+	spin_lock_irqsave(&qe_pio_lock, flags);
 
 	/* calculate pin location for single and 2 bits information */
 	pin_mask1bit = (u32) (1 << (NUM_OF_PINS - (pin + 1)));
@@ -126,6 +142,8 @@ int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
 		out_be32(&par_io[port].cppar1, new_mask2bits | tmp_val);
 	}
 
+	spin_unlock_irqrestore(&qe_pio_lock, flags);
+
 	return 0;
 }
 EXPORT_SYMBOL(par_io_config_pin);
@@ -133,6 +151,7 @@ EXPORT_SYMBOL(par_io_config_pin);
 int par_io_data_set(u8 port, u8 pin, u8 val)
 {
 	u32 pin_mask, tmp_val;
+	unsigned long flags;
 
 	if (port >= num_par_io_ports)
 		return -EINVAL;
@@ -141,6 +160,8 @@ int par_io_data_set(u8 port, u8 pin, u8 val)
 	/* calculate pin location */
 	pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - pin));
 
+	spin_lock_irqsave(&qe_pio_lock, flags);
+
 	tmp_val = in_be32(&par_io[port].cpdata);
 
 	if (val == 0)		/* clear */
@@ -148,10 +169,25 @@ int par_io_data_set(u8 port, u8 pin, u8 val)
 	else			/* set */
 		out_be32(&par_io[port].cpdata, pin_mask | tmp_val);
 
+	spin_unlock_irqrestore(&qe_pio_lock, flags);
 	return 0;
 }
 EXPORT_SYMBOL(par_io_data_set);
 
+static inline int par_io_data_get(u8 port, u8 pin)
+{
+	u32 pin_mask;
+
+	if (port >= num_par_io_ports)
+		return -EINVAL;
+	if (pin >= NUM_OF_PINS)
+		return -EINVAL;
+	/* calculate pin location */
+	pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - pin));
+
+	return !!(in_be32(&par_io[port].cpdata) & pin_mask);
+}
+
 int par_io_of_config(struct device_node *np)
 {
 	struct device_node *pio;
@@ -195,6 +231,48 @@ int par_io_of_config(struct device_node *np)
 }
 EXPORT_SYMBOL(par_io_of_config);
 
+int gpio_request(unsigned int gpio, const char *label)
+{
+	if (!par_io)
+		return -ENODEV;
+
+	if (gpio / 32 >= num_par_io_ports)
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpio_request);
+
+int gpio_direction_input(unsigned int gpio)
+{
+	return par_io_config_pin(gpio / 32, gpio % 32, 2, 0, 0, 0);
+}
+EXPORT_SYMBOL_GPL(gpio_direction_input);
+
+int gpio_direction_output(unsigned int gpio, int value)
+{
+	int ret;
+
+	ret = par_io_config_pin(gpio / 32, gpio % 32, 1, 0, 0, 0);
+	if (ret)
+		return ret;
+
+	return par_io_data_set(gpio / 32, gpio % 32, value);
+}
+EXPORT_SYMBOL_GPL(gpio_direction_output);
+
+int gpio_get_value(unsigned int gpio)
+{
+	return par_io_data_get(gpio / 32, gpio % 32);
+}
+EXPORT_SYMBOL_GPL(gpio_get_value);
+
+int gpio_set_value(unsigned int gpio, int value)
+{
+	return par_io_data_set(gpio / 32, gpio % 32, value);
+}
+EXPORT_SYMBOL_GPL(gpio_set_value);
+
 #ifdef DEBUG
 static void dump_par_io(void)
 {
-- 
1.5.2.2

[PATCH 3/4] [POWERPC] CPM2: implement GPIO API

[Anton Vorontsov]

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/platforms/Kconfig    |    1 +
 arch/powerpc/sysdev/cpm2_common.c |  121 +++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index 3d9ff27..cc2d54e 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -277,6 +277,7 @@ config CPM2
 	default n
 	select CPM
 	select PPC_LIB_RHEAP
+	select GENERIC_GPIO
 	help
 	  The CPM2 (Communications Processor Module) is a coprocessor on
 	  embedded CPUs made by Freescale.  Selecting this option means that
diff --git a/arch/powerpc/sysdev/cpm2_common.c b/arch/powerpc/sysdev/cpm2_common.c
index f7188e2..fe25978 100644
--- a/arch/powerpc/sysdev/cpm2_common.c
+++ b/arch/powerpc/sysdev/cpm2_common.c
@@ -31,12 +31,14 @@
 #include <linux/param.h>
 #include <linux/string.h>
 #include <linux/mm.h>
+#include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
+#include <asm/gpio.h>
 #include <asm/mpc8260.h>
 #include <asm/page.h>
 #include <asm/pgtable.h>
@@ -61,8 +63,43 @@ cpm2_map_t __iomem *cpm2_immr;
 					   of space for CPM as it is larger
 					   than on PQ2 */
 
+static spinlock_t cpm2_port_lock = __SPIN_LOCK_UNLOCKED(cpm2_port_lock);
+static int cpm2_num_ports;
+
+static int par_io_xlate(struct device_node *np, int index)
+{
+	return __of_parse_gpio_bank_pin(np, index, 32, cpm2_num_ports);
+}
+
+static struct of_gpio_chip of_gpio_chip = {
+	.xlate = par_io_xlate,
+};
+
+int cpm2_init_par_io(void)
+{
+	struct device_node *np;
+	const u32 *num_ports;
+
+	np = of_find_compatible_node(NULL, NULL, "fsl,cpm2-pario");
+	if (!np)
+		return -ENOENT;
+
+	num_ports = of_get_property(np, "num-ports", NULL);
+	if (!num_ports) {
+		of_node_put(np);
+		return -ENOENT;
+	}
+	cpm2_num_ports = *num_ports;
+
+	np->data = &of_gpio_chip;
+
+	return 0;
+}
+
 void __init cpm2_reset(void)
 {
+	int ret;
+
 #ifdef CONFIG_PPC_85xx
 	cpm2_immr = ioremap(CPM_MAP_ADDR, CPM_MAP_SIZE);
 #else
@@ -80,6 +117,10 @@ void __init cpm2_reset(void)
 	/* Tell everyone where the comm processor resides.
 	 */
 	cpmp = &cpm2_immr->im_cpm;
+
+	ret = cpm2_init_par_io();
+	if (ret)
+		pr_warning("CPM2 PIO not initialized!\n");
 }
 
 static DEFINE_SPINLOCK(cmd_lock);
@@ -468,3 +509,83 @@ void cpm2_set_pin(int port, int pin, int flags)
 	else
 		clrbits32(&iop[port].odr, pin);
 }
+
+int gpio_request(unsigned int gpio, const char *label)
+{
+	if (gpio / 32 >= cpm2_num_ports)
+		return -EINVAL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpio_request);
+
+int gpio_direction_input(unsigned int gpio)
+{
+	unsigned long flags;
+	int port = gpio / 32;
+	int pin = gpio % 32;
+
+	spin_lock_irqsave(&cpm2_port_lock, flags);
+
+	cpm2_set_pin(port, pin, CPM_PIN_INPUT | CPM_PIN_GPIO);
+
+	spin_unlock_irqrestore(&cpm2_port_lock, flags);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpio_direction_input);
+
+int gpio_direction_output(unsigned int gpio, int value)
+{
+	struct cpm2_ioports __iomem *iop =
+		(struct cpm2_ioports __iomem *)&cpm2_immr->im_ioport;
+	int port = gpio / 32;
+	int pin = gpio % 32;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cpm2_port_lock, flags);
+
+	cpm2_set_pin(port, pin, CPM_PIN_OUTPUT | CPM_PIN_GPIO);
+
+	pin = 1 << (31 - pin);
+	if (value)
+		setbits32(&iop[port].dat, pin);
+	else
+		clrbits32(&iop[port].dat, pin);
+
+	spin_unlock_irqrestore(&cpm2_port_lock, flags);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpio_direction_output);
+
+int gpio_get_value(unsigned int gpio)
+{
+	struct cpm2_ioports __iomem *iop =
+		(struct cpm2_ioports __iomem *)&cpm2_immr->im_ioport;
+	int port = gpio / 32;
+	int pin = gpio % 32;
+
+	pin = 1 << (31 - pin);
+
+	return !!(in_be32(&iop[port].dat) & pin);
+}
+EXPORT_SYMBOL_GPL(gpio_get_value);
+
+int gpio_set_value(unsigned int gpio, int value)
+{
+	struct cpm2_ioports __iomem *iop =
+		(struct cpm2_ioports __iomem *)&cpm2_immr->im_ioport;
+	int port = gpio / 32;
+	int pin = gpio % 32;
+	unsigned long flags;
+
+	pin = 1 << (31 - pin);
+
+	spin_lock_irqsave(&cpm2_port_lock, flags);
+	if (value)
+		setbits32(&iop[port].dat, pin);
+	else
+		clrbits32(&iop[port].dat, pin);
+	spin_unlock_irqrestore(&cpm2_port_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpio_set_value);
-- 
1.5.2.2

Re: [PATCH 3/4] [POWERPC] CPM2: implement GPIO API

[Arnd Bergmann]

On Friday 21 December 2007, Anton Vorontsov wrote:
>  
> +static spinlock_t cpm2_port_lock = __SPIN_LOCK_UNLOCKED(cpm2_port_lock);

This needs to be

static DEFINE_SPINLOCK(cpm2_port_lock);

I think at least lockdep doesn't work the way you do it here.

> +int cpm2_init_par_io(void)
> +{
> +	struct device_node *np;
> +	const u32 *num_ports;
> +
> +	np = of_find_compatible_node(NULL, NULL, "fsl,cpm2-pario");
> +	if (!np)
> +		return -ENOENT;
> +
> +	num_ports = of_get_property(np, "num-ports", NULL);
> +	if (!num_ports) {
> +		of_node_put(np);
> +		return -ENOENT;
> +	}
> +	cpm2_num_ports = *num_ports;
> +
> +	np->data = &of_gpio_chip;
> +
> +	return 0;
> +}

This function should also do the call to of_iomap, so you don't
need to pull the address out of the cpm2_immr, which I believe
we're trying to get rid of.

	Arnd <><

Re: [PATCH 3/4] [POWERPC] CPM2: implement GPIO API

[Anton Vorontsov]

On Fri, Dec 21, 2007 at 10:16:32PM +0100, Arnd Bergmann wrote:
> On Friday 21 December 2007, Anton Vorontsov wrote:
> >  
> > +static spinlock_t cpm2_port_lock = __SPIN_LOCK_UNLOCKED(cpm2_port_lock);
> 
> This needs to be
> 
> static DEFINE_SPINLOCK(cpm2_port_lock);

These are equivalents.

#define DEFINE_SPINLOCK(x)      spinlock_t x = __SPIN_LOCK_UNLOCKED(x)

> I think at least lockdep doesn't work the way you do it here.

Is it anyhow special regarding what exact macro is used?..

spinlocks.txt says:

SPIN_LOCK_UNLOCKED and RW_LOCK_UNLOCKED defeat lockdep state tracking and
are hence deprecated.

Please use DEFINE_SPINLOCK()/DEFINE_RWLOCK() or
__SPIN_LOCK_UNLOCKED()/__RW_LOCK_UNLOCKED() as appropriate for static
initialization.
-

..should be equivalent, though I prefer open-coded version, until
it fits 80 column width. ;-)

> > +int cpm2_init_par_io(void)
> > +{
> > +	struct device_node *np;
> > +	const u32 *num_ports;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "fsl,cpm2-pario");
> > +	if (!np)
> > +		return -ENOENT;
> > +
> > +	num_ports = of_get_property(np, "num-ports", NULL);
> > +	if (!num_ports) {
> > +		of_node_put(np);
> > +		return -ENOENT;
> > +	}
> > +	cpm2_num_ports = *num_ports;
> > +
> > +	np->data = &of_gpio_chip;
> > +
> > +	return 0;
> > +}
> 
> This function should also do the call to of_iomap, so you don't
> need to pull the address out of the cpm2_immr, which I believe
> we're trying to get rid of.

Yup, thanks!

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

[PATCH 4/4] [POWERPC] CPM1: implement GPIO API

[Anton Vorontsov]

From: Jochen Friedrich <jochen@scram.de>

Signed-off-by: Jochen Friedrich <jochen@scram.de>
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

Jochen, I kept your Signed-off-by, though this isn't your original
patch. Hope you're okay with it. I also hope you'll test it. ;-)

 arch/powerpc/platforms/8xx/Kconfig |    1 +
 arch/powerpc/sysdev/commproc.c     |  178 +++++++++++++++++++++++++++++++++++-
 2 files changed, 178 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/platforms/8xx/Kconfig b/arch/powerpc/platforms/8xx/Kconfig
index 91fbe42..6962914 100644
--- a/arch/powerpc/platforms/8xx/Kconfig
+++ b/arch/powerpc/platforms/8xx/Kconfig
@@ -4,6 +4,7 @@ config FADS
 config CPM1
 	bool
 	select CPM
+	select GENERIC_GPIO
 
 choice
 	prompt "8xx Machine Type"
diff --git a/arch/powerpc/sysdev/commproc.c b/arch/powerpc/sysdev/commproc.c
index 621bc6c..8aea29c 100644
--- a/arch/powerpc/sysdev/commproc.c
+++ b/arch/powerpc/sysdev/commproc.c
@@ -27,6 +27,7 @@
 #include <linux/param.h>
 #include <linux/string.h>
 #include <linux/mm.h>
+#include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/module.h>
@@ -36,6 +37,7 @@
 #include <asm/8xx_immap.h>
 #include <asm/commproc.h>
 #include <asm/io.h>
+#include <asm/gpio.h>
 #include <asm/tlbflush.h>
 #include <asm/rheap.h>
 #include <asm/prom.h>
@@ -56,6 +58,39 @@ static cpic8xx_t __iomem *cpic_reg;
 
 static struct irq_host *cpm_pic_host;
 
+static spinlock_t cpm1_port_lock = __SPIN_LOCK_UNLOCKED(cpm1_port_lock);
+static int cpm1_num_ports;
+
+static int par_io_xlate(struct device_node *np, int index)
+{
+	return __of_parse_gpio_bank_pin(np, index, 32, cpm1_num_ports);
+}
+
+static struct of_gpio_chip of_gpio_chip = {
+	.xlate = par_io_xlate,
+};
+
+int cpm_init_par_io(void)
+{
+	struct device_node *np;
+	const u32 *num_ports;
+
+	np = of_find_node_by_name(NULL, "fsl,cpm1-pario");
+	if (!np)
+		return -ENOENT;
+
+	num_ports = of_get_property(np, "num-ports", NULL);
+	if (!num_ports) {
+		of_node_put(np);
+		return -ENOENT;
+	}
+	cpm1_num_ports = *num_ports;
+
+	np->data = &of_gpio_chip;
+
+	return 0;
+}
+
 static void cpm_mask_irq(unsigned int irq)
 {
 	unsigned int cpm_vec = (unsigned int)irq_map[irq].hwirq;
@@ -199,6 +234,7 @@ end:
 void __init cpm_reset(void)
 {
 	sysconf8xx_t __iomem *siu_conf;
+	int ret;
 
 	mpc8xx_immr = ioremap(get_immrbase(), 0x4000);
 	if (!mpc8xx_immr) {
@@ -238,6 +274,10 @@ void __init cpm_reset(void)
 	/* Reclaim the DP memory for our use. */
 	m8xx_cpm_dpinit();
 #endif
+
+	ret = cpm_init_par_io();
+	if (ret)
+		pr_warning("CPM PIO not initialized!\n");
 }
 
 static DEFINE_SPINLOCK(cmd_lock);
@@ -441,7 +481,7 @@ struct cpm_ioport16 {
 };
 
 struct cpm_ioport32 {
-	__be32 dir, par, sor;
+	__be32 dir, par, sor, dat;
 };
 
 static void cpm1_set_pin32(int port, int pin, int flags)
@@ -486,6 +526,39 @@ static void cpm1_set_pin32(int port, int pin, int flags)
 	}
 }
 
+static void cpm1_set_value32(int port, int pin, int value)
+{
+	struct cpm_ioport32 __iomem *iop;
+	pin = 1 << (31 - pin);
+
+	if (port == CPM_PORTB)
+		iop = (struct cpm_ioport32 __iomem *)
+		      &mpc8xx_immr->im_cpm.cp_pbdir;
+	else
+		iop = (struct cpm_ioport32 __iomem *)
+		      &mpc8xx_immr->im_cpm.cp_pedir;
+
+	if (value)
+		setbits32(&iop->dat, pin);
+	else
+		clrbits32(&iop->dat, pin);
+}
+
+static int cpm1_get_value32(int port, int pin)
+{
+	struct cpm_ioport32 __iomem *iop;
+	pin = 1 << (31 - pin);
+
+	if (port == CPM_PORTB)
+		iop = (struct cpm_ioport32 __iomem *)
+		      &mpc8xx_immr->im_cpm.cp_pbdir;
+	else
+		iop = (struct cpm_ioport32 __iomem *)
+		      &mpc8xx_immr->im_cpm.cp_pedir;
+
+	return !!(in_be32(&iop->dat) & pin);
+}
+
 static void cpm1_set_pin16(int port, int pin, int flags)
 {
 	struct cpm_ioport16 __iomem *iop =
@@ -520,6 +593,35 @@ static void cpm1_set_pin16(int port, int pin, int flags)
 	}
 }
 
+static void cpm1_set_value16(int port, int pin, int value)
+{
+	struct cpm_ioport16 __iomem *iop =
+		(struct cpm_ioport16 __iomem *)&mpc8xx_immr->im_ioport;
+
+	pin = 1 << (15 - pin);
+
+	if (port != 0)
+		iop += port - 1;
+
+	if (value)
+		setbits16(&iop->dat, pin);
+	else
+		clrbits16(&iop->dat, pin);
+}
+
+static int cpm1_get_value16(int port, int pin)
+{
+	struct cpm_ioport16 __iomem *iop =
+		(struct cpm_ioport16 __iomem *)&mpc8xx_immr->im_ioport;
+
+	pin = 1 << (15 - pin);
+
+	if (port != 0)
+		iop += port - 1;
+
+	return !!(in_be16(&iop->dat) & pin);
+}
+
 void cpm1_set_pin(enum cpm_port port, int pin, int flags)
 {
 	if (port == CPM_PORTB || port == CPM_PORTE)
@@ -648,3 +750,77 @@ int cpm1_clk_setup(enum cpm_clk_target target, int clock, int mode)
 
 	return 0;
 }
+
+int gpio_request(unsigned int gpio, const char *label)
+{
+	if (gpio / 32 >= cpm1_num_ports)
+		return -EINVAL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpio_request);
+
+int gpio_direction_input(unsigned int gpio)
+{
+	unsigned long flags;
+	int port = gpio / 32;
+	int pin = gpio % 32;
+
+	spin_lock_irqsave(&cpm1_port_lock, flags);
+
+	cpm1_set_pin(port, pin, CPM_PIN_INPUT | CPM_PIN_GPIO);
+
+	spin_unlock_irqrestore(&cpm1_port_lock, flags);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpio_direction_input);
+
+int gpio_direction_output(unsigned int gpio, int value)
+{
+	int port = gpio / 32;
+	int pin = gpio % 32;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cpm1_port_lock, flags);
+
+	cpm1_set_pin(port, pin, CPM_PIN_OUTPUT | CPM_PIN_GPIO);
+
+	if (port == CPM_PORTB || port == CPM_PORTE)
+		cpm1_set_value32(port, pin, value);
+	else
+		cpm1_set_value16(port, pin, value);
+
+	spin_unlock_irqrestore(&cpm1_port_lock, flags);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpio_direction_output);
+
+int gpio_get_value(unsigned int gpio)
+{
+	int port = gpio / 32;
+	int pin = gpio % 32;
+
+	if (port == CPM_PORTB || port == CPM_PORTE)
+		return cpm1_get_value32(port, pin);
+	else
+		return cpm1_get_value16(port, pin);
+}
+EXPORT_SYMBOL_GPL(gpio_get_value);
+
+int gpio_set_value(unsigned int gpio, int value)
+{
+	int port = gpio / 32;
+	int pin = gpio % 32;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cpm1_port_lock, flags);
+
+	if (port == CPM_PORTB || port == CPM_PORTE)
+		cpm1_set_value32(port, pin, value);
+	else
+		cpm1_set_value16(port, pin, value);
+
+	spin_unlock_irqrestore(&cpm1_port_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpio_set_value);
-- 
1.5.2.2

Re: [PATCH 4/4] [POWERPC] CPM1: implement GPIO API

[Jochen Friedrich]

Hi Anton,

> Jochen, I kept your Signed-off-by, though this isn't your original
> patch. Hope you're okay with it. I also hope you'll test it. ;-)
>   

that's OK.

Thanks,
Jochen

Re: [PATCH 4/4] [POWERPC] CPM1: implement GPIO API

[Jochen Friedrich]

Hi Anton,

> I also hope you'll test it. ;-)

yes.

> +int cpm_init_par_io(void)
> +{
> +       struct device_node *np;
> +       const u32 *num_ports;
> +
> +       np = of_find_node_by_name(NULL, "fsl,cpm1-pario");
> +       if (!np)
> +               return -ENOENT;
> +

I guess this should be:

 np = of_find_compatible_node(NULL, NULL, "fsl,cpm1-pario");

With this modification it works OK for me :)

Thanks,
Jochen

Re: [PATCH 4/4] [POWERPC] CPM1: implement GPIO API

[Anton Vorontsov]

On Sat, Dec 22, 2007 at 05:08:14PM +0100, Jochen Friedrich wrote:
> Hi Anton,
> 
> > I also hope you'll test it. ;-)
> 
> yes.
> 
> > +int cpm_init_par_io(void)
> > +{
> > +       struct device_node *np;
> > +       const u32 *num_ports;
> > +
> > +       np = of_find_node_by_name(NULL, "fsl,cpm1-pario");
> > +       if (!np)
> > +               return -ENOENT;
> > +
> 
> I guess this should be:
> 
>  np = of_find_compatible_node(NULL, NULL, "fsl,cpm1-pario");
> 
> With this modification it works OK for me :)

Fixed, thanks!

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 0/4] PowerPC: implement GPIO API

[Grant Likely]

On 12/21/07, Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> Also, in the upcoming kernels, there will be GPIOLIB[1] addition to
> the generic GPIO API, to support off-chip GPIO expanders (like MFDs
> on I2C/LBC). But so far we support on-chip GPIOs only, with single
> controller built-in.
>
> Changes since RFC:
> - Implemented #gpio-cells handling;
> - Per-bank spinlocks removed;
> - Added a patch which implements GPIO API for CPM1;
> - Few minor fixes.

Also need to add documentation to booting-without-of.txt.

In general this looks like a good direction, but I do not like the
hard linking for QE and CPM gpios to the 'top level' gpio API.  I
think I'd prefer this stuff to stay out of mainline until the gpiolib
stuff gets merged (which should be soon IIRC).

Cheers,
g.


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH 0/4] PowerPC: implement GPIO API

[Anton Vorontsov]

On Fri, Dec 21, 2007 at 01:50:10PM -0700, Grant Likely wrote:
> On 12/21/07, Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > Also, in the upcoming kernels, there will be GPIOLIB[1] addition to
> > the generic GPIO API, to support off-chip GPIO expanders (like MFDs
> > on I2C/LBC). But so far we support on-chip GPIOs only, with single
> > controller built-in.
> >
> > Changes since RFC:
> > - Implemented #gpio-cells handling;
> > - Per-bank spinlocks removed;
> > - Added a patch which implements GPIO API for CPM1;
> > - Few minor fixes.
> 
> Also need to add documentation to booting-without-of.txt.
> 
> In general this looks like a good direction, but I do not like the
> hard linking for QE and CPM gpios to the 'top level' gpio API.  I
> think I'd prefer this stuff to stay out of mainline until the gpiolib

Well, generally I'm okay to wait for gpiolib. Though...

> stuff gets merged (which should be soon IIRC).
                                     ^^^^ I doubt about that. :-)

I'm looking after gpiolib development (and, well, I also partipiated
in the discussion of earlier versions with former name "gpiodev") for
almost a _year_.

And they're still arguing about fluffy details of implementation.. :-/

As I've probably said once already: if there are plans to build single
kernel with QE+CPM1+CPM2 inside tomorrow -- then of course, I'd better
wait.

But if these plans are distant enough, I see no reason why we can't
enjoy of current API.

Thanks!

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 0/4] PowerPC: implement GPIO API

[Grant Likely]

On 12/21/07, Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> As I've probably said once already: if there are plans to build single
> kernel with QE+CPM1+CPM2 inside tomorrow -- then of course, I'd better
> wait.
>
> But if these plans are distant enough, I see no reason why we can't
> enjoy of current API.

Oh, I'm not saying don't enjoy it.  :-)  I'm just saying keep it out
of mainline.  I've got a bunch of Virtex stuff that falls into that
category.  Those who are interested can pick the non mainlined patches
out of my git tree.

I suggest doing the same with the GPIO support.  Either that or do our
own simple ppc specific GPIO multiplexer until the common stuff has
its act together.  :-)  We can always migrate over later.

I don't want to see more barriers added to prevent all of 8xxx going
multiplatform.

Cheers,
g.


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH 0/4] PowerPC: implement GPIO API

[Anton Vorontsov]

On Fri, Dec 21, 2007 at 02:17:57PM -0700, Grant Likely wrote:
> On 12/21/07, Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > As I've probably said once already: if there are plans to build single
> > kernel with QE+CPM1+CPM2 inside tomorrow -- then of course, I'd better
> > wait.
> >
> > But if these plans are distant enough, I see no reason why we can't
> > enjoy of current API.
> 
> Oh, I'm not saying don't enjoy it.  :-)  I'm just saying keep it out
> of mainline.  I've got a bunch of Virtex stuff that falls into that
> category.  Those who are interested can pick the non mainlined patches
> out of my git tree.

Ok. I'm fine either way.

Here we go. For anyone interested in the GPIOs on PowerPC, you
can keep an eye on this git tree:

	git://git.infradead.org/users/cbou/powerpc-gpio.git

As time will permit, I'll start gpiolib work in the "gpiolib"
branch in that repo, which will include gpiolib -mm patches +
powerpc bits.

Note: I'm quite often rebasing my work, be aware. ;-)


Thanks!

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 0/4] PowerPC: implement GPIO API

[Segher Boessenkool]

> OF device tree GPIOs bindings are similar to IRQs:

But GPIOs are a very different thing.  Most importantly, the "number"
of a GPIO is completely local to the GPIO controller.

> pario0: gpio-controller@0 {
> 	#gpio-cells = <2>;

Your Linux code doesn't actually use this.  Why is it needed, anyway?
You should be able to encode a GPIO identifier in a single 32-bit word,
for any possible GPIO controller.

> 	num-ports = <7>;

What is this?  What is a "port"?  This doesn't belong in a generic GPIO
binding.

> device@0 {
> 	gpios = <bank pin bank pin bank pin>;
> 	gpio-parent = <&pario0>;

Not every GPIO controller has banks.  Not every device uses GPIOs
on a single GPIO controller.  It is inconvenient to force all bindings
to use the same name ("gpios") for its property that shows the GPIOs
(and for it to have only one such property).

So I recommend:

-- Advise (in the generic GPIO binding) people to use
	< phandle-of-gpio-controller gpio-id-on-that-controller >
to refer to a GPIO from some device node;

-- And either:
	-- Define (in the generic GPIO binding) that a "gpio-id" is a single
	   32-bit cell;
    or
	-- Define (in the generic GPIO binding) that a "gpio-id" is a number
	   of 32-bit cells, and that that number of cells is encoded as a 
32-bit
	   integer in the "#gpio-cells" property in the device node of the
	   respective GPIO controller.

(I like the first option better, unless someone can think of some 
reasonable
situation where some specific GPIO controller binding needs more than 
32 bits
to encode GPIO #).


Segher

Re: [PATCH 0/4] PowerPC: implement GPIO API

[David Gibson]

On Sun, Dec 23, 2007 at 03:47:50AM +0100, Segher Boessenkool wrote:
> > OF device tree GPIOs bindings are similar to IRQs:
> 
> But GPIOs are a very different thing.  Most importantly, the "number"
> of a GPIO is completely local to the GPIO controller.

Yes... just as interrupt specifiers are local to their interrupt
domain.

> > pario0: gpio-controller@0 {
> > 	#gpio-cells = <2>;
> 
> Your Linux code doesn't actually use this.  Why is it needed, anyway?
> You should be able to encode a GPIO identifier in a single 32-bit word,
> for any possible GPIO controller.
> 
> > 	num-ports = <7>;
> 
> What is this?  What is a "port"?  This doesn't belong in a generic GPIO
> binding.
> 
> > device@0 {
> > 	gpios = <bank pin bank pin bank pin>;
> > 	gpio-parent = <&pario0>;
> 
> Not every GPIO controller has banks.

That's just bad terminology in the example.  "bank pin" means an
arbitrary format gpio specifier.

> Not every device uses GPIOs
> on a single GPIO controller.  It is inconvenient to force all bindings
> to use the same name ("gpios") for its property that shows the GPIOs
> (and for it to have only one such property).
> 
> So I recommend:
> 
> -- Advise (in the generic GPIO binding) people to use
> 	< phandle-of-gpio-controller gpio-id-on-that-controller >
> to refer to a GPIO from some device node;

Ah, yes, that's a good point.  Given the ugly workarounds we need to
deal with devices which have interrupts from multiple domains, we
don't want to copy that limitation to the GPIO scheme.

> -- And either:
> 	-- Define (in the generic GPIO binding) that a "gpio-id" is a single
> 	   32-bit cell;
>     or
> 	-- Define (in the generic GPIO binding) that a "gpio-id" is a number
> 	   of 32-bit cells, and that that number of cells is encoded as a 
> 32-bit
> 	   integer in the "#gpio-cells" property in the device node of the
> 	   respective GPIO controller.

This option was the idea; the "bank pin" information has a format
local to the gpio controller.  I agree the terminology needs to change
to "gpio specifier" by analogy with the interrupt tree, though.

> (I like the first option better, unless someone can think of some 
> reasonable
> situation where some specific GPIO controller binding needs more than 
> 32 bits
> to encode GPIO #).

I can't think of a situation where it would be strictly speaking
necessary, but I can think of several where it would be more
convenient.  GPIO controllers that do have a bank/pin arrangement is
one.  GPIO controllers than have a pin number, plus some sort of
direction or level information is another.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 0/4] PowerPC: implement GPIO API

[Segher Boessenkool]

>>> OF device tree GPIOs bindings are similar to IRQs:
>>
>> But GPIOs are a very different thing.  Most importantly, the "number"
>> of a GPIO is completely local to the GPIO controller.
>
> Yes... just as interrupt specifiers are local to their interrupt
> domain.

Sure, but an interrupt domain can be (and usually is) more than
one device node.  A GPIO controller is just one node.

>>> device@0 {
>>> 	gpios = <bank pin bank pin bank pin>;
>>> 	gpio-parent = <&pario0>;
>>
>> Not every GPIO controller has banks.
>
> That's just bad terminology in the example.  "bank pin" means an
> arbitrary format gpio specifier.

Okay.  Don't split it into two things then, just say "gpio-spec"
or such.

>> Not every device uses GPIOs
>> on a single GPIO controller.  It is inconvenient to force all bindings
>> to use the same name ("gpios") for its property that shows the GPIOs
>> (and for it to have only one such property).
>>
>> So I recommend:
>>
>> -- Advise (in the generic GPIO binding) people to use
>> 	< phandle-of-gpio-controller gpio-id-on-that-controller >
>> to refer to a GPIO from some device node;
>
> Ah, yes, that's a good point.  Given the ugly workarounds we need to
> deal with devices which have interrupts from multiple domains, we
> don't want to copy that limitation to the GPIO scheme.

Yeah, and we even know for a fact that devices exist that do GPIOs
on multiple GPIO controllers.  In the interrupt case, no one thought
anyone would be crazy enough to route their IRQs like that :-)

>> 	-- Define (in the generic GPIO binding) that a "gpio-id" is a number
>> 	   of 32-bit cells, and that that number of cells is encoded as a
>> 32-bit
>> 	   integer in the "#gpio-cells" property in the device node of the
>> 	   respective GPIO controller.
>
> This option was the idea; the "bank pin" information has a format
> local to the gpio controller.  I agree the terminology needs to change
> to "gpio specifier" by analogy with the interrupt tree, though.

Right, with that cleared up, and the binding doc expanded a bit,
you won't hear complaints from me :-)

>> (I like the first option better, unless someone can think of some
>> reasonable
>> situation where some specific GPIO controller binding needs more than
>> 32 bits
>> to encode GPIO #).
>
> I can't think of a situation where it would be strictly speaking
> necessary, but I can think of several where it would be more
> convenient.  GPIO controllers that do have a bank/pin arrangement is
> one.  GPIO controllers than have a pin number, plus some sort of
> direction or level information is another.

Ah yes, a second word for pin "type" information makes a lot of sense.
#gpio-cells it is, then.  Let's please make sure that we put that "type"
thing in the documentation (as an example), and that the first 
controller
bindings we put in use it.


Segher

Re: [PATCH 0/4] PowerPC: implement GPIO API

[Anton Vorontsov]

On Sun, Dec 23, 2007 at 10:53:05AM +0100, Segher Boessenkool wrote:
[...]
> >>> device@0 {
> >>> 	gpios = <bank pin bank pin bank pin>;
> >>> 	gpio-parent = <&pario0>;
> >>
> >> Not every GPIO controller has banks.
> >
> > That's just bad terminology in the example.  "bank pin" means an
> > arbitrary format gpio specifier.
> 
> Okay.  Don't split it into two things then, just say "gpio-spec"
> or such.

Will do. "bank pin" was just an example of QE/CPMs gpio specifiers, they
could be arbitrary in general.

> >> Not every device uses GPIOs
> >> on a single GPIO controller.  It is inconvenient to force all bindings
> >> to use the same name ("gpios") for its property that shows the GPIOs
> >> (and for it to have only one such property).
> >>
> >> So I recommend:
> >>
> >> -- Advise (in the generic GPIO binding) people to use
> >> 	< phandle-of-gpio-controller gpio-id-on-that-controller >
> >> to refer to a GPIO from some device node;
> >
> > Ah, yes, that's a good point.  Given the ugly workarounds we need to
> > deal with devices which have interrupts from multiple domains, we
> > don't want to copy that limitation to the GPIO scheme.
> 
> Yeah, and we even know for a fact that devices exist that do GPIOs
> on multiple GPIO controllers.  In the interrupt case, no one thought
> anyone would be crazy enough to route their IRQs like that :-)

Oh, <&gpio-controller-phandle gpio-spec> is indeed better
scheme. Well, parsing it would be a bit more complicated, as gpio-spec
is variable length: next placement of phandle depends on previous
gpio-specs... But this is doable of course.

> >> 	-- Define (in the generic GPIO binding) that a "gpio-id" is a number
> >> 	   of 32-bit cells, and that that number of cells is encoded as a
> >> 32-bit
> >> 	   integer in the "#gpio-cells" property in the device node of the
> >> 	   respective GPIO controller.
> >
> > This option was the idea; the "bank pin" information has a format
> > local to the gpio controller.  I agree the terminology needs to change
> > to "gpio specifier" by analogy with the interrupt tree, though.
> 
> Right, with that cleared up, and the binding doc expanded a bit,
> you won't hear complaints from me :-)

Ok, I should write documentation indeed. ;-)

> >> (I like the first option better, unless someone can think of some
> >> reasonable
> >> situation where some specific GPIO controller binding needs more than
> >> 32 bits
> >> to encode GPIO #).
> >
> > I can't think of a situation where it would be strictly speaking
> > necessary, but I can think of several where it would be more
> > convenient.  GPIO controllers that do have a bank/pin arrangement is
> > one.  GPIO controllers than have a pin number, plus some sort of
> > direction or level information is another.
> 
> Ah yes, a second word for pin "type" information makes a lot of sense.
> #gpio-cells it is, then.  Let's please make sure that we put that "type"
> thing in the documentation (as an example), and that the first 
> controller
> bindings we put in use it.

There is no limitation to define gpio direction in the gpio-spec, but
the thing is: we're passing gpios to the drivers which are already
know in what direction gpio should be set up, and we have an API to
set up GPIOs.

Example: fsl_nand, we're passing gpio, and driver is doing
gpio_direction_output() call on it. So we don't have to pass
gpio direction information in the gpio specifier.

As for level, yes this is important information, and encoding it
into gpio-spec seems reasonable (in fsl_nand example, ready-not-busy
(rnb) gpio. GPIO could be wired to be !rnb or just rnb).


Thanks!

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2