From: Linus Walleij <[email protected]>
This is a modification of the previous generic GPIO configuration
patch series.
I'm Cc:ing everyone involved in earlier discussions so we can
move forward on this consolidation work, lest I cannot convert
my drivers to use struct gpio_chip / gpiolib.
Background: Grant didn't like the idea of an ioctl() like
configuration function in the struct gpio_chip vtable, so we
decided to see if it was wiser to go back to the initial suggestion
of making it possible for drivers to dereference the struct
gpio_chip and perform driver-local operations via regular
function calls instead.
So in this patch set I expose gpio_to_chip(), then alter the
previous rewrite of the U300 GPIO driver to instead use a local
configuration function by wrapping the previously defined config
function into this:
/* External function to configure pins */
int u300_gpio_set_config(unsigned gpio, u16 param, unsigned long *data)
{
struct gpio_chip *chip = gpio_to_chip(gpio);
unsigned offset = gpio - chip->base;
return u300_gpio_config(chip, offset, param, data);
}
This one is then exposed in the chip-specific header in
<linux/gpio/u300.h>, and all the configuration defines that
were previously globally generic in <linux/gpio.h> are also
moved there and made driver-specific without any attempt of
generalizing this.
So for anything that is not currently in struct gpio_chip you
will have to implement something like this, per driver.
I'm open for keeping the generic config defines in <linux/gpio.h>
if Grant accepts that, it certainly will avoid some define
duplication in driver headers. Right now I stay away from the
gpiolib as much as I can.
Comments welcome!
Thanks,
Linus Walleij
Linus Walleij (2):
gpio: expose gpio_to_chip()
gpio/u300: rewrite gpio driver
arch/arm/Kconfig | 2 +-
arch/arm/mach-u300/Kconfig | 1 +
arch/arm/mach-u300/core.c | 31 +-
arch/arm/mach-u300/include/mach/gpio.h | 163 +-----
arch/arm/mach-u300/include/mach/irqs.h | 25 +-
drivers/gpio/Kconfig | 9 +
drivers/gpio/gpio-u300.c | 1176 +++++++++++++++++++-------------
drivers/gpio/gpiolib.c | 2 +-
include/asm-generic/gpio.h | 1 +
include/linux/gpio/u300.h | 82 +++
10 files changed, 833 insertions(+), 659 deletions(-)
create mode 100644 include/linux/gpio/u300.h
--
1.7.3.2
2011/6/10 Linus Walleij <[email protected]>:
> in this patch set I expose gpio_to_chip(), then alter the
> previous rewrite of the U300 GPIO driver to instead use a local
> configuration function (...)
Any feedback on this?
Good? Bad? Linus (W) is an idiot?
Any third path to doing custom config?
Thanks,
Linus Walleij
On Fri, Jun 10, 2011 at 10:48 AM, Linus Walleij
<[email protected]> wrote:
> From: Linus Walleij <[email protected]>
>
> This is a modification of the previous generic GPIO configuration
> patch series.
>
> I'm Cc:ing everyone involved in earlier discussions so we can
> move forward on this consolidation work, lest I cannot convert
> my drivers to use struct gpio_chip / gpiolib.
>
> Background: Grant didn't like the idea of an ioctl() like
> configuration function in the struct gpio_chip vtable, so we
> decided to see if it was wiser to go back to the initial suggestion
> of making it possible for drivers to dereference the struct
> gpio_chip and perform driver-local operations via regular
> function calls instead.
>
I couldn't find Grant's rationale in an e-mail thread somewhere, but
except from the few comments I passed on, I liked the approach.
> So in this patch set I expose gpio_to_chip(), then alter the
> previous rewrite of the U300 GPIO driver to instead use a local
> configuration function by wrapping the previously defined config
> function into this:
>
I rather dislike exposing the gpio_to_chip. It makes implementations
work around gpiolib completely. We might as well strip it out
completely then and go back to drivers doing platform specific GPIO
register accesses.
I have a patch lying around somewhere which introduces the concept of
gpio groups. This is already a step up from the single gpio-pin access
and will duplicate every effort to do things like the configuration below.
It already duplicates most of the calls for multiple pins...
> /* External function to configure pins */
> int u300_gpio_set_config(unsigned gpio, u16 param, unsigned long *data)
> {
> ? ? ? ?struct gpio_chip *chip = gpio_to_chip(gpio);
> ? ? ? ?unsigned offset = gpio - chip->base;
>
> ? ? ? ?return u300_gpio_config(chip, offset, param, data);
> }
>
> This one is then exposed in the chip-specific header ?in
> <linux/gpio/u300.h>, and all the configuration defines that
> were previously globally generic in <linux/gpio.h> are also
> moved there and made driver-specific without any attempt of
> generalizing this.
>
How about a SPI flash that has its chip select hooked up to a
GPIO that requires setting open-drain for example. Now that
SPI-driver needs to be aware of each independent gpio-chip
implementation.
Stijn
On Mon, Jun 27, 2011 at 12:57 PM, Stijn Devriendt <[email protected]> wrote:
> On Fri, Jun 10, 2011 at 10:48 AM, Linus Walleij
> <[email protected]> wrote:
>>
>> Background: Grant didn't like the idea of an ioctl() like
>> configuration function in the struct gpio_chip vtable, so we
>> decided to see if it was wiser to go back to the initial suggestion
>> of making it possible for drivers to dereference the struct
>> gpio_chip and perform driver-local operations via regular
>> function calls instead.
>>
> I couldn't find Grant's rationale in an e-mail thread somewhere, but
> except from the few comments I passed on, I liked the approach.
Grant gave me these comments in person. Grant, maybe you
can restate? I might be referring it the wrong way.
> I rather dislike exposing the gpio_to_chip. It makes implementations
> work around gpiolib completely. We might as well strip it out
> completely then and go back to drivers doing platform specific GPIO
> register accesses.
Myself I am pretty neutral and happy with any of these two
approaches, I just want to be able to migrate my U300 and Nomadik
drivers to gpio_chip and irq_chip, and without a handle on the memory
map I cannot do that.
>> This one is then exposed in the chip-specific header ?in
>> <linux/gpio/u300.h>, and all the configuration defines that
>> were previously globally generic in <linux/gpio.h> are also
>> moved there and made driver-specific without any attempt of
>> generalizing this.
>>
> How about a SPI flash that has its chip select hooked up to a
> GPIO that requires setting open-drain for example. Now that
> SPI-driver needs to be aware of each independent gpio-chip
> implementation.
Yes. To make the driver platform neutral, it needs to for example
provide a callback in the platform data like (* set_pin_bias) or so,
and then your platform has to implement this biasing.
In this specific case that kind of stuff would likely be preferable
to have in the platform anyway, but I understand what you mean.
In the pinctrl framework I try to handle all such pin control stuff
with the intention of letting the GPIO drivers wrap it if they
prefer, but in this framework platforms could also handle GPIO
and pin control in an orthogonal way, which would likely be
preferable in most cases.
Linus Walleij
On Mon, Jun 27, 2011 at 01:44:43PM +0200, Linus Walleij wrote:
> Yes. To make the driver platform neutral, it needs to for example
> provide a callback in the platform data like (* set_pin_bias) or so,
> and then your platform has to implement this biasing.
> In this specific case that kind of stuff would likely be preferable
> to have in the platform anyway, but I understand what you mean.
How about device tree usage? I guess there we'd end up doing it by
putting the configuration on the GPIO end of things rather than on the
GPIO user side?
On Mon, Jun 27, 2011 at 2:02 PM, Mark Brown
<[email protected]> wrote:
> On Mon, Jun 27, 2011 at 01:44:43PM +0200, Linus Walleij wrote:
>
>> Yes. To make the driver platform neutral, it needs to for example
>> provide a callback in the platform data like (* set_pin_bias) or so,
>> and then your platform has to implement this biasing.
>
>> In this specific case that kind of stuff would likely be preferable
>> to have in the platform anyway, but I understand what you mean.
>
> How about device tree usage? ?I guess there we'd end up doing it by
> putting the configuration on the GPIO end of things rather than on the
> GPIO user side?
Sorry I can't quite understand that, please elaborate!
Thanks,
Linus Walleij
On Mon, Jun 27, 2011 at 2:37 PM, Linus Walleij <[email protected]> wrote:
> On Mon, Jun 27, 2011 at 2:02 PM, Mark Brown
> <[email protected]> wrote:
>> On Mon, Jun 27, 2011 at 01:44:43PM +0200, Linus Walleij wrote:
>>
>>> Yes. To make the driver platform neutral, it needs to for example
>>> provide a callback in the platform data like (* set_pin_bias) or so,
>>> and then your platform has to implement this biasing.
>>
>>> In this specific case that kind of stuff would likely be preferable
>>> to have in the platform anyway, but I understand what you mean.
>>
>> How about device tree usage? ?I guess there we'd end up doing it by
>> putting the configuration on the GPIO end of things rather than on the
>> GPIO user side?
>
> Sorry I can't quite understand that, please elaborate!
>
I have some code doing this as well (in a very limited fashion):
int of_request_gpio(..., int* remaining_flags)
{
of_get_gpio_flags(of_dev, i, remaining_flags)
if (flags & bias_X) {
gpio_set_bias(gpio, ...)
flags &= ~bias_X
}
// interpret all generic flags here
};
So drivers need not worry about all gpio flags and special things.
They just request the pin; what they receive is a fully configured
pin (with the exception of unknown flags passed out via
remaining_flags).
Regards,
Stijn
2011/6/28 Stijn Devriendt <[email protected]>:
> On Mon, Jun 27, 2011 at 2:37 PM, Linus Walleij <[email protected]> wrote:
>> On Mon, Jun 27, 2011 at 2:02 PM, Mark Brown
>>> How about device tree usage? ?I guess there we'd end up doing it by
>>> putting the configuration on the GPIO end of things rather than on the
>>> GPIO user side?
>>
>> Sorry I can't quite understand that, please elaborate!
>>
>
> I have some code doing this as well (in a very limited fashion):
>
> int of_request_gpio(..., int* remaining_flags)
> {
> ?of_get_gpio_flags(of_dev, i, remaining_flags)
> ?if (flags & bias_X) {
> ? ? gpio_set_bias(gpio, ...)
> ? ? flags &= ~bias_X
> ?}
> ?// interpret all generic flags here
> };
>
> So drivers need not worry about all gpio flags and special things.
> They just request the pin; what they receive is a fully configured
> pin (with the exception of unknown flags passed out via
> remaining_flags).
Sorry there is something I don't get here or there is something you
assume, maybe.
How does this kind of design account for the case where you need
to change the biasing at runtime?
Thanks,
Linus Walleij
On Wed, Jun 29, 2011 at 8:18 AM, Linus Walleij <[email protected]> wrote:
> 2011/6/28 Stijn Devriendt <[email protected]>:
>> On Mon, Jun 27, 2011 at 2:37 PM, Linus Walleij <[email protected]> wrote:
>>> On Mon, Jun 27, 2011 at 2:02 PM, Mark Brown
>>>> How about device tree usage? ?I guess there we'd end up doing it by
>>>> putting the configuration on the GPIO end of things rather than on the
>>>> GPIO user side?
>>>
>>> Sorry I can't quite understand that, please elaborate!
>>>
>>
>> I have some code doing this as well (in a very limited fashion):
>>
>> int of_request_gpio(..., int* remaining_flags)
>> {
>> ?of_get_gpio_flags(of_dev, i, remaining_flags)
>> ?if (flags & bias_X) {
>> ? ? gpio_set_bias(gpio, ...)
>> ? ? flags &= ~bias_X
>> ?}
>> ?// interpret all generic flags here
>> };
>>
>> So drivers need not worry about all gpio flags and special things.
>> They just request the pin; what they receive is a fully configured
>> pin (with the exception of unknown flags passed out via
>> remaining_flags).
>
> Sorry there is something I don't get here or there is something you
> assume, maybe.
>
> How does this kind of design account for the case where you need
> to change the biasing at runtime?
>
You're right, it doesn't. It only accounts for preconfiguring pins from
device-tree of platform data.
Regards,
Stijn