[PATCH v2 0/4] OF GPIO integration for I2C/SPI GPIO chips

[PATCH v2 0/4] OF GPIO integration for I2C/SPI GPIO chips

[Anton Vorontsov]

Hi all,

Here comes v2. Changes:

- In 'gpiolib: Introduce chip addition/removal notifier', open-coded
  gpio_call_chain().

- In 'of/gpio: Add support for two-stage registration for the
  of_gpio_chips', improved commit message.


David, I think it would be best if these patches could go via powerpc
or OF trees, so your Ack would be highly appreciated.

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 1/4] gpiolib: Introduce chip addition/removal notifier

[Anton Vorontsov]

Some platforms (e.g. OpenFirmware) want to know when a particular chip
added or removed, so that the platforms could add their specifics for
non-platform devices, like I2C or SPI GPIO chips.

This patch implements the notifier for chip addition and removal events.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/gpio/gpiolib.c     |   14 ++++++++++++++
 include/asm-generic/gpio.h |    8 ++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 350842a..375c03a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -9,6 +9,7 @@
 #include <linux/seq_file.h>
 #include <linux/gpio.h>
 #include <linux/idr.h>
+#include <linux/notifier.h>
 
 
 /* Optional implementation infrastructure for GPIO interfaces.
@@ -1029,6 +1030,9 @@ static inline void gpiochip_unexport(struct gpio_chip *chip)
 
 #endif /* CONFIG_GPIO_SYSFS */
 
+BLOCKING_NOTIFIER_HEAD(gpio_notifier);
+EXPORT_SYMBOL_GPL(gpio_notifier);
+
 /**
  * gpiochip_add() - register a gpio_chip
  * @chip: the chip to register, with chip->base initialized
@@ -1103,6 +1107,9 @@ fail:
 		pr_err("gpiochip_add: gpios %d..%d (%s) not registered\n",
 			chip->base, chip->base + chip->ngpio - 1,
 			chip->label ? : "generic");
+	else
+		blocking_notifier_call_chain(&gpio_notifier,
+					     GPIO_NOTIFY_CHIP_ADDED, chip);
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpiochip_add);
@@ -1119,6 +1126,13 @@ int gpiochip_remove(struct gpio_chip *chip)
 	int		status = 0;
 	unsigned	id;
 
+	/* Ask external subsystems to release the chip. */
+	status = blocking_notifier_call_chain(&gpio_notifier,
+					      GPIO_NOTIFY_CHIP_REMOVE, chip);
+	status = notifier_to_errno(status);
+	if (status)
+		return status;
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	for (id = chip->base; id < chip->base + chip->ngpio; id++) {
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 485eeb6..84faae4 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -4,6 +4,7 @@
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/errno.h>
+#include <linux/notifier.h>
 
 #ifdef CONFIG_GPIOLIB
 
@@ -208,4 +209,11 @@ static inline void gpio_unexport(unsigned gpio)
 }
 #endif	/* CONFIG_GPIO_SYSFS */
 
+enum gpio_notify_msg {
+	GPIO_NOTIFY_CHIP_ADDED = 0,
+	GPIO_NOTIFY_CHIP_REMOVE = 1,
+};
+
+extern struct blocking_notifier_head gpio_notifier;
+
 #endif /* _ASM_GENERIC_GPIO_H */
-- 
1.6.5.7

Re: [PATCH 1/4] gpiolib: Introduce chip addition/removal notifier

[Grant Likely]

