[PATCH 0/7 RFC] Handle I2C GPIO controllers with the OF (was: pca9539 I2C gpio expander)

[PATCH 0/7 RFC] Handle I2C GPIO controllers with the OF (was: pca9539 I2C gpio expander)

[Anton Vorontsov]

Hi all,

Recently there was a question about I2C GPIO controllers and how should
we handle them with the OpenFirmware and such.

Here is the attempt to "connect" I2C GPIO controllers to the
"OpenFirmware" device tree, without writing an OF-specific bindings
for each driver.

The salt is in these two patches:

[PATCH 3/7] of: fill the archdata for I2C devices
^ Here we're storing the device tree node into the I2C device.

[PATCH 5/7] of/gpio: implement of_dev_gpiochip_{add,remove} calls
^ And here we extracting the the stored node to put the registered
  of_gpio_chip into that node.


How does it look?

p.s. The original question:

----- Forwarded message from "Steven A. Falco" <sfalco@harris.com> -----

Date: Tue, 14 Oct 2008 14:10:25 -0400
From: "Steven A. Falco" <sfalco@harris.com>
To: "linuxppc-dev@ozlabs.org" <linuxppc-dev@ozlabs.org>
Subject: pca9539 I2C gpio expander
List-Id: Linux on PowerPC Developers Mail List <linuxppc-dev.ozlabs.org>

I am attempting to use a pca9539 I2C gpio driver on a PPC440EPx board.  The
driver is "drivers/gpio/pca953x.c".  I've added an entry to the .dts file:

IIC0: i2c@ef600700 {
	compatible = "ibm,iic-440epx", "ibm,iic";
	...
	pca9539@76 {
		compatible = "ti,pca9539";
		reg = <76>;
	};
};

of_register_i2c_devices sees this entry and calls i2c_new_device.
i2c_new_device copies info->platform_data to client->dev.platform_data, but
I think that this structure is empty (at least I don't see where
of_register_i2c_devices would set it).

pca953x_probe is eventually called, but it expects to find its "lowest gpio
number" in client->dev.platform_data->gpio_base, which has not been set.  So
pca953x_probe returns -ENODEV.

I don't understand where the disconnect is.  Should I be able to use the
pca953x.c driver, or is it somehow incompatible?

If it is incompatible, is there a strategy for making it compatible?

	Steve
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

----- End forwarded message -----

[PATCH 1/7] powerpc and sparc: introduce dev_archdata node accessors

[Anton Vorontsov]

The name of the device_node field differ across the platforms, so we
have to implement inlined accessors. This is needed to avoid ugly
#ifdef in the generic code.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/include/asm/device.h |   12 ++++++++++++
 arch/sparc/include/asm/device.h   |   12 ++++++++++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/device.h b/arch/powerpc/include/asm/device.h
index dfd504c..7d2277c 100644
--- a/arch/powerpc/include/asm/device.h
+++ b/arch/powerpc/include/asm/device.h
@@ -18,4 +18,16 @@ struct dev_archdata {
 	void			*dma_data;
 };
 
+static inline void dev_archdata_set_node(struct dev_archdata *ad,
+					 struct device_node *np)
+{
+	ad->of_node = np;
+}
+
+static inline struct device_node *
+dev_archdata_get_node(const struct dev_archdata *ad)
+{
+	return ad->of_node;
+}
+
 #endif /* _ASM_POWERPC_DEVICE_H */
diff --git a/arch/sparc/include/asm/device.h b/arch/sparc/include/asm/device.h
index 19790eb..3702e08 100644
--- a/arch/sparc/include/asm/device.h
+++ b/arch/sparc/include/asm/device.h
@@ -20,4 +20,16 @@ struct dev_archdata {
 	int			numa_node;
 };
 
+static inline void dev_archdata_set_node(struct dev_archdata *ad,
+					 struct device_node *np)
+{
+	ad->prom_node = np;
+}
+
+static inline struct device_node *
+dev_archdata_get_node(const struct dev_archdata *ad)
+{
+	return ad->prom_node;
+}
+
 #endif /* _ASM_SPARC_DEVICE_H */
-- 
1.5.6.3

Re: [PATCH 1/7] powerpc and sparc: introduce dev_archdata node accessors

[David Miller]

From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Thu, 16 Oct 2008 21:12:51 +0400

> The name of the device_node field differ across the platforms, so we
> have to implement inlined accessors. This is needed to avoid ugly
> #ifdef in the generic code.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

I'm happy to just change this member name on sparc to match
powerpc's choice.

That being said, this abstraction still has value and I'd
like to let you merge this now and make forward progress
so:

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH 1/7] powerpc and sparc: introduce dev_archdata node accessors

[Grant Likely]

On Thu, Oct 16, 2008 at 4:36 PM, David Miller <davem@davemloft.net> wrote:
> From: Anton Vorontsov <avorontsov@ru.mvista.com>
> Date: Thu, 16 Oct 2008 21:12:51 +0400
>
>> The name of the device_node field differ across the platforms, so we
>> have to implement inlined accessors. This is needed to avoid ugly
>> #ifdef in the generic code.
>>
>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>
> I'm happy to just change this member name on sparc to match
> powerpc's choice.
>
> That being said, this abstraction still has value and I'd
> like to let you merge this now and make forward progress
> so:
>
> Acked-by: David S. Miller <davem@davemloft.net>

I agree;

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



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

[PATCH 2/7] i2c: add info->archdata field

[Anton Vorontsov]

If present the info->archdata is copied into the dev->archdata.
Some (OpenFirmware) platforms need it.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/i2c/i2c-core.c |    3 +++
 include/linux/i2c.h    |    2 ++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 42e852d..5a485c2 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -266,6 +266,9 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 
 	client->dev.platform_data = info->platform_data;
 
+	if (info->archdata)
+		client->dev.archdata = *info->archdata;
+
 	client->flags = info->flags;
 	client->addr = info->addr;
 	client->irq = info->irq;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 0611512..3e358d3 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -240,6 +240,7 @@ static inline void i2c_set_clientdata (struct i2c_client *dev, void *data)
  * @flags: to initialize i2c_client.flags
  * @addr: stored in i2c_client.addr
  * @platform_data: stored in i2c_client.dev.platform_data
+ * @archdata: copied into i2c_client.dev.archdata
  * @irq: stored in i2c_client.irq
  *
  * I2C doesn't actually support hardware probing, although controllers and
@@ -259,6 +260,7 @@ struct i2c_board_info {
 	unsigned short	flags;
 	unsigned short	addr;
 	void		*platform_data;
+	struct dev_archdata	*archdata;
 	int		irq;
 };
 
-- 
1.5.6.3

Re: [PATCH 2/7] i2c: add info->archdata field

[Jean Delvare]

Hi Anton,

On Thu, 16 Oct 2008 21:12:53 +0400, Anton Vorontsov wrote:
> If present the info->archdata is copied into the dev->archdata.
> Some (OpenFirmware) platforms need it.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  drivers/i2c/i2c-core.c |    3 +++
>  include/linux/i2c.h    |    2 ++
>  2 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 42e852d..5a485c2 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -266,6 +266,9 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
>  
>  	client->dev.platform_data = info->platform_data;
>  
> +	if (info->archdata)
> +		client->dev.archdata = *info->archdata;
> +
>  	client->flags = info->flags;
>  	client->addr = info->addr;
>  	client->irq = info->irq;
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 0611512..3e358d3 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -240,6 +240,7 @@ static inline void i2c_set_clientdata (struct i2c_client *dev, void *data)
>   * @flags: to initialize i2c_client.flags
>   * @addr: stored in i2c_client.addr
>   * @platform_data: stored in i2c_client.dev.platform_data
> + * @archdata: copied into i2c_client.dev.archdata
>   * @irq: stored in i2c_client.irq
>   *
>   * I2C doesn't actually support hardware probing, although controllers and
> @@ -259,6 +260,7 @@ struct i2c_board_info {
>  	unsigned short	flags;
>  	unsigned short	addr;
>  	void		*platform_data;
> +	struct dev_archdata	*archdata;
>  	int		irq;
>  };
>  

No objection from me:

Acked-by: Jean Delvare <khali@linux-fr.org>

Let me know if I should push this patch upstream myself.

-- 
Jean Delvare

Re: [PATCH 2/7] i2c: add info->archdata field

[Benjamin Herrenschmidt]

On Fri, 2008-10-17 at 11:21 +0200, Jean Delvare wrote:
> Hi Anton,
> 
> On Thu, 16 Oct 2008 21:12:53 +0400, Anton Vorontsov wrote:
> > If present the info->archdata is copied into the dev->archdata.
> > Some (OpenFirmware) platforms need it.
> > 
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---

So who's pushing this one ? Jean ?

Cheers,
Ben.

Re: [PATCH 2/7] i2c: add info->archdata field

[Jean Delvare]

On Wed, 22 Oct 2008 11:27:34 +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2008-10-17 at 11:21 +0200, Jean Delvare wrote:
> > Hi Anton,
> > 
> > On Thu, 16 Oct 2008 21:12:53 +0400, Anton Vorontsov wrote:
> > > If present the info->archdata is copied into the dev->archdata.
> > > Some (OpenFirmware) platforms need it.
> > > 
> > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > > ---
> 
> So who's pushing this one ? Jean ?

As I wrote before, I'm happy to take this patch if you want me to, but
I also have no objection if the author of this patchset wants to push
it himself together with the rest of the set. Just let me know what you
expect from me (preferably in the next few hours, as I will be sending
my second and last round of i2c patches for kernel 2.6.28 to Linus
today.)

-- 
Jean Delvare

Re: [PATCH 2/7] i2c: add info->archdata field

[Benjamin Herrenschmidt]

On Wed, 2008-10-22 at 08:50 +0200, Jean Delvare wrote:
> On Wed, 22 Oct 2008 11:27:34 +1100, Benjamin Herrenschmidt wrote:
> > On Fri, 2008-10-17 at 11:21 +0200, Jean Delvare wrote:
> > > Hi Anton,
> > > 
> > > On Thu, 16 Oct 2008 21:12:53 +0400, Anton Vorontsov wrote:
> > > > If present the info->archdata is copied into the dev->archdata.
> > > > Some (OpenFirmware) platforms need it.
> > > > 
> > > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > > > ---
> > 
> > So who's pushing this one ? Jean ?
> 
> As I wrote before, I'm happy to take this patch if you want me to, but
> I also have no objection if the author of this patchset wants to push
> it himself together with the rest of the set. Just let me know what you
> expect from me (preferably in the next few hours, as I will be sending
> my second and last round of i2c patches for kernel 2.6.28 to Linus
> today.)

Anton, I've done my last batch for 2.6.28 before -rc1 so it might be
better if it goes through Jean no ?

Cheers,
Ben.

Re: [PATCH 2/7] i2c: add info->archdata field

[Anton Vorontsov]

