[PATCH v2] leds: leds-gpio: adopt pinctrl support

[PATCH v2] leds: leds-gpio: adopt pinctrl support

[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 <anilkumar-l0cyMroinI0@public.gmane.org>
---
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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[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.
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



-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Marek Vasut]

Dear Bryan Wu,

> On Sat, Sep 1, 2012 at 4:16 PM, AnilKumar Ch <anilkumar-l0cyMroinI0@public.gmane.org> 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-l0cyMroinI0@public.gmane.org>
> > ---
> > 
> > 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Best regards,
Marek Vasut

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Tony Lindgren]

* 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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Marek Vasut]

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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[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 ;)

Or do you get some warning if CONFIG_PINCTRL is not selected for your
hardware?

Tony

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Marek Vasut]

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

RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[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?

Thanks
AnilKumar

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Marek Vasut]

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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Bryan Wu]

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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Tony Lindgren]

* Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> [120907 14:40]:
> On Thu, Sep 6, 2012 at 7:45 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> 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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Domenico Andreoli]

On Sat, Sep 1, 2012 at 10:16 AM, AnilKumar Ch <anilkumar-l0cyMroinI0@public.gmane.org> 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

RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[AnilKumar, Chimata]

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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Domenico Andreoli]

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

RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[AnilKumar, Chimata]

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-l0cyMroinI0@public.gmane.org> 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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Domenico Andreoli]

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-l0cyMroinI0@public.gmane.org> 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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Tony Lindgren]

* Domenico Andreoli <cavokz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Linus Walleij]

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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Domenico Andreoli]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Linus Walleij]

On Sun, Sep 9, 2012 at 1:44 AM, Domenico Andreoli <cavokz@gmail.com> wrote:
> On Fri, Sep 07, 2012 at 11:57:59PM +0200, Linus Walleij wrote:
>>
>> If all you need to to is to multiplex the pins into GPIO mode,
>> then the gpio_get() call on this driver *can* call through to
>> pinctrl_request_gpio() which will in turn fall through to the
>> above pinmux driver calls (.gpio_request_enable, etc).
>
> So if the GPIO driver doesn't coordinate with the pinctrl driver, it's
> all left to the GPIO user to configure the pin before using it, right?

Yes, more or less, or should I say that certain aspects of pinctrl
are orthogonal to GPIO and the two mostly do not know of each
other due to a separation of concerns.

So the driver may need to tie things up and request its pinctrl
and GPIOs independently.

> I can understand the concerns of Tony, whether a pin must be requested
> or not before the gpio then depends on the GPIO driver implementation,
> which may or may not call through the pinctrl layer, isn't it?.

Yes that is a sematic limitation, indeed.

I think the best way of trying to eliminate that is to bring the two
subsystems closer (which is some long-term project) but in the
meantime we could propose a documentation fixup to make the
semantics clear, what about this:

>From a92d754367861cf564c09e0b15746e02f0a96f3f Mon Sep 17 00:00:00 2001
From: Linus Walleij <linus.walleij@linaro.org>
Date: Mon, 10 Sep 2012 17:22:00 +0200
Subject: [PATCH] pinctrl: document semantics vs GPIO

The semantics of the interactions between GPIO and pinctrl may be
unclear, e.g. which one do you request first? This amends the
documentation to make this clear.

Reported-by: Domenico Andreoli <cavokz@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/pinctrl.txt | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 1479aca..941e783 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -289,6 +289,29 @@ Interaction with the GPIO subsystem
 The GPIO drivers may want to perform operations of various types on the same
 physical pins that are also registered as pin controller pins.

+First and foremost, the two subsystems can be used as completely orthogonal,
+so say that your driver is fetching its resources like this:
+
+#include <linux/pinctrl/consumer.h>
+#include <linux/gpio.h>
+
+struct pinctrl *pinctrl;
+int gpio;
+
+pinctrl = devm_pinctrl_get_select_default(&dev);
+gpio = devm_gpio_request(&dev, 14, "foo");
+
+Here we first request a certain pin state and then request GPIO 14 to be
+used. If you're using the subsystems orthogonally like this, always get
+your pinctrl handle and select the desired pinctrl state BEFORE requesting
+the GPIO. This is a semantic convention to avoid situations that can be
+electrically unpleasant, you may certainly want to mux in and bias pins
+a certain way before the GPIO subsystems starts to deal with them.
+
+But there are also situations where it makes sense for the GPIO subsystem
+to communicate with with the pinctrl subsystem, using the latter as a
+back-end.
+
 Since the pin controller subsystem have its pinspace local to the pin
 controller we need a mapping so that the pin control subsystem can figure out
 which pin controller handles control of a certain GPIO pin. Since a single
