linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] gpio: stmpe: Add DT support for stmpe gpio
@ 2012-11-23  5:51 Viresh Kumar
  2012-11-23 10:34 ` Lee Jones
  0 siblings, 1 reply; 15+ messages in thread
From: Viresh Kumar @ 2012-11-23  5:51 UTC (permalink / raw)
  To: linus.walleij, grant.likely
  Cc: devicetree-discuss, linux-kernel, spear-devel, lee.jones,
	Vipul Kumar Samar, Viresh Kumar

From: Vipul Kumar Samar <vipulkumar.samar@st.com>

This patch allows the STMPE GPIO driver to be successfully probed and
initialised when Device Tree support is enabled. Bindings are mentioned in
Documentation too.

Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/devicetree/bindings/gpio/gpio-stmpe.txt | 18 ++++++++++++++++++
 drivers/gpio/gpio-stmpe.c                             | 15 ++++++++++++---
 drivers/mfd/stmpe.c                                   |  2 ++
 3 files changed, 32 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-stmpe.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-stmpe.txt b/Documentation/devicetree/bindings/gpio/gpio-stmpe.txt
new file mode 100644
index 0000000..7f010e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-stmpe.txt
@@ -0,0 +1,18 @@
+STMPE gpio
+----------
+
+Required properties:
+ - compatible: "st,stmpe-gpio"
+
+Optional properties:
+ - norequest-mask: bitmask specifying which GPIOs should _not_ be requestable
+   due to different usage (e.g. touch, keypad)
+
+Node name must be stmpe_gpio and should be child node of stmpe node to which it
+belongs.
+
+Example:
+	stmpe_gpio {
+		compatible = "st,stmpe-gpio";
+		st,norequest-mask = <0x20>;	//gpio 5 can't be used
+	};
diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index dce3472..91455d4 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -12,6 +12,7 @@
 #include <linux/gpio.h>
 #include <linux/irq.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/mfd/stmpe.h>
 
 /*
@@ -304,6 +305,7 @@ static void stmpe_gpio_irq_remove(struct stmpe_gpio *stmpe_gpio)
 static int __devinit stmpe_gpio_probe(struct platform_device *pdev)
 {
 	struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
+	struct device_node *np = pdev->dev.of_node;
 	struct stmpe_gpio_platform_data *pdata;
 	struct stmpe_gpio *stmpe_gpio;
 	int ret;
@@ -321,12 +323,19 @@ static int __devinit stmpe_gpio_probe(struct platform_device *pdev)
 
 	stmpe_gpio->dev = &pdev->dev;
 	stmpe_gpio->stmpe = stmpe;
-	stmpe_gpio->norequest_mask = pdata ? pdata->norequest_mask : 0;
-
 	stmpe_gpio->chip = template_chip;
 	stmpe_gpio->chip.ngpio = stmpe->num_gpios;
 	stmpe_gpio->chip.dev = &pdev->dev;
-	stmpe_gpio->chip.base = pdata ? pdata->gpio_base : -1;
+
+	if (pdata) {
+		stmpe_gpio->norequest_mask = pdata->norequest_mask;
+		stmpe_gpio->chip.base = pdata->gpio_base;
+	} else {
+		stmpe_gpio->chip.base = -1;
+		if (np)
+			of_property_read_u32(np, "st,norequest-mask",
+					&pdata->norequest_mask);
+	}
 
 	if (irq >= 0)
 		stmpe_gpio->irq_base = stmpe->irq_base + STMPE_INT_GPIO(0);
diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index e2c0dda..c757ac3 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -298,12 +298,14 @@ static struct resource stmpe_gpio_resources[] = {
 
 static struct mfd_cell stmpe_gpio_cell = {
 	.name		= "stmpe-gpio",
+	.of_compatible	= "st,stmpe-gpio",
 	.resources	= stmpe_gpio_resources,
 	.num_resources	= ARRAY_SIZE(stmpe_gpio_resources),
 };
 
 static struct mfd_cell stmpe_gpio_cell_noirq = {
 	.name		= "stmpe-gpio",
+	.of_compatible	= "st,stmpe-gpio",
 	/* gpio cell resources consist of an irq only so no resources here */
 };
 
-- 
1.7.12.rc2.18.g61b472e


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-23  5:51 [PATCH] gpio: stmpe: Add DT support for stmpe gpio Viresh Kumar
@ 2012-11-23 10:34 ` Lee Jones
  2012-11-23 10:41   ` Lee Jones
  2012-11-23 10:43   ` Viresh Kumar
  0 siblings, 2 replies; 15+ messages in thread