On Wed, Oct 22, 2008 at 06:37:55PM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2008-10-22 at 08:50 +0200, Jean Delvare wrote:
> > On Wed, 22 Oct 2008 11:27:34 +1100, Benjamin Herrenschmidt wrote:
> > > On Fri, 2008-10-17 at 11:21 +0200, Jean Delvare wrote:
> > > > Hi Anton,
> > > > 
> > > > On Thu, 16 Oct 2008 21:12:53 +0400, Anton Vorontsov wrote:
> > > > > If present the info->archdata is copied into the dev->archdata.
> > > > > Some (OpenFirmware) platforms need it.
> > > > > 
> > > > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > > > > ---
> > > 
> > > So who's pushing this one ? Jean ?
> > 
> > As I wrote before, I'm happy to take this patch if you want me to, but
> > I also have no objection if the author of this patchset wants to push
> > it himself together with the rest of the set. Just let me know what you
> > expect from me (preferably in the next few hours, as I will be sending
> > my second and last round of i2c patches for kernel 2.6.28 to Linus
> > today.)
> 
> Anton, I've done my last batch for 2.6.28 before -rc1 so it might be
> better if it goes through Jean no ?

Sorry, I guess the "few hours" limit was done exactly when I was
sleeping. ;-) Anyway, I'm fine either way. Don't see any problem
if it goes i2c or powerpc tree.

Thanks,

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

Re: [PATCH 2/7] i2c: add info->archdata field

[Jean Delvare]

On Wed, 22 Oct 2008 14:08:13 +0400, Anton Vorontsov wrote:
> On Wed, Oct 22, 2008 at 06:37:55PM +1100, Benjamin Herrenschmidt wrote:
> > On Wed, 2008-10-22 at 08:50 +0200, Jean Delvare wrote:
> > > On Wed, 22 Oct 2008 11:27:34 +1100, Benjamin Herrenschmidt wrote:
> > > > On Fri, 2008-10-17 at 11:21 +0200, Jean Delvare wrote:
> > > > > Hi Anton,
> > > > > 
> > > > > On Thu, 16 Oct 2008 21:12:53 +0400, Anton Vorontsov wrote:
> > > > > > If present the info->archdata is copied into the dev->archdata.
> > > > > > Some (OpenFirmware) platforms need it.
> > > > > > 
> > > > > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > > > > > ---
> > > > 
> > > > So who's pushing this one ? Jean ?
> > > 
> > > As I wrote before, I'm happy to take this patch if you want me to, but
> > > I also have no objection if the author of this patchset wants to push
> > > it himself together with the rest of the set. Just let me know what you
> > > expect from me (preferably in the next few hours, as I will be sending
> > > my second and last round of i2c patches for kernel 2.6.28 to Linus
> > > today.)
> > 
> > Anton, I've done my last batch for 2.6.28 before -rc1 so it might be
> > better if it goes through Jean no ?
> 
> Sorry, I guess the "few hours" limit was done exactly when I was
> sleeping. ;-) Anyway, I'm fine either way. Don't see any problem
> if it goes i2c or powerpc tree.

OK, then I'm taking the patch in my tree and it will go to Linus later
today.

-- 
Jean Delvare

Re: [PATCH 2/7] i2c: add info->archdata field

[Anton Vorontsov]

On Wed, Oct 22, 2008 at 01:07:48PM +0200, Jean Delvare wrote:
> On Wed, 22 Oct 2008 14:08:13 +0400, Anton Vorontsov wrote:
> > On Wed, Oct 22, 2008 at 06:37:55PM +1100, Benjamin Herrenschmidt wrote:
> > > On Wed, 2008-10-22 at 08:50 +0200, Jean Delvare wrote:
> > > > On Wed, 22 Oct 2008 11:27:34 +1100, Benjamin Herrenschmidt wrote:
> > > > > On Fri, 2008-10-17 at 11:21 +0200, Jean Delvare wrote:
> > > > > > Hi Anton,
> > > > > > 
> > > > > > On Thu, 16 Oct 2008 21:12:53 +0400, Anton Vorontsov wrote:
> > > > > > > If present the info->archdata is copied into the dev->archdata.
> > > > > > > Some (OpenFirmware) platforms need it.
> > > > > > > 
> > > > > > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > > > > > > ---
> > > > > 
> > > > > So who's pushing this one ? Jean ?
> > > > 
> > > > As I wrote before, I'm happy to take this patch if you want me to, but
> > > > I also have no objection if the author of this patchset wants to push
> > > > it himself together with the rest of the set. Just let me know what you
> > > > expect from me (preferably in the next few hours, as I will be sending
> > > > my second and last round of i2c patches for kernel 2.6.28 to Linus
> > > > today.)
> > > 
> > > Anton, I've done my last batch for 2.6.28 before -rc1 so it might be
> > > better if it goes through Jean no ?
> > 
> > Sorry, I guess the "few hours" limit was done exactly when I was
> > sleeping. ;-) Anyway, I'm fine either way. Don't see any problem
> > if it goes i2c or powerpc tree.
> 
> OK, then I'm taking the patch in my tree and it will go to Linus later
> today.

Much appreciated, thanks.

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

[PATCH 3/7] of: fill the archdata for I2C devices

[Anton Vorontsov]

For I2C devices we just setting the the node pointer in the archdata.
This is needed so that the other code would know device's node.

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

diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
index 6a98dc8..f9e18ed 100644
--- a/drivers/of/of_i2c.c
+++ b/drivers/of/of_i2c.c
@@ -24,6 +24,7 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
 
 	for_each_child_of_node(adap_node, node) {
 		struct i2c_board_info info = {};
+		struct dev_archdata dev_ad = {};
 		const u32 *addr;
 		int len;
 
@@ -41,6 +42,9 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
 
 		info.addr = *addr;
 
+		dev_archdata_set_node(&dev_ad, node);
+		info.archdata = &dev_ad;
+
 		request_module(info.type);
 
 		result = i2c_new_device(adap, &info);
@@ -51,6 +55,13 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
 			irq_dispose_mapping(info.irq);
 			continue;
 		}
+
+		/*
+		 * Get the node to not lose the dev_archdata->of_node.
+		 * Currently there is no way to put it back, as well as no
+		 * of_unregister_i2c_devices() call.
+		 */
+		of_node_get(node);
 	}
 }
 EXPORT_SYMBOL(of_register_i2c_devices);
-- 
1.5.6.3

Re: [PATCH 3/7] of: fill the archdata for I2C devices

[Grant Likely]

On Thu, Oct 16, 2008 at 09:12:56PM +0400, Anton Vorontsov wrote:
> For I2C devices we just setting the the node pointer in the archdata.
> This is needed so that the other code would know device's node.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Looks okay to me.

Acked-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>  drivers/of/of_i2c.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> index 6a98dc8..f9e18ed 100644
> --- a/drivers/of/of_i2c.c
> +++ b/drivers/of/of_i2c.c
> @@ -24,6 +24,7 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
>  
>  	for_each_child_of_node(adap_node, node) {
>  		struct i2c_board_info info = {};
> +		struct dev_archdata dev_ad = {};
>  		const u32 *addr;
>  		int len;
>  
> @@ -41,6 +42,9 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
>  
>  		info.addr = *addr;
>  
> +		dev_archdata_set_node(&dev_ad, node);
> +		info.archdata = &dev_ad;
> +
>  		request_module(info.type);
>  
>  		result = i2c_new_device(adap, &info);
> @@ -51,6 +55,13 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
>  			irq_dispose_mapping(info.irq);
>  			continue;
>  		}
> +
> +		/*
> +		 * Get the node to not lose the dev_archdata->of_node.
> +		 * Currently there is no way to put it back, as well as no
> +		 * of_unregister_i2c_devices() call.
> +		 */
> +		of_node_get(node);
>  	}
>  }
>  EXPORT_SYMBOL(of_register_i2c_devices);
> -- 
> 1.5.6.3
> 

[PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

Any GPIO controller that have a device associated with it is encouraged to
register/unregister the gpiochips with dev_gpiochip_*() calls.

Platform may redefine these calls to glue the gpiochips with the
architecture-specific code.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 include/asm-generic/gpio.h |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 0f99ad3..f31c7ae 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -89,6 +89,50 @@ extern int __must_check gpiochip_reserve(int start, int ngpio);
 extern int gpiochip_add(struct gpio_chip *chip);
 extern int __must_check gpiochip_remove(struct gpio_chip *chip);
 
+/*
+ * Platforms can define their own __dev_ versions to glue gpio_chips with the
+ * architecture-specific code.
+ */
+#ifndef __dev_gpiochip_add
+#define __dev_gpiochip_add __dev_gpiochip_add
+static inline int __dev_gpiochip_add(struct device *dev,
+				     struct gpio_chip *chip)
+{
+	chip->dev = dev;
+	return gpiochip_add(chip);
+}
+#endif /* __dev_gpiochip_add */
+
+#ifndef __dev_gpiochip_remove
+#define __dev_gpiochip_remove __dev_gpiochip_remove
+static inline int __dev_gpiochip_remove(struct device *dev,
+					struct gpio_chip *chip)
+{
+	return gpiochip_remove(chip);
+}
+#endif /* __dev_gpiochip_remove */
+
+/**
+ * dev_gpiochip_add - register a gpio_chip for a device
+ * @dev: device to register gpio_chip for
+ * @chip: the chip to register
+ * Context: potentially before irqs or kmalloc will work
+ *
+ * This function takes the extra @dev argument that is used to associate
+ * the chip with a device. Otherwise it behaves the same way as the
+ * gpiochip_add().
+ */
+#define dev_gpiochip_add(dev, chip) __dev_gpiochip_add((dev), (chip))
+
+/**
+ * dev_gpiochip_remove - unregister a gpio_chip
+ * @dev: device to remove the chip from
+ * @chip: the chip to unregister
+ *
+ * Use this function to unregister the chip that was registered using
+ * dev_gpiochip_add().
+ */
+#define dev_gpiochip_remove(dev, chip) __dev_gpiochip_remove((dev), (chip))
 
 /* Always use the library code for GPIO management calls,
  * or when sleeping may be involved.
-- 
1.5.6.3

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[David Brownell]

On Thursday 16 October 2008, Anton Vorontsov wrote:
> +/*
> + * Platforms can define their own __dev_ versions to glue gpio_chips with the
> + * architecture-specific code.
> + */
> +#ifndef __dev_gpiochip_add
> +#define __dev_gpiochip_add __dev_gpiochip_add
> +static inline int __dev_gpiochip_add(struct device *dev,
> +                                    struct gpio_chip *chip)
> +{
> +       chip->dev = dev;
> +       return gpiochip_add(chip);
> +}
> +#endif /* __dev_gpiochip_add */

This is pretty ugly, especially the implication that *EVERY* gpio_chip
provider needs modification to use these calls.

Surely it would be a lot simpler to just add platform-specific hooks
to gpiochip_{add,remove}(), so that no providers need to be changed??


> +#ifndef __dev_gpiochip_remove
> +#define __dev_gpiochip_remove __dev_gpiochip_remove
> +static inline int __dev_gpiochip_remove(struct device *dev,
> +                                       struct gpio_chip *chip)
> +{
> +       return gpiochip_remove(chip);
> +}
> +#endif /* __dev_gpiochip_remove */

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

On Fri, Oct 17, 2008 at 01:24:42PM -0700, David Brownell wrote:
> On Thursday 16 October 2008, Anton Vorontsov wrote:
> > +/*
> > + * Platforms can define their own __dev_ versions to glue gpio_chips with the
> > + * architecture-specific code.
> > + */
> > +#ifndef __dev_gpiochip_add
> > +#define __dev_gpiochip_add __dev_gpiochip_add
> > +static inline int __dev_gpiochip_add(struct device *dev,
> > +                                    struct gpio_chip *chip)
> > +{
> > +       chip->dev = dev;
> > +       return gpiochip_add(chip);
> > +}
> > +#endif /* __dev_gpiochip_add */
> 
> This is pretty ugly, especially the implication that *EVERY* gpio_chip
> provider needs modification to use these calls.

Anyway most of them need some modifications to work with OF...

> Surely it would be a lot simpler to just add platform-specific hooks
> to gpiochip_{add,remove}(), [...]

We have printk and dev_printk. kzalloc and devm_kzalloc (though I
aware that devm_ are different than just dev_).  So I thought that
dev_gpiochip_* would be logical order of things...

If you don't like it, I can readily implement hooks for
gpiochip_{add,remove}().


Thanks for the comments,

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

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[David Brownell]

On Friday 17 October 2008, Anton Vorontsov wrote:
> On Fri, Oct 17, 2008 at 01:24:42PM -0700, David Brownell wrote:
> > On Thursday 16 October 2008, Anton Vorontsov wrote:
> > > +/*
> > > + * Platforms can define their own __dev_ versions to glue gpio_chips with the
> > > + * architecture-specific code.
> > > + */
> > > +#ifndef __dev_gpiochip_add
> > > +#define __dev_gpiochip_add __dev_gpiochip_add
> > > +static inline int __dev_gpiochip_add(struct device *dev,
> > > +                                    struct gpio_chip *chip)
> > > +{
> > > +       chip->dev = dev;
> > > +       return gpiochip_add(chip);
> > > +}
> > > +#endif /* __dev_gpiochip_add */
> > 
> > This is pretty ugly, especially the implication that *EVERY* gpio_chip
> > provider needs modification to use these calls.
> 
> Anyway most of them need some modifications to work with OF...

The changes I saw were just to cope with not having
the system-specific platform_data provided:  don't
fail if that pointer is NULL, and arrange for dynamic
allocation of some GPIO numbers.

With OpenFirmware, presumably the implication is that
the relevant data is in the OF device tree...


I think that it *barely* makes sense to allow the chips
to bind to drivers without platform data when there's
not even OF in the environment.  ONLY in the case where
the GPIOs are exported through sysfs, in fact, since
otherwise there's no way for other system components
to know those GPIOs even exist!!  And even that seems
pretty marginal to me...


> > Surely it would be a lot simpler to just add platform-specific hooks
> > to gpiochip_{add,remove}(), [...]
> 
> We have printk and dev_printk. kzalloc and devm_kzalloc (though I
> aware that devm_ are different than just dev_).  So I thought that
> dev_gpiochip_* would be logical order of things...

Those aren't platform hook mechanisms though, and
there's no need to modify every driver to use them
in order to work *at all* on OpenFirmware systems.


> If you don't like it, I can readily implement hooks for
> gpiochip_{add,remove}().

It seems a better way to a clean solution, IMO.  For
example, the OF hook for adding a gpio_chip might
know that it's got to stuff chip->base with a number
other than "-1" (say, "42") since that was stored in
some property of the device's OF shadow, and other
devices have properties associating them with GPIO
numbers derived from that (3rd gpio on that chip,
42 + 3 == 45) and so forth.

That said ... there's a LOT of configuration that
doesn't seem to me like it can be generic.  Pullups,
pulldowns, default values, polarity inversion,
what devices depend on those GPIOs being available
before they can come up (GPIO leds and power switches
come quickly to mind), all kinds of chip-specific
details, and more.

Did you look at providing chip-aware OF glue drivers
for this stuff?  Doing stuff like just turn the OF
device properties into the right platform_data, and
maybe runing FORTH bytecodes to do other configuration
magic needed...

- Dave

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

On Mon, Oct 20, 2008 at 12:29:57AM -0700, David Brownell wrote:
[...]
> > Anyway most of them need some modifications to work with OF...
> 
> The changes I saw were just to cope with not having
> the system-specific platform_data provided:  don't
> fail if that pointer is NULL, and arrange for dynamic
> allocation of some GPIO numbers.
> 
> With OpenFirmware, presumably the implication is that
> the relevant data is in the OF device tree...

Yes. Some data is in the device tree.

> I think that it *barely* makes sense to allow the chips
> to bind to drivers without platform data when there's
> not even OF in the environment. ONLY in the case where
> the GPIOs are exported through sysfs, in fact, since
> otherwise there's no way for other system components
> to know those GPIOs even exist!!  And even that seems
> pretty marginal to me...

Platform data is a completely different story. And yes, we can't
handle it properly with the device tree. By "properly" I mean without
adding an explicit OF stuff to the drivers, i.e. we should handle the
pdata transparently to the existing drivers.

I quite like the bus notifiers approach:

http://lkml.org/lkml/2008/6/5/209  (mmc_spi example)

But it doesn't work as a module (i.e. OF-specific bits should be
always in-kernel).

[...]
> > If you don't like it, I can readily implement hooks for
> > gpiochip_{add,remove}().
> 
> It seems a better way to a clean solution, IMO.

Ok. I will do it.

[...]
> Did you look at providing chip-aware OF glue drivers
> for this stuff?  Doing stuff like just turn the OF
> device properties into the right platform_data, and
> maybe runing FORTH bytecodes to do other configuration
> magic needed...

Yes. Few times already. To make the glue, every driver needs
some modifications, and it is always triggers huge discussions
about how to exactly refactor the driver to make it work with
the OF.

http://lkml.org/lkml/2008/5/23/297 (again mmc_spi example).

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

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Benjamin Herrenschmidt]

> But it doesn't work as a module (i.e. OF-specific bits should be
> always in-kernel).

Why not ?

Ben.

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

On Wed, Oct 22, 2008 at 11:29:20AM +1100, Benjamin Herrenschmidt wrote:
> 
> > But it doesn't work as a module (i.e. OF-specific bits should be
> > always in-kernel).
> 
> Why not ?

If say "X" driver loads prior to bus-notifier module (where we fill
the platform data), then X.0 device will try to probe w/o platform
data and will fail. The only way to re-probe things is to rmmod X &&
insmod of_pdata_filler_X && insmod X. So things depend on the module
load order.

The obvious solution is to link the OF stuff into the module, but
this also won't work, since modules have only one entry (and exit)
point. So there is no way* to hook our OF helpers into the module.

* Well, there is one solution to this problem. We can implement
arch-specific init_module and cleanup_module entry/exit points,
where we can load/unload the OF hooks. This is quite easy, but
may look ugly. I could show the drafts.

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

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

On Wed, Oct 22, 2008 at 05:03:47AM +0400, Anton Vorontsov wrote:
> On Wed, Oct 22, 2008 at 11:29:20AM +1100, Benjamin Herrenschmidt wrote:
> > 
> > > But it doesn't work as a module (i.e. OF-specific bits should be
> > > always in-kernel).
> > 
> > Why not ?
> 
> If say "X" driver loads prior to bus-notifier module (where we fill
> the platform data), then X.0 device will try to probe w/o platform
> data and will fail. The only way to re-probe things is to rmmod X &&
> insmod of_pdata_filler_X && insmod X. So things depend on the module
> load order.

Thinking about it more, I started recalling other issues. The bus
notifier chain doesn't replay previous events, so we also have to
register the notifier before the _devices_ are registered. And this
ruins the whole approach. :-/ Yeah, that's why I abandoned the bus
notifier idea.

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

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Benjamin Herrenschmidt]

On Wed, 2008-10-22 at 05:42 +0400, Anton Vorontsov wrote:
> 
> Thinking about it more, I started recalling other issues. The bus
> notifier chain doesn't replay previous events, so we also have to
> register the notifier before the _devices_ are registered. And this
> ruins the whole approach. :-/ Yeah, that's why I abandoned the bus
> notifier idea.

The notifier can be registered before the devices, though it's a little
bit fishy and fragile.

Easier I suppose to just have OF specific hooks in the bus code.

Ben.

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Grant Likely]

On Wed, Oct 22, 2008 at 01:28:17PM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2008-10-22 at 05:42 +0400, Anton Vorontsov wrote:
> > 
> > Thinking about it more, I started recalling other issues. The bus
> > notifier chain doesn't replay previous events, so we also have to
> > register the notifier before the _devices_ are registered. And this
> > ruins the whole approach. :-/ Yeah, that's why I abandoned the bus
> > notifier idea.
> 
> The notifier can be registered before the devices, though it's a little
> bit fishy and fragile.
> 
> Easier I suppose to just have OF specific hooks in the bus code.

Every time I think about the problem, this is the conclusion that I come
to.  Either have OF specific hooks in the probe/remove functions; or
have separate probe/remove for OF created instances of a device.

g.

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[David Brownell]

On Tuesday 21 October 2008, Benjamin Herrenschmidt wrote:
> The notifier can be registered before the devices, though it's a little
> bit fishy and fragile.
> 
> Easier I suppose to just have OF specific hooks in the bus code.

Like what I suggested:  "chip-aware OF glue drivers".  The relevant
bus code being the "of_platform_bus_type" infrastructure.

Example:  instead of Anton's patch #6 modifying the existing pca953x
driver, an of_pca953x driver that knows how to poke around in the OF
device attributes to (a) create the pca953x_platform_data, (b) call
i2c_register_board_info() to make that available later, and then
finally (c) vanish, since it's not needed any longer.

Better that than either the $SUBJECT patch, or modifying gpiolib to
grow OF-specific hooks ... hooks that can at best solve *one* of the
problems:  which GPIO numbers to use with this chip.  The platform
data does solve other problems(*) like:  (i) how to initialize the
polarity inversion register, (ii) arranging to set up other devices
only after their GPIOs are ready, (iii) initializing things that
device drivers won't always know about, or which may need to be
set up before such drivers are available.

- Dave

(*) A trivial example of (ii) would be LEDs driven by those GPIOs.

    A less trivial example:  see arch/arm/mach-davinci/board-evm.c
    in current GIT.  There are three pcf8574 I2C expanders used for
    various things ... LEDs, audio PLL, device power supplies, reset
    lines for external devices, more.

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

