| Message ID | 20081016171259.GD5515@oksana.dev.rtsoft.ru (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 */ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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,
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
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).
> But it doesn't work as a module (i.e. OF-specific bits should be > always in-kernel). Why not ? Ben.
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.
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.
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.
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.
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.
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.
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'.
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.
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,
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.
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.
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(-)