[PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order

[PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order

[Viresh Kumar]

This helps managing them better and also reduces chances of adding an header
file twice.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/mfd/stmpe-spi.c | 2 +-
 drivers/mfd/stmpe.c     | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/stmpe-spi.c b/drivers/mfd/stmpe-spi.c
index 9edfe86..f86e3fc 100644
--- a/drivers/mfd/stmpe-spi.c
+++ b/drivers/mfd/stmpe-spi.c
@@ -7,10 +7,10 @@
  * Author: Viresh Kumar <viresh.linux@gmail.com> for ST Microelectronics
  */
 
-#include <linux/spi/spi.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/spi/spi.h>
 #include <linux/types.h>
 #include "stmpe.h"
 
diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index 8a4247b..6086481 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -7,15 +7,15 @@
  * Author: Rabin Vincent <rabin.vincent@stericsson.com> for ST-Ericsson
  */
 
-#include <linux/gpio.h>
 #include <linux/export.h>
-#include <linux/kernel.h>
+#include <linux/gpio.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
-#include <linux/mfd/core.h>
 #include "stmpe.h"
 
 static int __stmpe_enable(struct stmpe *stmpe, unsigned int blocks)
-- 
1.7.12.rc2.18.g61b472e

[PATCH V1 2/3] mfd: stmpe-i2c: Move .driver structure fields inside {} in stmpe_i2c_driver

[Viresh Kumar]

Currently, few fields in stmpe_i2c_driver are initialized as:
.driver.owner = THIS_MODULE,

Group them under {}, like:
.driver = {
	.owner = THIS_MODULE,
	...
},

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/mfd/stmpe-i2c.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/stmpe-i2c.c b/drivers/mfd/stmpe-i2c.c
index 947a06a..c734dc3 100644
--- a/drivers/mfd/stmpe-i2c.c
+++ b/drivers/mfd/stmpe-i2c.c
@@ -82,11 +82,13 @@ static const struct i2c_device_id stmpe_i2c_id[] = {
 MODULE_DEVICE_TABLE(i2c, stmpe_id);
 
 static struct i2c_driver stmpe_i2c_driver = {
-	.driver.name	= "stmpe-i2c",
-	.driver.owner	= THIS_MODULE,
+	.driver = {
+		.name = "stmpe-i2c",
+		.owner = THIS_MODULE,
 #ifdef CONFIG_PM
-	.driver.pm	= &stmpe_dev_pm_ops,
+		.pm = &stmpe_dev_pm_ops,
 #endif
+	},
 	.probe		= stmpe_i2c_probe,
 	.remove		= __devexit_p(stmpe_i2c_remove),
 	.id_table	= stmpe_i2c_id,
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH V1 2/3] mfd: stmpe-i2c: Move .driver structure fields inside {} in stmpe_i2c_driver

[Samuel Ortiz]

Hi Viresh,

On Fri, Nov 23, 2012 at 12:26:19AM +0530, Viresh Kumar wrote:
> Currently, few fields in stmpe_i2c_driver are initialized as:
> .driver.owner = THIS_MODULE,
> 
> Group them under {}, like:
> .driver = {
> 	.owner = THIS_MODULE,
> 	...
> },
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/mfd/stmpe-i2c.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
Applied, thanks.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Viresh Kumar]

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

This patch extends existing DT support for stmpe devices. This updates:
- DT support from stmpe SPI and I2C drivers
- missing header files in stmpe.c
- stmpe_of_probe() with pwm, rotator and new bindings.
- Bindings are updated in binding document.

Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2->V3:
------
- Removed sub-modules DT bindings from this patch
- Retain original work done by Lee Jones

 Documentation/devicetree/bindings/mfd/stmpe.txt | 12 ++++++++----
 drivers/mfd/stmpe-i2c.c                         | 15 +++++++++++++++
 drivers/mfd/stmpe-spi.c                         | 15 +++++++++++++++
 drivers/mfd/stmpe.c                             | 21 +++++++++++++++------
 4 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
index 8f0aeda..8f65c8d 100644
--- a/Documentation/devicetree/bindings/mfd/stmpe.txt
+++ b/Documentation/devicetree/bindings/mfd/stmpe.txt
@@ -1,13 +1,17 @@
-* STMPE Multi-Functional Device
+* ST Microelectronics STMPE Multi-Functional Device
+
+STMPE is an MFD device which may expose following inbuilt devices: gpio, keypad,
+touchscreen, adc, pwm, rotator.
 
 Required properties:
- - compatible                   : "st,stmpe[811|1601|2401|2403]"
- - reg                          : I2C address of the device
+ - compatible                   : "st,stmpe[610|801|811|1601|2401|2403]"
+ - reg                          : I2C/SPI address of the device
 
 Optional properties:
  - interrupts                   : The interrupt outputs from the controller
- - interrupt-controller         : Marks the device node as an interrupt controller
  - interrupt-parent             : Specifies which IRQ controller we're connected to
+ - irq-trigger			: IRQ trigger to use for the interrupt to the host
+ - irq-invert-polarity		: bool, IRQ line is connected with reversed polarity
  - i2c-client-wake              : Marks the input device as wakable
  - st,autosleep-timeout         : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024
 
diff --git a/drivers/mfd/stmpe-i2c.c b/drivers/mfd/stmpe-i2c.c
index c734dc3..003c49b 100644
--- a/drivers/mfd/stmpe-i2c.c
+++ b/drivers/mfd/stmpe-i2c.c
@@ -13,6 +13,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/types.h>
 #include "stmpe.h"
 
@@ -81,6 +82,19 @@ static const struct i2c_device_id stmpe_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, stmpe_id);
 
+#ifdef CONFIG_OF
+static const struct of_device_id stmpe_dt_ids[] = {
+	{ .compatible = "st,stmpe610", .data = &stmpe_i2c_id[0], },
+	{ .compatible = "st,stmpe801", .data = &stmpe_i2c_id[1], },
+	{ .compatible = "st,stmpe811", .data = &stmpe_i2c_id[2], },
+	{ .compatible = "st,stmpe1601", .data = &stmpe_i2c_id[3], },
+	{ .compatible = "st,stmpe2401", .data = &stmpe_i2c_id[4], },
+	{ .compatible = "st,stmpe2403", .data = &stmpe_i2c_id[5], },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
+#endif
+
 static struct i2c_driver stmpe_i2c_driver = {
 	.driver = {
 		.name = "stmpe-i2c",
@@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = {
 #ifdef CONFIG_PM
 		.pm = &stmpe_dev_pm_ops,
 #endif
+		.of_match_table = of_match_ptr(stmpe_dt_ids),
 	},
 	.probe		= stmpe_i2c_probe,
 	.remove		= __devexit_p(stmpe_i2c_remove),
diff --git a/drivers/mfd/stmpe-spi.c b/drivers/mfd/stmpe-spi.c
index f86e3fc..e482f5f 100644
--- a/drivers/mfd/stmpe-spi.c
+++ b/drivers/mfd/stmpe-spi.c
@@ -10,6 +10,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/spi/spi.h>
 #include <linux/types.h>
 #include "stmpe.h"
@@ -119,6 +120,19 @@ static const struct spi_device_id stmpe_spi_id[] = {
 };
 MODULE_DEVICE_TABLE(spi, stmpe_id);
 
+#ifdef CONFIG_OF
+static const struct of_device_id stmpe_dt_ids[] = {
+	{ .compatible = "st,stmpe610", .data = (void *)STMPE610, },
+	{ .compatible = "st,stmpe801", .data = (void *)STMPE801, },
+	{ .compatible = "st,stmpe811", .data = (void *)STMPE811, },
+	{ .compatible = "st,stmpe1601", .data = (void *)STMPE1601, },
+	{ .compatible = "st,stmpe2401", .data = (void *)STMPE2401, },
+	{ .compatible = "st,stmpe2403", .data = (void *)STMPE2403, },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
+#endif
+
 static struct spi_driver stmpe_spi_driver = {
 	.driver = {
 		.name	= "stmpe-spi",
@@ -126,6 +140,7 @@ static struct spi_driver stmpe_spi_driver = {
 #ifdef CONFIG_PM
 		.pm	= &stmpe_dev_pm_ops,
 #endif
+		.of_match_table = of_match_ptr(stmpe_dt_ids),
 	},
 	.probe		= stmpe_spi_probe,
 	.remove		= __devexit_p(stmpe_spi_remove),
diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index 6086481..e2c0dda 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -7,6 +7,7 @@
  * Author: Rabin Vincent <rabin.vincent@stericsson.com> for ST-Ericsson
  */
 
+#include <linux/err.h>
 #include <linux/export.h>
 #include <linux/gpio.h>
 #include <linux/interrupt.h>
@@ -14,6 +15,8 @@
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/mfd/core.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
 #include "stmpe.h"
@@ -1028,18 +1031,24 @@ void __devinit stmpe_of_probe(struct stmpe_platform_data *pdata,
 
 	pdata->autosleep = (pdata->autosleep_timeout) ? true : false;
 
+	of_property_read_u32(np, "irq-trigger", &pdata->irq_trigger);
+
+	if (of_property_read_bool(np, "irq-invert-polarity"))
+		pdata->irq_invert_polarity = true;
+
 	for_each_child_of_node(np, child) {
 		if (!strcmp(child->name, "stmpe_gpio")) {
 			pdata->blocks |= STMPE_BLOCK_GPIO;
-		}
-		if (!strcmp(child->name, "stmpe_keypad")) {
+		} else if (!strcmp(child->name, "stmpe_keypad")) {
 			pdata->blocks |= STMPE_BLOCK_KEYPAD;
-		}
-		if (!strcmp(child->name, "stmpe_touchscreen")) {
+		} else if (!strcmp(child->name, "stmpe_touchscreen")) {
 			pdata->blocks |= STMPE_BLOCK_TOUCHSCREEN;
-		}
-		if (!strcmp(child->name, "stmpe_adc")) {
+		} else if (!strcmp(child->name, "stmpe_adc")) {
 			pdata->blocks |= STMPE_BLOCK_ADC;
+		} else if (!strcmp(child->name, "stmpe_pwm")) {
+			pdata->blocks |= STMPE_BLOCK_PWM;
+		} else if (!strcmp(child->name, "stmpe_rotator")) {
+			pdata->blocks |= STMPE_BLOCK_ROTATOR;
 		}
 	}
 }
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Grant Likely]

On Fri, 23 Nov 2012 00:26:20 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> From: Vipul Kumar Samar <vipulkumar.samar@st.com>
> 
> This patch extends existing DT support for stmpe devices. This updates:
> - DT support from stmpe SPI and I2C drivers
> - missing header files in stmpe.c
> - stmpe_of_probe() with pwm, rotator and new bindings.
> - Bindings are updated in binding document.
> 
> Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2->V3:
> ------
> - Removed sub-modules DT bindings from this patch
> - Retain original work done by Lee Jones
> 
>  Documentation/devicetree/bindings/mfd/stmpe.txt | 12 ++++++++----
>  drivers/mfd/stmpe-i2c.c                         | 15 +++++++++++++++
>  drivers/mfd/stmpe-spi.c                         | 15 +++++++++++++++
>  drivers/mfd/stmpe.c                             | 21 +++++++++++++++------
>  4 files changed, 53 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
> index 8f0aeda..8f65c8d 100644
> --- a/Documentation/devicetree/bindings/mfd/stmpe.txt
> +++ b/Documentation/devicetree/bindings/mfd/stmpe.txt
> @@ -1,13 +1,17 @@
> -* STMPE Multi-Functional Device
> +* ST Microelectronics STMPE Multi-Functional Device
> +
> +STMPE is an MFD device which may expose following inbuilt devices: gpio, keypad,
> +touchscreen, adc, pwm, rotator.
>  
>  Required properties:
> - - compatible                   : "st,stmpe[811|1601|2401|2403]"
> - - reg                          : I2C address of the device
> + - compatible                   : "st,stmpe[610|801|811|1601|2401|2403]"
> + - reg                          : I2C/SPI address of the device
>  
>  Optional properties:
>   - interrupts                   : The interrupt outputs from the controller
> - - interrupt-controller         : Marks the device node as an interrupt controller
>   - interrupt-parent             : Specifies which IRQ controller we're connected to
> + - irq-trigger			: IRQ trigger to use for the interrupt to the host
> + - irq-invert-polarity		: bool, IRQ line is connected with reversed polarity

This looks odd. Normally the interrupt polarity should be encoded in the irq
specifier flags field.

g.

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Viresh Kumar]

On 23 November 2012 15:11, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, 23 Nov 2012 00:26:20 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:

>> + - irq-trigger                       : IRQ trigger to use for the interrupt to the host
>> + - irq-invert-polarity               : bool, IRQ line is connected with reversed polarity
>
> This looks odd. Normally the interrupt polarity should be encoded in the irq
> specifier flags field.

Hi Grant and Lee Jones,

This looks odd because stmpe is odd, i am taking the discussion held
with Lee jones to this thread.

So, how interrupt stuff works currently in DT..
We have a interrupt controller IC:

	ic: interrupt-controller@40008000 {
		compatible = "foo";
		interrupt-controller;
		#interrupt-cells = <2>;
                ...
	};

And we have a user of this IC:

	foo-peripheral@40048000 {
		compatible = "foo-peripheral";
		interrupt-parent = <&ic>;
		interrupts = <39 4>;
	};

Here first field of "interrupts" gives interrupt line number and the second one
gives polarity, interrupt type etc..

All is good till now. Now, every interrupt controller supports the first
field, but the second one depends on its capabilities. An interrupt controller
might not have registers to configure interrupt polarity, type, etc of
the interrupt
it will service and so the second field wouldn't be available for them.

For now just think stmpe is not a MFD and not a interrupt controller
either. It is
just a simple device, dev-foo.

It will declare values of its interrupts field based on the type of
interrupt controller
that will service its interrupt and that can be anything like VIC/GIC/GPIO
controller.

Obviously nobody else than the parent IC driver can parse interrupts field
of dev-foo, because only that driver understands the real meaning of
these fields.

Now, stmpe has a special property. It can decide the way its output
interrupt line
will work. i.e. its polarity and interrupt type - edle/level, etc..
This is not commonly
seen in any peripheral. Now my original bindings and the real question here is
about passing this information to stmpe driver.

I can't pass it in interrupts field of stmpe node, as that field
belongs to parent
interrupt controller of stmpe.

I can't pass that from child nodes of stmpe, as we are programming the interrupt
coming out of stmpe and not the interrupt coming out of stmpe-gpio or
stmpe-keypad.

And that's why i added these bindings. Please suggest me if i am still missing
something.

--
viresh

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Lee Jones]

On Fri, 23 Nov 2012, Viresh Kumar wrote:

> On 23 November 2012 15:11, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Fri, 23 Nov 2012 00:26:20 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> >> + - irq-trigger                       : IRQ trigger to use for the interrupt to the host
> >> + - irq-invert-polarity               : bool, IRQ line is connected with reversed polarity
> >
> > This looks odd. Normally the interrupt polarity should be encoded in the irq
> > specifier flags field.
> 
> Hi Grant and Lee Jones,
> 
> This looks odd because stmpe is odd, i am taking the discussion held
> with Lee jones to this thread.

STMPE isn't odd or special in any way. :)

> So, how interrupt stuff works currently in DT..
> We have a interrupt controller IC:
> 
> 	ic: interrupt-controller@40008000 {
> 		compatible = "foo";
> 		interrupt-controller;
> 		#interrupt-cells = <2>;
>                 ...
> 	};
> 
> And we have a user of this IC:
> 
> 	foo-peripheral@40048000 {
> 		compatible = "foo-peripheral";
> 		interrupt-parent = <&ic>;
> 		interrupts = <39 4>;
> 	};
> 
> Here first field of "interrupts" gives interrupt line number and the second one
> gives polarity, interrupt type etc..

So far so good.

> All is good till now. Now, every interrupt controller supports the first
> field, but the second one depends on its capabilities. An interrupt controller
> might not have registers to configure interrupt polarity, type, etc of
> the interrupt
> it will service and so the second field wouldn't be available for them.
> 
> For now just think stmpe is not a MFD and not a interrupt controller
> either. It is
> just a simple device, dev-foo.
> 
> It will declare values of its interrupts field based on the type of
> interrupt controller
> that will service its interrupt and that can be anything like VIC/GIC/GPIO
> controller.
> 
> Obviously nobody else than the parent IC driver can parse interrupts field
> of dev-foo, because only that driver understands the real meaning of
> these fields.
> 
> Now, stmpe has a special property. It can decide the way its output
> interrupt line
> will work. i.e. its polarity and interrupt type - edle/level, etc..
> This is not commonly
> seen in any peripheral. Now my original bindings and the real question here is
> about passing this information to stmpe driver.
> 
> I can't pass it in interrupts field of stmpe node, as that field
> belongs to parent
> interrupt controller of stmpe.
> 
> I can't pass that from child nodes of stmpe, as we are programming the interrupt
> coming out of stmpe and not the interrupt coming out of stmpe-gpio or
> stmpe-keypad.
> 
> And that's why i added these bindings. Please suggest me if i am still missing
> something.

Look into .xlate functions.

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

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Grant Likely]

On Fri, 23 Nov 2012 23:03:47 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 23 November 2012 15:11, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Fri, 23 Nov 2012 00:26:20 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> >> + - irq-trigger                       : IRQ trigger to use for the interrupt to the host
> >> + - irq-invert-polarity               : bool, IRQ line is connected with reversed polarity
> >
> > This looks odd. Normally the interrupt polarity should be encoded in the irq
> > specifier flags field.
> 
> Hi Grant and Lee Jones,
> 
> This looks odd because stmpe is odd, i am taking the discussion held
> with Lee jones to this thread.
> 
> So, how interrupt stuff works currently in DT..
> We have a interrupt controller IC:
> 
> 	ic: interrupt-controller@40008000 {
> 		compatible = "foo";
> 		interrupt-controller;
> 		#interrupt-cells = <2>;
>                 ...
> 	};
> 
> And we have a user of this IC:
> 
> 	foo-peripheral@40048000 {
> 		compatible = "foo-peripheral";
> 		interrupt-parent = <&ic>;
> 		interrupts = <39 4>;
> 	};
> 
> Here first field of "interrupts" gives interrupt line number and the second one
> gives polarity, interrupt type etc..
> 
> All is good till now. Now, every interrupt controller supports the first
> field, but the second one depends on its capabilities. An interrupt controller
> might not have registers to configure interrupt polarity, type, etc of
> the interrupt
> it will service and so the second field wouldn't be available for them.
> 
> For now just think stmpe is not a MFD and not a interrupt controller
> either. It is
> just a simple device, dev-foo.
> 
> It will declare values of its interrupts field based on the type of
> interrupt controller
> that will service its interrupt and that can be anything like VIC/GIC/GPIO
> controller.

Ah, so it is configuring the way the device emits interrupts; not how
the interrupt controller processes them. Fair enough.

It would actually be good to ask the interrupt controller driver what
kind of interrupt signal it expects for a given interrupt line. That
should also solve the problem and I think it would be more useful to
other devices. Can you investigate whether or not
irqd_get_trigger_type() returns the information you need?

g.

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Viresh Kumar]

On 27 November 2012 00:10, Grant Likely <grant.likely@secretlab.ca> wrote:
> Ah, so it is configuring the way the device emits interrupts; not how
> the interrupt controller processes them. Fair enough.

Finally i am able to convince somebody that stmpe is different :)

> It would actually be good to ask the interrupt controller driver what
> kind of interrupt signal it expects for a given interrupt line. That
> should also solve the problem and I think it would be more useful to
> other devices. Can you investigate whether or not
> irqd_get_trigger_type() returns the information you need?

That's a pretty cool function to use. :)

Will check it out :)