On Tue, Oct 21, 2008 at 09:22:48PM -0700, David Brownell wrote:
> On Tuesday 21 October 2008, Benjamin Herrenschmidt wrote:
> > The notifier can be registered before the devices, though it's a little
> > bit fishy and fragile.
> > 
> > Easier I suppose to just have OF specific hooks in the bus code.
> 
> Like what I suggested:  "chip-aware OF glue drivers".  The relevant
> bus code being the "of_platform_bus_type" infrastructure.
> 
> Example:  instead of Anton's patch #6 modifying the existing pca953x
> driver, an of_pca953x driver that knows how to poke around in the OF
> device attributes to (a) create the pca953x_platform_data, (b) call
> i2c_register_board_info() to make that available later, and then
> finally (c) vanish, since it's not needed any longer.

Heh. You tell me my first approach:

http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056730.html (mmc_spi)

The OF people didn't like the patch which was used to support this
approach:
http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056728.html


The board info has another problem though. We can't remove it, thus
we can't implement module_exit() for the 'OF glue'.

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

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

On Wed, Oct 22, 2008 at 02:36:41PM +0400, Anton Vorontsov wrote:
> On Tue, Oct 21, 2008 at 09:22:48PM -0700, David Brownell wrote:
> > On Tuesday 21 October 2008, Benjamin Herrenschmidt wrote:
> > > The notifier can be registered before the devices, though it's a little
> > > bit fishy and fragile.
> > > 
> > > Easier I suppose to just have OF specific hooks in the bus code.
> > 
> > Like what I suggested:  "chip-aware OF glue drivers".  The relevant
> > bus code being the "of_platform_bus_type" infrastructure.
> > 
> > Example:  instead of Anton's patch #6 modifying the existing pca953x
> > driver, an of_pca953x driver that knows how to poke around in the OF
> > device attributes to (a) create the pca953x_platform_data, (b) call
> > i2c_register_board_info() to make that available later, and then
> > finally (c) vanish, since it's not needed any longer.
> 
> Heh. You tell me my first approach:
> 
> http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056730.html (mmc_spi)
> 
> The OF people didn't like the patch which was used to support this
> approach:
> http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056728.html

Though, I think I'll able to persuade Grant that two registration paths
are inevitable (i.e. for simple devices we should use
drivers/of/of_{i2c,spi}.c and for complex cases we'll have to have
another method of registration).

> The board info has another problem though. We can't remove it, thus
> we can't implement module_exit() for the 'OF glue'.

And try to solve this problem... maybe then things will begin to
move forward.

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

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

On Wed, Oct 22, 2008 at 02:46:06PM +0400, Anton Vorontsov wrote:
> On Wed, Oct 22, 2008 at 02:36:41PM +0400, Anton Vorontsov wrote:
> > On Tue, Oct 21, 2008 at 09:22:48PM -0700, David Brownell wrote:
> > > On Tuesday 21 October 2008, Benjamin Herrenschmidt wrote:
> > > > The notifier can be registered before the devices, though it's a little
> > > > bit fishy and fragile.
> > > > 
> > > > Easier I suppose to just have OF specific hooks in the bus code.
> > > 
> > > Like what I suggested:  "chip-aware OF glue drivers".  The relevant
> > > bus code being the "of_platform_bus_type" infrastructure.
> > > 
> > > Example:  instead of Anton's patch #6 modifying the existing pca953x
> > > driver, an of_pca953x driver that knows how to poke around in the OF
> > > device attributes to (a) create the pca953x_platform_data, (b) call
> > > i2c_register_board_info() to make that available later, and then
> > > finally (c) vanish, since it's not needed any longer.
> > 
> > Heh. You tell me my first approach:
> > 
> > http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056730.html (mmc_spi)
> > 
> > The OF people didn't like the patch which was used to support this
> > approach:
> > http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056728.html
> 
> Though, I think I'll able to persuade Grant that two registration paths
> are inevitable (i.e. for simple devices we should use
> drivers/of/of_{i2c,spi}.c and for complex cases we'll have to have
> another method of registration).
> 
> > The board info has another problem though. We can't remove it, thus
> > we can't implement module_exit() for the 'OF glue'.
> 
> And try to solve this problem... maybe then things will begin to
> move forward.

There is another problem: board infos are scanned at the controller
registration time only. So if we register the board infos after
the controller registered, then nobody will probe the board infos.

This is all solvable by hacking the i2c core code though. I started
it, but it turned out to be ugly. I'll finish it though, just to show
it someday.

But now I'm not sure if it worth the efforts. Maybe we could just
modify the drivers to do something like this?

This is not exactly "transparently" to the drivers, but well..

diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 01b4bbd..b1dfa7b 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -9,4 +9,7 @@ obj-$(CONFIG_GPIO_MAX732X)	+= max732x.o
 obj-$(CONFIG_GPIO_MCP23S08)	+= mcp23s08.o
 obj-$(CONFIG_GPIO_PCA953X)	+= pca953x.o
 obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x.o
+ifeq ($(CONFIG_OF),y)
+obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x_of.o
+endif
 obj-$(CONFIG_GPIO_BT8XX)	+= bt8xxgpio.o
diff --git a/drivers/gpio/pcf857x.c b/drivers/gpio/pcf857x.c
index 4bc2070..f8057d2 100644
--- a/drivers/gpio/pcf857x.c
+++ b/drivers/gpio/pcf857x.c
@@ -187,7 +187,7 @@ static int pcf857x_probe(struct i2c_client *client,
 	struct pcf857x			*gpio;
 	int				status;
 
-	pdata = client->dev.platform_data;
+	pdata = pcf857x_get_pdata(client);
 	if (!pdata)
 		return -ENODEV;
 
@@ -314,7 +314,7 @@ fail:
 
 static int pcf857x_remove(struct i2c_client *client)
 {
-	struct pcf857x_platform_data	*pdata = client->dev.platform_data;
+	struct pcf857x_platform_data	*pdata = pcf857x_get_pdata(client);
 	struct pcf857x			*gpio = i2c_get_clientdata(client);
 	int				status = 0;
 
@@ -334,6 +334,8 @@ static int pcf857x_remove(struct i2c_client *client)
 		kfree(gpio);
 	else
 		dev_err(&client->dev, "%s --> %d\n", "remove", status);
+
+	pcf857x_put_pdata(client);
 	return status;
 }
 
diff --git a/drivers/gpio/pcf857x_of.c b/drivers/gpio/pcf857x_of.c
new file mode 100644
index 0000000..414943b
--- /dev/null
+++ b/drivers/gpio/pcf857x_of.c
@@ -0,0 +1,36 @@
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/i2c/pcf857x.h>
+#include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+
+struct pcf857x_platform_data *pcf857x_get_pdata(struct i2c_client *client)
+{
+	struct pcf857x_platform_data *pdata = client->dev.platform_data;
+
+	if (pdata)
+		return pdata;
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	/*
+	 * Do the OF-specific setup here.
+	 */
+
+	client->dev.platform_data = pdata;
+}
+
+void pcf857x_put_pdata(struct i2c_client *client)
+{
+	struct pcf857x_platform_data *pdata = client->dev.platform_data;
+
+	/*
+	 * Do the OF-specific cleanup here.
+	 */
+
+	kfree(pdata);
+}
diff --git a/include/linux/i2c/pcf857x.h b/include/linux/i2c/pcf857x.h
index 0767a2a..bdc1aba 100644
--- a/include/linux/i2c/pcf857x.h
+++ b/include/linux/i2c/pcf857x.h
@@ -1,6 +1,8 @@
 #ifndef __LINUX_PCF857X_H
 #define __LINUX_PCF857X_H
 
+struct i2c_client;
+
 /**
  * struct pcf857x_platform_data - data to set up pcf857x driver
  * @gpio_base: number of the chip's first GPIO
@@ -41,4 +43,13 @@ struct pcf857x_platform_data {
 	void		*context;
 };
 
+#ifdef CONFIG_OF
+extern struct pcf857x_platform_data *
+	pcf857x_get_pdata(struct i2c_client *client);
+extern void pcf857x_put_pdata(struct i2c_client *client);
+#else
+#define pcf857x_get_pdata(client) ((client)->dev.platform_data)
+#define pcf857x_put_pdata(client)
+#endif
+
 #endif /* __LINUX_PCF857X_H */

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[David Brownell]

On Wednesday 22 October 2008, Anton Vorontsov wrote:
> 
> > > The board info has another problem though. We can't remove it, thus
> > > we can't implement module_exit() for the 'OF glue'.

That's not a problem.  Why would you want to remove it?


> > And try to solve this problem... maybe then things will begin to
> > move forward.
> 
> There is another problem: board infos are scanned at the controller
> registration time only.

Right.  Such board description data should be made available
early in boot.  As a rule:  before arch_initcall() finishes,
so that subsys_initcall() code can use the associated GPIOs.

(It's fairly well acknowledged that init dependency handling
has a lot of problems.  Until that's fixed ... for GPIOs, the
general advice is to make sure everything is available by
subsys_initcall time,  so the subsystems which rely on GPIOs
to initialize -- power switches, resets, etc -- can initialize.
That can require i2c adapter drivers to use subsys_initcall,
for example.)


> So if we register the board infos after 
> the controller registered, then nobody will probe the board infos.

See above.  If you're doing it right, there's no problem.
That is, scan the OF tables early.  Just like PNP tables
get scanned early, for example.

- Dave

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

On Wed, Oct 22, 2008 at 02:04:52PM -0700, David Brownell wrote:
> On Wednesday 22 October 2008, Anton Vorontsov wrote:
> > 
> > > > The board info has another problem though. We can't remove it, thus
> > > > we can't implement module_exit() for the 'OF glue'.
> 
> That's not a problem.  Why would you want to remove it?
> 
> 
> > > And try to solve this problem... maybe then things will begin to
> > > move forward.
> > 
> > There is another problem: board infos are scanned at the controller
> > registration time only.
> 
> Right.  Such board description data should be made available
> early in boot.  As a rule:  before arch_initcall() finishes,
> so that subsys_initcall() code can use the associated GPIOs.
> 
> (It's fairly well acknowledged that init dependency handling
> has a lot of problems.  Until that's fixed ... for GPIOs, the
> general advice is to make sure everything is available by
> subsys_initcall time,  so the subsystems which rely on GPIOs
> to initialize -- power switches, resets, etc -- can initialize.
> That can require i2c adapter drivers to use subsys_initcall,
> for example.)
> 
> 
> > So if we register the board infos after 
> > the controller registered, then nobody will probe the board infos.
> 
> See above.  If you're doing it right, there's no problem.
> That is, scan the OF tables early.  Just like PNP tables
> get scanned early, for example.

Heh. If we don't want to be able to make the OF-parsing code
be a module then there is no problem at all. I can use the bus
notifiers. And it is most straightforward solution then.

But I quite dislike to bloat the kernel image with
maybe-never-used-on-this-board code. My aim was to make the
OF-parsing part be a module too. Because in the long run we
need the OF-parsing stuff for _every_ driver that needs
platform data. It's quite expensive to have it always built-in,
don't you think?

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

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[David Brownell]

On Wednesday 22 October 2008, Anton Vorontsov wrote:
> > 
> > > So if we register the board infos after 
> > > the controller registered, then nobody will probe the board infos.
> > 
> > See above.  If you're doing it right, there's no problem.
> > That is, scan the OF tables early.  Just like PNP tables
> > get scanned early, for example.
> 
> Heh. If we don't want to be able to make the OF-parsing code
> be a module then there is no problem at all. I can use the bus
> notifiers. And it is most straightforward solution then.
> 
> But I quite dislike to bloat the kernel image with
> maybe-never-used-on-this-board code.

So have it live in the __init text section.  If you're
building a kernel with support for several boards, you
know it's necessarily going to be larger than it would
be if only one board were supported.  But you can shrink
kernel size by judicious use of __init sections..



> My aim was to make the 
> OF-parsing part be a module too. Because in the long run we
> need the OF-parsing stuff for _every_ driver that needs
> platform data. It's quite expensive to have it always built-in,
> don't you think?

If it's discarded early, after translating the data from
OF format into what the drivers need, there will be no
RAM footprint.

- Dave

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

On Wed, Oct 22, 2008 at 02:52:46PM -0700, David Brownell wrote:
> On Wednesday 22 October 2008, Anton Vorontsov wrote:
> > > 
> > > > So if we register the board infos after 
> > > > the controller registered, then nobody will probe the board infos.
> > > 
> > > See above.  If you're doing it right, there's no problem.
> > > That is, scan the OF tables early.  Just like PNP tables
> > > get scanned early, for example.
> > 
> > Heh. If we don't want to be able to make the OF-parsing code
> > be a module then there is no problem at all. I can use the bus
> > notifiers. And it is most straightforward solution then.
> > 
> > But I quite dislike to bloat the kernel image with
> > maybe-never-used-on-this-board code.
> 
> So have it live in the __init text section.  If you're
> building a kernel with support for several boards, you
> know it's necessarily going to be larger than it would
> be if only one board were supported.  But you can shrink
> kernel size by judicious use of __init sections..

Won't work, unfortunately. I2C devices are created by the
i2c controllers, via drivers/of_i2c.c  of_register_i2c_devices().

There is a good reason to do so, the code needs to know
controller's OF node to walk down and register the child nodes
(devices). See drivers/i2c/busses/i2c-mpc.c -- it calls
of_register_i2c_devices() at the end of the probe().

Since we can't call __init stuff from non-__init, the scheme
you purpose won't work.

The same is for SPI (drivers/of_spi.c of_register_spi_devices()).

> > My aim was to make the 
> > OF-parsing part be a module too. Because in the long run we
> > need the OF-parsing stuff for _every_ driver that needs
> > platform data. It's quite expensive to have it always built-in,
> > don't you think?
> 
> If it's discarded early, after translating the data from
> OF format into what the drivers need, there will be no
> RAM footprint.

There is also kernel image size that matters...

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

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[David Brownell]

On Wednesday 22 October 2008, Anton Vorontsov wrote:
> > 
> > So have it live in the __init text section...
> 
> Won't work, unfortunately. I2C devices are created by the
> i2c controllers, via drivers/of_i2c.c  of_register_i2c_devices().

And I'm pointing out a way to have the normal I2C core code
flow do that creation.  OF shouldn't need to be so much of a
special case.


> There is a good reason to do so, the code needs to know
> controller's OF node to walk down and register the child nodes
> (devices). See drivers/i2c/busses/i2c-mpc.c -- it calls
> of_register_i2c_devices() at the end of the probe().

I don't get it.  (But then, so much of the OF support seems
needlessly convoluted to me ... on top of seeming to be
insufficient for configuring board-specific details.)

