[PATCH] GPIOLIB: add generic gpio_set_pull API

[PATCH] GPIOLIB: add generic gpio_set_pull API

[Barry Song]

Now there are many different implementations for GPIO pull configuration, for
example:
s3c_gpio_setpull()
tegra_pinmux_set_pullupdown()
chipcHw_setPinPullup()
gpio_pullup()
s3c2410_gpio_pullup()

This patch adds a new generic gpio_set_pull API so that all SoCs can have unified
codes.

Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 drivers/gpio/gpiolib.c     |   17 +++++++++++++++++
 include/asm-generic/gpio.h |    3 +++
 include/linux/gpio.h       |    4 ++++
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a971e3d..a2afa95 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1620,6 +1620,23 @@ int __gpio_to_irq(unsigned gpio)
 }
 EXPORT_SYMBOL_GPL(__gpio_to_irq);
 
+/**
+ * __gpio_set_pull() - set pull up, pull down or no pull for a gpio
+ * @gpio: gpio which will be configurated
+ * @mode: one of GPIO_PULL_NONE, GPIO_PULL_UP and GPIO_PULL_DOWN
+ * Context: any
+ *
+ * This is used directly or indirectly to implement gpio_set_pull().
+ */
+void __gpio_set_pull(unsigned gpio, unsigned mode)
+{
+	struct gpio_chip	*chip;
+
+	chip = gpio_to_chip(gpio);
+
+	chip->pull(chip, gpio, mode);
+}
+EXPORT_SYMBOL_GPL(__gpio_set_pull);
 
 
 /* There's no value in making it easy to inline GPIO calls that may sleep.
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index d494001..f953835 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -112,6 +112,9 @@ struct gpio_chip {
 	int			(*to_irq)(struct gpio_chip *chip,
 						unsigned offset);
 
+	void			(*pull)(struct gpio_chip *chip,
+						unsigned offset, unsigned mode);
+
 	void			(*dbg_show)(struct seq_file *s,
 						struct gpio_chip *chip);
 	int			base;
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index 17b5a0d..ac48166 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -14,6 +14,10 @@
 #define GPIOF_OUT_INIT_LOW	(GPIOF_DIR_OUT | GPIOF_INIT_LOW)
 #define GPIOF_OUT_INIT_HIGH	(GPIOF_DIR_OUT | GPIOF_INIT_HIGH)
 
+#define GPIO_PULL_NONE	0
+#define GPIO_PULL_UP	1
+#define GPIO_PULL_DOWN	2
+
 #ifdef CONFIG_GENERIC_GPIO
 #include <asm/gpio.h>
 
-- 
1.7.1



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Paul Mundt]

On Sun, Aug 07, 2011 at 10:19:33PM -0700, Barry Song wrote:
> Now there are many different implementations for GPIO pull configuration, for
> example:
> s3c_gpio_setpull()
> tegra_pinmux_set_pullupdown()
> chipcHw_setPinPullup()
> gpio_pullup()
> s3c2410_gpio_pullup()
> 
> This patch adds a new generic gpio_set_pull API so that all SoCs can have unified
> codes.
> 
> Signed-off-by: Barry Song <Baohua.Song@csr.com>

For arch/arm/mach-shmobile we also have gpio_pull_up() for board-g4evm.c
and gpio_pull_down() for board-mackerel.c. Both of these would benefit
from this sort of an API addition.

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Grant Likely]

On Mon, Aug 8, 2011 at 2:15 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Sun, Aug 07, 2011 at 10:19:33PM -0700, Barry Song wrote:
>> Now there are many different implementations for GPIO pull configuration, for
>> example:
>> s3c_gpio_setpull()
>> tegra_pinmux_set_pullupdown()
>> chipcHw_setPinPullup()
>> gpio_pullup()
>> s3c2410_gpio_pullup()
>>
>> This patch adds a new generic gpio_set_pull API so that all SoCs can have unified
>> codes.
>>
>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>
> For arch/arm/mach-shmobile we also have gpio_pull_up() for board-g4evm.c
> and gpio_pull_down() for board-mackerel.c. Both of these would benefit
> from this sort of an API addition.

I think I'm okay with this API change.  Linus, what say you?  How does
this interact with your plans for pinctrl?

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Kyungmin Park]

On Tue, Aug 9, 2011 at 3:24 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Aug 8, 2011 at 2:15 AM, Paul Mundt <lethal@linux-sh.org> wrote:
>> On Sun, Aug 07, 2011 at 10:19:33PM -0700, Barry Song wrote:
>>> Now there are many different implementations for GPIO pull configuration, for
>>> example:
>>> s3c_gpio_setpull()
>>> tegra_pinmux_set_pullupdown()
>>> chipcHw_setPinPullup()
>>> gpio_pullup()
>>> s3c2410_gpio_pullup()
>>>
>>> This patch adds a new generic gpio_set_pull API so that all SoCs can have unified
>>> codes.
>>>
>>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>>
>> For arch/arm/mach-shmobile we also have gpio_pull_up() for board-g4evm.c
>> and gpio_pull_down() for board-mackerel.c. Both of these would benefit
>> from this sort of an API addition.
>
> I think I'm okay with this API change.  Linus, what say you?  How does
> this interact with your plans for pinctrl?

If gpiolib accept the pullup control. gpiolib is better place to
control gpio config.
then remains are the gpio driver strength, and power down mode
control. powerdown pull-up/down, powerdown in/out at samsung gpios.

Thank you,
Kyungmin Park

>
> g.
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Rohit Vaswani]

On 8/8/2011 3:57 PM, Kyungmin Park wrote:
> On Tue, Aug 9, 2011 at 3:24 AM, Grant Likely<grant.likely@secretlab.ca>  wrote:
>> On Mon, Aug 8, 2011 at 2:15 AM, Paul Mundt<lethal@linux-sh.org>  wrote:
>>> On Sun, Aug 07, 2011 at 10:19:33PM -0700, Barry Song wrote:
>>>> Now there are many different implementations for GPIO pull configuration, for
>>>> example:
>>>> s3c_gpio_setpull()
>>>> tegra_pinmux_set_pullupdown()
>>>> chipcHw_setPinPullup()
>>>> gpio_pullup()
>>>> s3c2410_gpio_pullup()
>>>>
>>>> This patch adds a new generic gpio_set_pull API so that all SoCs can have unified
>>>> codes.
>>>>
>>>> Signed-off-by: Barry Song<Baohua.Song@csr.com>
>>> For arch/arm/mach-shmobile we also have gpio_pull_up() for board-g4evm.c
>>> and gpio_pull_down() for board-mackerel.c. Both of these would benefit
>>> from this sort of an API addition.
>> I think I'm okay with this API change.  Linus, what say you?  How does
>> this interact with your plans for pinctrl?
> If gpiolib accept the pullup control. gpiolib is better place to
> control gpio config.
> then remains are the gpio driver strength, and power down mode
> control. powerdown pull-up/down, powerdown in/out at samsung gpios.
If we add this API - the remaining gpio controls like drive strength and 
function select could also be added, which eats into the pinmux domain.
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.

Thanks,
Rohit
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> --
> 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/


Thanks,
Rohit Vaswani

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Barry Song]

2011/8/9 Rohit Vaswani <rvaswani@codeaurora.org>:
> On 8/8/2011 3:57 PM, Kyungmin Park wrote:
>>
>> On Tue, Aug 9, 2011 at 3:24 AM, Grant Likely<grant.likely@secretlab.ca>
>>  wrote:
>>>
>>> On Mon, Aug 8, 2011 at 2:15 AM, Paul Mundt<lethal@linux-sh.org>  wrote:
>>>>
>>>> On Sun, Aug 07, 2011 at 10:19:33PM -0700, Barry Song wrote:
>>>>>
>>>>> Now there are many different implementations for GPIO pull
>>>>> configuration, for
>>>>> example:
>>>>> s3c_gpio_setpull()
>>>>> tegra_pinmux_set_pullupdown()
>>>>> chipcHw_setPinPullup()
>>>>> gpio_pullup()
>>>>> s3c2410_gpio_pullup()
>>>>>
>>>>> This patch adds a new generic gpio_set_pull API so that all SoCs can
>>>>> have unified
>>>>> codes.
>>>>>
>>>>> Signed-off-by: Barry Song<Baohua.Song@csr.com>
>>>>
>>>> For arch/arm/mach-shmobile we also have gpio_pull_up() for board-g4evm.c
>>>> and gpio_pull_down() for board-mackerel.c. Both of these would benefit
>>>> from this sort of an API addition.
>>>
>>> I think I'm okay with this API change.  Linus, what say you?  How does
>>> this interact with your plans for pinctrl?
>>
>> If gpiolib accept the pullup control. gpiolib is better place to
>> control gpio config.
>> then remains are the gpio driver strength, and power down mode
>> control. powerdown pull-up/down, powerdown in/out at samsung gpios.
>
> If we add this API - the remaining gpio controls like drive strength and
> function select could also be added, which eats into the pinmux domain.
> 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.

gpio_config() from Linus W. is an interface with multiple functions or
a function for multiple purpose. Its actions seem to be ambiguous,
then mean diffiicult to use or diffirent subsystems still result in
many differences after using it.

pullup/down/none is something very common and much frequently used to
all SoCs, i guess it can have individual, simple and direct API.

I don't oppose gpiolib cover more GPIO configurations, for example OC,
OD and powerdown. but we might keep APIs as simple and clear as
possible.

>
> Thanks,
> Rohit

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Linus Walleij]

On Mon, Aug 8, 2011 at 8:24 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Aug 8, 2011 at 2:15 AM, Paul Mundt <lethal@linux-sh.org> wrote:
>> On Sun, Aug 07, 2011 at 10:19:33PM -0700, Barry Song wrote:
>>> Now there are many different implementations for GPIO pull configuration, for
>>> example:
>>> s3c_gpio_setpull()
>>> tegra_pinmux_set_pullupdown()
>>> chipcHw_setPinPullup()
>>> gpio_pullup()
>>> s3c2410_gpio_pullup()
>>>
>>> This patch adds a new generic gpio_set_pull API so that all SoCs can have unified
>>> codes.
>>>
>>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>>
>> For arch/arm/mach-shmobile we also have gpio_pull_up() for board-g4evm.c
>> and gpio_pull_down() for board-mackerel.c. Both of these would benefit
>> from this sort of an API addition.
>
> I think I'm okay with this API change.  Linus, what say you?  How does
> this interact with your plans for pinctrl?

I have already proposed a similar mechanism in the past, so let's
recap:

1.First I proposed to expose gpio_to_chip() so each driver could
  provide any custom functions using foo_gpio_set_foo() or so
  by dereferencing the struct gpio_chip.
  NIXED: due to bad experience with doing exactly this for
  IRQ chips (Grant)

2. Second I added a sort-of generic control function;
   gpio_config() which would sit in gpiolib and take an enum
   for each operation, if one of these would be
   GPIO_SET_PULL, and a second argument would be
   whether to pull it up or down or open collector or
   open drain or schmitt-trigger or whatever. As you see
   the problem is not limited to up/down.
   This is equal to the proposed patch but with two
   arguments and broader scope, can also be used
   for drive strength etc.

3. Alan Cox suggested that we use a more generic
   control function instead so I wrapped it to an ioctl()-
   like operation with an opaque argument instead.
   This is especially good when you need to pass
   data *out* of the function, not just *in*.

4. After talks with Grant I submitted (1) again.

I'm basically happy with anything as long as there is some
progress, right now we're only bikeshedding, so I'm, resting
my case.

Linus Walleij

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Linus Walleij]

On Tue, Aug 9, 2011 at 12:57 AM, Kyungmin Park <kmpark@infradead.org> wrote:

> If gpiolib accept the pullup control. gpiolib is better place to
> control gpio config.
> then remains are the gpio driver strength, and power down mode
> control. powerdown pull-up/down, powerdown in/out at samsung gpios.

I agree. And this is what I need to move the U300 and Nomadik
GPIO drivers over to struct gpio_chip.

Yours,
Linus Walleij

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Linus Walleij]

On Tue, Aug 9, 2011 at 3:45 AM, Rohit Vaswani <rvaswani@codeaurora.org> 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

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Barry Song]

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 <linus.walleij@linaro.org>
>
> On Tue, Aug 9, 2011 at 3:45 AM, Rohit Vaswani <rvaswani@codeaurora.org>
> 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

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Linus Walleij]

On Tue, Jun 19, 2012 at 5:25 AM, Barry Song <21cnbao@gmail.com> 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

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Barry Song]

Hi Linus,

2012/6/20 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Jun 19, 2012 at 5:25 AM, Barry Song <21cnbao@gmail.com> 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

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Paul Mundt]

On Wed, Jun 20, 2012 at 10:15:01AM +0200, Linus Walleij wrote:
> On Tue, Jun 19, 2012 at 5:25 AM, Barry Song <21cnbao@gmail.com> 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?

Re: [PATCH] GPIOLIB: add generic gpio_set_pull API

[Linus Walleij]

On Wed, Jun 20, 2012 at 4:31 PM, Paul Mundt <lethal@linux-sh.org> 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 <21cnbao@gmail.com> 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