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

On 09/10/2012 01:34 PM, Linus Walleij wrote:

On Mon, Sep 10, 2012 at 7:41 PM, Stephen Warren [off-list ref] wrote:

quoted

On 09/10/2012 09:23 AM, Linus Walleij wrote:

quoted

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:

quoted

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...