@@ -359,6 +382,7 @@ will get an pin number into its handled number
range. Further it is also passed
 the range ID value, so that the pin controller knows which range it should
 deal with.

+
 PINMUX interfaces
 =================

-- 
1.7.11.3


Yours,
Linus Walleij

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Stephen Warren]

On 09/10/2012 09:23 AM, Linus Walleij wrote:
> On Sun, Sep 9, 2012 at 1:44 AM, Domenico Andreoli <cavokz@gmail.com> wrote:
>> On Fri, Sep 07, 2012 at 11:57:59PM +0200, Linus Walleij wrote:
>>>
>>> If all you need to to is to multiplex the pins into GPIO mode,
>>> then the gpio_get() call on this driver *can* call through to
>>> pinctrl_request_gpio() which will in turn fall through to the
>>> above pinmux driver calls (.gpio_request_enable, etc).
>>
>> So if the GPIO driver doesn't coordinate with the pinctrl driver, it's
>> all left to the GPIO user to configure the pin before using it, right?
> 
> Yes, more or less, or should I say that certain aspects of pinctrl
> are orthogonal to GPIO and the two mostly do not know of each
> other due to a separation of concerns.
> 
> So the driver may need to tie things up and request its pinctrl
> and GPIOs independently.

That seems like exactly what we were trying to avoid when we added the
possibility for GPIO to call into pinctrl.

Documentation/gpio.txt already contains:

> For GPIOs that use pins known to the pinctrl subsystem, that subsystem should
> be informed of their use; a gpiolib driver's .request() operation may call
> pinctrl_request_gpio(), and a gpiolib driver's .free() operation may call
> pinctrl_free_gpio(). The pinctrl subsystem allows a pinctrl_request_gpio()
> to succeed concurrently with a pin or pingroup being "owned" by a device for
> pin multiplexing.

In order to resolve this, shouldn't we simply change the "should" at the
end of the first line I quoted to "must"? That way, there'd never be any
need to use pinctrl if you're only relying on gpiolib APIs.

(and I'd argue that the text was already meant to say "must", so this
isn't actually a change to the intent, just a clarification).

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Linus Walleij]

On Mon, Sep 10, 2012 at 7:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/10/2012 09:23 AM, Linus Walleij wrote:

> That seems like exactly what we were trying to avoid when we added the
> possibility for GPIO to call into pinctrl.
>
> Documentation/gpio.txt already contains:
>
>> For GPIOs that use pins known to the pinctrl subsystem, that subsystem should
>> be informed of their use; a gpiolib driver's .request() operation may call
>> pinctrl_request_gpio(), and a gpiolib driver's .free() operation may call
>> pinctrl_free_gpio(). The pinctrl subsystem allows a pinctrl_request_gpio()
>> to succeed concurrently with a pin or pingroup being "owned" by a device for
>> pin multiplexing.
>
> In order to resolve this, shouldn't we simply change the "should" at the
> end of the first line I quoted to "must"? That way, there'd never be any
> need to use pinctrl if you're only relying on gpiolib APIs.
>
> (and I'd argue that the text was already meant to say "must", so this
> isn't actually a change to the intent, just a clarification).

It should deal with all the simple muxing use cases yes. And
I am uncertain about the scope for this patch, if it only pertains
to muxing, and in that case it would be solved by adding
a proper GPIO backend to pinctrl-single.c.

But it doesn't help with some real-world usecases if I'm
not mistaken.

If you want to set up a certain GPIO pin as pull-down (I guess
this could be the case for a LED array), this cannot be done
through any of these functions:

extern int pinctrl_request_gpio(unsigned gpio);
extern void pinctrl_free_gpio(unsigned gpio);
extern int pinctrl_gpio_direction_input(unsigned gpio);
extern int pinctrl_gpio_direction_output(unsigned gpio);

So either we have to use a pin config hog to do this, 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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Stephen Warren]