There's an OF device tree, distinct from the Linux driver
model tree.  Why should there be any obstacle to accessing
records from that tree before the relevant driver model nodes
have been created?  Remember that the various board_info
structs get registered before the driver model nodes for
which they are templates.  Just translate the OF tree data
to those templates(*), then register them.

I understand that it's currently structured differetnly
than that ... consulting the OF tree "late" not early.
But that's still newish, and from what I've heard so far
it doesn't seem like the best structure either... nothing
seems to plug in smoothly.

- Dave

(*) The role of the board_info structs is very similar
    to the role of OF device attributes.  As is the role
    of the platform_data ... except that's more specific
    to the chip involved (and its driver), and expects
    any callbacks to be in C code not FORTH.

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Benjamin Herrenschmidt]

On Wed, 2008-10-22 at 14:04 -0700, David Brownell wrote:
> > So if we register the board infos after 
> > the controller registered, then nobody will probe the board infos.
> 
> See above.  If you're doing it right, there's no problem.
> That is, scan the OF tables early.  Just like PNP tables
> get scanned early, for example.

It's still pretty yucky in that case to scan the device-tree to convert
it into some kind of fugly board info ... I'd rather have the end
drivers that actually use those GPIOs scan the device-tree directly.

But then, I'm not a believer in generic drivers for things like GPIOs,
i2c devices, etc.. :-)

Ben.

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[David Brownell]

On Wednesday 22 October 2008, Benjamin Herrenschmidt wrote:
> On Wed, 2008-10-22 at 14:04 -0700, David Brownell wrote:
> > > So if we register the board infos after 
> > > the controller registered, then nobody will probe the board infos.
> > 
> > See above.  If you're doing it right, there's no problem.
> > That is, scan the OF tables early.  Just like PNP tables
> > get scanned early, for example.
> 
> It's still pretty yucky in that case to scan the device-tree to convert
> it into some kind of fugly board info ... I'd rather have the end
> drivers that actually use those GPIOs scan the device-tree directly.

Keep in mind that these problems are not specific to GPIOs.

And, very important!!, most of the drivers run without OF...

Pretty much any little device that needs board-specific
customization has the same class of problems:  drivers
will need a variety of parameters that may are often not
sharable with other devices, with idiosyncratic usage.
And they hook up to other drivers in arbitrary ways.

When PCs have such issues, ACPI hides them from Linux.

If those parameters -- potentially including callbacks
that escape to FORTH -- are stored in the OF device tree,
so be it.  But "fugly" sounds like part of that problem
domain, so it's no surprise that it maps onto the solution
space too...


Specifically with respect to GPIOs ... what do you mean
by "end driver" though?  I previously pointed at one
example (Davinci EVM) where one bank of GPIOs is used
by about six different drivers ... none of which have
any reason to know they're using a pcf8574a vs any other
kind of GPIO.  Is the "end driver" the IDE/CF driver?
The USB OTG driver?  The driver sitting the next layer
above of one of those?  *Some* of the drivers need to
touch the GPIOs.  Others don't.

 
> But then, I'm not a believer in generic drivers for things
> like GPIOs, i2c devices, etc.. :-)

I kind of like being able to re-use code myself.  ;)

It's a win to have *one* pcf8574/5 driver that can be
reused -- with a bit of care configuring it into the
system -- instead of having every board contribute yet
another board-specific hack in drivers/i2c/chips ...

And I think such stuff can be done even with OF.

- Dave

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[David Brownell]