On Fri, Feb 5, 2010 at 1:32 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> Some platforms (e.g. OpenFirmware) want to know when a particular chip
> added or removed, so that the platforms could add their specifics for
> non-platform devices, like I2C or SPI GPIO chips.
>
> This patch implements the notifier for chip addition and removal events.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> =A0drivers/gpio/gpiolib.c =A0 =A0 | =A0 14 ++++++++++++++
> =A0include/asm-generic/gpio.h | =A0 =A08 ++++++++
> =A02 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 350842a..375c03a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -9,6 +9,7 @@
> =A0#include <linux/seq_file.h>
> =A0#include <linux/gpio.h>
> =A0#include <linux/idr.h>
> +#include <linux/notifier.h>
>
>
> =A0/* Optional implementation infrastructure for GPIO interfaces.
> @@ -1029,6 +1030,9 @@ static inline void gpiochip_unexport(struct gpio_ch=
ip *chip)
>
> =A0#endif /* CONFIG_GPIO_SYSFS */
>
> +BLOCKING_NOTIFIER_HEAD(gpio_notifier);
> +EXPORT_SYMBOL_GPL(gpio_notifier);
> +
> =A0/**
> =A0* gpiochip_add() - register a gpio_chip
> =A0* @chip: the chip to register, with chip->base initialized
> @@ -1103,6 +1107,9 @@ fail:
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("gpiochip_add: gpios %d..%d (%s) no=
t registered\n",
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0chip->base, chip->base + c=
hip->ngpio - 1,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0chip->label ? : "generic")=
;
> + =A0 =A0 =A0 else
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 blocking_notifier_call_chain(&gpio_notifier=
,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0GPIO_NOTIFY_CHIP_ADDED, chip);

Rather than doing an else block which will need to be reworked if/when
any additional code is added to the bottom of this routine, please
rework the if() block to bail on failure instead of implicitly falling
through to the return statement.

Otherwise, this patch looks okay to me, so you can go ahead and add my:

Acked-by: Grant Likely <grant.likely@secretlab.ca>

*however* (and don't kill me for saying this because I know I
suggested the notifier approach in the first place).  Looking at the
whole patch series, the notifier call adds a lot of code for very
little gain.  If you dropped just the notifier bits (but left the rest
of the series the same), then the the of gpio bits would be
considerably simpler, and the only impact on the core gpiolib would be
the addition of an of_gpiochip_register_simple() and
of_gpiochip_unregister() hooks that will be conditionally compiled.

And to address one of my previous concerns, I've got no problem with
the automatic registration of GPIO devices for OF usage, as long as
of-aware drivers have the option of overriding the simple defaults
when needed.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 1/4] gpiolib: Introduce chip addition/removal notifier

[Andrew Morton]

On Tue, 9 Feb 2010 10:16:44 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:

> On Fri, Feb 5, 2010 at 1:32 PM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > Some platforms (e.g. OpenFirmware) want to know when a particular chip
> > added or removed, so that the platforms could add their specifics for
> > non-platform devices, like I2C or SPI GPIO chips.
> >
> > This patch implements the notifier for chip addition and removal events.
> >
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> > drivers/gpio/gpiolib.c   |  14 ++++++++++++++
> > include/asm-generic/gpio.h |  8 ++++++++
> > 2 files changed, 22 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 350842a..375c03a 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -9,6 +9,7 @@
> > #include <linux/seq_file.h>
> > #include <linux/gpio.h>
> > #include <linux/idr.h>
> > +#include <linux/notifier.h>
> >
> >
> > /* Optional implementation infrastructure for GPIO interfaces.
> > @@ -1029,6 +1030,9 @@ static inline void gpiochip_unexport(struct gpio_chip *chip)
> >
> > #endif /* CONFIG_GPIO_SYSFS */
> >
> > +BLOCKING_NOTIFIER_HEAD(gpio_notifier);
> > +EXPORT_SYMBOL_GPL(gpio_notifier);
> > +
> > /**
> > * gpiochip_add() - register a gpio_chip
> > * @chip: the chip to register, with chip->base initialized
> > @@ -1103,6 +1107,9 @@ fail:
> >        pr_err("gpiochip_add: gpios %d..%d (%s) not registered\n",
> >            chip->base, chip->base + chip->ngpio - 1,
> >            chip->label ? : "generic");
> > +    else
> > +        blocking_notifier_call_chain(&gpio_notifier,
> > +                      GPIO_NOTIFY_CHIP_ADDED, chip);
> 
> Rather than doing an else block which will need to be reworked if/when
> any additional code is added to the bottom of this routine, please
> rework the if() block to bail on failure instead of implicitly falling
> through to the return statement.

This still hasn't happened.

> Otherwise, this patch looks okay to me, so you can go ahead and add my:
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> 

I'll merge it anyway and will ask you guys to keep track of this issue,
thanks.

Re: [PATCH 1/4] gpiolib: Introduce chip addition/removal notifier

[Anton Vorontsov]

On Fri, Mar 05, 2010 at 11:59:18AM -0800, Andrew Morton wrote:
[...]
> > > /**
> > > * gpiochip_add() - register a gpio_chip
> > > * @chip: the chip to register, with chip->base initialized
> > > @@ -1103,6 +1107,9 @@ fail:
> > >        pr_err("gpiochip_add: gpios %d..%d (%s) not registered\n",
> > >            chip->base, chip->base + chip->ngpio - 1,
> > >            chip->label ? : "generic");
> > > +    else
> > > +        blocking_notifier_call_chain(&gpio_notifier,
> > > +                      GPIO_NOTIFY_CHIP_ADDED, chip);
> > 
> > Rather than doing an else block which will need to be reworked if/when
> > any additional code is added to the bottom of this routine, please
> > rework the if() block to bail on failure instead of implicitly falling
> > through to the return statement.
> 
> This still hasn't happened.

It's in your tree already :-)

gpiolib-cosmetic-improvements-for-error-handling-in-gpiochip_add.patch

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 2/4] of/gpio: Add support for two-stage registration for the of_gpio_chips

[Anton Vorontsov]

I2C/SPI drivers allocate gpio_chip structure already, so we don't need
'struct gpio_chip gc' in the OF GPIO subsystem, instead we need to store
just a pointer, and then attach the already allocated gpio_chip to the
of_gpio_chip structure.

With this patch there are two ways to register OF GPIO controllers:

1. Allocating the of_gpio_chip structure and passing the
   &of_gc->gc pointer to the gpiochip_add. (Can use container_of
   to convert the gpio_chip to the of_gpio_chip.)

2. Allocating and registering the gpio_chip structure separately
   from the of_gpio_chip. (Since two allocations are separate,
   container_of won't work.)

As time goes by we'll kill the first option.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c |    1 +
 drivers/of/gpio.c                              |   23 +++++++++++++++++++++--
 include/linux/of_gpio.h                        |    3 ++-
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c
index 82a9bcb..73c7e6b 100644
--- a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c
+++ b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c
@@ -93,6 +93,7 @@ static int mcu_gpiochip_add(struct mcu *mcu)
 	gc->base = -1;
 	gc->set = mcu_gpio_set;
 	gc->direction_output = mcu_gpio_dir_out;
+	of_gc->chip = gc;
 	of_gc->gpio_cells = 2;
 	of_gc->xlate = of_gpio_simple_xlate;
 
diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
index 6eea601..12c4af0 100644
--- a/drivers/of/gpio.c
+++ b/drivers/of/gpio.c
@@ -70,7 +70,7 @@ int of_get_gpio_flags(struct device_node *np, int index,
 	if (ret < 0)
 		goto err1;
 
-	ret += of_gc->gc.base;
+	ret += of_gc->chip->base;
 err1:
 	of_node_put(gc);
 err0:
@@ -140,7 +140,7 @@ int of_gpio_simple_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
 		return -EINVAL;
 	}
 
-	if (*gpio > of_gc->gc.ngpio)
+	if (*gpio > of_gc->chip->ngpio)
 		return -EINVAL;
 
 	if (flags)
@@ -178,6 +178,25 @@ int of_mm_gpiochip_add(struct device_node *np,
 	struct of_gpio_chip *of_gc = &mm_gc->of_gc;
 	struct gpio_chip *gc = &of_gc->gc;
 
+	/*
+	 * Currently there are two ways to register OF GPIO controllers:
+	 *
+	 * 1. Allocating the of_gpio_chip structure and passing the
+	 *    &of_gc->gc pointer to the gpiochip_add. (Can use container_of
+	 *    to convert the gpio_chip to the of_gpio_chip.)
+	 *
+	 * 2. Allocating and registering the gpio_chip structure separately
+	 *    from the of_gpio_chip. (Since two allocations are separate,
+	 *    container_of won't work.)
+	 *
+	 * As time goes by we'll kill the first option. For now just check
+	 * if it's the new-style registration or the old-style.
+	 */
+	if (!of_gc->chip)
+		of_gc->chip = gc;
+	else
+		gc = of_gc->chip;
+
 	gc->label = kstrdup(np->full_name, GFP_KERNEL);
 	if (!gc->label)
 		goto err0;
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index fc2472c..c74cb37 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -36,7 +36,8 @@ enum of_gpio_flags {
  * Generic OF GPIO chip
  */
 struct of_gpio_chip {
-	struct gpio_chip gc;
+	struct gpio_chip gc; /* legacy, don't use for a new code */
+	struct gpio_chip *chip;
 	int gpio_cells;
 	int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
 		     const void *gpio_spec, enum of_gpio_flags *flags);
-- 
1.6.5.7

[PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Anton Vorontsov]

This patch implements GPIOLIB notifier hooks, and thus makes device-enabled
GPIO chips (i.e. the ones that have gpio_chip->dev specified) automatically
attached to the OpenFirmware subsystem. Which means that now we can handle
I2C and SPI GPIO chips almost* transparently.

* "Almost" because some chips still require platform data, and for these
  chips OF-glue is still needed, though with this support the glue will
  be much smaller.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/of/gpio.c |  100 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 100 insertions(+), 0 deletions(-)

diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
index 12c4af0..9d8df77 100644
--- a/drivers/of/gpio.c
+++ b/drivers/of/gpio.c
@@ -13,6 +13,7 @@
 
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/notifier.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
@@ -236,3 +237,102 @@ err0:
 	return ret;
 }
 EXPORT_SYMBOL(of_mm_gpiochip_add);
