Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
* Linus Walleij linus.wall...@linaro.org [121104 09:39]: On Tue, Oct 30, 2012 at 3:12 PM, AnilKumar, Chimata anilku...@ti.com wrote: I have two options now - add only default states for now, I can add reset of the state details once suspend/resume is supported. - add same values in all the states, modify those once suspend/resume is added for AM335x. The OMAP maintainer gets to choose how this is to be done, it's none of my business :-) Either way is fine with me as long as the changes have been tested and you don't have to redo things once you have suspend and resume working. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Tue, Oct 30, 2012 at 3:12 PM, AnilKumar, Chimata anilku...@ti.com wrote: I completely understood this named modes, I have added named modes like this in am335x-xxx.dts files I do not understand how the pinctrl-single dts files work actually, so please get Tony to review this part. I think pinctrl-1 state will be used in driver suspend/resume calls. Hopefully, I think you should test the code and monitor the system to make sure the right thing happens. I have the pinmux/conf settings for default state but fully optimized pinmux/conf values in idle suspend states are not available yet. You have defined a sleep state which is what should be used for suspend? Or do you mean that you do have a state but you're just not defining it to the most optimal setting yet? Even though if I add here, I am unable to test these pins in suspend state because suspend/resume of AM35xx is not added yet Aha. I have two options now - add only default states for now, I can add reset of the state details once suspend/resume is supported. - add same values in all the states, modify those once suspend/resume is added for AM335x. The OMAP maintainer gets to choose how this is to be done, it's none of my business :-) Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Sun, Nov 04, 2012 at 23:07:44, Linus Walleij wrote: On Tue, Oct 30, 2012 at 3:12 PM, AnilKumar, Chimata anilku...@ti.com wrote: I completely understood this named modes, I have added named modes like this in am335x-xxx.dts files I do not understand how the pinctrl-single dts files work actually, so please get Tony to review this part. I think pinctrl-1 state will be used in driver suspend/resume calls. Hopefully, I think you should test the code and monitor the system to make sure the right thing happens. I will test while adding pinctrl-1 data to .dts files. I have the pinmux/conf settings for default state but fully optimized pinmux/conf values in idle suspend states are not available yet. You have defined a sleep state which is what should be used for suspend? Or do you mean that you do have a state but you're just not defining it to the most optimal setting yet? Yes, sleep state is used for suspend. Regarding to this suspend state fully optimized pinmux/conf values are not available. Thanks AnilKumar -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Wed, Oct 03, 2012 at 18:06:10, Linus Walleij wrote: On Wed, Oct 3, 2012 at 12:52 PM, AnilKumar, Chimata anilku...@ti.com wrote: On Tue, Oct 02, 2012 at 01:29:37, Linus Walleij wrote: This is what we're doing for ux500 and should be a good model. I have looked into this, but not seen any named modes. OK maybe it's not easy to find. If you look into: arch/arm/mach-ux500/board-mop500-pins.c you find our work in progress. Note that this is not (yet) using device tree. (We want to migrate all our pinctrl stuff first, then do DT.) So for example this macro: #define DB8500_PIN(pin,conf,dev) \ PIN_MAP_CONFIGS_PIN_DEFAULT(dev, pinctrl-db8500, pin, conf) Will define a config for the default mode for a certain pin. This: #define DB8500_PIN_SLEEP(pin, conf, dev) \ PIN_MAP_CONFIGS_PIN(dev, PINCTRL_STATE_SLEEP, pinctrl-db8500, \ pin, conf) Will mutatis mutandis define a sleep mode for a pin. Sorry for the macros. We'll get rid of them in the DT. (Now that Stephen has patched in preprocessing it will even look good.) Hi Linus Walleij/Tony, I completely understood this named modes, I have added named modes like this in am335x-xxx.dts files am33xx_pinmux: pinmux@44e10800 { pinctrl-names = default, sleep; pinctrl-0 = user_leds_s0; pinctrl-1 = user_leds_s1; user_leds_s0: user_leds_s0 { pinctrl-single,pins = 0x54 0x7/* gpmc_a5.gpio1_21, OUTPUT | MODE7 */ 0x58 0x17 /* gpmc_a6.gpio1_22, OUT PULLUP | MODE7 */ 0x5c 0x7/* gpmc_a7.gpio1_23, OUTPUT | MODE7 */ 0x60 0x17 /* gpmc_a8.gpio1_24, OUT PULLUP | MODE7 */ ; }; user_leds_s1: user_leds_s1 { pinctrl-single,pins = 0x54 0x2e /* gpmc_a5.gpio1_21, INPUT | MODE7 */ 0x58 0x2e /* gpmc_a6.gpio1_22, INPUT | MODE7 */ 0x5c 0x2e /* gpmc_a7.gpio1_23, INPUT | MODE7 */ 0x60 0x2e /* gpmc_a8.gpio1_24, INPUT | MODE7 */ ; }; }; I think pinctrl-1 state will be used in driver suspend/resume calls. I have the pinmux/conf settings for default state but fully optimized pinmux/conf values in idle suspend states are not available yet. Even though if I add here, I am unable to test these pins in suspend state because suspend/resume of AM35xx is not added yet I have two options now - add only default states for now, I can add reset of the state details once suspend/resume is supported. - add same values in all the states, modify those once suspend/resume is added for AM335x. Thanks AnilKumar -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Tue, Oct 02, 2012 at 01:29:37, Linus Walleij wrote: On Mon, Oct 1, 2012 at 5:44 PM, Tony Lindgren t...@atomide.com wrote: OK that is typical pinctrl driver implementation work. I hope Tony can advice on this? I think we're best off to just stick to alternative named modes passed from device tree. For example, for GPIO wake-ups you can have named modes such as default, enabled and idle where idle muxes things for GPIO wake-ups for the duration of idle. In this case we need to add three different values according to three modes (default, enabled, idle) and for each node. It seems that should also work for leds-gpio. And you can define more named modes as needed. If we want to implement pinctrl_gpio functionality we have to separate function-mask bits to 1. pinmux-mask 2. pinconf-mask, to make it generic we need following bit masks a. receiver enable/disable bit b. slew rate fast/slow bit c. pull-up/down bit I have gone through nvidia pinctrl dt data (tegra20-seaboard.dts, node drive_sdio1) which has different pinconfig values, those are mapping to pinconf values. With the above bit masks and function-mask we can identify pull-up/down, slow/high speed slew rate and direction in/out. (or) Named modes:- Are you saying named modes like this? default-input-up default-input-down default-output-up default-output-down This 1, 2 and 2.a or named modes are required to implement pinctrl_gpio_direction_input/output and pinctrl_request/free_gpio. This is what we're doing for ux500 and should be a good model. I have looked into this, but not seen any named modes. Thanks AnilKumar -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Wed, Oct 3, 2012 at 12:52 PM, AnilKumar, Chimata anilku...@ti.com wrote: On Tue, Oct 02, 2012 at 01:29:37, Linus Walleij wrote: This is what we're doing for ux500 and should be a good model. I have looked into this, but not seen any named modes. OK maybe it's not easy to find. If you look into: arch/arm/mach-ux500/board-mop500-pins.c you find our work in progress. Note that this is not (yet) using device tree. (We want to migrate all our pinctrl stuff first, then do DT.) So for example this macro: #define DB8500_PIN(pin,conf,dev) \ PIN_MAP_CONFIGS_PIN_DEFAULT(dev, pinctrl-db8500, pin, conf) Will define a config for the default mode for a certain pin. This: #define DB8500_PIN_SLEEP(pin, conf, dev) \ PIN_MAP_CONFIGS_PIN(dev, PINCTRL_STATE_SLEEP, pinctrl-db8500, \ pin, conf) Will mutatis mutandis define a sleep mode for a pin. Sorry for the macros. We'll get rid of them in the DT. (Now that Stephen has patched in preprocessing it will even look good.) Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
* AnilKumar, Chimata anilku...@ti.com [121003 03:53]: On Tue, Oct 02, 2012 at 01:29:37, Linus Walleij wrote: On Mon, Oct 1, 2012 at 5:44 PM, Tony Lindgren t...@atomide.com wrote: OK that is typical pinctrl driver implementation work. I hope Tony can advice on this? I think we're best off to just stick to alternative named modes passed from device tree. For example, for GPIO wake-ups you can have named modes such as default, enabled and idle where idle muxes things for GPIO wake-ups for the duration of idle. In this case we need to add three different values according to three modes (default, enabled, idle) and for each node. Yes those make sense from the generic leds-gpio point of view for the platforms that implement pinctrl. It seems that should also work for leds-gpio. And you can define more named modes as needed. If we want to implement pinctrl_gpio functionality we have to separate function-mask bits to 1. pinmux-mask 2. pinconf-mask, to make it generic we need following bit masks a. receiver enable/disable bit b. slew rate fast/slow bit c. pull-up/down bit Yes those can be implemented, but the problem will always be that the driver will not know if the board is using external pulls. If you implement the board specific settings in the .dts file for default, enabled and idle, the leds-gpio does not need to care if the pull is internal or external. So that seems like a more generic way to do it. I have gone through nvidia pinctrl dt data (tegra20-seaboard.dts, node drive_sdio1) which has different pinconfig values, those are mapping to pinconf values. With the above bit masks and function-mask we can identify pull-up/down, slow/high speed slew rate and direction in/out. (or) Named modes:- Are you saying named modes like this? default-input-up default-input-down default-output-up default-output-down Hmm no, you want to implement named modes that make sense from the client driver point of view. It seems that default, enabled and idle should do here? Then for the enabled mode you can set the LED specific pins to whatever pull mode you want for the board, and then leds-gpio does the rest using the gpio framework. This 1, 2 and 2.a or named modes are required to implement pinctrl_gpio_direction_input/output and pinctrl_request/free_gpio. I would just add the named modes to leds-gpio because 2a does not work for the case where you use external pulls. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support
+Don Aisheng On Tue, Sep 11, 2012 at 01:10:12, Linus Walleij wrote: On Sat, Sep 1, 2012 at 10:16 AM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Signed-off-by: AnilKumar Ch anilku...@ti.com So looking back at this after Stephen posed a real good question, when you say configure SoC pins to GPIO mode, does that mean anything else than to mux them into GPIO mode? pinctrl-single.c driver sets mux mode as well as pin configuration like pull-up/pull-down, input/output and slew rate. In that case, have you considered augmenting pinctrl-single.c to implement .gpio_request_enable() .gpio_disable_free() and maybe also .gpio_set_direction() in its struct pinmux_ops pinmux backend? If not, why? Recently, I have gone through the details on implementing gpio_request_enable in pinctrl-single driver. To add this functionality we have to add gpio_range's to pinctrl driver. AM335x EVM supports total 128 GPIO pins (4 banks) and these are randomly distributed across AM33XX SoC pins. These are the concerns/questions I have:- 1. I haven't added yet but it will come to more than 30-40 pinctrl_gpio_range entries 2. If the silicon package is changed then these will change. 3. If the GPIO driver is common for multiple SoCs then these entries will be more more over SoC specific and driver has to change every time the gpio_range changes. I have gone through the Don Aisheng patch series, which adds pinctrl_dt_add_gpio_ranges support but not accepted yet. With this patch series we can overcome the driver changes. 4. The current pinctrl driver has support for these APIs pinctrl_request_gpio(), pinctrl_free_gpio(), pinctrl_gpio_direction_input/output() no API for slew rate control, pulled down/up control how can we handle this? 5. pinctrl-single driver has to modify to provide separate handles for pinmux and pinconfig. And we need separate pin configuration bit masks and values/flags to take care of pull-up/down, slew rate, receiver in/out and mux mode control. 6. This is for my understanding, on which node do we have to add pinctrl data i.e., pin mux-mode data. In leds-gpio node (mostly not) and if it is in gpio node then how can we pass pinmux data with the existing API pinctrl_request_gpio(gpio);? Here we are passing only gpio number. Few more driver patches are pending along with this (leds-gpio DT data additions according to this patch, similarly other drivers like matrix keypad and volume keys) Thanks AnilKumar -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Mon, Oct 1, 2012 at 9:03 AM, AnilKumar, Chimata anilku...@ti.com wrote: I have gone through the Don Aisheng patch series, which adds pinctrl_dt_add_gpio_ranges support but not accepted yet. With this patch series we can overcome the driver changes. OK then this is the direction we need to go. 4. The current pinctrl driver has support for these APIs pinctrl_request_gpio(), pinctrl_free_gpio(), pinctrl_gpio_direction_input/output() no API for slew rate control, pulled down/up control how can we handle this? You are not supposed to handle that from the GPIO level of the API. That is supposed to be handled by pinctrl. These two subsystems are orthogonal, with the exception of the above calls. If you need to pass more information between the GPIO and pinctrl interfaces it usually means you're doing something wrong. The reason why pinctrl was created in the first place was that Grant didn't like that we started to shoehorn all kind of pin control into the GPIO subsystem. 5. pinctrl-single driver has to modify to provide separate handles for pinmux and pinconfig. And we need separate pin configuration bit masks and values/flags to take care of pull-up/down, slew rate, receiver in/out and mux mode control. OK that is typical pinctrl driver implementation work. I hope Tony can advice on this? 6. This is for my understanding, on which node do we have to add pinctrl data i.e., pin mux-mode data. In leds-gpio node (mostly not) and if it is in gpio node then how can we pass pinmux data with the existing API pinctrl_request_gpio(gpio);? Here we are passing only gpio number. So with the pinctrl_request_gpio() call you are requesting a single pin to be used as GPIO, nothing else. No additional data should be passed with that call. Implementing it is up to the pinctrl driver, the pinctrl subsystem API does not say anything about how this should be done, but there are a few examples. The pinctrl maps of muxes and config from the pin control subsystem are for entire devices, and the single-pin GPIO reservation API is orthogonal to this, please consult Documentation/pinctrl.txt if you are uncertain about what I mean with this, I've really tried to explain it there. The docs were recently amended to reflect some corner-case GPIO uses, see e.g.: http://marc.info/?l=linux-arm-kernelm=134763067415678w=2 Few more driver patches are pending along with this (leds-gpio DT data additions according to this patch, similarly other drivers like matrix keypad and volume keys) OK so the threshold is that we need to get it right for the first one and then the others will look good too. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
* Linus Walleij linus.wall...@linaro.org [121001 01:25]: On Mon, Oct 1, 2012 at 9:03 AM, AnilKumar, Chimata anilku...@ti.com wrote: I have gone through the Don Aisheng patch series, which adds pinctrl_dt_add_gpio_ranges support but not accepted yet. With this patch series we can overcome the driver changes. OK then this is the direction we need to go. 4. The current pinctrl driver has support for these APIs pinctrl_request_gpio(), pinctrl_free_gpio(), pinctrl_gpio_direction_input/output() no API for slew rate control, pulled down/up control how can we handle this? You are not supposed to handle that from the GPIO level of the API. That is supposed to be handled by pinctrl. These two subsystems are orthogonal, with the exception of the above calls. If you need to pass more information between the GPIO and pinctrl interfaces it usually means you're doing something wrong. The reason why pinctrl was created in the first place was that Grant didn't like that we started to shoehorn all kind of pin control into the GPIO subsystem. Agreed. 5. pinctrl-single driver has to modify to provide separate handles for pinmux and pinconfig. And we need separate pin configuration bit masks and values/flags to take care of pull-up/down, slew rate, receiver in/out and mux mode control. OK that is typical pinctrl driver implementation work. I hope Tony can advice on this? I think we're best off to just stick to alternative named modes passed from device tree. For example, for GPIO wake-ups you can have named modes such as default, enabled and idle where idle muxes things for GPIO wake-ups for the duration of idle. It seems that should also work for leds-gpio. And you can define more named modes as needed. You really don't want the client driver or the GPIO driver doing things like pull-up/down automatically as that is board specific and can also depend on things like externall pull resistors. 6. This is for my understanding, on which node do we have to add pinctrl data i.e., pin mux-mode data. In leds-gpio node (mostly not) and if it is in gpio node then how can we pass pinmux data with the existing API pinctrl_request_gpio(gpio);? Here we are passing only gpio number. So with the pinctrl_request_gpio() call you are requesting a single pin to be used as GPIO, nothing else. No additional data should be passed with that call. Yeah I agree. Implementing it is up to the pinctrl driver, the pinctrl subsystem API does not say anything about how this should be done, but there are a few examples. The pinctrl maps of muxes and config from the pin control subsystem are for entire devices, and the single-pin GPIO reservation API is orthogonal to this, please consult Documentation/pinctrl.txt if you are uncertain about what I mean with this, I've really tried to explain it there. The docs were recently amended to reflect some corner-case GPIO uses, see e.g.: http://marc.info/?l=linux-arm-kernelm=134763067415678w=2 Few more driver patches are pending along with this (leds-gpio DT data additions according to this patch, similarly other drivers like matrix keypad and volume keys) OK so the threshold is that we need to get it right for the first one and then the others will look good too. It seems we want to keep leds-gpio, gpio framework and pinctrl framework generic. It also seems you should be able to do what you're describing using the pinctrl named modes. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Mon, Oct 1, 2012 at 5:44 PM, Tony Lindgren t...@atomide.com wrote: OK that is typical pinctrl driver implementation work. I hope Tony can advice on this? I think we're best off to just stick to alternative named modes passed from device tree. For example, for GPIO wake-ups you can have named modes such as default, enabled and idle where idle muxes things for GPIO wake-ups for the duration of idle. It seems that should also work for leds-gpio. And you can define more named modes as needed. This is what we're doing for ux500 and should be a good model. You really don't want the client driver or the GPIO driver doing things like pull-up/down automatically as that is board specific and can also depend on things like externall pull resistors. Nope. We've had instances of people getting bad leakage because of pulling down a line where there is already a pull-down resistor on the board :-( OK so the threshold is that we need to get it right for the first one and then the others will look good too. It seems we want to keep leds-gpio, gpio framework and pinctrl framework generic. It also seems you should be able to do what you're describing using the pinctrl named modes. I think so too. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Sun, Sep 9, 2012 at 1:44 AM, Domenico Andreoli cav...@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.wall...@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 cav...@gmail.com Signed-off-by: Linus Walleij linus.wall...@linaro.org --- Documentation/pinctrl.txt | 24 1 file changed, 24 insertions(+) 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 = -- 1.7.11.3 Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On 09/10/2012 09:23 AM, Linus Walleij wrote: On Sun, Sep 9, 2012 at 1:44 AM, Domenico Andreoli cav...@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). -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Sat, Sep 1, 2012 at 10:16 AM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Signed-off-by: AnilKumar Ch anilku...@ti.com So looking back at this after Stephen posed a real good question, when you say configure SoC pins to GPIO mode, does that mean anything else than to mux them into GPIO mode? In that case, have you considered augmenting pinctrl-single.c to implement .gpio_request_enable() .gpio_disable_free() and maybe also .gpio_set_direction() in its struct pinmux_ops pinmux backend? If not, why? Currently it looks like this: static int pcs_request_gpio(struct pinctrl_dev *pctldev, struct pinctrl_gpio_range *range, unsigned offset) { return -ENOTSUPP; } static struct pinmux_ops pcs_pinmux_ops = { .get_functions_count = pcs_get_functions_count, .get_function_name = pcs_get_function_name, .get_function_groups = pcs_get_function_groups, .enable = pcs_enable, .disable = pcs_disable, .gpio_request_enable = pcs_request_gpio, }; Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 swar...@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... -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Fri, Sep 07, 2012 at 11:57:59PM +0200, Linus Walleij wrote: On Fri, Sep 7, 2012 at 6:00 PM, Domenico Andreoli cav...@gmail.com wrote: On Fri, Sep 07, 2012 at 02:30:59PM +, AnilKumar, Chimata wrote: How can gpio driver knows that leds-gpio driver require these 4 pins? because leds-gpio requests each gpio (specified in the DT against a specific gpio controller) before assuming it is available? gpiolib then asks to pinctrl if those pins are available for gpio and possibly reserve them (configuring the mux, if necessary) for the device. So this is not an either-or situation but a both-and case. ... 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? 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?. But that's as far as it goes! The GPIO abstraction cannot call through to e.g. set some specific biasing on the pins (pull up etc). Doing that would require us to reimplement every interface that pinctrl already has again in the GPIO layer, which is not a good idea. Yes, clear. Never meant that, I thought that the pinctrl was anyway available for stuff not modeled by the GPIO layer, as you say below. So the pinctrl handle can be used for such config, and it can also be used for multiplexing if that is desired - if not done by the fall through functions pinctrl_gpio_*(). You can use a combination of both too, Stephen patched pinctrl some time back so that a pin can be muxed for a certain function and used as GPIO at the same time, so these two are now orthogonal, it's a bit relaxed and gives some feeling of loss of control but was necessary for certain usecases. (For example we can snoop on a I2C pin using its corresponding GPIO registers in the U300...) There is some flexibility here, I hope it's not too confusing :-/ Thank you for clarifying :) Regards, Domenico -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Fri, Sep 07, 2012 at 05:39:35, Marek Vasut wrote: Dear Tony Lindgren, * Marek Vasut ma...@denx.de [120905 19:05]: Hi Tony, * Marek Vasut ma...@denx.de [120904 20:13]: Dear Bryan Wu, On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Thanks for this, actually Marek Vasut submitted a similar patch before. I'm pretty fine with this patch. Thanks for submitting this actually ... I didn't have time to properly investigate this. But without proper DT setting, it will also give us warning I think. or we can provide some dummy functions as a temp solution as Shawn pointed out before. But this driver is also used on hardware that's not yet coverted to DT, so I'd say dev_warn() if CONFIG_OF is enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing if CONFIG_OF is disabled? Actually (2), what's the relationship between OF and pinctrl? The warning should be pinctrl related as the pinctrl drivers may not be device tree based drivers. Exactly my concern. Also the warning shouldnt be present on systems where pinctrl is disabled. But pinctrl_get_select() returns 0 in include/linux/pinctrl/consumer.h if CONFIG_PINCTRL is not selected, so no warning is produced AFAIK ;) Oh all right then. Bryan, If this patch looks fine, can you queue this for 3.7? Thanks AnilKumar -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
Dear AnilKumar, Chimata, On Fri, Sep 07, 2012 at 05:39:35, Marek Vasut wrote: Dear Tony Lindgren, * Marek Vasut ma...@denx.de [120905 19:05]: Hi Tony, * Marek Vasut ma...@denx.de [120904 20:13]: Dear Bryan Wu, On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Thanks for this, actually Marek Vasut submitted a similar patch before. I'm pretty fine with this patch. Thanks for submitting this actually ... I didn't have time to properly investigate this. But without proper DT setting, it will also give us warning I think. or we can provide some dummy functions as a temp solution as Shawn pointed out before. But this driver is also used on hardware that's not yet coverted to DT, so I'd say dev_warn() if CONFIG_OF is enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing if CONFIG_OF is disabled? Actually (2), what's the relationship between OF and pinctrl? The warning should be pinctrl related as the pinctrl drivers may not be device tree based drivers. Exactly my concern. Also the warning shouldnt be present on systems where pinctrl is disabled. But pinctrl_get_select() returns 0 in include/linux/pinctrl/consumer.h if CONFIG_PINCTRL is not selected, so no warning is produced AFAIK ;) Oh all right then. Bryan, If this patch looks fine, can you queue this for 3.7? Looks good to me. Thanks AnilKumar Best regards, Marek Vasut -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support
Hi Domenico, On Fri, Sep 07, 2012 at 14:18:39, Domenico Andreoli wrote: On Sat, Sep 1, 2012 at 10:16 AM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Shouldn't be the interaction with the pinctrl layer left to gpiolib? No, these gpio's are configured specifically for user leds. So, leds-gpio driver should have this call, because these gpio pins are used by leds-gpio driver. + am33xx_pinmux: pinmux@44e10800 { + userled_pins: pinmux_userled_pins { + pinctrl-single,pins = + 0x54 0x7 + 0x58 0x17 + 0x5c 0x7 + 0x60 0x17 + ; + }; + }; + [...] + leds { + compatible = gpio-leds; + pinctrl-names = default; + pinctrl-0 = userled_pins; This devm_pinctrl_get_select_default() call in leds-gpio driver will internally take userled_pins node and configure those pins according to the above definitions. Lets take gpio-keypad driver, in that case we have to configure pins as INPUT mode (generic gpio driver might not know what the end usecase is) and this leds case we configure as OUTPUT mode. Thanks AnilKumar -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Fri, Sep 07, 2012 at 09:10:50AM +, AnilKumar, Chimata wrote: Hi Domenico, Hi, On Fri, Sep 07, 2012 at 14:18:39, Domenico Andreoli wrote: On Sat, Sep 1, 2012 at 10:16 AM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Shouldn't be the interaction with the pinctrl layer left to gpiolib? No, these gpio's are configured specifically for user leds. So there are some special pad configs to make the leds work which are not only muxing and direction setting? Because I expect these to be managed privately between gpiolib and pinctrl but now I'm not sure any more, I'll look the code. So, leds-gpio driver should have this call, because these gpio pins are used by leds-gpio driver. + am33xx_pinmux: pinmux@44e10800 { + userled_pins: pinmux_userled_pins { + pinctrl-single,pins = + 0x54 0x7 + 0x58 0x17 + 0x5c 0x7 + 0x60 0x17 + ; + }; + }; + [...] + leds { + compatible = gpio-leds; + pinctrl-names = default; + pinctrl-0 = userled_pins; I'm surprised to not see any gpio controller (ala irq) involved. Lets take gpio-keypad driver, in that case we have to configure pins as INPUT mode (generic gpio driver might not know what the end usecase is) and this leds case we configure as OUTPUT mode. gpio direction is modeled by gpiolib so, if no other out-of-gpiolib capabilities are required for that led gpio, there is no need to directly use pinctrl. maybe I've just got it wrong. sorry. thanks, Domenico -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Fri, Sep 07, 2012 at 16:32:51, Domenico Andreoli wrote: On Fri, Sep 07, 2012 at 09:10:50AM +, AnilKumar, Chimata wrote: Hi Domenico, Hi, On Fri, Sep 07, 2012 at 14:18:39, Domenico Andreoli wrote: On Sat, Sep 1, 2012 at 10:16 AM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Shouldn't be the interaction with the pinctrl layer left to gpiolib? No, these gpio's are configured specifically for user leds. So there are some special pad configs to make the leds work which are not only muxing and direction setting? Because I expect these to be managed privately between gpiolib and pinctrl but now I'm not sure any more, I'll look the code. How can gpio driver knows that leds-gpio driver require these 4 pins? So, leds-gpio driver should have this call, because these gpio pins are used by leds-gpio driver. + am33xx_pinmux: pinmux@44e10800 { + userled_pins: pinmux_userled_pins { + pinctrl-single,pins = + 0x54 0x7 + 0x58 0x17 + 0x5c 0x7 + 0x60 0x17 + ; + }; + }; + [...] + leds { + compatible = gpio-leds; + pinctrl-names = default; + pinctrl-0 = userled_pins; I'm surprised to not see any gpio controller (ala irq) involved. GPIO controller data will be in GPIO node, should not be here. Lets take gpio-keypad driver, in that case we have to configure pins as INPUT mode (generic gpio driver might not know what the end usecase is) and this leds case we configure as OUTPUT mode. gpio direction is modeled by gpiolib so, if no other out-of-gpiolib capabilities are required for that led gpio, there is no need to directly use pinctrl. Here leds-gpio driver requirement is to set mux configuration of those 4 pins to GPIO mode (mode 7) as well direction OUTPUT/INPUT. Set mux mode to 7 (GPIO usage) should be from led driver, because this driver have the requirement. Thanks AnilKumar -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Fri, Sep 07, 2012 at 02:30:59PM +, AnilKumar, Chimata wrote: On Fri, Sep 07, 2012 at 16:32:51, Domenico Andreoli wrote: On Fri, Sep 07, 2012 at 09:10:50AM +, AnilKumar, Chimata wrote: On Fri, Sep 07, 2012 at 14:18:39, Domenico Andreoli wrote: On Sat, Sep 1, 2012 at 10:16 AM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Shouldn't be the interaction with the pinctrl layer left to gpiolib? No, these gpio's are configured specifically for user leds. So there are some special pad configs to make the leds work which are not only muxing and direction setting? Because I expect these to be managed privately between gpiolib and pinctrl but now I'm not sure any more, I'll look the code. How can gpio driver knows that leds-gpio driver require these 4 pins? because leds-gpio requests each gpio (specified in the DT against a specific gpio controller) before assuming it is available? gpiolib then asks to pinctrl if those pins are available for gpio and possibly reserve them (configuring the mux, if necessary) for the device. But this idea does not correspond to the code, so I must have filled in the blanks of something I didn't fully understand. So, leds-gpio driver should have this call, because these gpio pins are used by leds-gpio driver. + am33xx_pinmux: pinmux@44e10800 { + userled_pins: pinmux_userled_pins { + pinctrl-single,pins = + 0x54 0x7 + 0x58 0x17 + 0x5c 0x7 + 0x60 0x17 + ; + }; + }; + [...] + leds { + compatible = gpio-leds; + pinctrl-names = default; + pinctrl-0 = userled_pins; I'm surprised to not see any gpio controller (ala irq) involved. GPIO controller data will be in GPIO node, should not be here. So is this the preferred way to attach gpio users to gpio provides in DT whenever gpios are muxed? I would well see these info hidden in the gpio controller so, at least for gpios, no magic numbers would be required in the DT (except the gpio number relative to the owning controller). Lets take gpio-keypad driver, in that case we have to configure pins as INPUT mode (generic gpio driver might not know what the end usecase is) and this leds case we configure as OUTPUT mode. gpio direction is modeled by gpiolib so, if no other out-of-gpiolib capabilities are required for that led gpio, there is no need to directly use pinctrl. Here leds-gpio driver requirement is to set mux configuration of those 4 pins to GPIO mode (mode 7) as well direction OUTPUT/INPUT. Direction info is passed to gpiolib in the exact instant gpio_set_direction_*() is invoked. Set mux mode to 7 (GPIO usage) should be from led driver, because this driver have the requirement. This GPIO mode value is known to the pinctrl driver, actually it's _specific_ for that driver. So the only info pinctrl would really need to know is which pin is requested to be used as GPIO, something that gpiolib already manages. So I really don't see why the gpiolib could not be (I've understood it isn't) the one-stop for GPIO users. Of course direct pinctrl configuration would be required for all those specific gpio parameters not modeled by the gpiolib (like debounce times, etc). cheers, Domenico -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Fri, Sep 7, 2012 at 3:59 PM, AnilKumar, Chimata anilku...@ti.com wrote: On Fri, Sep 07, 2012 at 05:39:35, Marek Vasut wrote: Dear Tony Lindgren, * Marek Vasut ma...@denx.de [120905 19:05]: Hi Tony, * Marek Vasut ma...@denx.de [120904 20:13]: Dear Bryan Wu, On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Thanks for this, actually Marek Vasut submitted a similar patch before. I'm pretty fine with this patch. Thanks for submitting this actually ... I didn't have time to properly investigate this. But without proper DT setting, it will also give us warning I think. or we can provide some dummy functions as a temp solution as Shawn pointed out before. But this driver is also used on hardware that's not yet coverted to DT, so I'd say dev_warn() if CONFIG_OF is enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing if CONFIG_OF is disabled? Actually (2), what's the relationship between OF and pinctrl? The warning should be pinctrl related as the pinctrl drivers may not be device tree based drivers. Exactly my concern. Also the warning shouldnt be present on systems where pinctrl is disabled. But pinctrl_get_select() returns 0 in include/linux/pinctrl/consumer.h if CONFIG_PINCTRL is not selected, so no warning is produced AFAIK ;) Oh all right then. Bryan, If this patch looks fine, can you queue this for 3.7? I've applied this to my for-next branch. Thanks, -Bryan -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
* Domenico Andreoli cav...@gmail.com [120907 09:01]: So is this the preferred way to attach gpio users to gpio provides in DT whenever gpios are muxed? I would well see these info hidden in the gpio controller so, at least for gpios, no magic numbers would be required in the DT (except the gpio number relative to the owning controller). In the pure GPIO pins only case it could be all done in the GPIO controller, but it's probably best to have the pins muxed by the drivers using them. Some drivers doing runtime PM may need to dynamically toggle the pins for idle mode to stop leakage, enable wakeup events for rx pins etc. Probably no need for that in the gpio leds case, but it would be confusing to have some pins claimed by the GPIO driver and some by the drivers using the pins. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Sat, Sep 1, 2012 at 10:16 AM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Signed-off-by: AnilKumar Ch anilku...@ti.com Looks good from a pinctrl point of view! Reviewed-by: Linus Walleij linus.wall...@linaro.org Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Thu, Sep 6, 2012 at 7:45 PM, Tony Lindgren t...@atomide.com wrote: The warning should be pinctrl related as the pinctrl drivers may not be device tree based drivers. Exactly my concern. Also the warning shouldnt be present on systems where pinctrl is disabled. But pinctrl_get_select() returns 0 in include/linux/pinctrl/consumer.h if CONFIG_PINCTRL is not selected, so no warning is produced AFAIK ;) This is correct, nothing to worry about. The one troublesome case is if a pinctrl driver is present but not being used, then you might need to call pinctrl_provide_dummies(). Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
* Linus Walleij linus.wall...@linaro.org [120907 14:40]: On Thu, Sep 6, 2012 at 7:45 PM, Tony Lindgren t...@atomide.com wrote: The warning should be pinctrl related as the pinctrl drivers may not be device tree based drivers. Exactly my concern. Also the warning shouldnt be present on systems where pinctrl is disabled. But pinctrl_get_select() returns 0 in include/linux/pinctrl/consumer.h if CONFIG_PINCTRL is not selected, so no warning is produced AFAIK ;) This is correct, nothing to worry about. The one troublesome case is if a pinctrl driver is present but not being used, then you might need to call pinctrl_provide_dummies(). Thanks that's good to know. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Fri, Sep 7, 2012 at 6:00 PM, Domenico Andreoli cav...@gmail.com wrote: On Fri, Sep 07, 2012 at 02:30:59PM +, AnilKumar, Chimata wrote: How can gpio driver knows that leds-gpio driver require these 4 pins? because leds-gpio requests each gpio (specified in the DT against a specific gpio controller) before assuming it is available? gpiolib then asks to pinctrl if those pins are available for gpio and possibly reserve them (configuring the mux, if necessary) for the device. So this is not an either-or situation but a both-and case. So all muxing and configuration of pins can be handled by the pinctrl handle, and that may require setting up a single pinctrl function for every single pin, and that list can get long. But it works fine. In that case you don't write your pinctrl driver to do anything special with the GPIO callbacks, leave the .gpio_request_enable(), .gpio_disable_free() and .gpio_set_direction() callbacks in the vtable undefined. 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). Likewise for pinctrl_free_gpio(), pinctrl_gpio_direction_input() and pinctrl_gpio_direction_output(). But that's as far as it goes! The GPIO abstraction cannot call through to e.g. set some specific biasing on the pins (pull up etc). Doing that would require us to reimplement every interface that pinctrl already has again in the GPIO layer, which is not a good idea. So the pinctrl handle can be used for such config, and it can also be used for multiplexing if that is desired - if not done by the fall through functions pinctrl_gpio_*(). You can use a combination of both too, Stephen patched pinctrl some time back so that a pin can be muxed for a certain function and used as GPIO at the same time, so these two are now orthogonal, it's a bit relaxed and gives some feeling of loss of control but was necessary for certain usecases. (For example we can snoop on a I2C pin using its corresponding GPIO registers in the U300...) There is some flexibility here, I hope it's not too confusing :-/ Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Fri, Sep 7, 2012 at 6:35 PM, Tony Lindgren t...@atomide.com wrote: In the pure GPIO pins only case it could be all done in the GPIO controller, but it's probably best to have the pins muxed by the drivers using them. Yes, that's an easier way to say the long unreadable thing I just posted ... Some drivers doing runtime PM may need to dynamically toggle the pins for idle mode to stop leakage, enable wakeup events for rx pins etc. Probably no need for that in the gpio leds case, but it would be confusing to have some pins claimed by the GPIO driver and some by the drivers using the pins. True. drivers/tty/serial/amba-pl011.c provides a simple example. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
* Marek Vasut ma...@denx.de [120905 19:05]: Hi Tony, * Marek Vasut ma...@denx.de [120904 20:13]: Dear Bryan Wu, On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Thanks for this, actually Marek Vasut submitted a similar patch before. I'm pretty fine with this patch. Thanks for submitting this actually ... I didn't have time to properly investigate this. But without proper DT setting, it will also give us warning I think. or we can provide some dummy functions as a temp solution as Shawn pointed out before. But this driver is also used on hardware that's not yet coverted to DT, so I'd say dev_warn() if CONFIG_OF is enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing if CONFIG_OF is disabled? Actually (2), what's the relationship between OF and pinctrl? The warning should be pinctrl related as the pinctrl drivers may not be device tree based drivers. Exactly my concern. Also the warning shouldnt be present on systems where pinctrl is disabled. But pinctrl_get_select() returns 0 in include/linux/pinctrl/consumer.h if CONFIG_PINCTRL is not selected, so no warning is produced AFAIK ;) Or do you get some warning if CONFIG_PINCTRL is not selected for your hardware? Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
Dear Tony Lindgren, * Marek Vasut ma...@denx.de [120905 19:05]: Hi Tony, * Marek Vasut ma...@denx.de [120904 20:13]: Dear Bryan Wu, On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Thanks for this, actually Marek Vasut submitted a similar patch before. I'm pretty fine with this patch. Thanks for submitting this actually ... I didn't have time to properly investigate this. But without proper DT setting, it will also give us warning I think. or we can provide some dummy functions as a temp solution as Shawn pointed out before. But this driver is also used on hardware that's not yet coverted to DT, so I'd say dev_warn() if CONFIG_OF is enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing if CONFIG_OF is disabled? Actually (2), what's the relationship between OF and pinctrl? The warning should be pinctrl related as the pinctrl drivers may not be device tree based drivers. Exactly my concern. Also the warning shouldnt be present on systems where pinctrl is disabled. But pinctrl_get_select() returns 0 in include/linux/pinctrl/consumer.h if CONFIG_PINCTRL is not selected, so no warning is produced AFAIK ;) Oh all right then. Or do you get some warning if CONFIG_PINCTRL is not selected for your hardware? No, I don't have much hardware like such anymore :-( Best regards, Marek Vasut -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
* Marek Vasut ma...@denx.de [120904 20:13]: Dear Bryan Wu, On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Thanks for this, actually Marek Vasut submitted a similar patch before. I'm pretty fine with this patch. Thanks for submitting this actually ... I didn't have time to properly investigate this. But without proper DT setting, it will also give us warning I think. or we can provide some dummy functions as a temp solution as Shawn pointed out before. But this driver is also used on hardware that's not yet coverted to DT, so I'd say dev_warn() if CONFIG_OF is enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing if CONFIG_OF is disabled? Actually (2), what's the relationship between OF and pinctrl? The warning should be pinctrl related as the pinctrl drivers may not be device tree based drivers. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Thanks for this, actually Marek Vasut submitted a similar patch before. I'm pretty fine with this patch. But without proper DT setting, it will also give us warning I think. or we can provide some dummy functions as a temp solution as Shawn pointed out before. -Bryan Signed-off-by: AnilKumar Ch anilku...@ti.com --- Changes from v1: - Seperated from Add DT for AM33XX devices patch series - Incorporated Tony's comments on v1 * Changed to warning message instead od error return drivers/leds/leds-gpio.c |7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c index c032b21..ad577f4 100644 --- a/drivers/leds/leds-gpio.c +++ b/drivers/leds/leds-gpio.c @@ -20,6 +20,7 @@ #include linux/slab.h #include linux/workqueue.h #include linux/module.h +#include linux/pinctrl/consumer.h struct gpio_led_data { struct led_classdev cdev; @@ -236,8 +237,14 @@ static int __devinit gpio_led_probe(struct platform_device *pdev) { struct gpio_led_platform_data *pdata = pdev-dev.platform_data; struct gpio_leds_priv *priv; + struct pinctrl *pinctrl; int i, ret = 0; + pinctrl = devm_pinctrl_get_select_default(pdev-dev); + if (IS_ERR(pinctrl)) + dev_warn(pdev-dev, + pins are not configured from the driver\n); + if (pdata pdata-num_leds) { priv = devm_kzalloc(pdev-dev, sizeof_gpio_leds_priv(pdata-num_leds), -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-leds in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Bryan Wu bryan...@canonical.com Kernel Developer+86.186-168-78255 Mobile Canonical Ltd. www.canonical.com Ubuntu - Linux for human beings | www.ubuntu.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support
Dear Bryan Wu, On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch anilku...@ti.com wrote: Adopt pinctrl support to leds-gpio driver based on leds-gpio device pointer, pinctrl driver configure SoC pins to GPIO mode according to definitions provided in .dts file. Thanks for this, actually Marek Vasut submitted a similar patch before. I'm pretty fine with this patch. Thanks for submitting this actually ... I didn't have time to properly investigate this. But without proper DT setting, it will also give us warning I think. or we can provide some dummy functions as a temp solution as Shawn pointed out before. But this driver is also used on hardware that's not yet coverted to DT, so I'd say dev_warn() if CONFIG_OF is enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing if CONFIG_OF is disabled? Actually (2), what's the relationship between OF and pinctrl? -Bryan Signed-off-by: AnilKumar Ch anilku...@ti.com --- Changes from v1: - Seperated from Add DT for AM33XX devices patch series - Incorporated Tony's comments on v1 * Changed to warning message instead od error return drivers/leds/leds-gpio.c |7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c index c032b21..ad577f4 100644 --- a/drivers/leds/leds-gpio.c +++ b/drivers/leds/leds-gpio.c @@ -20,6 +20,7 @@ #include linux/slab.h #include linux/workqueue.h #include linux/module.h +#include linux/pinctrl/consumer.h struct gpio_led_data { struct led_classdev cdev; @@ -236,8 +237,14 @@ static int __devinit gpio_led_probe(struct platform_device *pdev) { struct gpio_led_platform_data *pdata = pdev-dev.platform_data; struct gpio_leds_priv *priv; + struct pinctrl *pinctrl; int i, ret = 0; + pinctrl = devm_pinctrl_get_select_default(pdev-dev); + if (IS_ERR(pinctrl)) + dev_warn(pdev-dev, + pins are not configured from the driver\n); + if (pdata pdata-num_leds) { priv = devm_kzalloc(pdev-dev, sizeof_gpio_leds_priv(pdata-num_leds), -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-leds in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Best regards, Marek Vasut -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html