On Wednesday 22 October 2008, Anton Vorontsov wrote:
> --- a/drivers/gpio/pcf857x.c
> +++ b/drivers/gpio/pcf857x.c
> @@ -187,7 +187,7 @@ static int pcf857x_probe(struct i2c_client *client,
>         struct pcf857x                  *gpio;
>         int                             status;
>  
> -       pdata = client->dev.platform_data;
> +       pdata = pcf857x_get_pdata(client);
>         if (!pdata)
>                 return -ENODEV;
>  

I suppose that can work.  Regardless, some OF-specific code will
need to map device tree state into a generic format that's fully
decoupled from OF.  (And there's something to be said to having
that mapping sit in the same directory as the driver needing it.)

[PATCH 0/6 RFC] OF-glue devices for I2C/SPI (was: Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

On Wed, Oct 22, 2008 at 02:46:06PM +0400, Anton Vorontsov wrote:
[...]
> > > Like what I suggested:  "chip-aware OF glue drivers".  The relevant
> > > bus code being the "of_platform_bus_type" infrastructure.
> > > 
> > > Example:  instead of Anton's patch #6 modifying the existing pca953x
> > > driver, an of_pca953x driver that knows how to poke around in the OF
> > > device attributes to (a) create the pca953x_platform_data, (b) call
> > > i2c_register_board_info() to make that available later, and then
> > > finally (c) vanish, since it's not needed any longer.
> > 
> > Heh. You tell me my first approach:
> > 
> > http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056730.html (mmc_spi)
> > 
> > The OF people didn't like the patch which was used to support this
> > approach:
> > http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056728.html
> 
> Though, I think I'll able to persuade Grant that two registration paths
> are inevitable (i.e. for simple devices we should use
> drivers/of/of_{i2c,spi}.c and for complex cases we'll have to have
> another method of registration).

Ok, here it is.

I don't like this approach because:

1. It feels like an overhead to create an of_device for each i2c
   device that needs platform data.

2. We have to do ugly of_should_create_pdev() in the i2c code,
   and duplicate lists of supported devices.

Could anybody convince me that this isn't a big deal? ;-)

Otherwise I'll stick with this approach:
http://lkml.org/lkml/2008/10/22/471

Thanks,

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

[PATCH 1/6] of/base: Add new helper of_should_create_pdev()

[Anton Vorontsov]

This is used to check list of devices for which we should create
a dedicated device.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/of/base.c  |   27 +++++++++++++++++++++++++++
 include/linux/of.h |    1 +
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 7c79e94..1baeee3 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -458,6 +458,33 @@ int of_modalias_node(struct device_node *node, char *modalias, int len)
 }
 EXPORT_SYMBOL_GPL(of_modalias_node);
 
+/*
+ * The list of device's compatible entries for which drivers require
+ * platform_data to be filled. For such cases we create OF platform
+ * devices.
+ */
+static const char *of_pdev_list[] = {
+};
+
+/**
+ * of_should_create_pdev - See if a node needs a full-fledged OF device
+ * @node:	pointer to a device tree node
+ *
+ * Based on the value of the compatible property, this routine will determine
+ * if a full-fledged device should be created for a node.
+ */
+bool of_should_create_pdev(struct device_node *node)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(of_pdev_list); i++) {
+		if (!of_device_is_compatible(node, of_pdev_list[i]))
+			return 1;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_should_create_pdev);
+
 /**
  * of_parse_phandles_with_args - Find a node pointed by phandle in a list
  * @np:		pointer to a device tree node containing a list
diff --git a/include/linux/of.h b/include/linux/of.h
index e2488f5..d183533 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -71,6 +71,7 @@ extern int of_n_size_cells(struct device_node *np);
 extern const struct of_device_id *of_match_node(
 	const struct of_device_id *matches, const struct device_node *node);
 extern int of_modalias_node(struct device_node *node, char *modalias, int len);
+extern bool of_should_create_pdev(struct device_node *node);
 extern int of_parse_phandles_with_args(struct device_node *np,
 	const char *list_name, const char *cells_name, int index,
 	struct device_node **out_node, const void **out_args);
-- 
1.5.6.3

[PATCH 2/6] of/of_i2c: implement of_{,un}register_i2c_device

[Anton Vorontsov]

of_register_i2c_device is factored out of of_register_i2c_devices.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/of/of_i2c.c    |   71 +++++++++++++++++++++++++++++------------------
 include/linux/of_i2c.h |    4 +++
 2 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
index 24bbef7..57de7c5 100644
--- a/drivers/of/of_i2c.c
+++ b/drivers/of/of_i2c.c
@@ -16,41 +16,58 @@
 #include <linux/of_i2c.h>
 #include <linux/module.h>
 
-void of_register_i2c_devices(struct i2c_adapter *adap,
-			     struct device_node *adap_node)
+struct i2c_client *of_register_i2c_device(struct i2c_adapter *adap,
+					  struct i2c_board_info *info,
+					  struct device_node *node)
 {
-	void *result;
-	struct device_node *node;
+	struct i2c_client *client;
+	const u32 *addr;
+	int len;
 
-	for_each_child_of_node(adap_node, node) {
-		struct i2c_board_info info = {};
-		const u32 *addr;
-		int len;
+	if (of_modalias_node(node, info->type, sizeof(info->type)))
+		return NULL;
 
-		if (of_modalias_node(node, info.type, sizeof(info.type)) < 0)
-			continue;
+	addr = of_get_property(node, "reg", &len);
+	if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
+		printk(KERN_ERR
+		       "of-i2c: invalid i2c device entry\n");
+		return NULL;
+	}
+
+	info->irq = irq_of_parse_and_map(node, 0);
+	info->addr = *addr;
 
-		addr = of_get_property(node, "reg", &len);
-		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
-			printk(KERN_ERR
-			       "of-i2c: invalid i2c device entry\n");
-			continue;
-		}
+	request_module("%s", info->type);
+
+	client = i2c_new_device(adap, info);
+	if (!client) {
+		printk(KERN_ERR
+		       "of-i2c: Failed to load driver for %s\n",
+		       info->type);
+		irq_dispose_mapping(info->irq);
+		return NULL;
+	}
+	return client;
+}
+EXPORT_SYMBOL(of_register_i2c_device);
 
-		info.irq = irq_of_parse_and_map(node, 0);
+void of_unregister_i2c_device(struct i2c_client *client)
+{
+	if (client->irq)
+		irq_dispose_mapping(client->irq);
+	i2c_unregister_device(client);
+}
+EXPORT_SYMBOL(of_unregister_i2c_device);
 
-		info.addr = *addr;
+void of_register_i2c_devices(struct i2c_adapter *adap,
+			     struct device_node *adap_node)
+{
+	struct device_node *node;
 
-		request_module("%s", info.type);
+	for_each_child_of_node(adap_node, node) {
+		struct i2c_board_info info = {};
 
-		result = i2c_new_device(adap, &info);
-		if (result == NULL) {
-			printk(KERN_ERR
-			       "of-i2c: Failed to load driver for %s\n",
-			       info.type);
-			irq_dispose_mapping(info.irq);
-			continue;
-		}
+		of_register_i2c_device(adap, &info, node);
 	}
 }
 EXPORT_SYMBOL(of_register_i2c_devices);
diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h
index bd2a870..68851c0 100644
--- a/include/linux/of_i2c.h
+++ b/include/linux/of_i2c.h
@@ -14,6 +14,10 @@
 
 #include <linux/i2c.h>
 
+struct i2c_client *of_register_i2c_device(struct i2c_adapter *adap,
+					  struct i2c_board_info *info,
+					  struct device_node *node);
+void of_unregister_i2c_device(struct i2c_client *client);
 void of_register_i2c_devices(struct i2c_adapter *adap,
 			     struct device_node *adap_node);
 
-- 
1.5.6.3

[PATCH 3/6] of/of_i2c: add support for dedicated OF I2C devices

[Anton Vorontsov]

of_i2c will create the OF platform device if it knows that the
device won't work without platform data. The OF platform driver
will fill the platform data and will register real I2C device.

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

diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
index 57de7c5..02aa1d6 100644
--- a/drivers/of/of_i2c.c
+++ b/drivers/of/of_i2c.c
@@ -13,6 +13,7 @@
 
 #include <linux/i2c.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/of_i2c.h>
 #include <linux/module.h>
 
@@ -67,6 +68,17 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
 	for_each_child_of_node(adap_node, node) {
 		struct i2c_board_info info = {};
 
+#ifdef CONFIG_PPC
+		/* TODO: of_platform_device_create() for SPARC. */
+		if (of_should_create_pdev(node)) {
+			struct of_device *of_pdev;
+
+			of_pdev = of_platform_device_create(node, NULL,
+							    &adap->dev);
+			WARN_ON(!of_pdev);
+			continue;
+		}
+#endif
 		of_register_i2c_device(adap, &info, node);
 	}
 }
-- 
1.5.6.3

Re: [PATCH 3/6] of/of_i2c: add support for dedicated OF I2C devices

[David Miller]

From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Tue, 28 Oct 2008 20:46:15 +0300

> of_i2c will create the OF platform device if it knows that the
> device won't work without platform data. The OF platform driver
> will fill the platform data and will register real I2C device.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Wrt. the comment in the ppc protected code, sparc doesn't need stuff
like this, we already create an of_device for every node in the device
tree.

Just use "of_find_device_by_node()" on sparc, which will work on any
device_node object.

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

[Anton Vorontsov]

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.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/i2c/chips/mcu_mpc8349emitx.c |    1 +
 drivers/of/gpio.c                    |   21 +++++++++++++++++++--
 include/linux/of_gpio.h              |    5 +++++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/chips/mcu_mpc8349emitx.c b/drivers/i2c/chips/mcu_mpc8349emitx.c