+
+/**
+ * of_gpiochip_register_simple - Register a chip with the OF GPIO subsystem
+ * @chip	pointer to a GPIO chip
+ * @np:		device node to register the GPIO chip with
+ *
+ * This function registers a GPIO chip with the OF infrastructure. It is
+ * assumed that the chip was previsously allocated and added to a generic
+ * GPIOLIB framework (using gpiochip_add() function).
+ *
+ * The `simple' name means that the chip is using simple two-cells scheme for
+ * the gpio-specifier.
+ */
+static int of_gpiochip_register_simple(struct gpio_chip *chip,
+				       struct device_node *np)
+{
+	struct of_gpio_chip *of_gc;
+
+	if (np->data) {
+		WARN_ON(1);
+		return -EBUSY;
+	}
+
+	of_gc = kzalloc(sizeof(*of_gc), GFP_KERNEL);
+	if (!of_gc)
+		return -ENOMEM;
+
+	of_gc->gpio_cells = 2;
+	of_gc->xlate = of_gpio_simple_xlate;
+	of_gc->chip = chip;
+	np->data = of_gc;
+	of_node_get(np);
+
+	return 0;
+}
+EXPORT_SYMBOL(of_gpiochip_register_simple);
+
+/**
+ * of_gpiochip_unregister - Unregister a GPIO chip
+ * @chip	pointer to a GPIO chip
+ * @np:		device node for which the GPIO chip was previously registered
+ *
+ * This function unregisters a GPIO chip that was previsously registered
+ * with of_gpiochip_register*().
+ */
+static int of_gpiochip_unregister(struct gpio_chip *chip,
+				  struct device_node *np)
+{
+	struct of_gpio_chip *of_gc = np->data;
+
+	if (!of_gc || of_gc->chip != chip) {
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	np->data = NULL;
+	kfree(of_gc);
+	of_node_put(np);
+
+	return 0;
+}
+
+static int of_gpio_notify(struct notifier_block *nb, unsigned long msg,
+			  void *chip)
+{
+	struct gpio_chip *gc = chip;
+	struct device_node *np;
+	int ret = 0;
+
+	if (!gc->dev)
+		return NOTIFY_DONE;
+
+	np = dev_archdata_get_node(&gc->dev->archdata);
+	if (!np)
+		return NOTIFY_DONE;
+
+	switch (msg) {
+	case GPIO_NOTIFY_CHIP_ADDED:
+		ret = of_gpiochip_register_simple(gc, np);
+		break;
+	case GPIO_NOTIFY_CHIP_REMOVE:
+		ret = of_gpiochip_unregister(gc, np);
+		break;
+	default:
+		break;
+	}
+
+	return ret ? notifier_from_errno(ret) : NOTIFY_OK;
+}
+
+static struct notifier_block of_gpio_nb = {
+	.notifier_call = of_gpio_notify,
+};
+
+static int __init of_gpio_notifier_init(void)
+{
+	return blocking_notifier_chain_register(&gpio_notifier, &of_gpio_nb);
+}
+arch_initcall(of_gpio_notifier_init);
-- 
1.6.5.7

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Andrew Morton]

On Fri, 5 Feb 2010 23:32:36 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> This patch implements GPIOLIB notifier hooks, and thus makes device-enabled
> GPIO chips (i.e. the ones that have gpio_chip->dev specified) automatically
> attached to the OpenFirmware subsystem. Which means that now we can handle
> I2C and SPI GPIO chips almost* transparently.
> 
> * "Almost" because some chips still require platform data, and for these
>   chips OF-glue is still needed, though with this support the glue will
>   be much smaller.
> 

We can clean this up a little with the WARN_ON trick:

diff -puN drivers/of/gpio.c~of-gpio-implement-gpiolib-notifier-hooks-fix drivers/of/gpio.c
--- a/drivers/of/gpio.c~of-gpio-implement-gpiolib-notifier-hooks-fix
+++ a/drivers/of/gpio.c
@@ -255,10 +255,8 @@ static int of_gpiochip_register_simple(s
 {
 	struct of_gpio_chip *of_gc;
 
-	if (np->data) {
-		WARN_ON(1);
+	if (WARN_ON(np->data))
 		return -EBUSY;
-	}
 
 	of_gc = kzalloc(sizeof(*of_gc), GFP_KERNEL);
 	if (!of_gc)
@@ -287,10 +285,8 @@ static int of_gpiochip_unregister(struct
 {
 	struct of_gpio_chip *of_gc = np->data;
 
-	if (!of_gc || of_gc->chip != chip) {
-		WARN_ON(1);
+	if (WARN_ON(!of_gc || of_gc->chip != chip))
 		return -EINVAL;
-	}
 
 	np->data = NULL;
 	kfree(of_gc);
_

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Grant Likely]

On Fri, Feb 5, 2010 at 1:32 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> This patch implements GPIOLIB notifier hooks, and thus makes device-enabl=
ed
> GPIO chips (i.e. the ones that have gpio_chip->dev specified) automatical=
ly
> attached to the OpenFirmware subsystem. Which means that now we can handl=
e
> I2C and SPI GPIO chips almost* transparently.
>
> * "Almost" because some chips still require platform data, and for these
> =A0chips OF-glue is still needed, though with this support the glue will
> =A0be much smaller.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> =A0drivers/of/gpio.c | =A0100 +++++++++++++++++++++++++++++++++++++++++++=
++++++++++
> =A01 files changed, 100 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
> index 12c4af0..9d8df77 100644
> --- a/drivers/of/gpio.c
> +++ b/drivers/of/gpio.c
> @@ -13,6 +13,7 @@
>
> =A0#include <linux/kernel.h>
> =A0#include <linux/errno.h>
> +#include <linux/notifier.h>
> =A0#include <linux/io.h>
> =A0#include <linux/of.h>
> =A0#include <linux/of_gpio.h>
> @@ -236,3 +237,102 @@ err0:
> =A0 =A0 =A0 =A0return ret;
> =A0}
> =A0EXPORT_SYMBOL(of_mm_gpiochip_add);
> +
> +/**
> + * of_gpiochip_register_simple - Register a chip with the OF GPIO subsys=
tem
> + * @chip =A0 =A0 =A0 pointer to a GPIO chip
> + * @np: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0device node to register the GPIO =
chip with
> + *
> + * This function registers a GPIO chip with the OF infrastructure. It is
> + * assumed that the chip was previsously allocated and added to a generi=
c
> + * GPIOLIB framework (using gpiochip_add() function).
> + *
> + * The `simple' name means that the chip is using simple two-cells schem=
e for
> + * the gpio-specifier.
> + */
> +static int of_gpiochip_register_simple(struct gpio_chip *chip,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0struct device_node *np)
> +{
> + =A0 =A0 =A0 struct of_gpio_chip *of_gc;
> +
> + =A0 =A0 =A0 if (np->data) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 WARN_ON(1);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EBUSY;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 of_gc =3D kzalloc(sizeof(*of_gc), GFP_KERNEL);
> + =A0 =A0 =A0 if (!of_gc)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> +
> + =A0 =A0 =A0 of_gc->gpio_cells =3D 2;
> + =A0 =A0 =A0 of_gc->xlate =3D of_gpio_simple_xlate;
> + =A0 =A0 =A0 of_gc->chip =3D chip;

One concern.

How does an OF-aware GPIO driver override these settings?  What is to
be done when a GPIO chip requires a different xlate hook?  Or a
different number of gpio_cells?

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Anton Vorontsov]

On Tue, Feb 09, 2010 at 10:08:00AM -0700, Grant Likely wrote:
> On Fri, Feb 5, 2010 at 1:32 PM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > This patch implements GPIOLIB notifier hooks, and thus makes device-enabled
> > GPIO chips (i.e. the ones that have gpio_chip->dev specified) automatically
> > attached to the OpenFirmware subsystem. Which means that now we can handle
> > I2C and SPI GPIO chips almost* transparently.
[...]
> One concern.
> 
> How does an OF-aware GPIO driver override these settings?  What is to
> be done when a GPIO chip requires a different xlate hook?  Or a
> different number of gpio_cells?

A lot of options...

1. They can hook up onto a notifier chain, ensure that their callback
   will be called after OF GPIO subsystem (using notifiers' priority
   mechanism), and fixup needed stuff.

2. They can write their own full fledged OF bindings, i.e. they will
   need to allocate of_gc struct and save it into np->data.
   of_gpiochip_register_simple() won't touch np->data if it's already
   used.

   If/when needed, we can write some helper function, i.e.
   of_gpiochip_register(gpiochip, node, xlate_callback).

3. Or of/gpio.c code will handle this by itself, iff the xlate and
   gpio-cells scheme seems generic enough.

4. May be more...

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Grant Likely]

On Fri, Feb 5, 2010 at 1:32 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> This patch implements GPIOLIB notifier hooks, and thus makes device-enabl=
ed
> GPIO chips (i.e. the ones that have gpio_chip->dev specified) automatical=
ly
> attached to the OpenFirmware subsystem. Which means that now we can handl=
e
> I2C and SPI GPIO chips almost* transparently.
>
> * "Almost" because some chips still require platform data, and for these
> =A0chips OF-glue is still needed, though with this support the glue will
> =A0be much smaller.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> +static struct notifier_block of_gpio_nb =3D {
> + =A0 =A0 =A0 .notifier_call =3D of_gpio_notify,
> +};
> +
> +static int __init of_gpio_notifier_init(void)
> +{
> + =A0 =A0 =A0 return blocking_notifier_chain_register(&gpio_notifier, &of=
_gpio_nb);
> +}
> +arch_initcall(of_gpio_notifier_init);

Another concern;  if any gpio chips get registered before this
arch_initcall (not sure if it is possible or not), then those chips
won't get registered with the of gpio infrastructure.

g.

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Anton Vorontsov]

On Tue, Feb 09, 2010 at 10:13:11AM -0700, Grant Likely wrote:
[...]
> > +static int __init of_gpio_notifier_init(void)
> > +{
> > +       return blocking_notifier_chain_register(&gpio_notifier, &of_gpio_nb);
> > +}
> > +arch_initcall(of_gpio_notifier_init);
> 
> Another concern;  if any gpio chips get registered before this
> arch_initcall (not sure if it is possible or not), then those chips
> won't get registered with the of gpio infrastructure.

Technically, it is possible, but registering usual GPIO controllers
in arch_initcall feels not quite right approach in the first place
(and, btw, it won't work most of the time, because even early drivers
do not register itself earlier than subsys_initcall).

And arch gpio controllers (like QE GPIO) are usually device-less,
and they use of_mm_gpiochip_add(), so we fully control them.

Plus I don't see any reason why we couldn't move
of_gpio_notifier_init() into, say, postcore_initcall, if we ever
need it.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Grant Likely]

