2012-06-19 03:26:06

by Barry Song

[permalink] [raw]
Subject: Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

Hi Linus,
it seems people still use self-defined structure and APIs to set GPIO
pull, for example:

drivers/pinctrl/pinctrl-nomadik.c:

48 /* Pull up/down values */
49 enum nmk_gpio_pull {
50 NMK_GPIO_PULL_NONE,
51 NMK_GPIO_PULL_UP,
52 NMK_GPIO_PULL_DOWN,
53 };

int nmk_gpio_set_pull(int gpio, enum nmk_gpio_pull pull)

or actually you mean use "pin_config_get" and "pin_config_set" with
self-defined configuration to set pull?

but i think at least the macros of GPIO_PULL_NONE, GPIO_PULL_UP and
GPIO_PULL_DOWN should be standardized.

2011/8/9 Linus Walleij <[email protected]>
>
> On Tue, Aug 9, 2011 at 3:45 AM, Rohit Vaswani <[email protected]>
> wrote:
>
> > If we add this API - the remaining gpio controls like drive strength and
> > function select could also be added,
>
> I agree.
>
> > which eats into the pinmux domain.
>
> That's not so bad, since the pinctrl/pinmux subsystem is just a prototype
> people may want to wrap up their drivers into gpio_chip/gpiolib as
> they stand today to atleast get some isolation. Later on they can
> refactor and migrate to a pinctrl/pinmux subsystem.
>
> The latter will take some time to provide anyway, since I have been
> asked to restructure it so as not to use a global pin number space.
>
> > Linus W. had a patch earlier which added an API for a gpio config to be
> > specified through gpiolib. " gpio: add a custom configuration mechanism
> > to
> > gpiolib" which is sort of an extensible model of this API.
>
> Yes I think I have already suggested a bunch of ways to skin this
> cat but somehow none of them seem to win general approval.
>
> Yours,
> Linus Walleij

-barry


2012-06-20 08:15:09

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

On Tue, Jun 19, 2012 at 5:25 AM, Barry Song <[email protected]> wrote:

> it seems people still use self-defined structure and APIs to set GPIO
> pull,

Yes. They came into existance before the pinctrl subsystem was invented.

> int nmk_gpio_set_pull(int gpio, enum nmk_gpio_pull pull)

Yes, that is one example...

> or actually you mean use "pin_config_get" and "pin_config_set" with
> self-defined configuration to set pull?

I don't quite undertand this part of question, can you elaborate?

The idea is to move this driver over to using the pinctrl API and
delete these functions (or atleast make them static so they are only
used inside the driver itself).

It's taking time since legacy code needs to be handled carefully
and tested on various hardware, sorry for that, but I'm onto it.

> but i think at least the macros of GPIO_PULL_NONE, GPIO_PULL_UP and
> GPIO_PULL_DOWN should be standardized.

You find an attempt at standardization in drivers/pinctrl/pinconf-generic.c
which is also used by the composite U300+COH901 drivers. Drivers
can select this support library and use the flags from
<linux/pinctrl/pinconf-generic.h>

The reason that it's not mandated and some modern SoCs use their
own custom definitions was that it was impossible at the time to
reach consensus (review the mailing list for the discussion, but
mainly it was Mark Brow's arguments that made me give up the
general approach).

So currently this support lib is available if your pin configs are
simple, if they are complex you can cook your own (like many
drivers do).

Yours,
Linus Walleij

2012-06-20 10:08:03

by Barry Song

[permalink] [raw]
Subject: Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

Hi Linus,

2012/6/20 Linus Walleij <[email protected]>:
> On Tue, Jun 19, 2012 at 5:25 AM, Barry Song <[email protected]> wrote:
>
>> it seems people still use self-defined structure and APIs to set GPIO
>> pull,
>
> Yes. They came into existance before the pinctrl subsystem was invented.
>
>> int nmk_gpio_set_pull(int gpio, enum nmk_gpio_pull pull)
>
> Yes, that is one example...
>
>> or actually you mean use "pin_config_get" and "pin_config_set" with
>> self-defined configuration to set pull?
>
> I don't quite undertand this part of question, can you elaborate?
>

