[PATCH] Input: gpio_keys: allocate pins

[PATCH] Input: gpio_keys: allocate pins

[Daniel Mack]

This allows DT driven boards to allocate and configure the pinmux once
the driver is probed.

Signed-off-by: Daniel Mack <zonque@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/gpio_keys.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 6a68041..53dff7d 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -29,6 +29,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_gpio.h>
 #include <linux/spinlock.h>
+#include <linux/pinctrl/consumer.h>
 
 struct gpio_button_data {
 	const struct gpio_keys_button *button;
@@ -664,6 +665,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 	const struct gpio_keys_platform_data *pdata = dev_get_platdata(dev);
 	struct gpio_keys_drvdata *ddata;
 	struct input_dev *input;
+	struct pinctrl *pinctrl;
 	int i, error;
 	int wakeup = 0;
 
@@ -701,6 +703,11 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 	input->id.product = 0x0001;
 	input->id.version = 0x0100;
 
+	/* request pin mux */
+	pinctrl = devm_pinctrl_get_select_default(dev);
+	if (IS_ERR(pinctrl))
+		dev_warn(dev, "pins are not configured from the driver\n");
+
 	/* Enable auto repeat feature of Linux input subsystem */
 	if (pdata->rep)
 		__set_bit(EV_REP, input->evbit);
-- 
1.7.11.4

Re: [PATCH] Input: gpio_keys: allocate pins

[Linus Walleij]

On Fri, Oct 12, 2012 at 5:55 PM, Daniel Mack <zonque@gmail.com> wrote:

> This allows DT driven boards to allocate and configure the pinmux once
> the driver is probed.
>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>

(...)
> +       /* request pin mux */
> +       pinctrl = devm_pinctrl_get_select_default(dev);
> +       if (IS_ERR(pinctrl))
> +               dev_warn(dev, "pins are not configured from the driver\n");

I think dev_warn() is rather nasty to throw in here, dev_info() is OK.

However I suspect this driver could actually handle default, idle and sleep
states, especially after the runtime PM patches discussed elsewhere,
but that can be patched later.

Yours,
Linus Walleij

Re: [PATCH] Input: gpio_keys: allocate pins

[Daniel Mack]

On 12.10.2012 23:26, Linus Walleij wrote:
> On Fri, Oct 12, 2012 at 5:55 PM, Daniel Mack <zonque@gmail.com> wrote:
> 
>> This allows DT driven boards to allocate and configure the pinmux once
>> the driver is probed.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> (...)
>> +       /* request pin mux */
>> +       pinctrl = devm_pinctrl_get_select_default(dev);
>> +       if (IS_ERR(pinctrl))
>> +               dev_warn(dev, "pins are not configured from the driver\n");
> 
> I think dev_warn() is rather nasty to throw in here, dev_info() is OK.

Well, dev_warn is used everywhere else for this particular warning, but
I can change that of course.

> However I suspect this driver could actually handle default, idle and sleep
> states, especially after the runtime PM patches discussed elsewhere,
> but that can be patched later.

Ok.


Thanks,
Daniel

Re: [PATCH] Input: gpio_keys: allocate pins

[Linus Walleij]

On Fri, Oct 12, 2012 at 11:27 PM, Daniel Mack <zonque@gmail.com> wrote:
> On 12.10.2012 23:26, Linus Walleij wrote:
>> On Fri, Oct 12, 2012 at 5:55 PM, Daniel Mack <zonque@gmail.com> wrote:
>>
>>> This allows DT driven boards to allocate and configure the pinmux once
>>> the driver is probed.
>>>
>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> (...)
>>> +       /* request pin mux */
>>> +       pinctrl = devm_pinctrl_get_select_default(dev);
>>> +       if (IS_ERR(pinctrl))
>>> +               dev_warn(dev, "pins are not configured from the driver\n");
>>
>> I think dev_warn() is rather nasty to throw in here, dev_info() is OK.
>
> Well, dev_warn is used everywhere else for this particular warning, but
> I can change that of course.

Yeah true ... just that I think that in this case most platforms will
rely on the fall through mechanism where the gpio_get()
and set_direction will fall through to the special-purpose functions
in the gpio driver calling out to pinctrl. So it's actually not an error.

This pinctrl handle is only intended for biasing pins etc, right?

NO muxing!

Because we wrote in Documentation/pinctrl.txt that if GPIO
and pin control handle the same lines, they should be
implemented in the gpio driver by calling out to pinctrl's
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);