On Fri, Mar 5, 2010 at 1:35 PM, Andrew Morton <akpm@linux-foundation.org> w=
rote:
> On Fri, 5 Mar 2010 13:28:32 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
>> On Fri, Mar 5, 2010 at 1:00 PM, Andrew Morton <akpm@linux-foundation.org=
> wrote:
>> > On Tue, 9 Feb 2010 22:16:20 +0300
>> > Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
>> >
>> >> On Tue, Feb 09, 2010 at 10:13:11AM -0700, Grant Likely wrote:
>> >> [...]
>> >> > > +static int __init of_gpio_notifier_init(void)
>> >> > > +{
>> >> > > + __ __ __ return blocking_notifier_chain_register(&gpio_notifier=
, &of_gpio_nb);
>> >> > > +}
>> >> > > +arch_initcall(of_gpio_notifier_init);
>> >> >
>> >> > Another concern; __if any gpio chips get registered before this
>> >> > arch_initcall (not sure if it is possible or not), then those chips
>> >> > won't get registered with the of gpio infrastructure.
>> >>
>> >> Technically, it is possible, but registering usual GPIO controllers
>> >> in arch_initcall feels not quite right approach in the first place
>> >> (and, btw, it won't work most of the time, because even early drivers
>> >> do not register itself earlier than subsys_initcall).
>> >>
>> >> And arch gpio controllers (like QE GPIO) are usually device-less,
>> >> and they use of_mm_gpiochip_add(), so we fully control them.
>> >>
>> >> Plus I don't see any reason why we couldn't move
>> >> of_gpio_notifier_init() into, say, postcore_initcall, if we ever
>> >> need it.
>> >>
>> >
>> > I'll assume that you're OK with that response.
>>
>> No, not really,
>
> You left me dangling :(

Sorry, got caught up with other things.

>> I'm not really very comfortable with the whole
>> approach being taken. =A0And, while I acked the first patch in the
>> series, that patch isn't needed by anything except patches 2, 3 & 4.
>>
>> Also, the OF stuff is a moving target at the moment with all the
>> rework is being undertaken. =A0I'd rather let this series sit out for
>> another merge cycle so that the underlying OF stuff can settle down.
>
> OK, please take it up on-list?

Okay.  I'm making this reply on list.

Anton, as I've stated before, I'm not thrilled with the approach.
Combine that with the changes being made to drivers/of right now and
the addition device tree to ARM and other architectures, my preference
is to let this patch series lie fallow for one more merge cycle so
that things can settle out in the OF infrastructure code.

g.

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Anton Vorontsov]

On Fri, Mar 05, 2010 at 04:47:06PM -0700, Grant Likely wrote:
[...]
> >> I'm not really very comfortable with the whole
> >> approach being taken.  And, while I acked the first patch in the
> >> series, that patch isn't needed by anything except patches 2, 3 & 4.

But you didn't answer my replies, ie were sitting silent like for
a month? So you didn't give my any chance to make them comfortable
to you.

Is there any punishment ready for that? ;-) I see one: apply
these patches, and rework this stuff as you like when you have some
time? Or tell me your idea, and I'll do the rework for you, in
2.6.35.

But in the meantime, these patches can be nicely used to support
I2C/SPI GPIO controllers.

> >> Also, the OF stuff is a moving target at the moment with all the
> >> rework is being undertaken.  I'd rather let this series sit out for
> >> another merge cycle so that the underlying OF stuff can settle down.
> >
> > OK, please take it up on-list?
> 
> Okay.  I'm making this reply on list.
> 
> Anton, as I've stated before, I'm not thrilled with the approach.

Again, great timing for telling that, I must say. Yes, you said
it once with some minor arguments (doubts and questions), to which
I replied long ago. Then nothing.

> Combine that with the changes being made to drivers/of right now and
> the addition device tree to ARM and other architectures, my preference
> is to let this patch series lie fallow for one more merge cycle so
> that things can settle out in the OF infrastructure code.

How exactly OF rework affects these patches? And why some rework
should be used as an excuse for not adding a hardware support?

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Grant Likely]

On Fri, Mar 5, 2010 at 5:28 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Fri, Mar 05, 2010 at 04:47:06PM -0700, Grant Likely wrote:
> [...]
>> >> I'm not really very comfortable with the whole
>> >> approach being taken. =A0And, while I acked the first patch in the
>> >> series, that patch isn't needed by anything except patches 2, 3 & 4.
>
> But you didn't answer my replies, ie were sitting silent like for
> a month? So you didn't give my any chance to make them comfortable
> to you.

The last version of the patches were posted on Feb 8.  -rc8 was
released on Feb 12.  For changes to common code, that is a little late
for getting queued up for the merge window.  If it was a subsystem
that I maintain, say SPI, then I doubt I would have picked it up for
2.6.34.

But I am not the GPIO maintainer.  I've stated my case, I'm not fond
of the approach, and I'd rather have another merge cycle before
committing to the method of making OF gpio bindings more generic.  I
missed your request to merge this via the powerpc tree and I had
higher priority concerns, so I really didn't think much of it.  I
assumed that David would look at the arguments and make his own
decision.

For the record, my main concerns are:
- Now that I see the implementation, I think that it is too complex.
The bus notifiers really aren't needed and it can be done with much
lower impact on the core gpiolib code.
- Using notifiers adds an unnecessary race condition, however unlikely.

> Is there any punishment ready for that? ;-) I see one: apply
> these patches, and rework this stuff as you like when you have some
> time?

Changes to common code don't work that way.  Sometimes things just
don't get enough attention and they wait another cycle, get reworked,
or get dropped entirely.

> Or tell me your idea, and I'll do the rework for you, in
> 2.6.35.
>
> But in the meantime, these patches can be nicely used to support
> I2C/SPI GPIO controllers.

...and anyone who need it immediately is welcome to pull your changes
into their private tree.  Skipping a cycle is not the end of the
world.

>> Combine that with the changes being made to drivers/of right now and
>> the addition device tree to ARM and other architectures, my preference
>> is to let this patch series lie fallow for one more merge cycle so
>> that things can settle out in the OF infrastructure code.
>
> How exactly OF rework affects these patches?

For one, the device node pointer is moving out of archdata into
'struct device' proper and I've got patches adding OF hooks into the
core of the platform bus.  If those patches look good to GregKH, then
I'll be pursing the same pattern for the other bus types (i2c, spi,
etc), and it will be further argument for putting the OF hooks
directly into gpiolib instead of using a notifier.  I'll be posting
the patches as soon as the merge window closes.

> And why some rework
> should be used as an excuse for not adding a hardware support?