i just want to make sure whether these two functions are provided to
support for users to set pin config dynamically.
for example, one driver might get GPIO number from DT and set
PIN_CONFIG_BIAS_xxx by pin_config_set() after that.
until now, no real driver has called pin_config_set(), so i am not sure....

> The idea is to move this driver over to using the pinctrl API and
> delete these functions (or atleast make them static so they are only
> used inside the driver itself).
>
> It's taking time since legacy code needs to be handled carefully
> and tested on various hardware, sorry for that, but I'm onto it.
>
>> but i think at least the macros of GPIO_PULL_NONE, GPIO_PULL_UP and
>> GPIO_PULL_DOWN should be standardized.
>
> You find an attempt at standardization in drivers/pinctrl/pinconf-generic.c
> which is also used by the composite U300+COH901 drivers. Drivers
> can select this support library and use the flags from
> <linux/pinctrl/pinconf-generic.h>
>
> The reason that it's not mandated and some modern SoCs use their
> own custom definitions was that it was impossible at the time to
> reach consensus (review the mailing list for the discussion, but
> mainly it was Mark Brow's arguments that made me give up the
> general approach).

Thanks for update. i am sorry recently i have missed many discussions ~^。^~
i guess the generic pin_config_param should be able to cover most cases.

>
> So currently this support lib is available if your pin configs are
> simple, if they are complex you can cook your own (like many
> drivers do).
>
> Yours,
> Linus Walleij

-barry

2012-06-20 14:33:18

by Paul Mundt

[permalink] [raw]
Subject: Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

On Wed, Jun 20, 2012 at 10:15:01AM +0200, Linus Walleij wrote:
> On Tue, Jun 19, 2012 at 5:25 AM, Barry Song <[email protected]> wrote:
> > but i think at least the macros of GPIO_PULL_NONE, GPIO_PULL_UP and
> > GPIO_PULL_DOWN should be standardized.
>
> You find an attempt at standardization in drivers/pinctrl/pinconf-generic.c
> which is also used by the composite U300+COH901 drivers. Drivers
> can select this support library and use the flags from
> <linux/pinctrl/pinconf-generic.h>
>
What's with PIN_CONFIG_END using such an insane value?

I'm happy to consolidate on the provided definitions and add my own on top
of that, but I'm not going to waste an insane amount of bits that I could
be using for driver-specific data and so on instead to comply with the
PIN_CONFIG_END comment. Is there any valid reason why this would ever
need to exceed 4 bits?

2012-06-21 07:49:03

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

On Wed, Jun 20, 2012 at 4:31 PM, Paul Mundt <[email protected]> wrote:
> On Wed, Jun 20, 2012 at 10:15:01AM +0200, Linus Walleij wrote:
>> On Tue, Jun 19, 2012 at 5:25 AM, Barry Song <[email protected]> wrote:
>> > but i think at least the macros of GPIO_PULL_NONE, GPIO_PULL_UP and
>> > GPIO_PULL_DOWN should be standardized.
>>
>> You find an attempt at standardization in drivers/pinctrl/pinconf-generic.c
>> which is also used by the composite U300+COH901 drivers. Drivers
>> can select this support library and use the flags from
>> <linux/pinctrl/pinconf-generic.h>
>>
> What's with PIN_CONFIG_END using such an insane value?

This: PIN_CONFIG_END = 0x7FFF

It's just that I reserved 16 bits for the different stuff, and then
the bitstuffing
functions below use 16 bits for this. If it's too much we can shrink
it.

> I'm happy to consolidate on the provided definitions and add my own on top
> of that, but I'm not going to waste an insane amount of bits that I could
> be using for driver-specific data and so on instead to comply with the
> PIN_CONFIG_END comment. Is there any valid reason why this would ever
> need to exceed 4 bits?

I think we need more than 4 bits for sure, but not more than 8.

If you want to patch it down to have 8 bits for the param and 24 bits
for the argument, go ahead, it won't hurt.

Yours,
Linus Walleij