| Message ID | 1346487390-11399-1-git-send-email-anilkumar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch <anilkumar@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 <anilkumar@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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Bryan Wu, > On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch <anilkumar@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 <anilkumar@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 majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html Best regards, Marek Vasut
* Marek Vasut <marex@denx.de> [120904 20:13]: > Dear Bryan Wu, > > > On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch <anilkumar@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
Hi Tony, > * Marek Vasut <marex@denx.de> [120904 20:13]: > > Dear Bryan Wu, > > > > > On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch <anilkumar@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. > Regards, > > Tony Best regards, Marek Vasut
* Marek Vasut <marex@denx.de> [120905 19:05]: > Hi Tony, > > > * Marek Vasut <marex@denx.de> [120904 20:13]: > > > Dear Bryan Wu, > > > > > > > On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch <anilkumar@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
Dear Tony Lindgren, > * Marek Vasut <marex@denx.de> [120905 19:05]: > > Hi Tony, > > > > > * Marek Vasut <marex@denx.de> [120904 20:13]: > > > > Dear Bryan Wu, > > > > > > > > > On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch <anilkumar@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
On Fri, Sep 07, 2012 at 05:39:35, Marek Vasut wrote: > Dear Tony Lindgren, > > > * Marek Vasut <marex@denx.de> [120905 19:05]: > > > Hi Tony, > > > > > > > * Marek Vasut <marex@denx.de> [120904 20:13]: > > > > > Dear Bryan Wu, > > > > > > > > > > > On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch <anilkumar@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
Dear AnilKumar, Chimata, > On Fri, Sep 07, 2012 at 05:39:35, Marek Vasut wrote: > > Dear Tony Lindgren, > > > > > * Marek Vasut <marex@denx.de> [120905 19:05]: > > > > Hi Tony, > > > > > > > > > * Marek Vasut <marex@denx.de> [120904 20:13]: > > > > > > Dear Bryan Wu, > > > > > > > > > > > > > On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch <anilkumar@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
On Sat, Sep 1, 2012 at 10:16 AM, AnilKumar Ch <anilkumar@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? Cheers, Domenico
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 <anilkumar@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
On Fri, Sep 07, 2012 at 09:10:50AM +0000, 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 <anilkumar@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
On Fri, Sep 07, 2012 at 16:32:51, Domenico Andreoli wrote: > On Fri, Sep 07, 2012 at 09:10:50AM +0000, 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 <anilkumar@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
On Fri, Sep 07, 2012 at 02:30:59PM +0000, AnilKumar, Chimata wrote: > On Fri, Sep 07, 2012 at 16:32:51, Domenico Andreoli wrote: > > On Fri, Sep 07, 2012 at 09:10:50AM +0000, 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 <anilkumar@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
On Fri, Sep 7, 2012 at 3:59 PM, AnilKumar, Chimata <anilkumar@ti.com> wrote: > On Fri, Sep 07, 2012 at 05:39:35, Marek Vasut wrote: >> Dear Tony Lindgren, >> >> > * Marek Vasut <marex@denx.de> [120905 19:05]: >> > > Hi Tony, >> > > >> > > > * Marek Vasut <marex@denx.de> [120904 20:13]: >> > > > > Dear Bryan Wu, >> > > > > >> > > > > > On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch <anilkumar@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
* Domenico Andreoli <cavokz@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
On Sat, Sep 1, 2012 at 10:16 AM, AnilKumar Ch <anilkumar@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 <anilkumar@ti.com> Looks good from a pinctrl point of view! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Sep 6, 2012 at 7:45 PM, Tony Lindgren <tony@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
* Linus Walleij <linus.walleij@linaro.org> [120907 14:40]: > On Thu, Sep 6, 2012 at 7:45 PM, Tony Lindgren <tony@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
On Fri, Sep 7, 2012 at 6:00 PM, Domenico Andreoli <cavokz@gmail.com> wrote: > On Fri, Sep 07, 2012 at 02:30:59PM +0000, 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
On Fri, Sep 7, 2012 at 6:35 PM, Tony Lindgren <tony@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
On Fri, Sep 07, 2012 at 11:57:59PM +0200, Linus Walleij wrote: > On Fri, Sep 7, 2012 at 6:00 PM, Domenico Andreoli <cavokz@gmail.com> wrote: > > On Fri, Sep 07, 2012 at 02:30:59PM +0000, 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
On Sat, Sep 1, 2012 at 10:16 AM, AnilKumar Ch <anilkumar@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 <anilkumar@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
+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 <anilkumar@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 <anilkumar@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
On Mon, Oct 1, 2012 at 9:03 AM, AnilKumar, Chimata <anilkumar@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-kernel&m=134763067415678&w=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
* Linus Walleij <linus.walleij@linaro.org> [121001 01:25]: > On Mon, Oct 1, 2012 at 9:03 AM, AnilKumar, Chimata <anilkumar@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-kernel&m=134763067415678&w=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
On Mon, Oct 1, 2012 at 5:44 PM, Tony Lindgren <tony@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
On Tue, Oct 02, 2012 at 01:29:37, Linus Walleij wrote: > On Mon, Oct 1, 2012 at 5:44 PM, Tony Lindgren <tony@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
On Wed, Oct 3, 2012 at 12:52 PM, AnilKumar, Chimata <anilkumar@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
* AnilKumar, Chimata <anilkumar@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 <tony@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
On Wed, Oct 03, 2012 at 18:06:10, Linus Walleij wrote: > On Wed, Oct 3, 2012 at 12:52 PM, AnilKumar, Chimata <anilkumar@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
On Tue, Oct 30, 2012 at 3:12 PM, AnilKumar, Chimata <anilkumar@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
On Sun, Nov 04, 2012 at 23:07:44, Linus Walleij wrote: > On Tue, Oct 30, 2012 at 3:12 PM, AnilKumar, Chimata <anilkumar@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
* Linus Walleij <linus.walleij@linaro.org> [121104 09:39]: > On Tue, Oct 30, 2012 at 3:12 PM, AnilKumar, Chimata <anilkumar@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
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),
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 <anilkumar@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(+)