If this was a standalone device driver then I'd agree.  However, this
is an infrastructure change.  Infrastructure changes get more scrutiny
and are always harder to merge.  Especially just before the merge
window opens with very little linux-next exposure.

g.

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Anton Vorontsov]

On Fri, Mar 05, 2010 at 08:54:56PM -0700, Grant Likely wrote:
[...]
> The last version of the patches were posted on Feb 8.  -rc8 was
> released on Feb 12.  For changes to common code, that is a little late
> for getting queued up for the merge window.  If it was a subsystem
> that I maintain, say SPI, then I doubt I would have picked it up for
> 2.6.34.

And of course the part of the OF rework, which was first posted
for *review* on Feb 03, is a completely different story?

 48 files changed, 317 insertions(+), 575 deletions(-)

It's in Linus' tree now.

And the other part of the OF rework that was posted for review
on Feb 13 is another story too? It's in Linus' tree as well.

Your patches touch 3 architectures, and a lot of the code that
is used by all the OF drivers, still 03 and 13 Feb was OK for
them.

> But I am not the GPIO maintainer.

David is. And I heard only positive feedback on the patches
last time.

> For the record, my main concerns are:
> - Now that I see the implementation, I think that it is too complex.
> The bus notifiers really aren't needed and it can be done with much
> lower impact on the core gpiolib code.

That's a non-argument, what is "lower impact"? Do I touch any
hot paths? And if nothing has changed, David (again, the gpiolib
maintainer) is happy with the notifiers approach, why would you
care?

Anyhow, changing the notifier to a direct call is a matter of
a trivial patch that we can queue anytime in 2.6.35.

> - Using notifiers adds an unnecessary race condition, however unlikely.

Where? Is it a real one? I see a lot of race conditions, but none of
them because of the notifiers approach.

> > Is there any punishment ready for that? ;-) I see one: apply
> > these patches, and rework this stuff as you like when you have some
> > time?
> 
> Changes to common code don't work that way.  Sometimes things just
> don't get enough attention and they wait another cycle, get reworked,
> or get dropped entirely.

See above wrt OF rework patches.

> > Or tell me your idea, and I'll do the rework for you, in
> > 2.6.35.
> >
> > But in the meantime, these patches can be nicely used to support
> > I2C/SPI GPIO controllers.
> 
> ...and anyone who need it immediately is welcome to pull your changes
> into their private tree.  Skipping a cycle is not the end of the
> world.

Using notifiers is not the end of the world either.

> >> Combine that with the changes being made to drivers/of right now and
> >> the addition device tree to ARM and other architectures, my preference
> >> is to let this patch series lie fallow for one more merge cycle so
> >> that things can settle out in the OF infrastructure code.
> >
> > How exactly OF rework affects these patches?
> 
> For one, the device node pointer is moving out of archdata into
> 'struct device' proper and I've got patches adding OF hooks into the
> core of the platform bus.  If those patches look good to GregKH, then
> I'll be pursing the same pattern for the other bus types (i2c, spi,
> etc), and it will be further argument for putting the OF hooks
> directly into gpiolib instead of using a notifier.  I'll be posting
> the patches as soon as the merge window closes.

I don't get it. Why is it a problem to change your patches that
ought to be queued for 2.6.*35*?

> > And why some rework
> > should be used as an excuse for not adding a hardware support?
> 
> If this was a standalone device driver then I'd agree.  However, this
> is an infrastructure change.  Infrastructure changes get more scrutiny
> and are always harder to merge.  Especially just before the merge
> window opens with very little linux-next exposure.

See above wrt OF rework patches.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Grant Likely]

On Fri, Mar 5, 2010 at 10:05 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Fri, Mar 05, 2010 at 08:54:56PM -0700, Grant Likely wrote:
> [...]
>> The last version of the patches were posted on Feb 8. =A0-rc8 was
>> released on Feb 12. =A0For changes to common code, that is a little late
>> for getting queued up for the merge window. =A0If it was a subsystem
>> that I maintain, say SPI, then I doubt I would have picked it up for
>> 2.6.34.
>
> And of course the part of the OF rework, which was first posted
> for *review* on Feb 03, is a completely different story?
>
> =A048 files changed, 317 insertions(+), 575 deletions(-)

Completely uncontroversial changes with zero functional behaviour
change.  There was no uncertainty about these ones and they were
posted almost a week earlier.

> It's in Linus' tree now.
>
> And the other part of the OF rework that was posted for review
> on Feb 13 is another story too? It's in Linus' tree as well.

All cleanups and bugfixes except for "Don't assume HAVE_LMB" which
Jeremy had already posted earlier for review.

> Your patches touch 3 architectures, and a lot of the code that
> is used by all the OF drivers, still 03 and 13 Feb was OK for
> them.
>
>> But I am not the GPIO maintainer.
>
> David is. And I heard only positive feedback on the patches
> last time.
>
>> For the record, my main concerns are:
>> - Now that I see the implementation, I think that it is too complex.
>> The bus notifiers really aren't needed and it can be done with much
>> lower impact on the core gpiolib code.
>
> That's a non-argument, what is "lower impact"? Do I touch any
> hot paths? And if nothing has changed, David (again, the gpiolib
> maintainer) is happy with the notifiers approach, why would you
> care?

Adding unneeded notifier infrastructure is churn I don't want to see.

>> Changes to common code don't work that way. =A0Sometimes things just
>> don't get enough attention and they wait another cycle, get reworked,
>> or get dropped entirely.
>
> See above wrt OF rework patches.

which all got attention, were uncontroversial, and did not introduce
functional changes.

>> For one, the device node pointer is moving out of archdata into
>> 'struct device' proper and I've got patches adding OF hooks into the
>> core of the platform bus. =A0If those patches look good to GregKH, then
>> I'll be pursing the same pattern for the other bus types (i2c, spi,
>> etc), and it will be further argument for putting the OF hooks
>> directly into gpiolib instead of using a notifier. =A0I'll be posting
>> the patches as soon as the merge window closes.
>
> I don't get it. Why is it a problem to change your patches that
> ought to be queued for 2.6.*35*?

It's not, and they are going to be queued for 2.6.35.  In fact, I
didn't posted them this week to avoid adding confusion to the merge
window.  The issues isn't changing my patches.  It is that I don't
like the notifier approach, and I intend to prove that it can be done
in a better way.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Anton Vorontsov]

