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

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

[Viresh Kumar]

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 <lee.jones@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
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

[PATCH V5 2/2] 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>
---
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 <rabin.vincent@stericsson.com> 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

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

[Samuel Ortiz]

Hi Viresh, Lee,

On Thu, Nov 29, 2012 at 08:10:18PM +0530, 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>
> ---
> 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/

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

[Lee Jones]

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

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

[Viresh Kumar]

On 30 November 2012 18:15, Lee Jones <lee.jones@linaro.org> 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 <rdunlap@infradead.org>
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 <rdunlap@infradead.org>
    Acked-by: Linus Walleij <linus.walleij@linaro.org>
    Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
---
 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

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

[Lee Jones]

On Fri, 30 Nov 2012, Viresh Kumar wrote:

> On 30 November 2012 18:15, Lee Jones <lee.jones@linaro.org> 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 <rdunlap@infradead.org>
> 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

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

[Lee Jones]

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

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

[Viresh Kumar]

On 30 November 2012 21:15, Lee Jones <lee.jones@linaro.org> 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

Re: [PATCH V5 1/2] mfd: stmpe: Get rid of irq_invert_polarity

[Linus Walleij]

On Thu, Nov 29, 2012 at 3:40 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> 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 <lee.jones@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Viresh Kumar]

Ping!!!

On 1 December 2012 00:33, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 30 November 2012 21:15, Lee Jones <lee.jones@linaro.org> 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

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

[Lee Jones]

> Ping!!!

Documentation/development-process/2.Process:

- Avoid top-posting (the practice of putting your answer above the quoted
  text you are responding to).  It makes your response harder to read and
  makes a poor impression.

:)

> On 1 December 2012 00:33, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 30 November 2012 21:15, Lee Jones <lee.jones@linaro.org> 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

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

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

[Viresh Kumar]

On 5 December 2012 18:49, Lee Jones <lee.jones@linaro.org> wrote:
>> Ping!!!
>
> Documentation/development-process/2.Process:
>
> - Avoid top-posting (the practice of putting your answer above the quoted
>   text you are responding to).  It makes your response harder to read and
>   makes a poor impression.

Yes, i know that. Did that in hurry, as it was just an ping and wasn't answering
any question.

--
viresh

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

[Grant Likely]

On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 30 November 2012 21:15, Lee Jones <lee.jones@linaro.org> 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.

of_driver_match_device() matches against the compatible list in
dev->of_node, not against the device name. So, if the compatible
property has a string that is in the table, then it really should match
against it.

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

Here things are a bit wonky. Even if matched against the table, it is
possible that it also matches against i2c_match_id() and that data is
passed to the driver.

But regardless, it is the responsiblity of the probe function to go and
look if of_driver_match_device() matches against anything if it cares
about the of_match_table entries (for instance, if there is extra data
attached).

>         ....
> }
> 
> 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?

A bit of history is valuable here. The whole of_modalias_node() thing is
really just a best-effort heuristic for figuring out which driver
*might* work against a device described in the device tree. It won't
work in all circumstances (and it was created at a time when there was
resistance to adding DT knowledge to drivers). An of_match_table is the
robust way of identifying specific devices and allows for matching
against any entry in the compatible list.

g.

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

[Viresh Kumar]

First of all, thanks for explaining :)

On 6 December 2012 04:12, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> 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.
>
> of_driver_match_device() matches against the compatible list in
> dev->of_node, not against the device name. So, if the compatible
> property has a string that is in the table, then it really should match
> against it.

How could i misread it? Yes you are correct.