Yours,
Linus Walleij

Re: [PATCH] Input: gpio_keys: allocate pins

[Daniel Mack]

On 12.10.2012 23:37, Linus Walleij wrote:
> On Fri, Oct 12, 2012 at 11:27 PM, Daniel Mack <zonque@gmail.com> wrote:
>> On 12.10.2012 23:26, Linus Walleij wrote:
>>> On Fri, Oct 12, 2012 at 5:55 PM, Daniel Mack <zonque@gmail.com> wrote:
>>>
>>>> This allows DT driven boards to allocate and configure the pinmux once
>>>> the driver is probed.
>>>>
>>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>
>>> (...)
>>>> +       /* request pin mux */
>>>> +       pinctrl = devm_pinctrl_get_select_default(dev);
>>>> +       if (IS_ERR(pinctrl))
>>>> +               dev_warn(dev, "pins are not configured from the driver\n");
>>>
>>> I think dev_warn() is rather nasty to throw in here, dev_info() is OK.
>>
>> Well, dev_warn is used everywhere else for this particular warning, but
>> I can change that of course.
> 
> Yeah true ... just that I think that in this case most platforms will
> rely on the fall through mechanism where the gpio_get()
> and set_direction will fall through to the special-purpose functions
> in the gpio driver calling out to pinctrl. So it's actually not an error.
> 
> This pinctrl handle is only intended for biasing pins etc, right?
> 
> NO muxing!
> 
> Because we wrote in Documentation/pinctrl.txt that if GPIO
> and pin control handle the same lines, they should be
> implemented in the gpio driver by calling out to pinctrl's
> 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);

Hmm. So how is a certain pin muxed to its GPIO function then? And how
can pullup/pulldown features be selected?

I admittedly might lack some background here, and if there's better
solution to what I want to do, I'd be happy to hear about it :)


Many thanks,
Daniel

Re: [PATCH] Input: gpio_keys: allocate pins

[Linus Walleij]

On Fri, Oct 12, 2012 at 11:40 PM, Daniel Mack <zonque@gmail.com> wrote:

>> Because we wrote in Documentation/pinctrl.txt that if GPIO
>> and pin control handle the same lines, they should be
>> implemented in the gpio driver by calling out to pinctrl's
>> 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);
>
> Hmm. So how is a certain pin muxed to its GPIO function then?

By calling exactly the above functions from the GPIO
driver.

> And how
> can pullup/pulldown features be selected?

So as stated in:
"Drivers needing both pin control and GPIOs"

This can be done in several ways, but this way is one option
indeed, so that is a valid reason for having this pinctrl here.

Is biasing what you need to do?

> I admittedly might lack some background here, and if there's better
> solution to what I want to do, I'd be happy to hear about it :)

Sure, no problem we've even tried to document it :-)

All I really want is that platforms have a clear idea about
how and where the pins will be handled, and that if GPIO
and pinctrl handle the same lines, they need to interact.

Yours,
Linus Walleij

Re: [PATCH] Input: gpio_keys: allocate pins

[Lee Jones]

On Fri, 12 Oct 2012, Linus Walleij wrote:

> On Fri, Oct 12, 2012 at 11:40 PM, Daniel Mack <zonque@gmail.com> wrote:
> 
> >> Because we wrote in Documentation/pinctrl.txt that if GPIO
> >> and pin control handle the same lines, they should be
> >> implemented in the gpio driver by calling out to pinctrl's
> >> 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);
> >
> > Hmm. So how is a certain pin muxed to its GPIO function then?
> 
> By calling exactly the above functions from the GPIO
> driver.
> 
> > And how
> > can pullup/pulldown features be selected?
> 
> So as stated in:
> "Drivers needing both pin control and GPIOs"
> 
> This can be done in several ways, but this way is one option
> indeed, so that is a valid reason for having this pinctrl here.
> 
> Is biasing what you need to do?
> 
> > I admittedly might lack some background here, and if there's better
> > solution to what I want to do, I'd be happy to hear about it :)
> 
> Sure, no problem we've even tried to document it :-)
> 
> All I really want is that platforms have a clear idea about
> how and where the pins will be handled, and that if GPIO
> and pinctrl handle the same lines, they need to interact.
> 
> Yours,
> Linus Walleij

Friendly poke.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: gpio_keys: allocate pins

[Linus Walleij]

On Thu, Oct 25, 2012 at 9:58 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 12 Oct 2012, Linus Walleij wrote:
(...)
>> Is biasing what you need to do?
(...)
>> All I really want is that platforms have a clear idea about
>> how and where the pins will be handled, and that if GPIO
>> and pinctrl handle the same lines, they need to interact.
>>
>> Yours,
>> Linus Walleij
>
> Friendly poke.

I don't know how to respond to that? I asked a question about
what the intent of the patch was and the generic thinking
behind this approach and it remains unanswered.

I think I have seen other patches doing the proper thing
for pinctrl-single by implementing the proper
pinctrl_request_gpio()
pinctrl_free_gpio()
pinctrl_gpio_direction_input()
pinctrl_gpio_direction_output()
in that very GPIO driver.

So I suspect that this patch should be dropped, unless you
have some other compelling usecase to bring to the show?

Yours,
Linus Walleij

Re: [PATCH] Input: gpio_keys: allocate pins

[Lee Jones]

On Thu, 25 Oct 2012, Linus Walleij wrote:

> On Thu, Oct 25, 2012 at 9:58 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Fri, 12 Oct 2012, Linus Walleij wrote:
> (...)
> >> Is biasing what you need to do?
> (...)
> >> All I really want is that platforms have a clear idea about
> >> how and where the pins will be handled, and that if GPIO
> >> and pinctrl handle the same lines, they need to interact.
> >>
> >> Yours,
> >> Linus Walleij
> >
> > Friendly poke.
> 
> I don't know how to respond to that? I asked a question about
> what the intent of the patch was and the generic thinking
> behind this approach and it remains unanswered.
> 
> I think I have seen other patches doing the proper thing
> for pinctrl-single by implementing the proper
> pinctrl_request_gpio()
> pinctrl_free_gpio()
> pinctrl_gpio_direction_input()
> pinctrl_gpio_direction_output()
> in that very GPIO driver.
> 
> So I suspect that this patch should be dropped, unless you
> have some other compelling usecase to bring to the show?

Actually it is I who is confused. I thought you answered some of
the questions which was posed to me. The poke was a kind of does
anyone else have any comments.

I'm happy for the patch to be dropped. Does that mean it can
be eradicated from the internal tree too then, because that is
actually a suitable end result.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: gpio_keys: allocate pins

[Daniel Mack]

On 25.10.2012 10:08, Linus Walleij wrote:
> On Thu, Oct 25, 2012 at 9:58 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> On Fri, 12 Oct 2012, Linus Walleij wrote:
> (...)
>>> Is biasing what you need to do?
> (...)
>>> All I really want is that platforms have a clear idea about
>>> how and where the pins will be handled, and that if GPIO
>>> and pinctrl handle the same lines, they need to interact.
>>>
>>> Yours,
>>> Linus Walleij
>>
>> Friendly poke.
> 
> I don't know how to respond to that? I asked a question about
> what the intent of the patch was and the generic thinking
> behind this approach and it remains unanswered.
> 
> I think I have seen other patches doing the proper thing
> for pinctrl-single by implementing the proper
> pinctrl_request_gpio()
> pinctrl_free_gpio()
> pinctrl_gpio_direction_input()
> pinctrl_gpio_direction_output()
> in that very GPIO driver.
> 
> So I suspect that this patch should be dropped, unless you
> have some other compelling usecase to bring to the show?