index 82a9bcb..73c7e6b 100644
--- a/drivers/i2c/chips/mcu_mpc8349emitx.c
+++ b/drivers/i2c/chips/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 7cd7301..8de478e 100644
--- a/drivers/of/gpio.c
+++ b/drivers/of/gpio.c
@@ -63,7 +63,7 @@ int of_get_gpio(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:
@@ -87,7 +87,7 @@ int of_gpio_simple_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
 {
 	const u32 *gpio = gpio_spec;
 
-	if (*gpio > of_gc->gc.ngpio)
+	if (*gpio > of_gc->chip->ngpio)
 		return -EINVAL;
 
 	return *gpio;
@@ -122,6 +122,23 @@ 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 "new-style" registration or "old-style" one.
+	 */
+	if (of_gc->chip)
+		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 67db101..3a8545c 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -14,16 +14,21 @@
 #ifndef __LINUX_OF_GPIO_H
 #define __LINUX_OF_GPIO_H
 
+#include <linux/compiler.h>
 #include <linux/errno.h>
 #include <linux/gpio.h>
 
 #ifdef CONFIG_OF_GPIO
 
+struct device_node;
+struct device;
+
 /*
  * Generic OF GPIO chip
  */
 struct of_gpio_chip {
 	struct gpio_chip gc;
+	struct gpio_chip *chip;
 	int gpio_cells;
 	int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
 		     const void *gpio_spec);
-- 
1.5.6.3

[PATCH 5/6] gpio/pca953x: pass gpio_chip pointer to the setup/teardown callbacks

[Anton Vorontsov]

This is needed so that the platform code could register the chip
with the platform internal structures.

While at it, also change gpio_base to the signed type (we'll pass
-1 for the dynamic allocation).

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/gpio/pca953x.c      |    7 +++----
 include/linux/i2c/pca953x.h |    8 +++++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index 9ceeb89..7597e01 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -235,8 +235,7 @@ static int __devinit pca953x_probe(struct i2c_client *client,
 		goto out_failed;
 
 	if (pdata->setup) {
-		ret = pdata->setup(client, chip->gpio_chip.base,
-				chip->gpio_chip.ngpio, pdata->context);
+		ret = pdata->setup(client, &chip->gpio_chip, pdata->context);
 		if (ret < 0)
 			dev_warn(&client->dev, "setup failed, %d\n", ret);
 	}
@@ -256,8 +255,8 @@ static int pca953x_remove(struct i2c_client *client)
 	int ret = 0;
 
 	if (pdata->teardown) {
-		ret = pdata->teardown(client, chip->gpio_chip.base,
-				chip->gpio_chip.ngpio, pdata->context);
+		ret = pdata->teardown(client, &chip->gpio_chip,
+				      pdata->context);
 		if (ret < 0) {
 			dev_err(&client->dev, "%s failed, %d\n",
 					"teardown", ret);
diff --git a/include/linux/i2c/pca953x.h b/include/linux/i2c/pca953x.h
index 3c73612..60d60c0 100644
--- a/include/linux/i2c/pca953x.h
+++ b/include/linux/i2c/pca953x.h
@@ -1,8 +1,10 @@
 /* platform data for the PCA9539 16-bit I/O expander driver */
 
+struct gpio_chip;
+
 struct pca953x_platform_data {
 	/* number of the first GPIO */
-	unsigned	gpio_base;
+	int		gpio_base;
 
 	/* initial polarity inversion setting */
 	uint16_t	invert;
@@ -10,9 +12,9 @@ struct pca953x_platform_data {
 	void		*context;	/* param to setup/teardown */
 
 	int		(*setup)(struct i2c_client *client,
-				unsigned gpio, unsigned ngpio,
+				struct gpio_chip *chip,
 				void *context);
 	int		(*teardown)(struct i2c_client *client,
-				unsigned gpio, unsigned ngpio,
+				struct gpio_chip *chip,
 				void *context);
 };
-- 
1.5.6.3

[PATCH 6/6] gpio: OpenFirmware bindings for the pca953x

[Anton Vorontsov]

Unfortunately we have to duplicate compatibles list, since
MODULE_DEVICE_TABLE() doesn't work with the extern symbols. :-(

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/gpio/Kconfig      |    7 ++
 drivers/gpio/Makefile     |    1 +
 drivers/gpio/of_pca953x.c |  134 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/of/base.c         |   21 +++++++
 4 files changed, 163 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/of_pca953x.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 7f2ee27..d5a9e2c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -103,6 +103,13 @@ config GPIO_PCA953X
 	  This driver can also be built as a module.  If so, the module
 	  will be called pca953x.
 
+config GPIO_OF_PCA953X
+	tristate "OpenFirmware Bindings for PCA953x, PCA955x, and MAX7310"
+	depends on GPIO_PCA953X
+	help
+	  Say yes here to enable OpenFirmware bindings for PCA953x, PCA955x,
+	  and MAX7310 I/O ports.
+
 config GPIO_PCF857X
 	tristate "PCF857x, PCA{85,96}7x, and MAX732[89] I2C GPIO expanders"
 	depends on I2C
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 6aafdeb..b6edf33 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_GPIO_MAX7301)	+= max7301.o
 obj-$(CONFIG_GPIO_MAX732X)	+= max732x.o
 obj-$(CONFIG_GPIO_MCP23S08)	+= mcp23s08.o
 obj-$(CONFIG_GPIO_PCA953X)	+= pca953x.o
+obj-$(CONFIG_GPIO_OF_PCA953X)	+= of_pca953x.o
 obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x.o
 obj-$(CONFIG_GPIO_TWL4030)	+= twl4030-gpio.o
 obj-$(CONFIG_GPIO_BT8XX)	+= bt8xxgpio.o
diff --git a/drivers/gpio/of_pca953x.c b/drivers/gpio/of_pca953x.c
new file mode 100644
index 0000000..6884776
--- /dev/null
+++ b/drivers/gpio/of_pca953x.c
@@ -0,0 +1,134 @@
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/of.h>
+#include <linux/of_i2c.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/i2c/pca953x.h>
+
+struct of_pca953x {
+	struct device_node *node;
+	struct pca953x_platform_data pdata;
+	struct i2c_client *client;
+};
+
+static int of_pca953x_setup(struct i2c_client *client, struct gpio_chip *chip,
+			    void *context)
+{
+	struct of_pca953x *of_pca = context;
+	struct of_gpio_chip *of_gc;
+
+	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;
+	of_pca->node->data = of_gc;
+
+	return 0;
+}
+
+static int of_pca953x_teardown(struct i2c_client *client,
+			       struct gpio_chip *chip,
+			       void *context)
+{
+	struct of_pca953x *of_pca = context;
+	struct of_gpio_chip *of_gc = of_pca->node->data;
+
+	of_pca->node->data = NULL;
+	kfree(of_gc);
+	return 0;
+}
+
+static int __devinit of_pca953x_platform_probe(struct of_device *ofdev,
+					    const struct of_device_id *match)
+{
+	struct device *dev = &ofdev->dev;
+	struct device *adap_dev = dev->parent;
+	struct i2c_adapter *adap = to_i2c_adapter(adap_dev);
+	struct device_node *node = ofdev->node;
+	struct i2c_board_info info = {};
+	struct of_pca953x *of_pca;
+	int ret;
+
+	of_pca = kzalloc(sizeof(*of_pca), GFP_KERNEL);
+	if (!of_pca)
+		return -ENOMEM;
+
+	of_pca->node = node;
+	of_pca->pdata.gpio_base = -1;
+	of_pca->pdata.context = of_pca;
+	of_pca->pdata.setup = of_pca953x_setup;
+	of_pca->pdata.teardown = of_pca953x_teardown;
+	info.platform_data = &of_pca->pdata;
+
+	of_pca->client = of_register_i2c_device(adap, &info, node);
+	if (!of_pca->client) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	dev_set_drvdata(dev, of_pca);
+	return 0;
+err:
+	kfree(of_pca);
+	return ret;
+}
+
+static int __devexit of_pca953x_platform_remove(struct of_device *ofdev)
+{
+	struct of_pca953x *of_pca = dev_get_drvdata(&ofdev->dev);
+
+	of_unregister_i2c_device(of_pca->client);
+	return 0;
+}
+
+static const struct of_device_id of_pca953x_ids[] = {
+	{ "nxp,pca9534" },
+	{ "nxp,pca9535" },
+	{ "nxp,pca9536" },
+	{ "nxp,pca9537" },
+	{ "nxp,pca9538" },
+	{ "nxp,pca9539" },
+	{ "nxp,pca9554" },
+	{ "nxp,pca9555" },
+	{ "nxp,pca9557" },
+	{ "ti,pca9534" },
+	{ "ti,pca9535" },
+	{ "ti,pca9536" },
+	{ "ti,pca9537" },
+	{ "ti,pca9538" },
+	{ "ti,pca9539" },
+	{ "ti,pca9554" },
+	{ "ti,pca9555" },
+	{ "ti,pca9557" },
+	{ "maxim,max7310" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_pca953x_ids);
+
+static struct of_platform_driver of_pca953x_platform_driver = {
+	.name		= "of_pca953x_platform",
+	.match_table	= of_pca953x_ids,
+	.probe		= of_pca953x_platform_probe,
+	.remove		= __devexit_p(of_pca953x_platform_remove),
+};
+
+static int __init of_pca953x_platform_init(void)
+{
+	return of_register_platform_driver(&of_pca953x_platform_driver);
+}
+module_init(of_pca953x_platform_init);
+
+static void __exit of_pca953x_platform_exit(void)
+{
+	of_unregister_platform_driver(&of_pca953x_platform_driver);
+}
+module_exit(of_pca953x_platform_exit);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 1baeee3..f828792 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -464,6 +464,27 @@ EXPORT_SYMBOL_GPL(of_modalias_node);
  * devices.
  */
 static const char *of_pdev_list[] = {
+#if defined(CONFIG_GPIO_OF_PCA953X) || defined(CONFIG_GPIO_OF_PCA953X_MODULE)
+	"nxp,pca9534",
+	"nxp,pca9535",
+	"nxp,pca9536",
+	"nxp,pca9537",
+	"nxp,pca9538",
+	"nxp,pca9539",
+	"nxp,pca9554",
+	"nxp,pca9555",
+	"nxp,pca9557",
+	"ti,pca9534",
+	"ti,pca9535",
+	"ti,pca9536",
+	"ti,pca9537",
+	"ti,pca9538",
+	"ti,pca9539",
+	"ti,pca9554",
+	"ti,pca9555",
+	"ti,pca9557",
+	"maxim,max7310",
+#endif
 };
 
 /**
-- 
1.5.6.3

Re: [PATCH 0/6 RFC] OF-glue devices for I2C/SPI (was: Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Grant Likely]

On Tue, Oct 28, 2008 at 11:45 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Wed, Oct 22, 2008 at 02:46:06PM +0400, Anton Vorontsov wrote:
> [...]
>> > > Like what I suggested:  "chip-aware OF glue drivers".  The relevant
>> > > bus code being the "of_platform_bus_type" infrastructure.
>> > >
>> > > Example:  instead of Anton's patch #6 modifying the existing pca953x
>> > > driver, an of_pca953x driver that knows how to poke around in the OF
>> > > device attributes to (a) create the pca953x_platform_data, (b) call
>> > > i2c_register_board_info() to make that available later, and then
>> > > finally (c) vanish, since it's not needed any longer.
>> >
>> > Heh. You tell me my first approach:
>> >
>> > http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056730.html (mmc_spi)
>> >
>> > The OF people didn't like the patch which was used to support this
>> > approach:
>> > http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056728.html
>>
>> Though, I think I'll able to persuade Grant that two registration paths
>> are inevitable (i.e. for simple devices we should use
>> drivers/of/of_{i2c,spi}.c and for complex cases we'll have to have
>> another method of registration).
>
> Ok, here it is.
>
> I don't like this approach because:
>
> 1. It feels like an overhead to create an of_device for each i2c
>   device that needs platform data.
>
> 2. We have to do ugly of_should_create_pdev() in the i2c code,
>   and duplicate lists of supported devices.
>
> Could anybody convince me that this isn't a big deal? ;-)

I really don't like this approach either.  I think it is a big deal.
It greatly increases the complexity of the probe path for the device.
Adapting the OF data to pdata is entirely driver specific and it
doesn't make sense to break it out into a separate of_device or use
the board_info mechanism to disassociate the adapter code from the
driver.  Each instance of adapter code will never be associated with a
different driver and I think it is entirely reasonable to have a
driver specific hook in the drivers probe path to build pdata from the
device tree.

>
> Otherwise I'll stick with this approach:
> http://lkml.org/lkml/2008/10/22/471

I like this approach.

g.

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

Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls

[Benjamin Herrenschmidt]

On Wed, 2008-10-22 at 05:03 +0400, Anton Vorontsov wrote:
> If say "X" driver loads prior to bus-notifier module (where we fill
> the platform data), then X.0 device will try to probe w/o platform
> data and will fail. The only way to re-probe things is to rmmod X &&
> insmod of_pdata_filler_X && insmod X. So things depend on the module
> load order.
> 
> The obvious solution is to link the OF stuff into the module, but
> this also won't work, since modules have only one entry (and exit)
> point. So there is no way* to hook our OF helpers into the module.

Well, right, we need the bus notifier to be registered before any
device gets added ... which mean from the same module_init that
registers the bus itself. A bit annoying ...

> * Well, there is one solution to this problem. We can implement
> arch-specific init_module and cleanup_module entry/exit points,
> where we can load/unload the OF hooks. This is quite easy, but
> may look ugly. I could show the drafts.

Yuck :-)

Cheers,
Ben.

[PATCH 5/7] of/gpio: implement of_dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

And let the gpiolib forward all dev_gpiochip_ calls to of_ versions, there
we can glue the gpiochips with the device tree.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/include/asm/gpio.h |    7 +++-
 drivers/of/gpio.c               |   75 +++++++++++++++++++++++++++++++++++++-
 include/linux/of_gpio.h         |    7 ++++
 3 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/gpio.h b/arch/powerpc/include/asm/gpio.h
index ea04632..92610b1 100644
--- a/arch/powerpc/include/asm/gpio.h
+++ b/arch/powerpc/include/asm/gpio.h
@@ -14,8 +14,13 @@
 #ifndef __ASM_POWERPC_GPIO_H
 #define __ASM_POWERPC_GPIO_H
 
-#include <linux/errno.h>
+/* Tell the gpiolib that we'll handle the dev_gpiochip_* calls. */
+#define __dev_gpiochip_add of_dev_gpiochip_add
+#define __dev_gpiochip_remove of_dev_gpiochip_remove
+
 #include <asm-generic/gpio.h>
+#include <linux/errno.h>
+#include <linux/of_gpio.h>
 
 #ifdef CONFIG_GPIOLIB
 
diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
index 7cd7301..b6f56af 100644
--- a/drivers/of/gpio.c
+++ b/drivers/of/gpio.c
@@ -12,12 +12,33 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <asm/prom.h>
 
+static struct gpio_chip *of_gc_to_gc(struct of_gpio_chip *of_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 may kill the first option.
+	 */
+	if (of_gc->chip)
+		return of_gc->chip;
+	return &of_gc->gc;
+}
+
 /**
  * of_get_gpio - Get a GPIO number from the device tree to use with GPIO API
  * @np:		device node to get GPIO from
@@ -63,7 +84,7 @@ int of_get_gpio(struct device_node *np, int index)
 	if (ret < 0)
 		goto err1;
 
-	ret += of_gc->gc.base;
+	ret += of_gc_to_gc(of_gc)->base;
 err1:
 	of_node_put(gc);
 err0:
@@ -87,7 +108,7 @@ int of_gpio_simple_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
 {
 	const u32 *gpio = gpio_spec;
 
-	if (*gpio > of_gc->gc.ngpio)
+	if (*gpio > of_gc_to_gc(of_gc)->ngpio)
 		return -EINVAL;
 
 	return *gpio;
@@ -161,3 +182,53 @@ err0:
 	return ret;
 }
 EXPORT_SYMBOL(of_mm_gpiochip_add);
+
+int of_dev_gpiochip_add(struct device *dev, struct gpio_chip *chip)
+{
+	struct device_node *np = dev_archdata_get_node(&dev->archdata);
+	struct of_gpio_chip *of_gc;
+	int ret;
+
+	if (!np || np->data)
+		return -EINVAL;
+
+	of_gc = kzalloc(sizeof(*of_gc), GFP_KERNEL);
+	if (!of_gc)
+		return -ENOMEM;
+	/*
+	 * NOTE: for simple cases we use the simple_xlate with 2 cells scheme.
+	 * You can always overwrite it with an exceptions list that would
+	 * match on of_device_is_compatible().
+	 */
+	of_gc->gpio_cells = 2;
+	of_gc->xlate = of_gpio_simple_xlate;
+
+	chip->dev = dev;
+	of_gc->chip = chip;
+	np->data = of_gc;
+
+	ret = gpiochip_add(chip);
+	if (ret)
+		goto err_gpiochip_add;
+	return 0;
+
+err_gpiochip_add:
+	np->data = NULL;
+	chip->dev = NULL;
+	kfree(of_gc);
+	return ret;
+}
+EXPORT_SYMBOL(of_dev_gpiochip_add);
+
+int of_dev_gpiochip_remove(struct device *dev, struct gpio_chip *chip)
+{
+	struct device_node *np = dev_archdata_get_node(&dev->archdata);
+	int ret;
+
+	ret = gpiochip_remove(chip);
+	if (ret)
+		return ret;
+	np->data = NULL;
+	return 0;
+}
+EXPORT_SYMBOL(of_dev_gpiochip_remove);
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 67db101..273cd79 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -14,16 +14,21 @@
 #ifndef __LINUX_OF_GPIO_H
 #define __LINUX_OF_GPIO_H
 
+#include <linux/compiler.h>
 #include <linux/errno.h>
 #include <linux/gpio.h>
 
 #ifdef CONFIG_OF_GPIO
 
+struct device_node;
+struct device;
+
 /*
  * Generic OF GPIO chip
  */
 struct of_gpio_chip {
 	struct gpio_chip gc;
+	struct gpio_chip *chip;
 	int gpio_cells;
 	int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
 		     const void *gpio_spec);
@@ -53,6 +58,8 @@ static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
 extern int of_get_gpio(struct device_node *np, int index);
 extern int of_mm_gpiochip_add(struct device_node *np,
 			      struct of_mm_gpio_chip *mm_gc);
+extern int of_dev_gpiochip_add(struct device *dev, struct gpio_chip *chip);
+extern int of_dev_gpiochip_remove(struct device *dev, struct gpio_chip *chip);
 extern int of_gpio_simple_xlate(struct of_gpio_chip *of_gc,
 				struct device_node *np,
 				const void *gpio_spec);
-- 
1.5.6.3

Re: [PATCH 5/7] of/gpio: implement of_dev_gpiochip_{add,remove} calls

[David Brownell]

On Thursday 16 October 2008, Anton Vorontsov wrote:
> +       if (of_gc->chip)
> +               return of_gc->chip;
> +       return &of_gc->gc;

presumably there's a reason not to

	of_gc->chip = &of_gc->gc;

when this gets set up, so this can always be a simple

	return of_gc->chip;

(and inlined)?  Needlessly complex...

Re: [PATCH 5/7] of/gpio: implement of_dev_gpiochip_{add,remove} calls

[Anton Vorontsov]

On Fri, Oct 17, 2008 at 01:25:01PM -0700, David Brownell wrote:
> On Thursday 16 October 2008, Anton Vorontsov wrote:
> > +       if (of_gc->chip)
> > +               return of_gc->chip;
> > +       return &of_gc->gc;
> 
> presumably there's a reason not to
> 
> 	of_gc->chip = &of_gc->gc;

Yes, there are two reasons:

1. I need some place to insert the huge comment, which you snipped. ;-)

2. I didn't want to break current users. That is, the line you purpose
   ("of_gc->chip = &of_gc->gc;") should be inserted to every of_gpio
   controller. Most of them use of_mm_gpiochip_add(), so these are easy.
   But some (mcu_mpc8349emitx.c) are doing things by themselves.

Thanks,

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

[PATCH 6/7] gpio/pca953x: convert to dev_gpiochip_add and make it work with the OF

[Anton Vorontsov]

With OpenFirmware we can't handle platform data for the devices, so let's
not strictly depend on it. That way we can't set polarity invertion for
this controller, but we'll handle this with active-low/high GPIO flags in
the OF tree.

Also switch the driver to dev_gpiochip calls, so that OF subsystem would
know about the newly registered chips.

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

diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index cc84686..6e51fda 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -188,7 +188,6 @@ static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
 	gc->base = chip->gpio_start;
 	gc->ngpio = gpios;
 	gc->label = chip->client->name;
-	gc->dev = &chip->client->dev;
 	gc->owner = THIS_MODULE;
 }
 
@@ -200,8 +199,6 @@ static int __devinit pca953x_probe(struct i2c_client *client,
 	int ret;
 
 	pdata = client->dev.platform_data;
-	if (pdata == NULL)
-		return -ENODEV;
 
 	chip = kzalloc(sizeof(struct pca953x_chip), GFP_KERNEL);
 	if (chip == NULL)
@@ -209,7 +206,10 @@ static int __devinit pca953x_probe(struct i2c_client *client,
 
 	chip->client = client;
 
-	chip->gpio_start = pdata->gpio_base;
+	if (pdata)
+		chip->gpio_start = pdata->gpio_base;
+	else
+		chip->gpio_start = -1;
 
 	/* initialize cached registers from their original values.
 	 * we can't share this chip with another i2c master.
@@ -225,16 +225,18 @@ static int __devinit pca953x_probe(struct i2c_client *client,
 		goto out_failed;
 
 	/* set platform specific polarity inversion */
-	ret = pca953x_write_reg(chip, PCA953X_INVERT, pdata->invert);
-	if (ret)
-		goto out_failed;
+	if (pdata) {
+		ret = pca953x_write_reg(chip, PCA953X_INVERT, pdata->invert);
+		if (ret)
+			goto out_failed;
+	}
 
 
-	ret = gpiochip_add(&chip->gpio_chip);
+	ret = dev_gpiochip_add(&client->dev, &chip->gpio_chip);
 	if (ret)
 		goto out_failed;
 
-	if (pdata->setup) {
+	if (pdata && pdata->setup) {
 		ret = pdata->setup(client, chip->gpio_chip.base,
 				chip->gpio_chip.ngpio, pdata->context);
 		if (ret < 0)
@@ -255,7 +257,7 @@ static int pca953x_remove(struct i2c_client *client)
 	struct pca953x_chip *chip = i2c_get_clientdata(client);
 	int ret = 0;
 
-	if (pdata->teardown) {
+	if (pdata && pdata->teardown) {
 		ret = pdata->teardown(client, chip->gpio_chip.base,
 				chip->gpio_chip.ngpio, pdata->context);
 		if (ret < 0) {
@@ -265,7 +267,7 @@ static int pca953x_remove(struct i2c_client *client)
 		}
 	}
 
-	ret = gpiochip_remove(&chip->gpio_chip);
+	ret = dev_gpiochip_remove(&client->dev, &chip->gpio_chip);
 	if (ret) {
 		dev_err(&client->dev, "%s failed, %d\n",
 				"gpiochip_remove()", ret);
-- 
1.5.6.3

[PATCH 7/7] i2c/mcu_mpc8349emitx: convert to the new I2C/OF/GPIO infrastructure

[Anton Vorontsov]

With the new subsystem it's much easier to handle I2C GPIO controllers.
Now drivers don't need to deal with the OF-specific bits.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/i2c/chips/mcu_mpc8349emitx.c |   65 ++++++---------------------------
 1 files changed, 12 insertions(+), 53 deletions(-)

diff --git a/drivers/i2c/chips/mcu_mpc8349emitx.c b/drivers/i2c/chips/mcu_mpc8349emitx.c
index 82a9bcb..985a1a4 100644
--- a/drivers/i2c/chips/mcu_mpc8349emitx.c
+++ b/drivers/i2c/chips/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,52 +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->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)
 {
@@ -140,7 +91,15 @@ static int __devinit mcu_probe(struct i2c_client *client,
 		goto err;
 	mcu->reg_ctrl = ret;
 
-	ret = mcu_gpiochip_add(mcu);
+	mcu->gc.owner = THIS_MODULE;
+	mcu->gc.label = client->dev.bus_id;
+	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 = dev_gpiochip_add(&client->dev, &mcu->gc);
 	if (ret)
 		goto err;
 
@@ -167,7 +126,7 @@ static int __devexit mcu_remove(struct i2c_client *client)
 		glob_mcu = NULL;
 	}
 
-	ret = mcu_gpiochip_remove(mcu);
+	ret = dev_gpiochip_remove(&client->dev, &mcu->gc);
 	if (ret)
 		return ret;
 	i2c_set_clientdata(client, NULL);
-- 
1.5.6.3

Re: [PATCH 0/7 RFC] Handle I2C GPIO controllers with the OF (was: pca9539 I2C gpio expander)

[Steven A. Falco]

Anton Vorontsov wrote:
> Hi all,
> 
> Recently there was a question about I2C GPIO controllers and how should
> we handle them with the OpenFirmware and such.
> 
> Here is the attempt to "connect" I2C GPIO controllers to the
> "OpenFirmware" device tree, without writing an OF-specific bindings
> for each driver.
> 
> The salt is in these two patches:
> 
> [PATCH 3/7] of: fill the archdata for I2C devices
> ^ Here we're storing the device tree node into the I2C device.
> 
> [PATCH 5/7] of/gpio: implement of_dev_gpiochip_{add,remove} calls
> ^ And here we extracting the the stored node to put the registered
>   of_gpio_chip into that node.
> 
> 
> How does it look?

I've just tested this with a pca9539 chip attached to my Sequoia board,
and it works perfectly for me.  I don't have the mcu_mpc8349emitx.c file
in my tree, so I was not able to apply part 7 of 7, but the rest looks
fine.  Thanks for doing this.

Acked by: Steve Falco <sfalco@harris.com>