>> static int i2c_device_probe(struct device *dev)
>> {
>>         .....
>>       status = driver->probe(client, i2c_match_id(driver->id_table, client));
>
> Here things are a bit wonky. Even if matched against the table, it is

table means of_device_id table ?

> possible that it also matches against i2c_match_id() and that data is
> passed to the driver.

It is a possibility or guarantee ? And so whatever device name we got from
modalias routine, should match with the names in driver->id_table.

> But regardless, it is the responsiblity of the probe function to go and
> look if of_driver_match_device() matches against anything if it cares
> about the of_match_table entries (for instance, if there is extra data
> attached).

Ok, so filling .data field in of_device_id[] is not required for our case as
we aren't doing anything special in our drivers.

>> 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?
>
> A bit of history is valuable here. The whole of_modalias_node() thing is
> really just a best-effort heuristic for figuring out which driver
> *might* work against a device described in the device tree. It won't
> work in all circumstances (and it was created at a time when there was
> resistance to adding DT knowledge to drivers). An of_match_table is the
> robust way of identifying specific devices and allows for matching
> against any entry in the compatible list.

Got it.

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

[Viresh Kumar]

On 6 December 2012 04:12, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> 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.
>
> of_driver_match_device() matches against the compatible list in
> dev->of_node, not against the device name. So, if the compatible
> property has a string that is in the table, then it really should match
> against it.

Grant, but isn't it true that the final device's name would not be the DT
way of names? It would simply be "stmpe810" for us and so if we have
multiple instances of stmpe on a board, we need to distinguish them
ourselves? One way for that was passing id for these instances, which
would finally be used, when we create platform devices for sub-modules
of stmpe (gpio, keypad, ts, etc).

--
viresh

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

[Lee Jones]

> > But regardless, it is the responsiblity of the probe function to go and
> > look if of_driver_match_device() matches against anything if it cares
> > about the of_match_table entries (for instance, if there is extra data
> > attached).
> 
> Ok, so filling .data field in of_device_id[] is not required for our case as
> we aren't doing anything special in our drivers.

This is exactly my point, and the reason I bought it up in the
first place. Normally when you specify an ID table and populate
the .data attribute, you parse for it in the code and then cast
it back to some kind of useful data. However, you're not doing
that, which is precisely why I wondered if the table was
necessary at all. In all my testing, the DT portion worked and
the correct STMPE chip was identified without it.

So, are you adding the table for good reason, or because you
think it's the right thing to do?

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

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

[Viresh Kumar]

On 6 December 2012 15:20, Lee Jones <lee.jones@linaro.org> wrote:
>> > But regardless, it is the responsiblity of the probe function to go and
>> > look if of_driver_match_device() matches against anything if it cares
>> > about the of_match_table entries (for instance, if there is extra data
>> > attached).
>>
>> Ok, so filling .data field in of_device_id[] is not required for our case as
>> we aren't doing anything special in our drivers.
>
> This is exactly my point, and the reason I bought it up in the
> first place. Normally when you specify an ID table and populate
> the .data attribute, you parse for it in the code and then cast
> it back to some kind of useful data. However, you're not doing
> that, which is precisely why I wondered if the table was
> necessary at all. In all my testing, the DT portion worked and
> the correct STMPE chip was identified without it.

Probably Vipul (Author of this patch), copied it from existing i2c/spi
clients, which have also added this blindly :)

> So, are you adding the table for good reason, or because you
> think it's the right thing to do?

I would be keeping the table as that's the right thing to do. By chance
our non-DT and DT tables had a difference of "st," only in the name
of instances and so it worked without tables. Otherwise it couldn't
have worked.

Over that, i am looking to bring the "stmpe,id" binding back again (unless
you have a better option), as device name is not coming from DT currently,
which we discussed earlier.

--
viresh

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

[Lee Jones]

On Thu, 06 Dec 2012, Viresh Kumar wrote:

> On 6 December 2012 15:20, Lee Jones <lee.jones@linaro.org> wrote:
> >> > But regardless, it is the responsiblity of the probe function to go and
> >> > look if of_driver_match_device() matches against anything if it cares
> >> > about the of_match_table entries (for instance, if there is extra data
> >> > attached).
> >>
> >> Ok, so filling .data field in of_device_id[] is not required for our case as
> >> we aren't doing anything special in our drivers.
> >
> > This is exactly my point, and the reason I bought it up in the
> > first place. Normally when you specify an ID table and populate
> > the .data attribute, you parse for it in the code and then cast
> > it back to some kind of useful data. However, you're not doing
> > that, which is precisely why I wondered if the table was
> > necessary at all. In all my testing, the DT portion worked and
> > the correct STMPE chip was identified without it.
> 
> Probably Vipul (Author of this patch), copied it from existing i2c/spi
> clients, which have also added this blindly :)
> 
> > So, are you adding the table for good reason, or because you
> > think it's the right thing to do?
> 
> I would be keeping the table as that's the right thing to do. 

So then I'm back to my original question, why?

What is it used for? What difference does it make?

I could understand if the .data attribute was used in the driver
to make vital decisions based on STMPE version, but it's not. So
why are we burdening the driver with unused code that's not
required? Other than to get your mainlined patch-count up of
course? This has an air of "placing header files in alphabetical
order" about it. ;)

> By chance
> our non-DT and DT tables had a difference of "st," only in the name
> of instances and so it worked without tables. Otherwise it couldn't
> have worked.
> 
> Over that, i am looking to bring the "stmpe,id" binding back again (unless
> you have a better option), as device name is not coming from DT currently,
> which we discussed earlier.

Or you could not put unnecessary bindings into the Device Tree
by putting two and two together and realise that using the table
is the correct thing to do instead. This actually gives reason
to you previous patch, but should probably be fixed-up into it
so it has some proper meaning/purpose. ;)

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

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