--
viresh

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Viresh Kumar]

On 27 November 2012 08:10, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 27 November 2012 00:10, Grant Likely <grant.likely@secretlab.ca> wrote:
>> It would actually be good to ask the interrupt controller driver what
>> kind of interrupt signal it expects for a given interrupt line. That
>> should also solve the problem and I think it would be more useful to
>> other devices. Can you investigate whether or not
>> irqd_get_trigger_type() returns the information you need?
>
> That's a pretty cool function to use. :)
>
> Will check it out :)

I was thinking about this logic in my earlier mail, don't know what stopped me
from thinking it is wrong. :(

Problem is with invert polarity, which the interrupt controller is not aware of.
For example, suppose interrupt controller needs Rising edge interrupt, but
the board has inverted the line between stmpe and IC. So, we will get
Rising high from the routine you mentioned, but we need to generate
opposite of that to make it rising high.

And so interrupt polarity field is still required.

--
viresh

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Lee Jones]

On Tue, 27 Nov 2012, Viresh Kumar wrote:

> On 27 November 2012 08:10, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 27 November 2012 00:10, Grant Likely <grant.likely@secretlab.ca> wrote:
> >> It would actually be good to ask the interrupt controller driver what
> >> kind of interrupt signal it expects for a given interrupt line. That
> >> should also solve the problem and I think it would be more useful to
> >> other devices. Can you investigate whether or not
> >> irqd_get_trigger_type() returns the information you need?
> >
> > That's a pretty cool function to use. :)
> >
> > Will check it out :)
> 
> I was thinking about this logic in my earlier mail, don't know what stopped me
> from thinking it is wrong. :(
> 
> Problem is with invert polarity, which the interrupt controller is not aware of.
> For example, suppose interrupt controller needs Rising edge interrupt, but
> the board has inverted the line between stmpe and IC. So, we will get
> Rising high from the routine you mentioned, but we need to generate
> opposite of that to make it rising high.

Surely that would be a hardware design error/quirk?

Can you give an example where this has happened?

> And so interrupt polarity field is still required.

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

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Viresh Kumar]