From: Lee Jones @ 2012-11-23 10:34 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linus.walleij, grant.likely, devicetree-discuss, linux-kernel,
	spear-devel, Vipul Kumar Samar

On Fri, 23 Nov 2012, Viresh Kumar wrote:

> From: Vipul Kumar Samar <vipulkumar.samar@st.com>
> 
> This patch allows the STMPE GPIO driver to be successfully probed and
> initialised when Device Tree support is enabled. Bindings are mentioned in
> Documentation too.
> 
> Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  Documentation/devicetree/bindings/gpio/gpio-stmpe.txt | 18 ++++++++++++++++++
>  drivers/gpio/gpio-stmpe.c                             | 15 ++++++++++++---
>  drivers/mfd/stmpe.c                                   |  2 ++
>  3 files changed, 32 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-stmpe.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-stmpe.txt b/Documentation/devicetree/bindings/gpio/gpio-stmpe.txt
> new file mode 100644
> index 0000000..7f010e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-stmpe.txt
> @@ -0,0 +1,18 @@
> +STMPE gpio
> +----------
> +
> +Required properties:
> + - compatible: "st,stmpe-gpio"
> +
> +Optional properties:
> + - norequest-mask: bitmask specifying which GPIOs should _not_ be requestable
> +   due to different usage (e.g. touch, keypad)
> +
> +Node name must be stmpe_gpio and should be child node of stmpe node to which it
> +belongs.
> +
> +Example:
> +	stmpe_gpio {
> +		compatible = "st,stmpe-gpio";
> +		st,norequest-mask = <0x20>;	//gpio 5 can't be used
> +	};
> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
> index dce3472..91455d4 100644
> --- a/drivers/gpio/gpio-stmpe.c
> +++ b/drivers/gpio/gpio-stmpe.c
> @@ -12,6 +12,7 @@
>  #include <linux/gpio.h>
>  #include <linux/irq.h>
>  #include <linux/interrupt.h>
> +#include <linux/of.h>
>  #include <linux/mfd/stmpe.h>
>  
>  /*
> @@ -304,6 +305,7 @@ static void stmpe_gpio_irq_remove(struct stmpe_gpio *stmpe_gpio)
>  static int __devinit stmpe_gpio_probe(struct platform_device *pdev)
>  {
>  	struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
> +	struct device_node *np = pdev->dev.of_node;
>  	struct stmpe_gpio_platform_data *pdata;
>  	struct stmpe_gpio *stmpe_gpio;
>  	int ret;
> @@ -321,12 +323,19 @@ static int __devinit stmpe_gpio_probe(struct platform_device *pdev)
>  
>  	stmpe_gpio->dev = &pdev->dev;
>  	stmpe_gpio->stmpe = stmpe;
> -	stmpe_gpio->norequest_mask = pdata ? pdata->norequest_mask : 0;
> -
>  	stmpe_gpio->chip = template_chip;
>  	stmpe_gpio->chip.ngpio = stmpe->num_gpios;
>  	stmpe_gpio->chip.dev = &pdev->dev;
> -	stmpe_gpio->chip.base = pdata ? pdata->gpio_base : -1;

Why have you deleted this?

> +
> +	if (pdata) {
> +		stmpe_gpio->norequest_mask = pdata->norequest_mask;
> +		stmpe_gpio->chip.base = pdata->gpio_base;

Then added this?

> +	} else {
> +		stmpe_gpio->chip.base = -1;

And this?

Just leave the top line in and it saves you lots of complecations.

> +		if (np)
> +			of_property_read_u32(np, "st,norequest-mask",
> +					&pdata->norequest_mask);

Can you explain to me what this does?

> +	}
>  
>  	if (irq >= 0)
>  		stmpe_gpio->irq_base = stmpe->irq_base + STMPE_INT_GPIO(0);
> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> index e2c0dda..c757ac3 100644
> --- a/drivers/mfd/stmpe.c
> +++ b/drivers/mfd/stmpe.c
> @@ -298,12 +298,14 @@ static struct resource stmpe_gpio_resources[] = {
>  
>  static struct mfd_cell stmpe_gpio_cell = {
>  	.name		= "stmpe-gpio",
> +	.of_compatible	= "st,stmpe-gpio",

There's no need for any of the STMPE to have their own compatible
string, as they are MFD devices. They are registered as platform
devices from the MFD subsystem.

>  	.resources	= stmpe_gpio_resources,
>  	.num_resources	= ARRAY_SIZE(stmpe_gpio_resources),
>  };
>  
>  static struct mfd_cell stmpe_gpio_cell_noirq = {
>  	.name		= "stmpe-gpio",
> +	.of_compatible	= "st,stmpe-gpio",
>  	/* gpio cell resources consist of an irq only so no resources here */
>  };
>  
> -- 
> 1.7.12.rc2.18.g61b472e
> 

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-23 10:34 ` Lee Jones
@ 2012-11-23 10:41   ` Lee Jones
  2012-11-23 10:47     ` Viresh Kumar
  2012-11-23 10:43   ` Viresh Kumar
  1 sibling, 1 reply; 15+ messages in thread
