diff mbox

[v2] leds: leds-gpio: adopt pinctrl support

Message ID CACRpkdZySZqf-gPoQLGgMZ_B9m+8+Jbr4b6aA+j_OFDCHc2cGg@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Linus Walleij Sept. 10, 2012, 3:23 p.m. UTC
On Sun, Sep 9, 2012 at 1:44 AM, Domenico Andreoli <cavokz@gmail.com> wrote:
> On Fri, Sep 07, 2012 at 11:57:59PM +0200, Linus Walleij wrote:
>>
>> If all you need to to is to multiplex the pins into GPIO mode,
>> then the gpio_get() call on this driver *can* call through to
>> pinctrl_request_gpio() which will in turn fall through to the
>> above pinmux driver calls (.gpio_request_enable, etc).
>
> So if the GPIO driver doesn't coordinate with the pinctrl driver, it's
> all left to the GPIO user to configure the pin before using it, right?

Yes, more or less, or should I say that certain aspects of pinctrl
are orthogonal to GPIO and the two mostly do not know of each
other due to a separation of concerns.

So the driver may need to tie things up and request its pinctrl
and GPIOs independently.

> I can understand the concerns of Tony, whether a pin must be requested
> or not before the gpio then depends on the GPIO driver implementation,
> which may or may not call through the pinctrl layer, isn't it?.

Yes that is a sematic limitation, indeed.

I think the best way of trying to eliminate that is to bring the two
subsystems closer (which is some long-term project) but in the
meantime we could propose a documentation fixup to make the
semantics clear, what about this:

From a92d754367861cf564c09e0b15746e02f0a96f3f Mon Sep 17 00:00:00 2001
From: Linus Walleij <linus.walleij@linaro.org>
Date: Mon, 10 Sep 2012 17:22:00 +0200
Subject: [PATCH] pinctrl: document semantics vs GPIO

The semantics of the interactions between GPIO and pinctrl may be
unclear, e.g. which one do you request first? This amends the
documentation to make this clear.

Reported-by: Domenico Andreoli <cavokz@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/pinctrl.txt | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Stephen Warren Sept. 10, 2012, 5:41 p.m. UTC | #1
On 09/10/2012 09:23 AM, Linus Walleij wrote:
> On Sun, Sep 9, 2012 at 1:44 AM, Domenico Andreoli <cavokz@gmail.com> wrote:
>> On Fri, Sep 07, 2012 at 11:57:59PM +0200, Linus Walleij wrote:
>>>
>>> If all you need to to is to multiplex the pins into GPIO mode,
>>> then the gpio_get() call on this driver *can* call through to
>>> pinctrl_request_gpio() which will in turn fall through to the
>>> above pinmux driver calls (.gpio_request_enable, etc).
>>
>> So if the GPIO driver doesn't coordinate with the pinctrl driver, it's
>> all left to the GPIO user to configure the pin before using it, right?
> 
> Yes, more or less, or should I say that certain aspects of pinctrl
> are orthogonal to GPIO and the two mostly do not know of each
> other due to a separation of concerns.
> 
> So the driver may need to tie things up and request its pinctrl
> and GPIOs independently.

That seems like exactly what we were trying to avoid when we added the
possibility for GPIO to call into pinctrl.

Documentation/gpio.txt already contains:

> For GPIOs that use pins known to the pinctrl subsystem, that subsystem should
> be informed of their use; a gpiolib driver's .request() operation may call
> pinctrl_request_gpio(), and a gpiolib driver's .free() operation may call
> pinctrl_free_gpio(). The pinctrl subsystem allows a pinctrl_request_gpio()
> to succeed concurrently with a pin or pingroup being "owned" by a device for
> pin multiplexing.

In order to resolve this, shouldn't we simply change the "should" at the
end of the first line I quoted to "must"? That way, there'd never be any
need to use pinctrl if you're only relying on gpiolib APIs.