On 27 November 2012 14:10, Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 27 Nov 2012, Viresh Kumar wrote:
>> Problem is with invert polarity, which the interrupt controller is not aware of.
>> For example, suppose interrupt controller needs Rising edge interrupt, but
>> the board has inverted the line between stmpe and IC. So, we will get
>> Rising high from the routine you mentioned, but we need to generate
>> opposite of that to make it rising high.
>
> Surely that would be a hardware design error/quirk?

Yes.

> Can you give an example where this has happened?

I haven't seen this in any of SPEAr boards i have worked on. Maybe Rabin
would have, that's why he added that part of code :)

@Rabin/Linus: Do you remember why have you added this in stmpe driver:

+       if (stmpe->pdata->irq_invert_polarity)
+               icr ^= STMPE_ICR_LSB_HIGH;
+

Does somebody actually need it?

--
viresh

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Rabin Vincent]

2012/11/27 Viresh Kumar <viresh.kumar@linaro.org>:
> On 27 November 2012 14:10, Lee Jones <lee.jones@linaro.org> wrote:
> I haven't seen this in any of SPEAr boards i have worked on. Maybe Rabin
> would have, that's why he added that part of code :)
>
> @Rabin/Linus: Do you remember why have you added this in stmpe driver:
>
> +       if (stmpe->pdata->irq_invert_polarity)
> +               icr ^= STMPE_ICR_LSB_HIGH;
> +
>
> Does somebody actually need it?