From: Lee Jones @ 2012-11-23 10:41 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linus.walleij, grant.likely, devicetree-discuss, linux-kernel,
	spear-devel, Vipul Kumar Samar

> >  static struct mfd_cell stmpe_gpio_cell = {
> >  	.name		= "stmpe-gpio",
> > +	.of_compatible	= "st,stmpe-gpio",
> 
> There's no need for any of the STMPE to have their own compatible
> string, as they are MFD devices. They are registered as platform
> devices from the MFD subsystem.

Whoops, I've written this in the wrong place. 

Sorry, for the confusion. It does need to be here.

> >  	.resources	= stmpe_gpio_resources,
> >  	.num_resources	= ARRAY_SIZE(stmpe_gpio_resources),
> >  };
> >  
> >  static struct mfd_cell stmpe_gpio_cell_noirq = {
> >  	.name		= "stmpe-gpio",
> > +	.of_compatible	= "st,stmpe-gpio",

... and here.

> > +++ b/Documentation/devicetree/bindings/gpio/gpio-stmpe.txt
> > @@ -0,0 +1,18 @@
> > +STMPE gpio
> > +----------
> > +
> > +Required properties:
> > + - compatible: "st,stmpe-gpio"

... but this is wrong.

> > +Example:
> > +	stmpe_gpio {
> > +		compatible = "st,stmpe-gpio";
> > +		st,norequest-mask = <0x20>;	//gpio 5 can't be used
> > +	};

As is the example.

So will be the the DT - if you've already written it.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-23 10:34 ` Lee Jones
  2012-11-23 10:41   ` Lee Jones
@ 2012-11-23 10:43   ` Viresh Kumar
  2012-11-23 12:14     ` Lee Jones
  1 sibling, 1 reply; 15+ messages in thread
From: Viresh Kumar @ 2012-11-23 10:43 UTC (permalink / raw)
  To: Lee Jones
  Cc: linus.walleij, grant.likely, devicetree-discuss, linux-kernel,
	spear-devel, Vipul Kumar Samar

On 23 November 2012 16:04, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 23 Nov 2012, Viresh Kumar wrote:
>> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c

>>  static int __devinit stmpe_gpio_probe(struct platform_device *pdev)
>>  {

>> -     stmpe_gpio->chip.base = pdata ? pdata->gpio_base : -1;
>
> Why have you deleted this?
>
>> +
>> +     if (pdata) {
>> +             stmpe_gpio->norequest_mask = pdata->norequest_mask;
>> +             stmpe_gpio->chip.base = pdata->gpio_base;
>
> Then added this?
>
>> +     } else {
>> +             stmpe_gpio->chip.base = -1;
>
> And this?

To group all non-DT assignments in a single if block, instead of two.

> Just leave the top line in and it saves you lots of complecations.

Sorry, Couldn't get this one.

>> +             if (np)
>> +                     of_property_read_u32(np, "st,norequest-mask",
>> +                                     &pdata->norequest_mask);
>
> Can you explain to me what this does?

You mean pdata->norequest_mask?  It marks few gpios as unusable.
Because these pads might be used by other blocks of stmpe.

>> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c

>>  static struct mfd_cell stmpe_gpio_cell = {
>>       .name           = "stmpe-gpio",
>> +     .of_compatible  = "st,stmpe-gpio",
>
> There's no need for any of the STMPE to have their own compatible
> string, as they are MFD devices. They are registered as platform
> devices from the MFD subsystem.

This is required by mfd-core.c, mfd_add_device() isn't it?

	if (parent->of_node && cell->of_compatible) {
		for_each_child_of_node(parent->of_node, np) {
			if (of_device_is_compatible(np, cell->of_compatible)) {
				pdev->dev.of_node = np;
				break;
			}
		}
	}

--
viresh

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-23 10:41   ` Lee Jones
@ 2012-11-23 10:47     ` Viresh Kumar
  0 siblings, 0 replies; 15+ messages in thread
From: Viresh Kumar @ 2012-11-23 10:47 UTC (permalink / raw)
  To: Lee Jones
  Cc: linus.walleij, grant.likely, devicetree-discuss, linux-kernel,
	spear-devel, Vipul Kumar Samar

On 23 November 2012 16:11, Lee Jones <lee.jones@linaro.org> wrote:
>> > +++ b/Documentation/devicetree/bindings/gpio/gpio-stmpe.txt
>> > @@ -0,0 +1,18 @@
>> > +STMPE gpio
>> > +----------
>> > +
>> > +Required properties:
>> > + - compatible: "st,stmpe-gpio"
>
> ... but this is wrong.
>
>> > +Example:
>> > +   stmpe_gpio {
>> > +           compatible = "st,stmpe-gpio";
>> > +           st,norequest-mask = <0x20>;     //gpio 5 can't be used
>> > +   };
>
> As is the example.
>
> So will be the the DT - if you've already written it.

Again, I believe these are required by the code you wrote in mfd-core.c

        if (parent->of_node && cell->of_compatible) {
                for_each_child_of_node(parent->of_node, np) {
                        if (of_device_is_compatible(np, cell->of_compatible)) {
                                pdev->dev.of_node = np;
                                break;
                        }
                }
        }

This matches compatible of child node with compatible of cell. And that's
why you have added that in your keypad mappings as well.

--
viresh

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-23 10:43   ` Viresh Kumar
@ 2012-11-23 12:14     ` Lee Jones
  2012-11-23 12:25       ` Viresh Kumar
  2012-11-23 12:30       ` Shiraz Hashim
  0 siblings, 2 replies; 15+ messages in thread
From: Lee Jones @ 2012-11-23 12:14 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linus.walleij, grant.likely, devicetree-discuss, linux-kernel,
	spear-devel, Vipul Kumar Samar

> >> -     stmpe_gpio->chip.base = pdata ? pdata->gpio_base : -1;
> >
> > Why have you deleted this?
> >
> >> +
> >> +     if (pdata) {
> >> +             stmpe_gpio->norequest_mask = pdata->norequest_mask;
> >> +             stmpe_gpio->chip.base = pdata->gpio_base;
> >
> > Then added this?
> >
> >> +     } else {
> >> +             stmpe_gpio->chip.base = -1;
> >
> > And this?
> 
> To group all non-DT assignments in a single if block, instead of two.

That assignment has nothing to do with DT.

> > Just leave the top line in and it saves you lots of complecations.
> 
> Sorry, Couldn't get this one.

I'm saying, just leave it where it is.

> >> +             if (np)
> >> +                     of_property_read_u32(np, "st,norequest-mask",
> >> +                                     &pdata->norequest_mask);
> >
> > Can you explain to me what this does?
> 
> You mean pdata->norequest_mask?  It marks few gpios as unusable.
> Because these pads might be used by other blocks of stmpe.

I'm not sure if that should be set with DT or not.

Second opinion anyone?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-23 12:14     ` Lee Jones
@ 2012-11-23 12:25       ` Viresh Kumar
  2012-11-23 15:45         ` Lee Jones
  2012-11-23 12:30       ` Shiraz Hashim
  1 sibling, 1 reply; 15+ messages in thread
From: Viresh Kumar @ 2012-11-23 12:25 UTC (permalink / raw)
  To: Lee Jones
  Cc: linus.walleij, grant.likely, devicetree-discuss, linux-kernel,
	spear-devel, Vipul Kumar Samar

On 23 November 2012 17:44, Lee Jones <lee.jones@linaro.org> wrote:
> I'm saying, just leave it where it is.

So you are suggesting this code:

        stmpe_gpio->chip.base = pdata ? pdata->gpio_base : -1;

       if (pdata)
               stmpe_gpio->norequest_mask = pdata->norequest_mask;
       else if (np)
               of_property_read_u32(np, "st,norequest-mask",
&pdata->norequest_mask);

Right? Then yes i can do it.

>> >> +             if (np)
>> >> +                     of_property_read_u32(np, "st,norequest-mask",
>> >> +                                     &pdata->norequest_mask);
>> >
>> > Can you explain to me what this does?
>>
>> You mean pdata->norequest_mask?  It marks few gpios as unusable.
>> Because these pads might be used by other blocks of stmpe.
>
> I'm not sure if that should be set with DT or not.
>
> Second opinion anyone?

Why i kept it in DT is because it is board dependent and there is no better
way of communicating this from board to driver.

--
viresh

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-23 12:14     ` Lee Jones
  2012-11-23 12:25       ` Viresh Kumar
@ 2012-11-23 12:30       ` Shiraz Hashim
  2012-11-26 11:28         ` Lee Jones
  1 sibling, 1 reply; 15+ messages in thread
From: Shiraz Hashim @ 2012-11-23 12:30 UTC (permalink / raw)
  To: Lee Jones
  Cc: Viresh Kumar, linus.walleij, grant.likely, devicetree-discuss,
	linux-kernel, spear-devel, Vipul Kumar Samar

On Fri, Nov 23, 2012 at 12:14:13PM +0000, Lee Jones wrote:
> > >> +             if (np)
> > >> +                     of_property_read_u32(np, "st,norequest-mask",
> > >> +                                     &pdata->norequest_mask);
> > >
> > > Can you explain to me what this does?
> > 
> > You mean pdata->norequest_mask?  It marks few gpios as unusable.
> > Because these pads might be used by other blocks of stmpe.
> 
> I'm not sure if that should be set with DT or not.
> 
> Second opinion anyone?

This is a board dependent parameter which just informs gpio driver
about pins, which must not be requested. It may happen for a stmpe
variant where such gpio pins are multiplexed with some other
function.

Hence it must be part of DT itself.

--
regards
Shiraz

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-23 12:25       ` Viresh Kumar
@ 2012-11-23 15:45         ` Lee Jones
  0 siblings, 0 replies; 15+ messages in thread
From: Lee Jones @ 2012-11-23 15:45 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linus.walleij, grant.likely, devicetree-discuss, linux-kernel,
	spear-devel, Vipul Kumar Samar

On Fri, 23 Nov 2012, Viresh Kumar wrote:

> On 23 November 2012 17:44, Lee Jones <lee.jones@linaro.org> wrote:
> > I'm saying, just leave it where it is.
> 
> So you are suggesting this code:
> 
>         stmpe_gpio->chip.base = pdata ? pdata->gpio_base : -1;
> 
>        if (pdata)
>                stmpe_gpio->norequest_mask = pdata->norequest_mask;
>        else if (np)
>                of_property_read_u32(np, "st,norequest-mask",
> &pdata->norequest_mask);
> 
> Right? Then yes i can do it.

It would be better if you'd sent it as a diff, but yes, leave the
top line as it is and just add the norequest-mask stuff (if it's
required).

> >> >> +             if (np)
> >> >> +                     of_property_read_u32(np, "st,norequest-mask",
> >> >> +                                     &pdata->norequest_mask);
> >> >
> >> > Can you explain to me what this does?
> >>
> >> You mean pdata->norequest_mask?  It marks few gpios as unusable.
> >> Because these pads might be used by other blocks of stmpe.
> >
> > I'm not sure if that should be set with DT or not.
> >
> > Second opinion anyone?
> 
> Why i kept it in DT is because it is board dependent and there is no better
> way of communicating this from board to driver.

I can't comment, as I really don't know.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-23 12:30       ` Shiraz Hashim
@ 2012-11-26 11:28         ` Lee Jones
  2012-11-26 11:32           ` Shiraz Hashim
  2012-12-01 19:34           ` Linus Walleij
  0 siblings, 2 replies; 15+ messages in thread
From: Lee Jones @ 2012-11-26 11:28 UTC (permalink / raw)
  To: Shiraz Hashim, linus.walleij
  Cc: Viresh Kumar, linus.walleij, grant.likely, devicetree-discuss,
	linux-kernel, spear-devel, Vipul Kumar Samar

On Fri, 23 Nov 2012, Shiraz Hashim wrote:

> On Fri, Nov 23, 2012 at 12:14:13PM +0000, Lee Jones wrote:
> > > >> +             if (np)
> > > >> +                     of_property_read_u32(np, "st,norequest-mask",
> > > >> +                                     &pdata->norequest_mask);
> > > >
> > > > Can you explain to me what this does?
> > > 
> > > You mean pdata->norequest_mask?  It marks few gpios as unusable.
> > > Because these pads might be used by other blocks of stmpe.
> > 
> > I'm not sure if that should be set with DT or not.
> > 
> > Second opinion anyone?
> 
> This is a board dependent parameter which just informs gpio driver
> about pins, which must not be requested. It may happen for a stmpe
> variant where such gpio pins are multiplexed with some other
> function.
> 
> Hence it must be part of DT itself.

Doesn't pinctrl normally handle this kind of stuff?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-26 11:28         ` Lee Jones
@ 2012-11-26 11:32           ` Shiraz Hashim
  2012-12-01 16:35             ` Linus Walleij
  2012-12-01 19:34           ` Linus Walleij
  1 sibling, 1 reply; 15+ messages in thread
From: Shiraz Hashim @ 2012-11-26 11:32 UTC (permalink / raw)
  To: Lee Jones
  Cc: linus.walleij, Viresh Kumar, grant.likely, devicetree-discuss,
	linux-kernel, spear-devel, Vipul Kumar Samar

On Mon, Nov 26, 2012 at 11:28:23AM +0000, Lee Jones wrote:
> On Fri, 23 Nov 2012, Shiraz Hashim wrote:
> 
> > On Fri, Nov 23, 2012 at 12:14:13PM +0000, Lee Jones wrote:
> > > > >> +             if (np)
> > > > >> +                     of_property_read_u32(np, "st,norequest-mask",
> > > > >> +                                     &pdata->norequest_mask);
> > > > >
> > > > > Can you explain to me what this does?
> > > > 
> > > > You mean pdata->norequest_mask?  It marks few gpios as unusable.
> > > > Because these pads might be used by other blocks of stmpe.
> > > 
> > > I'm not sure if that should be set with DT or not.
> > > 
> > > Second opinion anyone?
> > 
> > This is a board dependent parameter which just informs gpio driver
> > about pins, which must not be requested. It may happen for a stmpe
> > variant where such gpio pins are multiplexed with some other
> > function.
> > 
> > Hence it must be part of DT itself.
> 
> Doesn't pinctrl normally handle this kind of stuff?

Yes, but I think it is only for managing the SoC and its pads.

--
regards
Shiraz

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-26 11:32           ` Shiraz Hashim
@ 2012-12-01 16:35             ` Linus Walleij
  2012-12-01 18:31               ` Shiraz Hashim
  0 siblings, 1 reply; 15+ messages in thread
From: Linus Walleij @ 2012-12-01 16:35 UTC (permalink / raw)
  To: Shiraz Hashim
  Cc: Lee Jones, Viresh Kumar, grant.likely, devicetree-discuss,
	linux-kernel, spear-devel, Vipul Kumar Samar

On Mon, Nov 26, 2012 at 12:32 PM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> On Mon, Nov 26, 2012 at 11:28:23AM +0000, Lee Jones wrote:
>>
>> Doesn't pinctrl normally handle this kind of stuff?
>
> Yes, but I think it is only for managing the SoC and its pads.

No pinctrl is also for off-SoC pin controllers, you can have
pinctrl0, pinctrl1 ... pinctrlN on a system. The number space is
local per-pincontroller too.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-12-01 16:35             ` Linus Walleij
@ 2012-12-01 18:31               ` Shiraz Hashim
  0 siblings, 0 replies; 15+ messages in thread
From: Shiraz Hashim @ 2012-12-01 18:31 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Viresh Kumar, devicetree-discuss, spear-devel, linux-kernel, Lee Jones

On Sat, Dec 1, 2012 at 10:05 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Nov 26, 2012 at 12:32 PM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
>> On Mon, Nov 26, 2012 at 11:28:23AM +0000, Lee Jones wrote:
>>>
>>> Doesn't pinctrl normally handle this kind of stuff?
>>
>> Yes, but I think it is only for managing the SoC and its pads.
>
> No pinctrl is also for off-SoC pin controllers, you can have
> pinctrl0, pinctrl1 ... pinctrlN on a system. The number space is
> local per-pincontroller too.

OK. Thanks for correcting.

-- 
regards
Shiraz Hashim

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-11-26 11:28         ` Lee Jones
  2012-11-26 11:32           ` Shiraz Hashim
@ 2012-12-01 19:34           ` Linus Walleij
  2012-12-01 19:55             ` Linus Walleij
  1 sibling, 1 reply; 15+ messages in thread
From: Linus Walleij @ 2012-12-01 19:34 UTC (permalink / raw)
  To: Lee Jones
  Cc: Shiraz Hashim, Viresh Kumar, grant.likely, devicetree-discuss,
	linux-kernel, spear-devel, Vipul Kumar Samar, Rabin VINCENT

On Mon, Nov 26, 2012 at 12:28 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 23 Nov 2012, Shiraz Hashim wrote:

[About st-norequest-mask]
>> This is a board dependent parameter which just informs gpio driver
>> about pins, which must not be requested. It may happen for a stmpe
>> variant where such gpio pins are multiplexed with some other
>> function.
>>
>> Hence it must be part of DT itself.
>
> Doesn't pinctrl normally handle this kind of stuff?

So this is a signal that something might be strange about the
driver architecture at large.

The datasheet for STMPE1601 says:

"The STMPE1601 offers great flexibility, as each
I/O can be configured as input, output or specific
functions."

Hm hm. Well this driver existed before the pin control
system so we have to live with this. We *could* assume
that the above DT property could be set beacause someone
connected a GPIO to ground and trying to use it
would burn the system or something. But if it's actually
dealing with the sideeffects of pinmuxing it's done in the
wrong place.

Apart from the pinctrl API another way to handle these
beasts is what I do in the drivers/mfd/tps6105x.c,
where the hardware is such that you basically always
nail down the hardware for one specific use case
of 3 possible. Then it's done by configuring the root
MFD device.

The crucial question is: can the STMPE controllers be
used for GPIO, PWM, keypad and touchscreen at the
*same time* or are they nailed to *one* usecase during
electronics design?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] gpio: stmpe: Add DT support for stmpe gpio
  2012-12-01 19:34           ` Linus Walleij
@ 2012-12-01 19:55             ` Linus Walleij
  0 siblings, 0 replies; 15+ messages in thread
