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

login
register
mail settings
Submitter Anton Vorontsov
Date Oct. 16, 2008, 5:12 p.m.
Message ID <20081016171259.GD5515@oksana.dev.rtsoft.ru>
Download mbox | patch
Permalink /patch/4724/
State Not Applicable, archived
Headers show

Comments

Anton Vorontsov - Oct. 16, 2008, 5:12 p.m.
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(-)
David Brownell - Oct. 17, 2008, 8:24 p.m.
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/
Anton Vorontsov - Oct. 17, 2008, 9:29 p.m.
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,
David Brownell - Oct. 20, 2008, 7:29 a.m.
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
Anton Vorontsov - Oct. 20, 2008, 3:48 p.m.
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).
Benjamin Herrenschmidt - Oct. 22, 2008, 12:29 a.m.
> But it doesn't work as a module (i.e. OF-specific bits should be
> always in-kernel).

Why not ?

Ben.
Anton Vorontsov - Oct. 22, 2008, 1:03 a.m.
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 - Oct. 22, 2008, 1:42 a.m.
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.
Benjamin Herrenschmidt - Oct. 22, 2008, 2:27 a.m.
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.
Benjamin Herrenschmidt - Oct. 22, 2008, 2:28 a.m.
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.
Grant Likely - Oct. 22, 2008, 4:20 a.m.
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.
David Brownell - Oct. 22, 2008, 4:22 a.m.
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.
Anton Vorontsov - Oct. 22, 2008, 10:36 a.m.
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 - Oct. 22, 2008, 10:46 a.m.
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 - Oct. 28, 2008, 5:45 p.m.
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,
Grant Likely - Oct. 28, 2008, 5:53 p.m.
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.

Patch

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.