It was (as irq_rev_pol) part of Luotao Fu's proposed STMPE811 patchset
(https://patchwork.kernel.org/patch/106173/) which I integrated into my
version of the STMPE driver, which didn't have it in its initial version
(https://patchwork.kernel.org/patch/103273/).

It's not something _I_ ever used.

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Viresh Kumar]

On 28 November 2012 01:25, Rabin Vincent <rabin@rab.in> wrote:
> 2012/11/27 Viresh Kumar <viresh.kumar@linaro.org>:
>> On 27 November 2012 14:10, Lee Jones <lee.jones@linaro.org> wrote:
>> I haven't seen this in any of SPEAr boards i have worked on. Maybe Rabin
>> would have, that's why he added that part of code :)
>>
>> @Rabin/Linus: Do you remember why have you added this in stmpe driver:
>>
>> +       if (stmpe->pdata->irq_invert_polarity)
>> +               icr ^= STMPE_ICR_LSB_HIGH;
>> +
>>
>> Does somebody actually need it?
>
> It was (as irq_rev_pol) part of Luotao Fu's proposed STMPE811 patchset
> (https://patchwork.kernel.org/patch/106173/) which I integrated into my
> version of the STMPE driver, which didn't have it in its initial version
> (https://patchwork.kernel.org/patch/103273/).
>
> It's not something _I_ ever used.

I grep'd kernel and nobody is using it there, so lets get rid of it
completely :)
I will patch it.

--
viresh

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Lee Jones]

On Wed, 28 Nov 2012, Viresh Kumar wrote:

> On 28 November 2012 01:25, Rabin Vincent <rabin@rab.in> wrote:
> > 2012/11/27 Viresh Kumar <viresh.kumar@linaro.org>:
> >> On 27 November 2012 14:10, Lee Jones <lee.jones@linaro.org> wrote:
> >> I haven't seen this in any of SPEAr boards i have worked on. Maybe Rabin
> >> would have, that's why he added that part of code :)
> >>
> >> @Rabin/Linus: Do you remember why have you added this in stmpe driver:
> >>
> >> +       if (stmpe->pdata->irq_invert_polarity)
> >> +               icr ^= STMPE_ICR_LSB_HIGH;
> >> +
> >>
> >> Does somebody actually need it?
> >
> > It was (as irq_rev_pol) part of Luotao Fu's proposed STMPE811 patchset
> > (https://patchwork.kernel.org/patch/106173/) which I integrated into my
> > version of the STMPE driver, which didn't have it in its initial version
> > (https://patchwork.kernel.org/patch/103273/).
> >
> > It's not something _I_ ever used.
> 
> I grep'd kernel and nobody is using it there, so lets get rid of it
> completely :)
> I will patch it.

Great. :)

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

Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver

[Lee Jones]

On Fri, 23 Nov 2012, Viresh Kumar wrote:

> From: Vipul Kumar Samar <vipulkumar.samar@st.com>
> 
> This patch extends existing DT support for stmpe devices. This updates:
> - DT support from stmpe SPI and I2C drivers
> - missing header files in stmpe.c
> - stmpe_of_probe() with pwm, rotator and new bindings.
> - Bindings are updated in binding document.
> 
> Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2->V3:
> ------
> - Removed sub-modules DT bindings from this patch
> - Retain original work done by Lee Jones
> 
>  Documentation/devicetree/bindings/mfd/stmpe.txt | 12 ++++++++----
>  drivers/mfd/stmpe-i2c.c                         | 15 +++++++++++++++
>  drivers/mfd/stmpe-spi.c                         | 15 +++++++++++++++
>  drivers/mfd/stmpe.c                             | 21 +++++++++++++++------
>  4 files changed, 53 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
> index 8f0aeda..8f65c8d 100644
> --- a/Documentation/devicetree/bindings/mfd/stmpe.txt
> +++ b/Documentation/devicetree/bindings/mfd/stmpe.txt
> @@ -1,13 +1,17 @@
> -* STMPE Multi-Functional Device
> +* ST Microelectronics STMPE Multi-Functional Device
> +
> +STMPE is an MFD device which may expose following inbuilt devices: gpio, keypad,
> +touchscreen, adc, pwm, rotator.
>  
>  Required properties:
> - - compatible                   : "st,stmpe[811|1601|2401|2403]"
> - - reg                          : I2C address of the device
> + - compatible                   : "st,stmpe[610|801|811|1601|2401|2403]"
> + - reg                          : I2C/SPI address of the device
>  
>  Optional properties:
>   - interrupts                   : The interrupt outputs from the controller
> - - interrupt-controller         : Marks the device node as an interrupt controller

On closer inspection, this is actually incorrect.

The STMPE _is_ an interrupt-controller.

>   - interrupt-parent             : Specifies which IRQ controller we're connected to
> + - irq-trigger			: IRQ trigger to use for the interrupt to the host
> + - irq-invert-polarity		: bool, IRQ line is connected with reversed polarity

I've told you about this already. ;)

>   - i2c-client-wake              : Marks the input device as wakable
>   - st,autosleep-timeout         : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024
>  
> diff --git a/drivers/mfd/stmpe-i2c.c b/drivers/mfd/stmpe-i2c.c
> index c734dc3..003c49b 100644
> --- a/drivers/mfd/stmpe-i2c.c
> +++ b/drivers/mfd/stmpe-i2c.c
> @@ -13,6 +13,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/types.h>
>  #include "stmpe.h"
>  
> @@ -81,6 +82,19 @@ static const struct i2c_device_id stmpe_i2c_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, stmpe_id);
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id stmpe_dt_ids[] = {
> +	{ .compatible = "st,stmpe610", .data = &stmpe_i2c_id[0], },
> +	{ .compatible = "st,stmpe801", .data = &stmpe_i2c_id[1], },
> +	{ .compatible = "st,stmpe811", .data = &stmpe_i2c_id[2], },
> +	{ .compatible = "st,stmpe1601", .data = &stmpe_i2c_id[3], },
> +	{ .compatible = "st,stmpe2401", .data = &stmpe_i2c_id[4], },
> +	{ .compatible = "st,stmpe2403", .data = &stmpe_i2c_id[5], },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
> +#endif
> +
>  static struct i2c_driver stmpe_i2c_driver = {
>  	.driver = {
>  		.name = "stmpe-i2c",
> @@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = {
>  #ifdef CONFIG_PM
>  		.pm = &stmpe_dev_pm_ops,
>  #endif
> +		.of_match_table = of_match_ptr(stmpe_dt_ids),
>  	},
>  	.probe		= stmpe_i2c_probe,
>  	.remove		= __devexit_p(stmpe_i2c_remove),
> diff --git a/drivers/mfd/stmpe-spi.c b/drivers/mfd/stmpe-spi.c
> index f86e3fc..e482f5f 100644
> --- a/drivers/mfd/stmpe-spi.c
> +++ b/drivers/mfd/stmpe-spi.c
> @@ -10,6 +10,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/spi/spi.h>
>  #include <linux/types.h>
>  #include "stmpe.h"
> @@ -119,6 +120,19 @@ static const struct spi_device_id stmpe_spi_id[] = {
>  };
>  MODULE_DEVICE_TABLE(spi, stmpe_id);
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id stmpe_dt_ids[] = {
> +	{ .compatible = "st,stmpe610", .data = (void *)STMPE610, },
> +	{ .compatible = "st,stmpe801", .data = (void *)STMPE801, },
> +	{ .compatible = "st,stmpe811", .data = (void *)STMPE811, },
> +	{ .compatible = "st,stmpe1601", .data = (void *)STMPE1601, },
> +	{ .compatible = "st,stmpe2401", .data = (void *)STMPE2401, },
> +	{ .compatible = "st,stmpe2403", .data = (void *)STMPE2403, },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
> +#endif
> +
>  static struct spi_driver stmpe_spi_driver = {
>  	.driver = {
>  		.name	= "stmpe-spi",
> @@ -126,6 +140,7 @@ static struct spi_driver stmpe_spi_driver = {
>  #ifdef CONFIG_PM
>  		.pm	= &stmpe_dev_pm_ops,
>  #endif
> +		.of_match_table = of_match_ptr(stmpe_dt_ids),
>  	},
>  	.probe		= stmpe_spi_probe,
>  	.remove		= __devexit_p(stmpe_spi_remove),
> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> index 6086481..e2c0dda 100644
> --- a/drivers/mfd/stmpe.c
> +++ b/drivers/mfd/stmpe.c
> @@ -7,6 +7,7 @@
>   * Author: Rabin Vincent <rabin.vincent@stericsson.com> for ST-Ericsson
>   */
>  
> +#include <linux/err.h>
>  #include <linux/export.h>
>  #include <linux/gpio.h>
>  #include <linux/interrupt.h>
> @@ -14,6 +15,8 @@
>  #include <linux/irqdomain.h>
>  #include <linux/kernel.h>
>  #include <linux/mfd/core.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
>  #include "stmpe.h"
> @@ -1028,18 +1031,24 @@ void __devinit stmpe_of_probe(struct stmpe_platform_data *pdata,
>  
>  	pdata->autosleep = (pdata->autosleep_timeout) ? true : false;
>  
> +	of_property_read_u32(np, "irq-trigger", &pdata->irq_trigger);
> +
> +	if (of_property_read_bool(np, "irq-invert-polarity"))
> +		pdata->irq_invert_polarity = true;
> +

You don't need these.

>  	for_each_child_of_node(np, child) {
>  		if (!strcmp(child->name, "stmpe_gpio")) {
>  			pdata->blocks |= STMPE_BLOCK_GPIO;
> -		}
> -		if (!strcmp(child->name, "stmpe_keypad")) {
> +		} else if (!strcmp(child->name, "stmpe_keypad")) {
>  			pdata->blocks |= STMPE_BLOCK_KEYPAD;
> -		}
> -		if (!strcmp(child->name, "stmpe_touchscreen")) {
> +		} else if (!strcmp(child->name, "stmpe_touchscreen")) {
>  			pdata->blocks |= STMPE_BLOCK_TOUCHSCREEN;
> -		}
> -		if (!strcmp(child->name, "stmpe_adc")) {
> +		} else if (!strcmp(child->name, "stmpe_adc")) {
>  			pdata->blocks |= STMPE_BLOCK_ADC;
> +		} else if (!strcmp(child->name, "stmpe_pwm")) {
> +			pdata->blocks |= STMPE_BLOCK_PWM;
> +		} else if (!strcmp(child->name, "stmpe_rotator")) {
> +			pdata->blocks |= STMPE_BLOCK_ROTATOR;
>  		}
>  	}
>  }
> -- 
> 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

Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order

[Samuel Ortiz]

Hi Viresh,

On Fri, Nov 23, 2012 at 12:26:18AM +0530, Viresh Kumar wrote:
> This helps managing them better and also reduces chances of adding an header
> file twice.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/mfd/stmpe-spi.c | 2 +-
>  drivers/mfd/stmpe.c     | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
I fail to see the point of this patch, sorry.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order

[Viresh Kumar]

On 26 November 2012 16:46, Samuel Ortiz <sameo@linux.intel.com> wrote:
> Hi Viresh,
>
> On Fri, Nov 23, 2012 at 12:26:18AM +0530, Viresh Kumar wrote:
>> This helps managing them better and also reduces chances of adding an header
>> file twice.

The aim is to maintain the list of header files in alphabetical order.
It helps in maintaining
them.. I was adding some header files for my 3rd patch and was looking
for the best
place to add them and because the list wasn't sorted, i sorted it out
in a separate patch.

--
viresh

Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order

[Lee Jones]

On Mon, 26 Nov 2012, Viresh Kumar wrote:

> On 26 November 2012 16:46, Samuel Ortiz <sameo@linux.intel.com> wrote:
> > Hi Viresh,
> >
> > On Fri, Nov 23, 2012 at 12:26:18AM +0530, Viresh Kumar wrote:
> >> This helps managing them better and also reduces chances of adding an header
> >> file twice.
> 
> The aim is to maintain the list of header files in alphabetical order.
> It helps in maintaining
> them.. I was adding some header files for my 3rd patch and was looking
> for the best
> place to add them and because the list wasn't sorted, i sorted it out
> in a separate patch.

Why do you need to sort them? Is there _really_ a need?

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

Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order

[Viresh Kumar]

On 26 November 2012 18:55, Lee Jones <lee.jones@linaro.org> wrote:

> Why do you need to sort them? Is there _really_ a need?

Many people & maintainers like to have their header files ordered. The
reason for that is:

If they are not ordered, there is a possibility of adding an header file
multiple times in a file. This might not do something serious
when thinking about compilation time or Image size, but adding an header
file multiple times is simply wrong.

This can't be caught in patch reviews most of the time, as diff may not show
the earlier inclusion.

That's why i ordered them. And i don't see any harm in doing so.

--
viresh

Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order

[Lee Jones]

On Mon, 26 Nov 2012, Viresh Kumar wrote:

> On 26 November 2012 18:55, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > Why do you need to sort them? Is there _really_ a need?
> 
> Many people & maintainers like to have their header files ordered. The
> reason for that is:
> 
> If they are not ordered, there is a possibility of adding an header file
> multiple times in a file. This might not do something serious
> when thinking about compilation time or Image size, but adding an header
> file multiple times is simply wrong.
> 
> This can't be caught in patch reviews most of the time, as diff may not show
> the earlier inclusion.
> 
> That's why i ordered them. And i don't see any harm in doing so.

I don't see any harm in it, but it's pretty pointless.

There aren't many maintainers who insist on such things, and the
ones that do normally carry out these OCD actions themselves. :)

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

Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order

[Samuel Ortiz]

On Mon, Nov 26, 2012 at 03:31:45PM +0000, Lee Jones wrote:
> On Mon, 26 Nov 2012, Viresh Kumar wrote:
> 
> > On 26 November 2012 18:55, Lee Jones <lee.jones@linaro.org> wrote:
> > 
> > > Why do you need to sort them? Is there _really_ a need?
> > 
> > Many people & maintainers like to have their header files ordered. The
> > reason for that is:
> > 
> > If they are not ordered, there is a possibility of adding an header file
> > multiple times in a file. This might not do something serious
> > when thinking about compilation time or Image size, but adding an header
> > file multiple times is simply wrong.
> > 
> > This can't be caught in patch reviews most of the time, as diff may not show
> > the earlier inclusion.
> > 
> > That's why i ordered them. And i don't see any harm in doing so.
> 
> I don't see any harm in it, but it's pretty pointless.
> 
> There aren't many maintainers who insist on such things, 
And I'm not one of those.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/