Handle I2C GPIO controllers with the OF (was: pca9539 I2C gpio expander)

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 <[email protected]>

From: Anton Vorontsov <[email protected]>
Date: Thu, 16 Oct 2008 21:12:51 +0400

Quoted 5 lines

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 <[email protected]>

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 <[email protected]>

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

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/i2c/i2c-core.c |    3 +++
 include/linux/i2c.h    |    2 ++
 2 files changed, 5 insertions(+), 0 deletions(-)

drivers/i2c/i2c-core.c +3 -0

index 42e852d..5a485c2 100644
@@ -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;

include/linux/i2c.h +1 -0

index 0611512..3e358d3 100644
@@ -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

Hi Anton,

Quoted 5 lines · Anton Vorontsov

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

Signed-off-by Anton Vorontsov <[email protected]>

---

Quoted 8 lines · Jean Delvare

Hi Anton,

Quoted 5 lines · Anton Vorontsov

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

Signed-off-by Anton Vorontsov <[email protected]>

---

So who's pushing this one ? Jean ?

Quoted 10 lines · Benjamin Herrenschmidt

Quoted 8 lines · Jean Delvare

Hi Anton,

Quoted 5 lines · Anton Vorontsov

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

Signed-off-by Anton Vorontsov <[email protected]>

---

Quoted 1 line

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.)

Quoted 11 lines · Jean Delvare

Quoted 10 lines · Benjamin Herrenschmidt

Quoted 8 lines · Jean Delvare

Hi Anton,

Quoted 5 lines · Anton Vorontsov

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

Signed-off-by Anton Vorontsov <[email protected]>

---

Quoted 8 lines

Quoted 1 line

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 ?

Quoted 21 lines · Benjamin Herrenschmidt

Quoted 19 lines · Jean Delvare

Quoted 11 lines · Benjamin Herrenschmidt

Quoted 8 lines · Jean Delvare

Hi Anton,

Quoted 5 lines · Anton Vorontsov

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

Signed-off-by Anton Vorontsov <[email protected]>

---

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.)

Quoted 2 lines

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.

Quoted 25 lines · Anton Vorontsov

Quoted 23 lines · Benjamin Herrenschmidt

Quoted 11 lines · Jean Delvare

Quoted 10 lines · Benjamin Herrenschmidt

Quoted 8 lines · Jean Delvare

Hi Anton,

Quoted 5 lines · Anton Vorontsov

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

Signed-off-by Anton Vorontsov <[email protected]>

---

Quoted 8 lines

Quoted 1 line

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 ?

Quoted 3 lines

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.

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 <[email protected]>

---
 drivers/of/of_i2c.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

drivers/of/of_i2c.c +11 -0

index 6a98dc8..f9e18ed 100644
@@ -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);

Signature · 1 line

1.5.6.3

include/asm-generic/gpio.h +8 -0

+#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 */

include/asm-generic/gpio.h extern int __must_check gpiochip_reserve(int start, int ngpio); · line 104

Quoted 13 lines · Anton Vorontsov

include/asm-generic/gpio.h +13 -0

+/*
+ * 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}(), [...]

Quoted 17 lines · David Brownell

include/asm-generic/gpio.h extern int __must_check gpiochip_reserve(int start, int ngpio); · line 104

Quoted 13 lines · Anton Vorontsov

include/asm-generic/gpio.h +13 -0

+/*
+ * 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...

Quoted 2 lines

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}().

Quoted 1 line

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...

Quoted 2 lines

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.

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...

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

Quoted 3 lines

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

Why not ?

Quoted 4 lines · Benjamin Herrenschmidt

Quoted 3 lines

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

Quoted 1 line

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.

Quoted 6 lines · Anton Vorontsov

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.

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.

Quoted 4 lines · Benjamin Herrenschmidt

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.

Quoted 6 lines · David Brownell

Quoted 4 lines · Benjamin Herrenschmidt

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.

Quoted 8 lines

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'.

Quoted 16 lines · Anton Vorontsov

Quoted 6 lines · David Brownell

Quoted 4 lines · Benjamin Herrenschmidt

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.

Quoted 8 lines

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.

Quoted 7 lines

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).

Quoted 2 lines

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.

Quoted 3 lines

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 3 lines · Anton Vorontsov

Quoted 3 lines

Quoted 3 lines

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?

Quoted 5 lines

Quoted 2 lines

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.)

Quoted 2 lines

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.

Quoted 7 lines

Quoted 3 lines

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?

Quoted 12 lines · Anton Vorontsov

Quoted 7 lines

Quoted 3 lines

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.

Quoted 2 lines

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..

Quoted 5 lines

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.

Quoted 2 lines

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().

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().

Quoted 2 lines

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.

Quoted 6 lines · David Brownell

Quoted 2 lines

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.. :-)

drivers/gpio/pcf857x.c +0 -0

@@ -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;

Quoted 13 lines

Quoted 8 lines

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)

Quoted 3 lines

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).

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 <[email protected]>

On Wed, Oct 22, 2008 at 02:46:06PM +0400, Anton Vorontsov wrote:
[...]

Quoted 13 lines

Quoted 13 lines

Quoted 8 lines

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)

Quoted 8 lines

Quoted 3 lines

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

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.

+       if (of_gc->chip)
+               return of_gc->chip;
+       return &of_gc->gc;

Quoted 3 lines · Anton Vorontsov

+       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;

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?