On 09/10/2012 01:34 PM, Linus Walleij wrote:
> On Mon, Sep 10, 2012 at 7:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 09/10/2012 09:23 AM, Linus Walleij wrote:
> 
>> That seems like exactly what we were trying to avoid when we added the
>> possibility for GPIO to call into pinctrl.
>>
>> Documentation/gpio.txt already contains:
>>
>>> For GPIOs that use pins known to the pinctrl subsystem, that subsystem should
>>> be informed of their use; a gpiolib driver's .request() operation may call
>>> pinctrl_request_gpio(), and a gpiolib driver's .free() operation may call
>>> pinctrl_free_gpio(). The pinctrl subsystem allows a pinctrl_request_gpio()
>>> to succeed concurrently with a pin or pingroup being "owned" by a device for
>>> pin multiplexing.
>>
>> In order to resolve this, shouldn't we simply change the "should" at the
>> end of the first line I quoted to "must"? That way, there'd never be any
>> need to use pinctrl if you're only relying on gpiolib APIs.
>>
>> (and I'd argue that the text was already meant to say "must", so this
>> isn't actually a change to the intent, just a clarification).
> 
> It should deal with all the simple muxing use cases yes. And
> I am uncertain about the scope for this patch, if it only pertains
> to muxing, and in that case it would be solved by adding
> a proper GPIO backend to pinctrl-single.c.
> 
> But it doesn't help with some real-world usecases if I'm
> not mistaken.
> 
> If you want to set up a certain GPIO pin as pull-down (I guess
> this could be the case for a LED array), this cannot be done
> through any of these functions:
> 
> extern int pinctrl_request_gpio(unsigned gpio);
> extern void pinctrl_free_gpio(unsigned gpio);
> extern int pinctrl_gpio_direction_input(unsigned gpio);
> extern int pinctrl_gpio_direction_output(unsigned gpio);
> 
> So either we have to use a pin config hog to do this,

I'd certainly expect that to be the common case; I'd imagine it's pretty
common you'd never want to change the pulls at runtime, so hogging would
be appropriate.

> or we have
> to use devm_pinctrl_get_select_default(&pdev->dev); from the
> driver (as this patch does).

Yes, true.

> Either way it is using the pinctrl
> system orthogonally to the GPIO system, it doesn't happen
> from pinctrl_request_gpio() or so.
> 
> An alternative solution would be to add functions for
> controlling pinconfig and whatnot to the GPIO glue, which
> in turn would require adding frontends all over <linux/gpio.h>
> which in turn was the thing that Grant nixed to I got
> started with pinctrl instead...

Maybe the first gpio_request that GPIO passes to pinctrl could activate
some default "gpio" state or similar? But then you'd get into issues
with: what if the driver selects a pinctrl state for other reasons -
then you'd end up wanting multiple states active at once; the
gpiolib-requested state and the driver-requested state, and maybe they
conflict, ... probably madness ensues!

> But I'm open to any other suggestions. Would it be possible
> for pinctrl_request_gpio() to activate a pin config in the
> map for example? Currently it can only do muxing.
> 
> It's also possible to have the driver do something custom
> behind the back of pinctrl altogether as a response to
> pinctrl_request_gpio() but it wouldn't be
> any elegant...

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Linus Walleij]

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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Linus Walleij]

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

RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[AnilKumar, Chimata]

+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-l0cyMroinI0@public.gmane.org> 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-l0cyMroinI0@public.gmane.org>
> 
> 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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Linus Walleij]

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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Tony Lindgren]

* 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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Linus Walleij]

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

RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[AnilKumar, Chimata]

On Tue, Oct 02, 2012 at 01:29:37, Linus Walleij wrote:
> On Mon, Oct 1, 2012 at 5:44 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> 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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Linus Walleij]

On Wed, Oct 3, 2012 at 12:52 PM, AnilKumar, Chimata <anilkumar-l0cyMroinI0@public.gmane.org> 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

RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[AnilKumar, Chimata]

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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Linus Walleij]

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

RE: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[AnilKumar, Chimata]

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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Tony Lindgren]

* 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

Re: [PATCH v2] leds: leds-gpio: adopt pinctrl support

[Tony Lindgren]

* 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