Message-ID: <20121205131904.GJ2718@gmail.com>
Date:	Wed, 5 Dec 2012 13:19:04 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	grant.likely@...retlab.ca, 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

> 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@...aro.org> wrote:
> > 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

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/

Powered by blists - more mailing lists