(and I'd argue that the text was already meant to say "must", so this
isn't actually a change to the intent, just a clarification).
Linus Walleij Sept. 10, 2012, 7:34 p.m. UTC | #2
On Mon, Sep 10, 2012 at 7:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/10/2012 09:23 AM, Linus Walleij wrote:

> That seems like exactly what we were trying to avoid when we added the
> possibility for GPIO to call into pinctrl.
>
> Documentation/gpio.txt already contains:
>
>> For GPIOs that use pins known to the pinctrl subsystem, that subsystem should
>> be informed of their use; a gpiolib driver's .request() operation may call
>> pinctrl_request_gpio(), and a gpiolib driver's .free() operation may call
>> pinctrl_free_gpio(). The pinctrl subsystem allows a pinctrl_request_gpio()
>> to succeed concurrently with a pin or pingroup being "owned" by a device for
>> pin multiplexing.
>
> In order to resolve this, shouldn't we simply change the "should" at the
> end of the first line I quoted to "must"? That way, there'd never be any
> need to use pinctrl if you're only relying on gpiolib APIs.
>
> (and I'd argue that the text was already meant to say "must", so this
> isn't actually a change to the intent, just a clarification).

It should deal with all the simple muxing use cases yes. And
I am uncertain about the scope for this patch, if it only pertains
to muxing, and in that case it would be solved by adding
a proper GPIO backend to pinctrl-single.c.

But it doesn't help with some real-world usecases if I'm
not mistaken.

If you want to set up a certain GPIO pin as pull-down (I guess
this could be the case for a LED array), this cannot be done
through any of these functions:

extern int pinctrl_request_gpio(unsigned gpio);
extern void pinctrl_free_gpio(unsigned gpio);
extern int pinctrl_gpio_direction_input(unsigned gpio);
extern int pinctrl_gpio_direction_output(unsigned gpio);

So either we have to use a pin config hog to do this, or we have
to use devm_pinctrl_get_select_default(&pdev->dev); from the
driver (as this patch does). Either way it is using the pinctrl
system orthogonally to the GPIO system, it doesn't happen
from pinctrl_request_gpio() or so.

An alternative solution would be to add functions for
controlling pinconfig and whatnot to the GPIO glue, which
in turn would require adding frontends all over <linux/gpio.h>
which in turn was the thing that Grant nixed to I got
started with pinctrl instead...

But I'm open to any other suggestions. Would it be possible
for pinctrl_request_gpio() to activate a pin config in the
map for example? Currently it can only do muxing.

It's also possible to have the driver do something custom
behind the back of pinctrl altogether as a response to
pinctrl_request_gpio() but it wouldn't be
any elegant...

Yours,
Linus Walleij
Stephen Warren Sept. 10, 2012, 7:44 p.m. UTC | #3
On 09/10/2012 01:34 PM, Linus Walleij wrote:
> On Mon, Sep 10, 2012 at 7:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 09/10/2012 09:23 AM, Linus Walleij wrote:
> 
>> That seems like exactly what we were trying to avoid when we added the
>> possibility for GPIO to call into pinctrl.
>>
>> Documentation/gpio.txt already contains:
>>
>>> For GPIOs that use pins known to the pinctrl subsystem, that subsystem should
>>> be informed of their use; a gpiolib driver's .request() operation may call
>>> pinctrl_request_gpio(), and a gpiolib driver's .free() operation may call
>>> pinctrl_free_gpio(). The pinctrl subsystem allows a pinctrl_request_gpio()
>>> to succeed concurrently with a pin or pingroup being "owned" by a device for
>>> pin multiplexing.
>>
>> In order to resolve this, shouldn't we simply change the "should" at the
>> end of the first line I quoted to "must"? That way, there'd never be any
>> need to use pinctrl if you're only relying on gpiolib APIs.
>>
>> (and I'd argue that the text was already meant to say "must", so this
>> isn't actually a change to the intent, just a clarification).
> 
> It should deal with all the simple muxing use cases yes. And
> I am uncertain about the scope for this patch, if it only pertains
> to muxing, and in that case it would be solved by adding
> a proper GPIO backend to pinctrl-single.c.
> 
> But it doesn't help with some real-world usecases if I'm
> not mistaken.
> 
> If you want to set up a certain GPIO pin as pull-down (I guess
> this could be the case for a LED array), this cannot be done
> through any of these functions:
> 
> extern int pinctrl_request_gpio(unsigned gpio);
> extern void pinctrl_free_gpio(unsigned gpio);
> extern int pinctrl_gpio_direction_input(unsigned gpio);
> extern int pinctrl_gpio_direction_output(unsigned gpio);
> 
> So either we have to use a pin config hog to do this,

I'd certainly expect that to be the common case; I'd imagine it's pretty
common you'd never want to change the pulls at runtime, so hogging would
be appropriate.

> or we have
> to use devm_pinctrl_get_select_default(&pdev->dev); from the
> driver (as this patch does).

Yes, true.

> Either way it is using the pinctrl
> system orthogonally to the GPIO system, it doesn't happen
> from pinctrl_request_gpio() or so.
> 
> An alternative solution would be to add functions for
> controlling pinconfig and whatnot to the GPIO glue, which
> in turn would require adding frontends all over <linux/gpio.h>
> which in turn was the thing that Grant nixed to I got
> started with pinctrl instead...

Maybe the first gpio_request that GPIO passes to pinctrl could activate
some default "gpio" state or similar? But then you'd get into issues
with: what if the driver selects a pinctrl state for other reasons -
then you'd end up wanting multiple states active at once; the
gpiolib-requested state and the driver-requested state, and maybe they
conflict, ... probably madness ensues!

> But I'm open to any other suggestions. Would it be possible
> for pinctrl_request_gpio() to activate a pin config in the
> map for example? Currently it can only do muxing.
> 
> It's also possible to have the driver do something custom
> behind the back of pinctrl altogether as a response to
> pinctrl_request_gpio() but it wouldn't be
> any elegant...
diff mbox

Patch

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 1479aca..941e783 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -289,6 +289,29 @@  Interaction with the GPIO subsystem
 The GPIO drivers may want to perform operations of various types on the same
 physical pins that are also registered as pin controller pins.

+First and foremost, the two subsystems can be used as completely orthogonal,
+so say that your driver is fetching its resources like this:
+
+#include <linux/pinctrl/consumer.h>
+#include <linux/gpio.h>
+
+struct pinctrl *pinctrl;
+int gpio;
+
+pinctrl = devm_pinctrl_get_select_default(&dev);
+gpio = devm_gpio_request(&dev, 14, "foo");
+
+Here we first request a certain pin state and then request GPIO 14 to be
+used. If you're using the subsystems orthogonally like this, always get
+your pinctrl handle and select the desired pinctrl state BEFORE requesting
+the GPIO. This is a semantic convention to avoid situations that can be
+electrically unpleasant, you may certainly want to mux in and bias pins
+a certain way before the GPIO subsystems starts to deal with them.
+
+But there are also situations where it makes sense for the GPIO subsystem
+to communicate with with the pinctrl subsystem, using the latter as a
+back-end.
+
 Since the pin controller subsystem have its pinspace local to the pin
 controller we need a mapping so that the pin control subsystem can figure out
 which pin controller handles control of a certain GPIO pin. Since a single
@@ -359,6 +382,7 @@  will get an pin number into its handled number
range. Further it is also passed
 the range ID value, so that the pin controller knows which range it should
 deal with.

+
 PINMUX interfaces
 =================