From: Linus Walleij @ 2012-12-01 19:55 UTC (permalink / raw)
  To: Lee Jones
  Cc: Shiraz Hashim, Viresh Kumar, grant.likely, devicetree-discuss,
	linux-kernel, spear-devel, Vipul Kumar Samar, Rabin VINCENT

On Sat, Dec 1, 2012 at 8:34 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> The crucial question is: can the STMPE controllers be
> used for GPIO, PWM, keypad and touchscreen at the
> *same time* or are they nailed to *one* usecase during
> electronics design?

Looking closer at the datasheet it seems to mux pins on individual per-pin
basis, so it is indeed pin-controller-like.

Was this driver submitted today, I would probably request that the
pinctrl + GPIO properties be properly exposed in the pinctrl
subsystem instead.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2012-12-01 19:55 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-23  5:51 [PATCH] gpio: stmpe: Add DT support for stmpe gpio Viresh Kumar
2012-11-23 10:34 ` Lee Jones
2012-11-23 10:41   ` Lee Jones
2012-11-23 10:47     ` Viresh Kumar
2012-11-23 10:43   ` Viresh Kumar
2012-11-23 12:14     ` Lee Jones
2012-11-23 12:25       ` Viresh Kumar
2012-11-23 15:45         ` Lee Jones
2012-11-23 12:30       ` Shiraz Hashim
2012-11-26 11:28         ` Lee Jones
2012-11-26 11:32           ` Shiraz Hashim
2012-12-01 16:35             ` Linus Walleij
2012-12-01 18:31               ` Shiraz Hashim
2012-12-01 19:34           ` Linus Walleij
2012-12-01 19:55             ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).