leds: leds-gpio: adopt pinctrl support
Subsystems (2)
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
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.
---
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(+)
drivers/leds/leds-gpio.c +7 -0
@@ -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),
Signature · 1 line
1.7.9.5
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 4 lines · AnilKumar Ch
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
Quoted 44 lines
Signed-off-by AnilKumar Ch <[email protected]>---
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 returndrivers/leds/leds-gpio.c | 7 +++++++ 1 file changed, 7 insertions(+)drivers/leds/leds-gpio.c +7
-1@@ -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 majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 4 lines
Bryan Wu <[email protected]> Kernel Developer +86.186-168-78255 Mobile Canonical Ltd. www.canonical.com Ubuntu - Linux for human beings | www.ubuntu.com
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Dear Bryan Wu,
Quoted 7 lines · AnilKumar Ch
Quoted 3 lines
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.
Quoted 3 lines
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?
Quoted 54 lines
-Bryan
Quoted 52 lines
Signed-off-by AnilKumar Ch <[email protected]>---
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(+)drivers/leds/leds-gpio.c +1
-0@@ -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;drivers/leds/leds-gpio.c +6
-0@@ -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),
Quoted 7 lines
Quoted 7 lines
--
1.7.9.5--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Best regards,
Marek Vasut
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 13 lines · Marek Vasut
Dear Bryan Wu,
Quoted 7 lines · AnilKumar Ch
Quoted 3 lines
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.
Quoted 8 lines
Quoted 3 lines
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()ifCONFIG_OFis enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing ifCONFIG_OFis 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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Hi Tony,
Quoted 18 lines · Marek Vasut
Quoted 17 lines
Dear Bryan Wu,
Quoted 7 lines · AnilKumar Ch
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 7 lines
Quoted 4 lines
But this driver is also used on hardware that's not yet coverted to DT, so I'd say
dev_warn()ifCONFIG_OFis enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing ifCONFIG_OFis 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.
Quoted 3 lines
Regards,
Tony
Best regards,
Marek Vasut
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 25 lines · Marek Vasut
Hi Tony,
Quoted 23 lines · Marek Vasut
Quoted 21 lines
Dear Bryan Wu,
Quoted 7 lines · AnilKumar Ch
Quoted 3 lines
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.
Quoted 3 lines
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()ifCONFIG_OFis enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing ifCONFIG_OFis disabled? Actually (2), what's the relationship between OF and pinctrl?
Quoted 5 lines
Quoted 2 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Dear Tony Lindgren,
Quoted 27 lines · Marek Vasut
Quoted 26 lines
Hi Tony,
Quoted 24 lines · Marek Vasut
Quoted 22 lines
Dear Bryan Wu,
Quoted 7 lines · AnilKumar Ch
Quoted 3 lines
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.
Quoted 3 lines
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()ifCONFIG_OFis enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing ifCONFIG_OFis disabled? Actually (2), what's the relationship between OF and pinctrl?
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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 ininclude/linux/pinctrl/consumer.hifCONFIG_PINCTRLis not selected, so no warning is produced AFAIK ;)
Oh all right then.
Quoted 2 lines
Or do you get some warning if
CONFIG_PINCTRLis not selected for your hardware?
No, I don't have much hardware like such anymore :-(
Best regards,
Marek Vasut
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 32 lines · Marek Vasut
Dear Tony Lindgren,
Quoted 30 lines · Marek Vasut
Quoted 29 lines
Hi Tony,
Quoted 26 lines · Marek Vasut
Quoted 17 lines
Dear Bryan Wu,
Quoted 7 lines · AnilKumar Ch
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
But this driver is also used on hardware that's not yet coverted to DT, so I'd say
dev_warn()ifCONFIG_OFis enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing ifCONFIG_OFis 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.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Exactly my concern. Also the warning shouldnt be present on systems where pinctrl is disabled.
But
pinctrl_get_select()returns 0 ininclude/linux/pinctrl/consumer.hifCONFIG_PINCTRLis 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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Dear AnilKumar, Chimata,
Quoted 10 lines · Marek Vasut
Quoted 9 lines
Dear Tony Lindgren,
Quoted 7 lines · Marek Vasut
Quoted 6 lines
Hi Tony,
Quoted 4 lines · Marek Vasut
Quoted 3 lines
Dear Bryan Wu,
Quoted 1 line
On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch <[email protected]>
Quoted 26 lines · wrote:
Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 14 lines
Quoted 14 lines
Quoted 6 lines
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 8 lines
Quoted 5 lines
But this driver is also used on hardware that's not yet coverted to DT, so I'd say
dev_warn()ifCONFIG_OFis enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing ifCONFIG_OFis 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.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
But
pinctrl_get_select()returns 0 ininclude/linux/pinctrl/consumer.hifCONFIG_PINCTRLis 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.
Quoted 2 lines
Thanks
AnilKumar
Best regards,
Marek Vasut
Bryan Wu
AppliedPATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
In reply to AnilKumar, ChimataQuoted 39 lines · Chimata
Quoted 38 lines · Marek Vasut
Dear Tony Lindgren,
Quoted 35 lines · Marek Vasut
Quoted 26 lines
Hi Tony,
Quoted 24 lines · Marek Vasut
Quoted 22 lines
Dear Bryan Wu,
Quoted 7 lines · AnilKumar Ch
Quoted 3 lines
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.
Quoted 3 lines
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()ifCONFIG_OFis enabled and otherwise simply go on ? Actually, can we not skip whole this pinctrl thing ifCONFIG_OFis disabled? Actually (2), what's the relationship between OF and pinctrl?Quoted 5 lines
Quoted 2 lines
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 ininclude/linux/pinctrl/consumer.hifCONFIG_PINCTRLis not selected, so no warning is produced AFAIK ;)
Quoted 7 lines
Quoted 2 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
In reply to Tony LindgrenQuoted 8 lines · Tony Lindgren
Quoted 5 lines
Quoted 2 lines
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 ininclude/linux/pinctrl/consumer.hifCONFIG_PINCTRLis 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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 8 lines · Linus Walleij
Quoted 6 lines · Tony Lindgren
Quoted 5 lines
Quoted 2 lines
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.
Quoted 7 lines
Quoted 2 lines
But
pinctrl_get_select()returns 0 ininclude/linux/pinctrl/consumer.hifCONFIG_PINCTRLis 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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
In reply to AnilKumar ChQuoted 3 lines · AnilKumar Ch
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?
Cheers,
Domenico
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Hi Domenico,
Quoted 7 lines · Domenico Andreoli
Quoted 3 lines · AnilKumar Ch
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 at 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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 1 line · Chimata
Hi Domenico,
Hi,
Quoted 10 lines · Domenico Andreoli
Quoted 7 lines · AnilKumar Ch
Quoted 3 lines
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.
Quoted 12 lines
So, leds-gpio driver should have this call, because these gpio pins are used by leds-gpio driver.
(quoted diff) +9
-0+ am33xx_pinmux: pinmux at 44e10800 { + userled_pins: pinmux_userled_pins { + pinctrl-single,pins = < + 0x54 0x7 + 0x58 0x17 + 0x5c 0x7 + 0x60 0x17 + >; + };
Review
Quoted 10 lines
(quoted diff) +6
-0+ }; + + leds { + compatible = "gpio-leds"; + pinctrl-names = "default"; + pinctrl-0 = <&userled_pins>; ^^^^^^^^^^^^
I'm surprised to not see any gpio controller (ala irq) involved.
Quoted 4 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 14 lines · Domenico Andreoli
Quoted 1 line · Chimata
Hi Domenico,
Hi,
Quoted 9 lines · Domenico Andreoli
Quoted 7 lines · AnilKumar Ch
Quoted 3 lines
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?
Quoted 6 lines
Quoted 1 line
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?
Quoted 18 lines
Quoted 18 lines
So, leds-gpio driver should have this call, because these gpio pins are used by leds-gpio driver.
(quoted diff) +11
-0+ am33xx_pinmux: pinmux at 44e10800 { + userled_pins: pinmux_userled_pins { + pinctrl-single,pins = < + 0x54 0x7 + 0x58 0x17 + 0x5c 0x7 + 0x60 0x17 + >; + }; + }; +
Quoted 7 lines
Review
Quoted 5 lines
(quoted diff) +4
-0+ 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.
Quoted 10 lines
Quoted 5 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 13 lines · Chimata
Quoted 12 lines · Domenico Andreoli
Quoted 10 lines · Chimata
Quoted 7 lines · Domenico Andreoli
Quoted 3 lines · AnilKumar Ch
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 23 lines
Quoted 23 lines
Quoted 22 lines
So, leds-gpio driver should have this call, because these gpio pins are used by leds-gpio driver.
(quoted diff) +15
-0+ am33xx_pinmux: pinmux at 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>; ^^^^^^^^^^^^
Quoted 3 lines
Quoted 1 line
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).
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 2 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 7 lines · Domenico Andreoli
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 2 lines · Tony Lindgren
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 ...
Quoted 5 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
In reply to Domenico AndreoliQuoted 9 lines · Domenico Andreoli
Quoted 2 lines · Chimata
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 13 lines · Linus Walleij
Quoted 9 lines · Domenico Andreoli
Quoted 2 lines · Chimata
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.
...
Quoted 5 lines
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 topinctrl_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?.
Quoted 5 lines
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.
Quoted 12 lines
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...)
Quoted 1 line
There is some flexibility here, I hope it's not too confusing :-/
Thank you for clarifying :)
Regards,
Domenico
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 9 lines · Domenico Andreoli
Quoted 5 lines · Linus Walleij
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 topinctrl_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.
Quoted 3 lines
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:
Quoted 1 line
From
a92d754367861cf564c09e0b15746e02f0a96f3fMon Sep 17 00:00:00 2001
From: Linus Walleij <[email protected]>
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.
--- Documentation/pinctrl.txt | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
Documentation/pinctrl.txt +23 -0
@@ -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
=================
Signature · 5 lines
1.7.11.3 Yours, Linus Walleij
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 11 lines · Linus Walleij
Quoted 9 lines · Domenico Andreoli
Quoted 5 lines · Linus Walleij
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 topinctrl_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?
Quoted 6 lines
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:
Quoted 6 lines
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 callpinctrl_free_gpio(). The pinctrl subsystem allows apinctrl_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).
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 14 lines · Stephen Warren
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.txtalready contains:Quoted 6 lines
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 callpinctrl_free_gpio(). The pinctrl subsystem allows apinctrl_request_gpio()to succeed concurrently with a pin or pingroup being "owned" by a device for pin multiplexing.
Quoted 6 lines
In order to resolve this, shouldn't we simply change the "should" at the end of the first line I quoted to "must"? That way, there'd never be any need to use pinctrl if you're only relying on gpiolib APIs.
(and I'd argue that the text was already meant to say "must", so this isn't actually a change to the intent, just a clarification).
It should deal with all the simple muxing use cases yes. And I am uncertain about the scope for this patch, if it only pertains to muxing, and in that case it would be solved by adding a proper GPIO backend to pinctrl-single.c.
But it doesn't help with some real-world usecases if I'm not mistaken.
If you want to set up a certain GPIO pin as pull-down (I guess this could be the case for a LED array), this cannot be done through any of these functions:
extern int pinctrl_request_gpio(unsigned gpio); extern void pinctrl_free_gpio(unsigned gpio); extern int pinctrl_gpio_direction_input(unsigned gpio); extern int pinctrl_gpio_direction_output(unsigned gpio);
So either we have to use a pin config hog to do this, or we have to use devm_pinctrl_get_select_default(&pdev->dev); from the driver (as this patch does). Either way it is using the pinctrl system orthogonally to the GPIO system, it doesn't happen from pinctrl_request_gpio() or so.
An alternative solution would be to add functions for controlling pinconfig and whatnot to the GPIO glue, which in turn would require adding frontends all over <linux/gpio.h> which in turn was the thing that Grant nixed to I got started with pinctrl instead...
But I'm open to any other suggestions. Would it be possible for pinctrl_request_gpio() to activate a pin config in the map for example? Currently it can only do muxing.
It's also possible to have the driver do something custom behind the back of pinctrl altogether as a response to pinctrl_request_gpio() but it wouldn't be
any elegant...
Yours,
Linus Walleij
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 34 lines · Linus Walleij
Quoted 14 lines · Stephen Warren
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.txtalready contains:Quoted 6 lines
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 callpinctrl_free_gpio(). The pinctrl subsystem allows apinctrl_request_gpio()to succeed concurrently with a pin or pingroup being "owned" by a device for pin multiplexing.Quoted 6 lines
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:
Quoted 6 lines
extern int
pinctrl_request_gpio(unsigned gpio); extern voidpinctrl_free_gpio(unsigned gpio); extern intpinctrl_gpio_direction_input(unsigned gpio); extern intpinctrl_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.
Quoted 3 lines
or we have
to usedevm_pinctrl_get_select_default(&pdev->dev); from the driver (as this patch does).
Yes, true.
Quoted 9 lines
Either way it is using the pinctrl
system orthogonally to the GPIO system, it doesn't happen frompinctrl_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!
Quoted 8 lines
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...
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
In reply to AnilKumar ChQuoted 5 lines · AnilKumar Ch
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 <[email protected]>
Looks good from a pinctrl point of view!
Yours,
Linus Walleij
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
In reply to AnilKumar ChQuoted 5 lines · AnilKumar Ch
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 <[email protected]>
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
+Don Aisheng
Quoted 6 lines · Linus Walleij
Quoted 4 lines · AnilKumar Ch
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.
Quoted 7 lines
Quoted 1 line
Signed-off-by AnilKumar Ch <[email protected]>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.
Quoted 6 lines
In that case, have you considered augmenting
pinctrl-single.cto implement .gpio_request_enable() .gpio_disable_free() and maybe also .gpio_set_direction() in its structpinmux_opspinmux 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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 3 lines · Chimata
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.
Quoted 5 lines
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.
Quoted 4 lines
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?
Quoted 5 lines
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-kernel&m=134763067415678&w=2
Quoted 3 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 17 lines · Linus Walleij
Quoted 3 lines · Chimata
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.
Quoted 5 lines
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.
Quoted 8 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 10 lines
Quoted 5 lines
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.
Quoted 14 lines
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.txtif 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-kernel&m=134763067415678&w=2
Quoted 6 lines
Quoted 3 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 10 lines · Tony Lindgren
Quoted 2 lines
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.
Quoted 3 lines
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 :-(
Quoted 6 lines
Quoted 2 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 10 lines · Linus Walleij
Quoted 8 lines · Tony Lindgren
Quoted 2 lines
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.
Quoted 2 lines
Quoted 2 lines
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.
Quoted 3 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 5 lines · Chimata
Quoted 1 line · Linus Walleij
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 27 lines · Linus Walleij
Quoted 5 lines · Chimata
Quoted 1 line · Linus Walleij
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)
Quoted 6 lines
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 at 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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 2 lines · Chimata
I completely understood this named modes, I have added named modes like this in
am335x-xxx.dtsfiles
I do not understand how the pinctrl-single dts files work actually, so please get Tony to review this part.
Quoted 2 lines
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.
Quoted 3 lines
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?
Quoted 3 lines
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.
Quoted 5 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
Quoted 5 lines · Linus Walleij
Quoted 2 lines · Chimata
I completely understood this named modes, I have added named modes like this in
am335x-xxx.dtsfiles
Quoted 8 lines
I do not understand how the pinctrl-single dts files work actually, so please get Tony to review this part.
Quoted 2 lines
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.
Quoted 8 lines
Quoted 4 lines
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
In reply to Linus WalleijQuoted 9 lines · Linus Walleij
Quoted 5 lines · Chimata
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
PATCH v2 1/1 leds: leds-gpio: adopt pinctrl support
In reply to AnilKumar, ChimataQuoted 6 lines · AnilKumar, Chimata
Quoted 5 lines · Linus Walleij
Quoted 3 lines · Tony Lindgren
Quoted 2 lines
OK that is typical pinctrl driver implementation work. I hope Tony can advice on this?
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
It seems that should also work for leds-gpio. And you can define more named modes as needed.
If we want to implement
pinctrl_gpiofunctionality we have to separate "function-mask" bits to
Quoted 6 lines
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.
Quoted 9 lines
I have gone through nvidia pinctrl dt data (
tegra20-seaboard.dts, nodedrive_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)
Quoted 7 lines
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.
Quoted 3 lines
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