On Sat, Mar 06, 2010 at 09:43:20AM -0700, Grant Likely wrote:
> > On Fri, Mar 05, 2010 at 08:54:56PM -0700, Grant Likely wrote:
> > And of course the part of the OF rework, which was first posted
> > for *review* on Feb 03, is a completely different story?
> >
> >  48 files changed, 317 insertions(+), 575 deletions(-)
> 
> Completely uncontroversial changes with zero functional behaviour
> change.  There was no uncertainty about these ones and they were
> posted almost a week earlier.

The patches simply touch too many things, so I'd say that the
possible breakage impact is on par with the OF GPIO stuff.

[...]
> > That's a non-argument, what is "lower impact"? Do I touch any
> > hot paths? And if nothing has changed, David (again, the gpiolib
> > maintainer) is happy with the notifiers approach, why would you
> > care?
> 
> Adding unneeded notifier infrastructure is churn I don't want to see.

You could reply to my answers earlier and I would change and
repost the patches in a jiffy, since I am interested in these
patches.

But you're obviously not interested in this support since you
didn't answer my replies. I'll explain. If you were interested
in some support you could give some chance to make patches
comfortable to you, and then you could even test them, and
maybe defend their inclusion.

Look at what an interested person does:

http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg00895.html

Note that that was v2 with my comments fixed, and that was
just seconds before 2.6.33 merge window closed. See?
Someone with a direct interest! I gave my comments, they
were fixed, and I felt grateful and responsible for pushing
the support upstream.

But you're not interested in the support, so I don't see
why you block it without any good technical reason.

And note that not only I'm interested in this support, the
I2C/SPI GPIO controllers issue was brought on ml several
times by many people.

[...]
> > I don't get it. Why is it a problem to change your patches that
> > ought to be queued for 2.6.*35*?
> 
> It's not, and they are going to be queued for 2.6.35.  In fact, I
> didn't posted them this week to avoid adding confusion to the merge
> window.  The issues isn't changing my patches.

Then why you mentioned OF rework as some reason to block
these patches?

> It is that I don't
> like the notifier approach, and I intend to prove that it can be done
> in a better way.

No doubt that you have some better ideas (not to mention that
notifiers was your idea as well :-).

Here are some technical arguments:

1. You can implement your new ideas on top of the current solution.
   Or I can happily do that for you.
2. The patches don't change any API, instead they just build
   a bridge between GPIOLIB and OF GPIO infrastructure.
   So it's just a matter *taste* how to build that bridge.
   It's an internal issue of how GPIOLIB and OF GPIO interact.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Grant Likely]

On Sat, Mar 6, 2010 at 6:47 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> You could reply to my answers earlier and I would change and
> repost the patches in a jiffy, since I am interested in these
> patches.
>
> But you're obviously not interested in this support since you
> didn't answer my replies. I'll explain. If you were interested
> in some support you could give some chance to make patches
> comfortable to you, and then you could even test them, and
> maybe defend their inclusion.

I'm sorry you feel that way, and I am sorry that I wasn't able to
reply earlier.  I am interested in generic gpio of support, but you
also need to understand that the run up to the merge window is a busy
time as I need to collect all the patches that I'm responsible for and
get them into linux-next for testing well before the merge window
opens.  I do not maintain gpiolib, and it was therefore quite low on
my priority list at that time.  After the merge window closes, I'll
have time to be more responsive again.

Regardless, this series was not going to be merged through any of my
trees because I'm not maintaining gpiolib.  Andrew asked me opinion on
them, and I gave him my answer.  Andrew can make his own decision
about whether or not to merge them, but my opinion still stands.

I'm sorry that I upset you.  It was not my intention.  Please keep in
mind that it is not unusual for patches to take more than one cycle to
get merged.  For example, I've got patches out on the ARM list that
were posted well before the merge window that have neither received
comments, nor will be merged.  I'll pursue them again after this merge
window closes and other maintainers have more bandwidth to make a good
decision about them.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Andrew Morton]

On Fri, 5 Feb 2010 23:32:36 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> This patch implements GPIOLIB notifier hooks, and thus makes device-enabled
> GPIO chips (i.e. the ones that have gpio_chip->dev specified) automatically
> attached to the OpenFirmware subsystem. Which means that now we can handle
> I2C and SPI GPIO chips almost* transparently.
> 
> ...
>
> +static int of_gpiochip_register_simple(struct gpio_chip *chip,
> +				       struct device_node *np)

Why is this called "register_simple" but the unregistration function
isn't called "unregister_simple"?

> +{
> +	struct of_gpio_chip *of_gc;
> +
> +	if (np->data) {
> +		WARN_ON(1);
> +		return -EBUSY;
> +	}
> +
> +	of_gc = kzalloc(sizeof(*of_gc), GFP_KERNEL);
> +	if (!of_gc)
> +		return -ENOMEM;
> +
> +	of_gc->gpio_cells = 2;
> +	of_gc->xlate = of_gpio_simple_xlate;
> +	of_gc->chip = chip;
> +	np->data = of_gc;
> +	of_node_get(np);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(of_gpiochip_register_simple);

Makes no sense to export a static symbol and to provide no declaration
of it in a .h file.  I assume the export was unintended.


My plot is somewhat lost.  Grant, could you please summarise in
easy-for-akpm-to-understand terms what your issues are with this
patchset and how you think we should proceed?

Thanks.

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Grant Likely]

On Fri, Mar 12, 2010 at 2:07 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 5 Feb 2010 23:32:36 +0300
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
>
>> This patch implements GPIOLIB notifier hooks, and thus makes device-enab=
led
>> GPIO chips (i.e. the ones that have gpio_chip->dev specified) automatica=
lly
>> attached to the OpenFirmware subsystem. Which means that now we can hand=
le
>> I2C and SPI GPIO chips almost* transparently.
>>
>> ...
>>
>> +static int of_gpiochip_register_simple(struct gpio_chip *chip,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0struct device_node *np)
>
> Why is this called "register_simple" but the unregistration function
> isn't called "unregister_simple"?
>
>> +{
>> + =A0 =A0 struct of_gpio_chip *of_gc;
>> +
>> + =A0 =A0 if (np->data) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 WARN_ON(1);
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -EBUSY;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 of_gc =3D kzalloc(sizeof(*of_gc), GFP_KERNEL);
>> + =A0 =A0 if (!of_gc)
>> + =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
>> +
>> + =A0 =A0 of_gc->gpio_cells =3D 2;
>> + =A0 =A0 of_gc->xlate =3D of_gpio_simple_xlate;
>> + =A0 =A0 of_gc->chip =3D chip;
>> + =A0 =A0 np->data =3D of_gc;
>> + =A0 =A0 of_node_get(np);
>> +
>> + =A0 =A0 return 0;
>> +}
>> +EXPORT_SYMBOL(of_gpiochip_register_simple);
>
> Makes no sense to export a static symbol and to provide no declaration
> of it in a .h file. =A0I assume the export was unintended.
>
>
> My plot is somewhat lost. =A0Grant, could you please summarise in
> easy-for-akpm-to-understand terms what your issues are with this
> patchset and how you think we should proceed?