I think so too. I misunderstood the concept of pinctrl in this area,
thanks for taking the time of explaining this. Shortly after I worked on
it, I was distracted by other topics so I didn't find time to continue.
But once I will, I'll follow up here again.


Thanks,
Daniel

Re: [PATCH] Input: gpio_keys: allocate pins

[Dmitry Torokhov]

On Friday, October 12, 2012 11:26:00 PM Linus Walleij wrote:
> On Fri, Oct 12, 2012 at 5:55 PM, Daniel Mack <zonque@gmail.com> wrote:
> > This allows DT driven boards to allocate and configure the pinmux once
> > the driver is probed.
> > 
> > Signed-off-by: Daniel Mack <zonque@gmail.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> (...)
> 
> > +       /* request pin mux */
> > +       pinctrl = devm_pinctrl_get_select_default(dev);
> > +       if (IS_ERR(pinctrl))
> > +               dev_warn(dev, "pins are not configured from the
> > driver\n");
> 
> I think dev_warn() is rather nasty to throw in here, dev_info() is OK.
> 
> However I suspect this driver could actually handle default, idle and sleep
> states, especially after the runtime PM patches discussed elsewhere,
> but that can be patched later.

I take this as Acked-by then?

-- 
Dmitry

Re: [PATCH] Input: gpio_keys: allocate pins

[Linus Walleij]

On Fri, Oct 12, 2012 at 11:35 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Friday, October 12, 2012 11:26:00 PM Linus Walleij wrote:
>> On Fri, Oct 12, 2012 at 5:55 PM, Daniel Mack <zonque@gmail.com> wrote:
>> > This allows DT driven boards to allocate and configure the pinmux once
>> > the driver is probed.
>> >
>> > Signed-off-by: Daniel Mack <zonque@gmail.com>
>> > Cc: Linus Walleij <linus.walleij@linaro.org>
>> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> (...)
>>
>> > +       /* request pin mux */

That is not good, pinctrl is not just about muxing.

>> > +       pinctrl = devm_pinctrl_get_select_default(dev);
>> > +       if (IS_ERR(pinctrl))
>> > +               dev_warn(dev, "pins are not configured from the
>> > driver\n");
>>
>> I think dev_warn() is rather nasty to throw in here, dev_info() is OK.
>>
>> However I suspect this driver could actually handle default, idle and sleep
>> states, especially after the runtime PM patches discussed elsewhere,
>> but that can be patched later.
>
> I take this as Acked-by then?

Not yet. I want to clarify the use. In the Documentation/pinctrl.txt
document we say:

-----------8<----------------------8<------------------8<-------------------
Drivers needing both pin control and GPIOs
==========================================

Again, it is discouraged to let drivers lookup and select pin control states
themselves, but again sometimes this is unavoidable.

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, you should
nominally 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 will certainly want to
mux in and bias pins in a certain way before the GPIO subsystems starts to
deal with them.

The above can be hidden: using pinctrl hogs, the pin control driver may be
setting up the config and muxing for the pins when it is probing,
nevertheless orthogonal to the GPIO subsystem.
But there are also situations where it makes sense for the GPIO subsystem
to communicate directly with with the pinctrl subsystem, using the latter
as a back-end. This is when the GPIO driver may call out to the functions
described in the section "Pin control interaction with the GPIO subsystem"
above. This only involves per-pin multiplexing, and will be completely
hidden behind the gpio_*() function namespace. In this case, the driver
need not interact with the pin control subsystem at all.

If a pin control driver and a GPIO driver is dealing with the same pins
and the use cases involve multiplexing, you MUST implement the pin controller
as a back-end for the GPIO driver like this, unless your hardware design
is such that the GPIO controller can override the pin controller's
multiplexing state through hardware without the need to interact with the
pin control system.
-----------8<----------------------8<------------------8<-------------------

Yours,
Linus Walleij