[Viresh Kumar]

On 6 December 2012 15:41, Lee Jones <lee.jones@linaro.org> wrote:
> So then I'm back to my original question, why?
>
> What is it used for? What difference does it make?
>
> I could understand if the .data attribute was used in the driver
> to make vital decisions based on STMPE version, but it's not. So
> why are we burdening the driver with unused code that's not
> required? Other than to get your mainlined patch-count up of
> course? This has an air of "placing header files in alphabetical
> order" about it. ;)

The count would still be the same as some part of this patch will
go :)

I said that because of Grant's comment: "An of_match_table is the
robust way of identifying specific devices and allows for matching
against any entry in the compatible list."

So, thought its better we keep it.

Now, the problem is, with this new table we will bind device and
driver based on of_device_id table and probe it using device_id
table. Ahh.. that's broken. Maybe yes, can remove it unless we
have a real need of it.

>> By chance
>> our non-DT and DT tables had a difference of "st," only in the name
>> of instances and so it worked without tables. Otherwise it couldn't
>> have worked.
>>
>> Over that, i am looking to bring the "stmpe,id" binding back again (unless
>> you have a better option), as device name is not coming from DT currently,
>> which we discussed earlier.
>
> Or you could not put unnecessary bindings into the Device Tree
> by putting two and two together and realise that using the table
> is the correct thing to do instead. This actually gives reason
> to you previous patch, but should probably be fixed-up into it
> so it has some proper meaning/purpose. ;)

Couldn't understand this one :(

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

[Lee Jones]

On Thu, 06 Dec 2012, Viresh Kumar wrote:

> On 6 December 2012 15:41, Lee Jones <lee.jones@linaro.org> wrote:
> > So then I'm back to my original question, why?
> >
> > What is it used for? What difference does it make?
> >
> > I could understand if the .data attribute was used in the driver
> > to make vital decisions based on STMPE version, but it's not. So
> > why are we burdening the driver with unused code that's not
> > required? Other than to get your mainlined patch-count up of
> > course? This has an air of "placing header files in alphabetical
> > order" about it. ;)
> 
> The count would still be the same as some part of this patch will
> go :)
> 
> I said that because of Grant's comment: "An of_match_table is the
> robust way of identifying specific devices and allows for matching
> against any entry in the compatible list."
> 
> So, thought its better we keep it.

Only if you're going to use it, or else it's just unused cruft.

> Now, the problem is, with this new table we will bind device and
> driver based on of_device_id table and probe it using device_id
> table. Ahh.. that's broken. Maybe yes, can remove it unless we
> have a real need of it.

Broken or otherwise, if it's unused, it's cruft. :)

> >> By chance
> >> our non-DT and DT tables had a difference of "st," only in the name
> >> of instances and so it worked without tables. Otherwise it couldn't
> >> have worked.
> >>
> >> Over that, i am looking to bring the "stmpe,id" binding back again (unless
> >> you have a better option), as device name is not coming from DT currently,
> >> which we discussed earlier.
> >
> > Or you could not put unnecessary bindings into the Device Tree
> > by putting two and two together and realise that using the table
> > is the correct thing to do instead. This actually gives reason
> > to you previous patch, but should probably be fixed-up into it
> > so it has some proper meaning/purpose. ;)
> 
> Couldn't understand this one :(

Really?

Let's break it down - what do you need "stmpe,id" for?

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

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

[Viresh Kumar]

On 6 December 2012 16:05, Lee Jones <lee.jones@linaro.org> wrote:
>> > Or you could not put unnecessary bindings into the Device Tree
>> > by putting two and two together and realise that using the table
>> > is the correct thing to do instead. This actually gives reason
>> > to you previous patch, but should probably be fixed-up into it
>> > so it has some proper meaning/purpose. ;)
>>
>> Couldn't understand this one :(
>
> Really?

I tried again, but couldn't get it :(

> Let's break it down - what do you need "stmpe,id" for?

To distinguish two instances of of stmpe811 (for instance) on a board.
Names of stmpe sub-modules would contain .0, .1, if we have an id
passed to it. Passing -1 would create same names for both gpio blocks
which belonged to different stmpe811's.

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

[Lee Jones]

On Thu, 06 Dec 2012, Viresh Kumar wrote:

> On 6 December 2012 16:05, Lee Jones <lee.jones@linaro.org> wrote:
> >> > Or you could not put unnecessary bindings into the Device Tree
> >> > by putting two and two together and realise that using the table
> >> > is the correct thing to do instead. This actually gives reason
> >> > to you previous patch, but should probably be fixed-up into it
> >> > so it has some proper meaning/purpose. ;)
> >>
> >> Couldn't understand this one :(
> >
> > Really?
> 
> I tried again, but couldn't get it :(
> 
> > Let's break it down - what do you need "stmpe,id" for?
> 
> To distinguish two instances of of stmpe811 (for instance) on a board.
> Names of stmpe sub-modules would contain .0, .1, if we have an id
> passed to it. Passing -1 would create same names for both gpio blocks
> which belonged to different stmpe811's.