Sure.
I suggested the notifier approach in the first place, but now that I
see what is required to implement it I don't like it.  I also don't
like the potential race condition between registering GPIO devices and
registering the OF gpio notifier.  It is simpler and less code to put
the of_gpio hook directly into the GPIO registration path.  If
CONFIG_OF is not set, then the OF hooks can resolve to empty static
inline functions.

How to proceed:  I'd like to leave this series out for the 2.6.34
cycle and I'll pick it into my OF tree before the 2.6.35 merge window,
but I'll probably modify it to call the OF hooks directly and leave
out the unnecessary notifier infrastructure.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 3/4] of/gpio: Implement GPIOLIB notifier hooks

[Anton Vorontsov]

On Fri, Mar 12, 2010 at 02:38:02PM -0700, Grant Likely wrote:
[...]
> How to proceed:  I'd like to leave this series out for the 2.6.34
> cycle and I'll pick it into my OF tree before the 2.6.35 merge window,
> but I'll probably modify it to call the OF hooks directly and leave
> out the unnecessary notifier infrastructure.

Ping?

[PATCH 4/4] powerpc/mcu_mpc8349emitx: Remove OF GPIO handling stuff

[Anton Vorontsov]

With the new OF GPIO infrastructure it's much easier to handle I2C
GPIO controllers, i.e. now drivers don't have to deal with the
OF-specific bits.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c |   68 +++++-------------------
 1 files changed, 14 insertions(+), 54 deletions(-)

diff --git a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c
index 73c7e6b..5525175 100644
--- a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c
+++ b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c
@@ -18,8 +18,6 @@
 #include <linux/mutex.h>
 #include <linux/i2c.h>
 #include <linux/gpio.h>
-#include <linux/of.h>
-#include <linux/of_gpio.h>
 #include <asm/prom.h>
 #include <asm/machdep.h>
 
@@ -36,7 +34,7 @@ struct mcu {
 	struct mutex lock;
 	struct device_node *np;
 	struct i2c_client *client;
-	struct of_gpio_chip of_gc;
+	struct gpio_chip gc;
 	u8 reg_ctrl;
 };
 
@@ -55,8 +53,7 @@ static void mcu_power_off(void)
 
 static void mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 {
-	struct of_gpio_chip *of_gc = to_of_gpio_chip(gc);
-	struct mcu *mcu = container_of(of_gc, struct mcu, of_gc);
+	struct mcu *mcu = container_of(gc, struct mcu, gc);
 	u8 bit = 1 << (4 + gpio);
 
 	mutex_lock(&mcu->lock);
@@ -75,53 +72,6 @@ static int mcu_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	return 0;
 }
 
-static int mcu_gpiochip_add(struct mcu *mcu)
-{
-	struct device_node *np;
-	struct of_gpio_chip *of_gc = &mcu->of_gc;
-	struct gpio_chip *gc = &of_gc->gc;
-	int ret;
-
-	np = of_find_compatible_node(NULL, NULL, "fsl,mcu-mpc8349emitx");
-	if (!np)
-		return -ENODEV;
-
-	gc->owner = THIS_MODULE;
-	gc->label = np->full_name;
-	gc->can_sleep = 1;
-	gc->ngpio = MCU_NUM_GPIO;
-	gc->base = -1;
-	gc->set = mcu_gpio_set;
-	gc->direction_output = mcu_gpio_dir_out;
-	of_gc->chip = gc;
-	of_gc->gpio_cells = 2;
-	of_gc->xlate = of_gpio_simple_xlate;
-
-	np->data = of_gc;
-	mcu->np = np;
-
-	/*
-	 * We don't want to lose the node, its ->data and ->full_name...
-	 * So, if succeeded, we don't put the node here.
-	 */
-	ret = gpiochip_add(gc);
-	if (ret)
-		of_node_put(np);
-	return ret;
-}
-
-static int mcu_gpiochip_remove(struct mcu *mcu)
-{
-	int ret;
-
-	ret = gpiochip_remove(&mcu->of_gc.gc);
-	if (ret)
-		return ret;
-	of_node_put(mcu->np);
-
-	return 0;
-}
-
 static int __devinit mcu_probe(struct i2c_client *client,
 			       const struct i2c_device_id *id)
 {
@@ -141,7 +91,16 @@ static int __devinit mcu_probe(struct i2c_client *client,
 		goto err;
 	mcu->reg_ctrl = ret;
 
-	ret = mcu_gpiochip_add(mcu);
+	mcu->gc.dev = &client->dev;
+	mcu->gc.owner = THIS_MODULE;
+	mcu->gc.label = dev_name(&client->dev);
+	mcu->gc.can_sleep = 1;
+	mcu->gc.ngpio = MCU_NUM_GPIO;
+	mcu->gc.base = -1;
+	mcu->gc.set = mcu_gpio_set;
+	mcu->gc.direction_output = mcu_gpio_dir_out;
+
+	ret = gpiochip_add(&mcu->gc);
 	if (ret)
 		goto err;
 
@@ -168,9 +127,10 @@ static int __devexit mcu_remove(struct i2c_client *client)
 		glob_mcu = NULL;
 	}
 
-	ret = mcu_gpiochip_remove(mcu);
+	ret = gpiochip_remove(&mcu->gc);
 	if (ret)
 		return ret;
+
 	i2c_set_clientdata(client, NULL);
 	kfree(mcu);
 	return 0;
-- 
1.6.5.7