| Message ID | 1350057346-15998-1-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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?
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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.
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 -- 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
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.
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 -- 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
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);
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(+)