i2c: core: make it possible to match a pure device tree driver

This tries to address an issue found when writing an MFD driver
for the Nomadik STw481x PMICs: as the platform is using device
tree exclusively I want to specify the driver matching like
this:

static const struct of_device_id stw481x_match[] = {
	{ .compatible = "st,stw4810", },
	{ .compatible = "st,stw4811", },
	{},
};

static struct i2c_driver stw481x_driver = {
	.driver = {
		.name	= "stw481x",
		.of_match_table = stw481x_match,
	},
	.probe		= stw481x_probe,
	.remove		= stw481x_remove,
};

However that turns out not to be possible: the I2C probe code
is written so that the probe() call is always passed a match
from i2c_match_id() using non-devicetree matches.

This is probably why most devices using device tree for I2C
clients currently will pass no .of_match_table *at all* but
instead just use .id_table from struct i2c_driver to match
the device. As you realize that means that the whole idea with
compatible strings is discarded, and that is why we find strange
device tree I2C device compatible strings like "product" instead
of "vendor,product" as you could expect.

Let's figure out how to fix this before the mess spreads. This
patch will allow probeing devices with only an of_match_table
as per above, and will pass NULL as the second argument to the
probe() function. If the driver wants to deduce secondary info
from the struct of_device_id .data field, it has to call
of_match_device() on its own match table in the probe function
device tree probe path.

If drivers define both an .of_match_table *AND* a i2c_driver
.id_table, the .of_match_table will take precedence, just
as is done in the i2c_device_match() function in i2c-core.c.

I2C devices probed from device tree should subsequently be
fixed to handle the case where of_match_table() is
used (I think none of them do that today), and platforms should
fix their device trees to use compatible strings for I2C devices
instead of setting the name to Linux device driver names as is
done in multiple cases today.

Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I would need some device tree core people to confirm that I am
on the right track with this. I was soooo confused when I found
that .of_match_table could not be used with I2C devices...
---
 drivers/i2c/i2c-core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Hi Linus,

On Mon, May 13, 2013 at 12:26:23AM +0200, Linus Walleij wrote:
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I would need some device tree core people to confirm that I am
> on the right track with this. I was soooo confused when I found
> that .of_match_table could not be used with I2C devices...
> ---
>  drivers/i2c/i2c-core.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 6b63cc7..30b5bb2 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -240,7 +240,7 @@ static int i2c_device_probe(struct device *dev)
>  		return 0;
>  
>  	driver = to_i2c_driver(dev->driver);
> -	if (!driver->probe || !driver->id_table)
> +	if (!driver->probe || (!driver->id_table && !dev->driver->of_match_table))
>  		return -ENODEV;
>  	client->driver = driver;
>  	if (!device_can_wakeup(&client->dev))
> @@ -248,7 +248,12 @@ static int i2c_device_probe(struct device *dev)
>  					client->flags & I2C_CLIENT_WAKE);
>  	dev_dbg(dev, "probe\n");
>  
> -	status = driver->probe(client, i2c_match_id(driver->id_table, client));
> +	if (dev->driver->of_match_table)
> +		/* Device tree matching */
> +		status = driver->probe(client, NULL);
> +	else
> +		/* Fall back to matching the id_table */
> +		status = driver->probe(client, i2c_match_id(driver->id_table, client));

If you correctly register a device with "vendor,product" in the devicetree
the driver can already fetch the of_device_id using of_match_device(dt_ids, &client->dev)
just like a platform driver would do aswell.

i2c_match_id will return a NULL pointer if called with "vendor,product",
because nothing matches in the drivers id_table, so for this case you
change nothing.

If anything, you introduce the problem that a devicetree capable driver
no longer gets a i2c_device_id if the device was instantiated with
i2c_board_info.

See how the mc13xxx driver does it:

	if (client->dev.of_node) {
		const struct of_device_id *of_id =
			of_match_device(mc13xxx_dt_ids, &client->dev);
		mc13xxx->variant = of_id->data;
	} else {
		mc13xxx->variant = (void *)id->driver_data;
	}

This works.

Sascha

On Mon, May 13, 2013 at 9:16 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

>> -     status = driver->probe(client, i2c_match_id(driver->id_table, client));
>> +     if (dev->driver->of_match_table)
>> +             /* Device tree matching */
>> +             status = driver->probe(client, NULL);
>> +     else
>> +             /* Fall back to matching the id_table */
>> +             status = driver->probe(client, i2c_match_id(driver->id_table, client));
>
> If you correctly register a device with "vendor,product" in the devicetree
> the driver can already fetch the of_device_id using of_match_device(dt_ids, &client->dev)
> just like a platform driver would do aswell.

Yes, this is what I write in the commit message:

  "(...) If the driver wants to deduce secondary info
  from the struct of_device_id .data field, it has to call
  of_match_device() on its own match table in the probe function
  device tree probe path."

> i2c_match_id will return a NULL pointer if called with "vendor,product",
> because nothing matches in the drivers id_table, so for this case you
> change nothing.
>
> If anything, you introduce the problem that a devicetree capable driver
> no longer gets a i2c_device_id if the device was instantiated with
> i2c_board_info.

Hm, you're right there, what about this:

+     if (dev->of_node)
+             /* Device tree matching */
+             status = driver->probe(client, NULL);
+     else
+             /* Fall back to matching the id_table */
+             status = driver->probe(client,
i2c_match_id(driver->id_table, client));

If the device has an of_node it surely should not be using the
id_table and it'd be correct to pass NULL, right?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 13 May 2013 10:26, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 13, 2013 at 9:16 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
>>> -     status = driver->probe(client, i2c_match_id(driver->id_table, client));
>>> +     if (dev->driver->of_match_table)
>>> +             /* Device tree matching */
>>> +             status = driver->probe(client, NULL);
>>> +     else
>>> +             /* Fall back to matching the id_table */
>>> +             status = driver->probe(client, i2c_match_id(driver->id_table, client));
>>
>> If you correctly register a device with "vendor,product" in the devicetree
>> the driver can already fetch the of_device_id using of_match_device(dt_ids, &client->dev)
>> just like a platform driver would do aswell.
>
> Yes, this is what I write in the commit message:
>
>   "(...) If the driver wants to deduce secondary info
>   from the struct of_device_id .data field, it has to call
>   of_match_device() on its own match table in the probe function
>   device tree probe path."
>
>> i2c_match_id will return a NULL pointer if called with "vendor,product",
>> because nothing matches in the drivers id_table, so for this case you
>> change nothing.
>>
>> If anything, you introduce the problem that a devicetree capable driver
>> no longer gets a i2c_device_id if the device was instantiated with
>> i2c_board_info.
>
> Hm, you're right there, what about this:
>
> +     if (dev->of_node)
> +             /* Device tree matching */
> +             status = driver->probe(client, NULL);
> +     else
> +             /* Fall back to matching the id_table */
> +             status = driver->probe(client,
> i2c_match_id(driver->id_table, client));
>
> If the device has an of_node it surely should not be using the
> id_table and it'd be correct to pass NULL, right?

That will break anything described in the device tree, but the driver
currently using the heuristic and depending on the data field. You
would instead need to test for an of_match_device() hit when
suppressing the data field.



g.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html