2012-11-29 14:40:33

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH V5 1/2] mfd: stmpe: Get rid of irq_invert_polarity

Since the very first patch, stmpe core driver is using irq_invert_polarity as
part of platform data. But, nobody is actually using it in kernel till now.

Also, this is not something part of hardware specs, but is included to cater
some board mistakes or quirks.

So, better get rid of it. This is earlier discussed here:

https://lkml.org/lkml/2012/11/27/636

Acked-by: Lee Jones <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
Nothing changed from last version.

drivers/mfd/stmpe.c | 7 -------
include/linux/mfd/stmpe.h | 2 --
2 files changed, 9 deletions(-)

diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index 6175aa4..34408b4 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -960,13 +960,6 @@ static int __devinit stmpe_chip_init(struct stmpe *stmpe)
else
icr |= STMPE_ICR_LSB_HIGH;
}
-
- if (stmpe->pdata->irq_invert_polarity) {
- if (id == STMPE801_ID)
- icr ^= STMPE801_REG_SYS_CTRL_INT_HI;
- else
- icr ^= STMPE_ICR_LSB_HIGH;
- }
}

if (stmpe->pdata->autosleep) {
diff --git a/include/linux/mfd/stmpe.h b/include/linux/mfd/stmpe.h
index 15dac79..383ac15 100644
--- a/include/linux/mfd/stmpe.h
+++ b/include/linux/mfd/stmpe.h
@@ -190,7 +190,6 @@ struct stmpe_ts_platform_data {
* @id: device id to distinguish between multiple STMPEs on the same board
* @blocks: bitmask of blocks to enable (use STMPE_BLOCK_*)
* @irq_trigger: IRQ trigger to use for the interrupt to the host
- * @irq_invert_polarity: IRQ line is connected with reversed polarity
* @autosleep: bool to enable/disable stmpe autosleep
* @autosleep_timeout: inactivity timeout in milliseconds for autosleep
* @irq_base: base IRQ number. %STMPE_NR_IRQS irqs will be used, or
@@ -207,7 +206,6 @@ struct stmpe_platform_data {
unsigned int blocks;
int irq_base;
unsigned int irq_trigger;
- bool irq_invert_polarity;
bool autosleep;
bool irq_over_gpio;
int irq_gpio;
--
1.7.12.rc2.18.g61b472e


2012-11-29 14:40:41

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver

From: Vipul Kumar Samar <[email protected]>

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 <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
V4->V5:
------
- 2/3 and 3/3 merged.
- irq_trigger is kept same for non-DT booti.

@Lee: I haven't added your Acked-by, because this differs from your Acked
version.

Documentation/devicetree/bindings/mfd/stmpe.txt | 9 ++++++---
drivers/mfd/stmpe-i2c.c | 15 ++++++++++++++
drivers/mfd/stmpe-spi.c | 15 ++++++++++++++
drivers/mfd/stmpe.c | 27 +++++++++++++++++++------
4 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
index 8f0aeda..d2d88d9 100644
--- a/Documentation/devicetree/bindings/mfd/stmpe.txt
+++ b/Documentation/devicetree/bindings/mfd/stmpe.txt
@@ -1,8 +1,11 @@
-* 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
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 9edfe86..1e2bff0 100644
--- a/drivers/mfd/stmpe-spi.c
+++ b/drivers/mfd/stmpe-spi.c
@@ -11,6 +11,7 @@
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/of.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 34408b4..bb2dfdb 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -7,12 +7,15 @@
* Author: Rabin Vincent <[email protected]> for ST-Ericsson
*/

+#include <linux/err.h>
#include <linux/gpio.h>
#include <linux/export.h>
#include <linux/kernel.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
#include <linux/pm.h>
#include <linux/slab.h>
#include <linux/mfd/core.h>
@@ -1019,6 +1022,14 @@ void __devinit stmpe_of_probe(struct stmpe_platform_data *pdata,
{
struct device_node *child;

+ /*
+ * Distinct names of same cell-type within multiple instances of stmpe
+ * will be guaranteed by DT.
+ */
+ pdata->id = -1;
+
+ pdata->irq_trigger = IRQF_TRIGGER_NONE;
+
of_property_read_u32(np, "st,autosleep-timeout",
&pdata->autosleep_timeout);

@@ -1027,15 +1038,16 @@ void __devinit stmpe_of_probe(struct stmpe_platform_data *pdata,
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;
}
}
}
@@ -1106,6 +1118,9 @@ int __devinit stmpe_probe(struct stmpe_client_info *ci, int partnum)
return -ENODEV;
}
stmpe->variant = stmpe_noirq_variant_info[stmpe->partnum];
+ } else if (pdata->irq_trigger == IRQF_TRIGGER_NONE) {
+ pdata->irq_trigger =
+ irqd_get_trigger_type(irq_get_irq_data(stmpe->irq));
}

ret = stmpe_chip_init(stmpe);
--
1.7.12.rc2.18.g61b472e

2012-11-30 10:57:39

by Samuel Ortiz

[permalink] [raw]
Subject: Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver

Hi Viresh, Lee,

On Thu, Nov 29, 2012 at 08:10:18PM +0530, Viresh Kumar wrote:
> From: Vipul Kumar Samar <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> V4->V5:
> ------
> - 2/3 and 3/3 merged.
> - irq_trigger is kept same for non-DT booti.
>
> @Lee: I haven't added your Acked-by, because this differs from your Acked
> version.
Lee, are you ok with this one ? Could you give it a test ?

Cheers,
Samuel.

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

2012-11-30 12:45:28

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver

On Fri, 30 Nov 2012, Samuel Ortiz wrote:

> Hi Viresh, Lee,
>
> On Thu, Nov 29, 2012 at 08:10:18PM +0530, Viresh Kumar wrote:
> > From: Vipul Kumar Samar <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Viresh Kumar <[email protected]>
> > ---
> > V4->V5:
> > ------
> > - 2/3 and 3/3 merged.
> > - irq_trigger is kept same for non-DT booti.
> >
> > @Lee: I haven't added your Acked-by, because this differs from your Acked
> > version.
> Lee, are you ok with this one ? Could you give it a test ?

The patch doesn't apply for me - does it for you?

Viresh, what's it based on?

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

2012-11-30 13:11:13

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver

On 30 November 2012 18:15, Lee Jones <[email protected]> wrote:
> The patch doesn't apply for me - does it for you?
>
> Viresh, what's it based on?

Because this was applied 2 days back by Samuel, and i didn't
fetch it again yesterday:

commit 20d5c7defc228cdaeff3ce3442f3a4e86af293c1
Author: Randy Dunlap <[email protected]>
Date: Mon Nov 12 09:20:49 2012 -0800

mfd: Fix stmpe.c build when OF is not enabled

Fix build errors when CONFIG_OF is not enabled by including
<linux/of.h> (needs to be added in any case).
An alternative fix could be to make the driver depend on OF.

drivers/mfd/stmpe.c:1025:2: error: implicit declaration of
function 'of_property_read_u32'
drivers/mfd/stmpe.c:1030:2: error: implicit declaration of
function 'for_each_child_of_node'
drivers/mfd/stmpe.c:1030:36: error: expected ';' before '{' token

Signed-off-by: Randy Dunlap <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Signed-off-by: Samuel Ortiz <[email protected]>
---
drivers/mfd/stmpe.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index 0061d1b..f9f7de7 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -13,6 +13,7 @@
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
+#include <linux/of.h>


Its a simple conflict to fix, as this is the only place of conflict.

--
viresh

2012-11-30 13:20:43

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver

On Fri, 30 Nov 2012, Viresh Kumar wrote:

> On 30 November 2012 18:15, Lee Jones <[email protected]> wrote:
> > The patch doesn't apply for me - does it for you?
> >
> > Viresh, what's it based on?
>
> Because this was applied 2 days back by Samuel, and i didn't
> fetch it again yesterday:
>
> commit 20d5c7defc228cdaeff3ce3442f3a4e86af293c1
> Author: Randy Dunlap <[email protected]>
> Date: Mon Nov 12 09:20:49 2012 -0800
>
> Its a simple conflict to fix, as this is the only place of conflict.

Eh? You don't know what my tree is based on. ;)

What does this patch apply on top of? Which -rc?

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

2012-11-30 15:45:51

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver

On Fri, 30 Nov 2012, Samuel Ortiz wrote:

> Hi Viresh, Lee,
>
> On Thu, Nov 29, 2012 at 08:10:18PM +0530, Viresh Kumar wrote:
> > From: Vipul Kumar Samar <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Viresh Kumar <[email protected]>
> > ---
> > V4->V5:
> > ------
> > - 2/3 and 3/3 merged.
> > - irq_trigger is kept same for non-DT booti.
> >
> > @Lee: I haven't added your Acked-by, because this differs from your Acked
> > version.
> Lee, are you ok with this one ? Could you give it a test ?

Okay, I've tested it, and it doesn't break anything.

But ... I don't see how the changes in the -i2c and -spi files
are of benefit either. When I boot without the ID table I still
get "stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212".

What is it that actually uses the IDs?

Perhaps Viresh can shine some light on the matter?

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

2012-11-30 19:03:51

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver

On 30 November 2012 21:15, Lee Jones <[email protected]> wrote:
> But ... I don't see how the changes in the -i2c and -spi files
> are of benefit either. When I boot without the ID table I still
> get "stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212".
>
> What is it that actually uses the IDs?
>
> Perhaps Viresh can shine some light on the matter?

As you can see, i wasn't the author of this patch and when you asked
this question, i didn't had an answer to it. I went through code and
formed some theory/story :) .

