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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 1 Dec 2012 00:33:46 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	grant.likely@...retlab.ca, Lee Jones <lee.jones@...aro.org>
Cc:	Samuel Ortiz <sameo@...ux.intel.com>, rabin.vincent@...ricsson.com,
	shiraz.hashim@...com, devicetree-discuss@...ts.ozlabs.org,
	linux-kernel@...r.kernel.org, spear-devel@...t.st.com,
	linus.walleij@...aro.org,
	Vipul Kumar Samar <vipulkumar.samar@...com>
Subject: Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver

On 30 November 2012 21:15, Lee Jones <lee.jones@...aro.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@...ress {
         ...
}

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/