I thought we'd be over this? The 'ID' will be represented by the
address of the chip i.e. stmpe1601@40, where '40' will be
distinguishing factor?

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

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

[Viresh Kumar]

On 6 December 2012 16:42, Lee Jones <lee.jones@linaro.org> wrote:
> I thought we'd be over this? The 'ID' will be represented by the
> address of the chip i.e. stmpe1601@40, where '40' will be
> distinguishing factor?

I haven't tested it but i thought we are getting i2c device name from
modalias() fn running on "st,stmpe1601" and so would give us "stmpe1601".

Now, i went back to your earlier mail and checked device name:
stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212

It came from DT :)

So, probably no more issues of id's. Will resend my patch, after getting
rid of tables.

--
viresh

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

[Lee Jones]

On Thu, 06 Dec 2012, Viresh Kumar wrote:

> On 6 December 2012 16:42, Lee Jones <lee.jones@linaro.org> wrote:
> > I thought we'd be over this? The 'ID' will be represented by the
> > address of the chip i.e. stmpe1601@40, where '40' will be
> > distinguishing factor?
> 
> I haven't tested it but i thought we are getting i2c device name from
> modalias() fn running on "st,stmpe1601" and so would give us "stmpe1601".
> 
> Now, i went back to your earlier mail and checked device name:
> stmpe-i2c 0-0040: stmpe1601 detected, chip id: 0x212
> 
> It came from DT :)
> 
> So, probably no more issues of id's. Will resend my patch, after getting
> rid of tables.

Now we're talking!

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

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

[Grant Likely]

On Thu, 6 Dec 2012 07:28:22 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> First of all, thanks for explaining :)
> 
> On 6 December 2012 04:12, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >> 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.
> >
> > of_driver_match_device() matches against the compatible list in
> > dev->of_node, not against the device name. So, if the compatible
> > property has a string that is in the table, then it really should match
> > against it.
> 
> How could i misread it? Yes you are correct.
> 
> >> static int i2c_device_probe(struct device *dev)
> >> {
> >>         .....
> >>       status = driver->probe(client, i2c_match_id(driver->id_table, client));
> >
> > Here things are a bit wonky. Even if matched against the table, it is
> 
> table means of_device_id table ?

yes

> 
> > possible that it also matches against i2c_match_id() and that data is
> > passed to the driver.
> 
> It is a possibility or guarantee ? And so whatever device name we got from
> modalias routine, should match with the names in driver->id_table.

possibility. If the generated alias name does match something in
driver->id_table, then that pointer will be returned here.

> > But regardless, it is the responsiblity of the probe function to go and
> > look if of_driver_match_device() matches against anything if it cares
> > about the of_match_table entries (for instance, if there is extra data
> > attached).
> 
> Ok, so filling .data field in of_device_id[] is not required for our case as
> we aren't doing anything special in our drivers.

As long as things are simple, correct.

g.

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

[Grant Likely]

On Thu, 6 Dec 2012 08:06:20 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 6 December 2012 04:12, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Sat, 1 Dec 2012 00:33:46 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >> 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.
> >
> > of_driver_match_device() matches against the compatible list in
> > dev->of_node, not against the device name. So, if the compatible
> > property has a string that is in the table, then it really should match
> > against it.
> 
> Grant, but isn't it true that the final device's name would not be the DT
> way of names? It would simply be "stmpe810" for us and so if we have
> multiple instances of stmpe on a board, we need to distinguish them
> ourselves? One way for that was passing id for these instances, which
> would finally be used, when we create platform devices for sub-modules
> of stmpe (gpio, keypad, ts, etc).

of_modalias_node() is based on a *heruistic*. It is a best-effort
attempt to convert the node's compatible lists into a string that will
match against an existing driver. In the simple case it works because
historically i2c has used the chip name for the driver name. We get
lucky and a lot of drivers will work with DT without changes.

However, it is in no way guaranteed. Sometimes the strings won't line up
or a certain silicon vendor will have an extra errata or feature. In
that case it makes sense to use a DT match table that can parse the
entries in the compatible list. Or a driver can call of_ helper
functions. For example, it might call of_alias_get_id() to figure out
which device id it needs. That's why the full DT parsing exists. It's
the fallback when the simple heuristic fails. Only use it when you need
to.

g.