@Grant: i need your help to check if my theory is correct or not. Question
is about adding below code in any i2c client driver:

+#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),

So, what is the use of this table when we already have i2c_driver.id_table
populated.

This is my theory:
---------------------
Adapter drivers supporting DT will call:
of_i2c_register_devices()
{
for_each_child_of_node(adap->dev.of_node, node) {
if (of_modalias_node(node, info.type, sizeof(info.type)) < 0)
error condition

...
result = i2c_new_device(adap, &info);

...
}

of_modalias_node(): expects compatible in child node, i.e. stmpe node in our
case. If it is not there, then that node is skipped. then it copies
string after ','
to info.type. So, for us only "stmpe810" out of "st,stmpe810" is copied.

Now this name, i.e. "stmpe810" is copied as client.name in i2c_new_device()
and device_register() is called, which will eventually call:

i2c_device_match()
{
/* Attempt an OF style match */
if (of_driver_match_device(dev, drv))
return 1;

driver = to_i2c_driver(drv);
/* match on an id table if there is one */
if (driver->id_table)
return i2c_match_id(driver->id_table, client) != NULL;
}

This first tries to match the table my patch added, _BUT_ the string will
never match as we had "st,stmpe810" in table and "stmpe810" in dev.

So, we fall back to i2c_match_id(), which will match it against
i2c_driver.id_table present in driver, which has entry for "stmpe810" and
so strings matched.

@Lee: This is what happened in your case. :)

So, whether its DT or non DT, true is returned from here if something
matched.

Later on, this will be called:

static int i2c_device_probe(struct device *dev)
{
.....
status = driver->probe(client, i2c_match_id(driver->id_table, client));
....
}

Which will again match the legacy table to find correct struct i2c_device_id *id
to pass to probe().

So, the final question: WTF is of_match_table for?

Then i thought maybe it is used when we don't have child nodes inside parent,
something related to the phandle way ? I grep'd i2c in arch/arm/boot/dts and
couldn't find anything of that sort, the way i2c clients are added is:

in dtsi file:

i2c0: i2c@address {
...
}

in dts file:
&i2c0 {
stmpe810 {
........
}
}

which i believe will be taken care by dtc and will fold client nodes
as child nodes
of i2c0.

@Grant: Please throw some light here :)

--
viresh