[PATCH 0/4] misc: Add OF device table to I2C drivers that are missing it

[PATCH 0/4] misc: Add OF device table to I2C drivers that are missing it

[Javier Martinez Canillas]

Hello,

This series add OF device ID tables to misc I2C drivers whose devices are
either used in Device Tree source files or are listed in binding docs as
a compatible string.

That's done because the plan is to change the I2C core to report proper OF
modaliases instead of always reporting a MODALIAS=i2c:<foo> regardless if
a device was registered via DT or using the legacy platform data mechanism.

So these patches will make sure that misc I2C drivers modules will continue
to be autoloaded once the I2C core is changed to report proper OF modalias.

Best regards,
Javier


Javier Martinez Canillas (4):
  misc: tsl2550: Add OF device ID table
  misc: ds1682: Add OF device ID table
  eeprom: idt_89hpesx: Add OF device ID table
  eeprom: at24: Add OF device ID table

 drivers/misc/ds1682.c             |   7 ++
 drivers/misc/eeprom/at24.c        | 189 +++++++++++++++++++++++++++++++++++++-
 drivers/misc/eeprom/idt_89hpesx.c |  57 ++++++++++++
 drivers/misc/tsl2550.c            |   7 ++
 4 files changed, 259 insertions(+), 1 deletion(-)

-- 
2.9.3

[PATCH 1/4] misc: tsl2550: Add OF device ID table

[Javier Martinez Canillas]

The driver doesn't have a struct of_device_id table but supported devices
are registered via Device Trees. This is working on the assumption that a
I2C device registered via OF will always match a legacy I2C device ID and
that the MODALIAS reported will always be of the form i2c:<device>.

But this could change in the future so the correct approach is to have an
OF device ID table if the devices are registered via OF.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/misc/tsl2550.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/misc/tsl2550.c b/drivers/misc/tsl2550.c
index 87a13374fdc0..9e7d01c68c68 100644
--- a/drivers/misc/tsl2550.c
+++ b/drivers/misc/tsl2550.c
@@ -443,9 +443,16 @@ static const struct i2c_device_id tsl2550_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, tsl2550_id);
 
+static const struct of_device_id tsl2550_of_match[] = {
+	{ .compatible = "taos,tsl2550" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tsl2550_of_match);
+
 static struct i2c_driver tsl2550_driver = {
 	.driver = {
 		.name	= TSL2550_DRV_NAME,
+		.of_match_table = of_match_ptr(tsl2550_of_match),
 		.pm	= TSL2550_PM_OPS,
 	},
 	.probe	= tsl2550_probe,
-- 
2.9.3

Re: [PATCH 1/4] misc: tsl2550: Add OF device ID table

[Arnd Bergmann]

\
>  static struct i2c_driver tsl2550_driver = {
>         .driver = {
>                 .name   = TSL2550_DRV_NAME,
> +               .of_match_table = of_match_ptr(tsl2550_of_match),
>                 .pm     = TSL2550_PM_OPS,
>         },

Please drop the incorrect of_match_ptr().

    Arnd

[PATCH 2/4] misc: ds1682: Add OF device ID table

[Javier Martinez Canillas]

The driver doesn't have a struct of_device_id table but supported devices
are registered via Device Trees. This is working on the assumption that a
I2C device registered via OF will always match a legacy I2C device ID and
that the MODALIAS reported will always be of the form i2c:<device>.

But this could change in the future so the correct approach is to have an
OF device ID table if the devices are registered via OF.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/misc/ds1682.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/misc/ds1682.c b/drivers/misc/ds1682.c
index c7112276a039..a9ad06646a9b 100644
--- a/drivers/misc/ds1682.c
+++ b/drivers/misc/ds1682.c
@@ -227,9 +227,16 @@ static const struct i2c_device_id ds1682_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ds1682_id);
 
+static const struct of_device_id ds1682_of_match[] = {
+	{ .compatible = "dallas,ds1682", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ds1682_of_match);
+
 static struct i2c_driver ds1682_driver = {
 	.driver = {
 		.name = "ds1682",
+		.of_match_table = of_match_ptr(ds1682_of_match),
 	},
 	.probe = ds1682_probe,
 	.remove = ds1682_remove,
-- 
2.9.3

Re: [PATCH 2/4] misc: ds1682: Add OF device ID table

[Arnd Bergmann]

On Tue, Mar 14, 2017 at 4:16 PM, Javier Martinez Canillas
<javier@osg.samsung.com> wrote:

> +static const struct of_device_id ds1682_of_match[] = {
> +       { .compatible = "dallas,ds1682", },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, ds1682_of_match);
> +
>  static struct i2c_driver ds1682_driver = {
>         .driver = {
>                 .name = "ds1682",
> +               .of_match_table = of_match_ptr(ds1682_of_match),
>         },
>         .probe = ds1682_probe,
>         .remove = ds1682_remove,

This will cause a warning if CONFIG_OF is disabled, since ds1682_of_match
becomes unused in this case. Please remove the of_match_ptr() around
the reference to ds1682_of_match.

    Arnd

Re: [PATCH 2/4] misc: ds1682: Add OF device ID table

[Javier Martinez Canillas]

Hello Arnd,

Thanks a lot for your feedback.

On 03/14/2017 05:46 PM, Arnd Bergmann wrote:
> On Tue, Mar 14, 2017 at 4:16 PM, Javier Martinez Canillas
> <javier@osg.samsung.com> wrote:
> 
>> +static const struct of_device_id ds1682_of_match[] = {
>> +       { .compatible = "dallas,ds1682", },
>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(of, ds1682_of_match);
>> +
>>  static struct i2c_driver ds1682_driver = {
>>         .driver = {
>>                 .name = "ds1682",
>> +               .of_match_table = of_match_ptr(ds1682_of_match),
>>         },
>>         .probe = ds1682_probe,
>>         .remove = ds1682_remove,
> 
> This will cause a warning if CONFIG_OF is disabled, since ds1682_of_match
> becomes unused in this case. Please remove the of_match_ptr() around
> the reference to ds1682_of_match.
>

Right, I tested it when CONFIG_OF is disabled with gcc (GCC) 6.3.1 20161221
and I didn't see any warning. But you are right and I'll re-spin the series
without using the macro.

>     Arnd
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 2/4] misc: ds1682: Add OF device ID table

[Javier Martinez Canillas]

On 03/14/2017 09:38 PM, Javier Martinez Canillas wrote:
> Hello Arnd,
> 
> Thanks a lot for your feedback.
> 
> On 03/14/2017 05:46 PM, Arnd Bergmann wrote:
>> On Tue, Mar 14, 2017 at 4:16 PM, Javier Martinez Canillas
>> <javier@osg.samsung.com> wrote:
>>
>>> +static const struct of_device_id ds1682_of_match[] = {
>>> +       { .compatible = "dallas,ds1682", },
>>> +       {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ds1682_of_match);
>>> +
>>>  static struct i2c_driver ds1682_driver = {
>>>         .driver = {
>>>                 .name = "ds1682",
>>> +               .of_match_table = of_match_ptr(ds1682_of_match),
>>>         },
>>>         .probe = ds1682_probe,
>>>         .remove = ds1682_remove,
>>
>> This will cause a warning if CONFIG_OF is disabled, since ds1682_of_match
>> becomes unused in this case. Please remove the of_match_ptr() around
>> the reference to ds1682_of_match.
>>
> 
> Right, I tested it when CONFIG_OF is disabled with gcc (GCC) 6.3.1 20161221
> and I didn't see any warning. But you are right and I'll re-spin the series

I didn't notice the build warning because I forgot to build with W=1 before
posting, sorry about that.

> without using the macro.
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[PATCH 3/4] eeprom: idt_89hpesx: Add OF device ID table

[Javier Martinez Canillas]

The driver doesn't have a struct of_device_id table but supported devices
are registered via Device Trees. This is working on the assumption that a
I2C device registered via OF will always match a legacy I2C device ID and
that the MODALIAS reported will always be of the form i2c:<device>.

But this could change in the future so the correct approach is to have an
OF device ID table if the devices are registered via OF.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/misc/eeprom/idt_89hpesx.c | 57 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/misc/eeprom/idt_89hpesx.c b/drivers/misc/eeprom/idt_89hpesx.c
index 4a22a1d99395..ec99f823a4ef 100644
--- a/drivers/misc/eeprom/idt_89hpesx.c
+++ b/drivers/misc/eeprom/idt_89hpesx.c
@@ -1541,12 +1541,69 @@ static const struct i2c_device_id idt_ids[] = {
 };
 MODULE_DEVICE_TABLE(i2c, idt_ids);
 
+static const struct of_device_id idt_of_match[] = {
+	{ .compatible = "idt,89hpes8nt2", },
+	{ .compatible = "idt,89hpes12nt3", },
+
+	{ .compatible = "idt,89hpes24nt6ag2", },
+	{ .compatible = "idt,89hpes32nt8ag2", },
+	{ .compatible = "idt,89hpes32nt8bg2", },
+	{ .compatible = "idt,89hpes12nt12g2", },
+	{ .compatible = "idt,89hpes16nt16g2", },
+	{ .compatible = "idt,89hpes24nt24g2", },
+	{ .compatible = "idt,89hpes32nt24ag2", },
+	{ .compatible = "idt,89hpes32nt24bg2", },
+
+	{ .compatible = "idt,89hpes12n3", },
+	{ .compatible = "idt,89hpes12n3a", },
+	{ .compatible = "idt,89hpes24n3", },
+	{ .compatible = "idt,89hpes24n3a", },
+
+	{ .compatible = "idt,89hpes32h8", },
+	{ .compatible = "idt,89hpes32h8g2", },
+	{ .compatible = "idt,89hpes48h12", },
+	{ .compatible = "idt,89hpes48h12g2", },
+	{ .compatible = "idt,89hpes48h12ag2", },
+	{ .compatible = "idt,89hpes16h16", },
+	{ .compatible = "idt,89hpes22h16", },
+	{ .compatible = "idt,89hpes22h16g2", },
+	{ .compatible = "idt,89hpes34h16", },
+	{ .compatible = "idt,89hpes34h16g2", },
+	{ .compatible = "idt,89hpes64h16", },
+	{ .compatible = "idt,89hpes64h16g2", },
+	{ .compatible = "idt,89hpes64h16ag2", },
+
+	{ .compatible = "idt,89hpes12t3g2", },
+	{ .compatible = "idt,89hpes24t3g2", },
+
+	{ .compatible = "idt,89hpes16t4", },
+	{ .compatible = "idt,89hpes4t4g2", },
+	{ .compatible = "idt,89hpes10t4g2", },
+	{ .compatible = "idt,89hpes16t4g2", },
+	{ .compatible = "idt,89hpes16t4ag2", },
+	{ .compatible = "idt,89hpes5t5", },
+	{ .compatible = "idt,89hpes6t5", },
+	{ .compatible = "idt,89hpes8t5", },
+	{ .compatible = "idt,89hpes8t5a", },
+	{ .compatible = "idt,89hpes24t6", },
+	{ .compatible = "idt,89hpes6t6g2", },
+	{ .compatible = "idt,89hpes24t6g2", },
+	{ .compatible = "idt,89hpes16t7", },
+	{ .compatible = "idt,89hpes32t8", },
+	{ .compatible = "idt,89hpes32t8g2", },
+	{ .compatible = "idt,89hpes48t12", },
+	{ .compatible = "idt,89hpes48t12g2", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, idt_of_match);
+
 /*
  * idt_driver - IDT 89HPESx driver structure
  */
 static struct i2c_driver idt_driver = {
 	.driver = {
 		.name = IDT_NAME,
+		.of_match_table = of_match_ptr(idt_of_match),
 	},
 	.probe = idt_probe,
 	.remove = idt_remove,
-- 
2.9.3

[PATCH 4/4] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

The driver doesn't have a struct of_device_id table but supported devices
are registered via Device Trees. This is working on the assumption that a
I2C device registered via OF will always match a legacy I2C device ID and
that the MODALIAS reported will always be of the form i2c:<device>.

But this could change in the future so the correct approach is to have an
OF device ID table if the devices are registered via OF.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---
Hello,

I added in the OF device ID table all the manufacturer,model combinations
that I found on either a DT bindings doc or Device Tree source files, but
the Documentation/devicetree/bindings/eeprom/eeprom.txt docs says that:

- compatible : should be "<manufacturer>,<type>", like these:

And give some examples. I wonder if such a lax definition for compatibles
is valid or if all possible combinations should be documented in the doc.

I would had expected the latter and keep both the OF device ID table and
bindings document updated as compatible EEPROM from a new vendor is used.

Best regards,
Javier

 drivers/misc/eeprom/at24.c | 189 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 188 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 764ff5df0dbc..b67b804855f9 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -12,6 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
@@ -175,6 +176,188 @@ static const struct i2c_device_id at24_ids[] = {
 };
 MODULE_DEVICE_TABLE(i2c, at24_ids);
 
+static const struct of_device_id at24_of_match[] = {
+	{
+		.compatible = "atmel,24c00",
+		.data = (void *)AT24_DEVICE_MAGIC(128 / 8, AT24_FLAG_TAKE8ADDR)
+	},
+	{
+		.compatible = "atmel,24c01",
+		.data = (void *)AT24_DEVICE_MAGIC(1024 / 8, 0)
+	},
+	{
+		.compatible = "at24,24c02",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_SERIAL | AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "atmel,24c02",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_SERIAL | AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "microchip,24c02",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_SERIAL | AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "nxp,24c02",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_SERIAL | AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "renesas,24c02",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_SERIAL | AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "renesas,r1ex24002",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_SERIAL | AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "at24,spd",
+		.data = (void *)AT24_DEVICE_MAGIC(2048 / 8,
+				AT24_FLAG_READONLY | AT24_FLAG_IRUGO)
+	},
+	{
+		.compatible = "at,24c04",
+		.data = (void *)AT24_DEVICE_MAGIC(4096 / 8, 0)
+	},
+	{
+		.compatible = "at24,24c04",
+		.data = (void *)AT24_DEVICE_MAGIC(4096 / 8, 0)
+	},
+	{
+		.compatible = "atmel,24c04",
+		.data = (void *)AT24_DEVICE_MAGIC(4096 / 8, 0)
+	},
+	{
+		.compatible = "at,24c08",
+		.data = (void *)AT24_DEVICE_MAGIC(8192 / 8, 0)
+	},
+	{
+		.compatible = "atmel,24c08",
+		.data = (void *)AT24_DEVICE_MAGIC(8192 / 8, 0)
+	},
+	{
+		.compatible = "atmel,24c16",
+		.data = (void *)AT24_DEVICE_MAGIC(16384 / 8, 0)
+	},
+	{
+		.compatible = "at,24c32",
+		.data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "at24,24c32",
+		.data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "atmel,24c32",
+		.data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "catalyst,24c32",
+		.data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "microchip,24c32",
+		.data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "at,24c64",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_ADDR16 |
+				AT24_FLAG_SERIAL |
+				AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "at24,24c64",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_ADDR16 |
+				AT24_FLAG_SERIAL |
+				AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "atmel,24c64",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_ADDR16 |
+				AT24_FLAG_SERIAL |
+				AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "microchip,24c64",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_ADDR16 |
+				AT24_FLAG_SERIAL |
+				AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "ramtron,24c64",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_ADDR16 |
+				AT24_FLAG_SERIAL |
+				AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "st,24c64",
+		.data = (void *)AT24_DEVICE_MAGIC(16,
+				AT24_FLAG_ADDR16 |
+				AT24_FLAG_SERIAL |
+				AT24_FLAG_READONLY)
+	},
+	{
+		.compatible = "atmel,24c128",
+		.data = (void *)AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "renesas,24c128",
+		.data = (void *)AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "at,24c256",
+		.data = (void *)AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "at24,24c256",
+		.data = (void *)AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "atmel,24c256",
+		.data = (void *)AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "st,24c256",
+		.data = (void *)AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "at24,24c512",
+		.data = (void *)AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "atmel,24c512",
+		.data = (void *)AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "microchip,24c512",
+		.data = (void *)AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "at,24c1024",
+		.data = (void *)AT24_DEVICE_MAGIC(1048576 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "atmel,24c1024",
+		.data = (void *)AT24_DEVICE_MAGIC(1048576 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "st,24c1024",
+		.data = (void *)AT24_DEVICE_MAGIC(1048576 / 8, AT24_FLAG_ADDR16)
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, at24_of_match);
+
 static const struct acpi_device_id at24_acpi_ids[] = {
 	{ "INT3499", AT24_DEVICE_MAGIC(8192 / 8, 0) },
 	{ }
@@ -598,7 +781,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (client->dev.platform_data) {
 		chip = *(struct at24_platform_data *)client->dev.platform_data;
 	} else {
-		if (id) {
+		if (client->dev.of_node) {
+			magic = (kernel_ulong_t)
+				of_device_get_match_data(&client->dev);
+		} else if (id) {
 			magic = id->driver_data;
 		} else {
 			const struct acpi_device_id *aid;
@@ -814,6 +1000,7 @@ static int at24_remove(struct i2c_client *client)
 static struct i2c_driver at24_driver = {
 	.driver = {
 		.name = "at24",
+		.of_match_table = of_match_ptr(at24_of_match),
 		.acpi_match_table = ACPI_PTR(at24_acpi_ids),
 	},
 	.probe = at24_probe,
-- 
2.9.3

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Andy Shevchenko]

On Tue, Mar 14, 2017 at 5:16 PM, Javier Martinez Canillas
<javier@osg.samsung.com> wrote:
> The driver doesn't have a struct of_device_id table but supported devices
> are registered via Device Trees. This is working on the assumption that a
> I2C device registered via OF will always match a legacy I2C device ID and
> that the MODALIAS reported will always be of the form i2c:<device>.
>
> But this could change in the future so the correct approach is to have an
> OF device ID table if the devices are registered via OF.

I'm bot sure this patch does something useful right now. Can we
survive without it? I think we may.

>  drivers/misc/eeprom/at24.c | 189 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 188 insertions(+), 1 deletion(-)

It's a huge! It will increase not only driver code base but memory
footprint for almost no benefit.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello Andy,

On 03/14/2017 07:59 PM, Andy Shevchenko wrote:
> On Tue, Mar 14, 2017 at 5:16 PM, Javier Martinez Canillas
> <javier@osg.samsung.com> wrote:
>> The driver doesn't have a struct of_device_id table but supported devices
>> are registered via Device Trees. This is working on the assumption that a
>> I2C device registered via OF will always match a legacy I2C device ID and
>> that the MODALIAS reported will always be of the form i2c:<device>.
>>
>> But this could change in the future so the correct approach is to have an
>> OF device ID table if the devices are registered via OF.
> 
> I'm bot sure this patch does something useful right now. Can we
> survive without it? I think we may.
> 

Yes, we can survive without it for now. But the problem is that with current
I2C core, DT-only I2C drivers must have an I2C device table in order to have
module auto-load working. That's because the core always reports as modalias
i2c:<foo> regardless if the device was registered via DT or legacy mechanism.

And some maintainers don't accept patches doing this duplication and instead
ask for the core to be fixed, i.e [0]. But to make sure that fixing the core
won't add regressions in drivers that are relying in the current behavior,
patches like $SUBJECT are needed.

So there isn't an agreement if is better to just rely in the current behavior
(and have a superfluous I2C device ID table) or fix the I2C core (and need a
OF device ID table). I personally prefer the latter since that means that an
DT-only driver will only need a OF table, and only drivers that supports both
will need both tables.

>>  drivers/misc/eeprom/at24.c | 189 ++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 188 insertions(+), 1 deletion(-)
> 
> It's a huge! It will increase not only driver code base but memory
> footprint for almost no benefit.
>

Indeed, but these all are compatible strings used by DTS in mainline and so
should be in the OF device ID table in order to be matched and the proper
modalias reported (once the I2C core is fixed).

One option is to add #ifdef CONFIG_OF guards for the OF device table definition
but again there's no agreement on that one since some maintainers say the it is
better to always build the OF ID table than having #ifdefery in C code...

[0]: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1321026.html

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]


> So there isn't an agreement if is better to just rely in the current behavior
> (and have a superfluous I2C device ID table) or fix the I2C core (and need a
> OF device ID table).

For at24, the i2c_device_id table is not superfluous! It is used outside
the DT world as well.

> Indeed, but these all are compatible strings used by DTS in mainline and so
> should be in the OF device ID table in order to be matched and the proper
> modalias reported (once the I2C core is fixed).

I'd think we should fix the DTS files instead to contain a fallback we
agree on. Say, we agree on "atmel,at24c01" as a the generic fallback,
the DTS should contain:

	compatible = "<your_vendor>,<your_type>", "atmel,at24c01"

And we shall only keep compatible values in the source file which differ
in behaviour fromt the generic case.

> One option is to add #ifdef CONFIG_OF guards for the OF device table definition
> but again there's no agreement on that one since some maintainers say the it is
> better to always build the OF ID table than having #ifdefery in C code...

I don't like the #ifdeffery as well.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello Wolfram,

On 03/15/2017 04:58 AM, Wolfram Sang wrote:
> 
>> So there isn't an agreement if is better to just rely in the current behavior
>> (and have a superfluous I2C device ID table) or fix the I2C core (and need a
>> OF device ID table).
> 
> For at24, the i2c_device_id table is not superfluous! It is used outside
> the DT world as well.
>

Yes, I know. I was trying to explain to Andy what's the problem that I want to
solve in general, not talking about this particular driver. Sorry if that was
confusing.
 
>> Indeed, but these all are compatible strings used by DTS in mainline and so
>> should be in the OF device ID table in order to be matched and the proper
>> modalias reported (once the I2C core is fixed).
> 
> I'd think we should fix the DTS files instead to contain a fallback we
> agree on. Say, we agree on "atmel,at24c01" as a the generic fallback,
> the DTS should contain:
> 
> 	compatible = "<your_vendor>,<your_type>", "atmel,at24c01"
> 
> And we shall only keep compatible values in the source file which differ
> in behaviour fromt the generic case.
>

Do you know who that's familiar with this device and driver can do this? I've
been trying to fix all the drivers that are relying on having an I2C ID table
but whose devices are registered via DT and I've now posted patches for all.

I wanted to do that so this patch can finally land [0] but to be honest I'm
about to give up on this... it seems is causing a lot of churn to maintainers
and many don't see the benefit on having the I2C core to report a proper OF
modalias, and don't think the current behavior is particularly bad.

Unfortunately some maintainers do and don't accept patches adding I2C tables
only to have module autoloading working so I still think it should be fixed.

[0]: https://patchwork.kernel.org/patch/6903991/

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Andy Shevchenko]

On Wed, Mar 15, 2017 at 12:58 PM, Javier Martinez Canillas
<javier@osg.samsung.com> wrote:
> On 03/15/2017 04:58 AM, Wolfram Sang wrote:

> Unfortunately some maintainers do and don't accept patches adding I2C tables
> only to have module autoloading working so I still think it should be fixed.

Wait, how does it work for now?!

Sounds for me you are trying to solve non-existing issue.

> [0]: https://patchwork.kernel.org/patch/6903991/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello Andy,

On 03/15/2017 08:21 AM, Andy Shevchenko wrote:
> On Wed, Mar 15, 2017 at 12:58 PM, Javier Martinez Canillas
> <javier@osg.samsung.com> wrote:
>> On 03/15/2017 04:58 AM, Wolfram Sang wrote:
> 
>> Unfortunately some maintainers do and don't accept patches adding I2C tables
>> only to have module autoloading working so I still think it should be fixed.
> 
> Wait, how does it work for now?!
>

It only works if you have an I2C device ID table, but that may not be the case
for DT-only drivers that could only have an OF device ID table. In the latter
case module autoload won't work.

> Sounds for me you are trying to solve non-existing issue.
>

It's an existing issue. You _must_ have an I2C device ID table if you want to
autload a device driver which is superfluous for DT-only drivers.

In other words, if you register an I2C device using OF the modalias will be:

$ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
i2c:maxtouch

While the correct thing to report should be:

$ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
of:NtrackpadT<NULL>Catmel,maxtouch

>> [0]: https://patchwork.kernel.org/patch/6903991/
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Andy Shevchenko]

On Wed, Mar 15, 2017 at 1:39 PM, Javier Martinez Canillas
<javier@osg.samsung.com> wrote:
> Hello Andy,
>
> On 03/15/2017 08:21 AM, Andy Shevchenko wrote:
>> On Wed, Mar 15, 2017 at 12:58 PM, Javier Martinez Canillas
>> <javier@osg.samsung.com> wrote:
>>> On 03/15/2017 04:58 AM, Wolfram Sang wrote:
>>
>>> Unfortunately some maintainers do and don't accept patches adding I2C tables
>>> only to have module autoloading working so I still think it should be fixed.
>>
>> Wait, how does it work for now?!

> It only works if you have an I2C device ID table, but that may not be the case
> for DT-only drivers that could only have an OF device ID table. In the latter
> case module autoload won't work.

OK.

>> Sounds for me you are trying to solve non-existing issue.

> It's an existing issue. You _must_ have an I2C device ID table if you want to
> autload a device driver which is superfluous for DT-only drivers.

Okay, can you scope only affected drivers then? Looking to spread
patches from you over all drivers I dunno they are all affected right
now.

P.S. Personally I agree with maintainers who do *not* apply this. Sorry.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello Andy,

On 03/15/2017 07:43 PM, Andy Shevchenko wrote:
> On Wed, Mar 15, 2017 at 1:39 PM, Javier Martinez Canillas
> <javier@osg.samsung.com> wrote:
>> Hello Andy,
>>
>> On 03/15/2017 08:21 AM, Andy Shevchenko wrote:
>>> On Wed, Mar 15, 2017 at 12:58 PM, Javier Martinez Canillas
>>> <javier@osg.samsung.com> wrote:
>>>> On 03/15/2017 04:58 AM, Wolfram Sang wrote:
>>>
>>>> Unfortunately some maintainers do and don't accept patches adding I2C tables
>>>> only to have module autoloading working so I still think it should be fixed.
>>>
>>> Wait, how does it work for now?!
> 
>> It only works if you have an I2C device ID table, but that may not be the case
>> for DT-only drivers that could only have an OF device ID table. In the latter
>> case module autoload won't work.
> 
> OK.
> 
>>> Sounds for me you are trying to solve non-existing issue.
> 
>> It's an existing issue. You _must_ have an I2C device ID table if you want to
>> autload a device driver which is superfluous for DT-only drivers.
> 
> Okay, can you scope only affected drivers then? Looking to spread
> patches from you over all drivers I dunno they are all affected right
> now.
>

That's what I did. I've only posted patches for drivers that have DT support
but don't have an OF device ID table since module autoload will be broken for
those if the I2C core is fixed to report a proper OF modalias.

And drivers/misc/eeprom/at24.c is one of those drivers.
 
> P.S. Personally I agree with maintainers who do *not* apply this. Sorry.
> 

So what's your suggestion to solve the issue then? When I said that some
maintainers don't want a superfluous device table to be added I was talking
about I2C device ID table for DT-only drivers, but $SUBJECT is the opposite.

I've the impression that you are nacking $SUBJECT without fully understanding
what the problem is and how this patch + the patch for I2C core are fixing it.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 532 bytes --]


> > I'd think we should fix the DTS files instead to contain a fallback we
> > agree on. Say, we agree on "atmel,at24c01" as a the generic fallback,
> > the DTS should contain:
> > 
> > 	compatible = "<your_vendor>,<your_type>", "atmel,at24c01"
> > 
> > And we shall only keep compatible values in the source file which differ
> > in behaviour fromt the generic case.
> >
> 
> Do you know who that's familiar with this device and driver can do this? I've

Sorry, I don't understand your question. What do you mean?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello Wolfram,

On 03/16/2017 10:07 AM, Wolfram Sang wrote:
> 
>>> I'd think we should fix the DTS files instead to contain a fallback we
>>> agree on. Say, we agree on "atmel,at24c01" as a the generic fallback,
>>> the DTS should contain:
>>>
>>> 	compatible = "<your_vendor>,<your_type>", "atmel,at24c01"
>>>
>>> And we shall only keep compatible values in the source file which differ
>>> in behaviour fromt the generic case.
>>>
>>
>> Do you know who that's familiar with this device and driver can do this? I've
> 
> Sorry, I don't understand your question. What do you mean?
> 

Sorry, for not explaining myself correctly. I meant to ask who can do what you
suggested before. I'm certainly not familiar with this driver to identify what
is the minimum set of compatible strings that can be used as generic fallback.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]


> Sorry, for not explaining myself correctly. I meant to ask who can do what you
> suggested before. I'm certainly not familiar with this driver to identify what
> is the minimum set of compatible strings that can be used as generic fallback.

Well, I am the maintainer of this driver :) But we should definately get
Rob into the boat if he is OK with updating all DTS files having such an
EEPROM.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello Wolfram,

On 03/16/2017 10:36 AM, Wolfram Sang wrote:
> 
>> Sorry, for not explaining myself correctly. I meant to ask who can do what you
>> suggested before. I'm certainly not familiar with this driver to identify what
>> is the minimum set of compatible strings that can be used as generic fallback.
> 
> Well, I am the maintainer of this driver :) But we should definately get

Oh right, silly me :)

> Rob into the boat if he is OK with updating all DTS files having such an
> EEPROM.
>

Agreed, are you going to take care of that? To be honest I think I'll just give
up on this task, it has been a big time sink and I had to explain over and over
to different people what the problem is with the I2C modalias uevent reporting.

I've posted patches for all the drivers that could be affected when reporting a
proper OF modalias by the core and also the RFC patch to properly report it [0].

But it seems that for many maintainers this is just an unnecessary churn and they
don't think there's an issue with the current behaviour. So it feels I'm causing
more harm than good by keep pushing this.

[0]: https://lkml.org/lkml/2015/7/30/494

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]


> > Rob into the boat if he is OK with updating all DTS files having such an
> > EEPROM.
> >
> 
> Agreed, are you going to take care of that?

Nope, sorry, no bandwidth. You might ask Lee, he was very interested in
getting proper I2C OF support upstream.

> up on this task, it has been a big time sink and I had to explain over and over
> to different people what the problem is with the I2C modalias uevent reporting.

Next time, maybe do a wiki page and point people to it? That being said,
we should probably create a wiki page on the I2C wiki anyhow.
Documenting the current state of affairs. That I would do when I finally
get around to brush up I2C wiki.

> But it seems that for many maintainers this is just an unnecessary churn and they
> don't think there's an issue with the current behaviour. So it feels I'm causing
> more harm than good by keep pushing this.

I understand somehow. They probably were reluctant to change something
that is working, even if it is not pretty. I'm not saying it's not worth
it, yet one needs energy and motivation to push it through.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello Wolfram,

On 03/16/2017 12:05 PM, Wolfram Sang wrote:
> 
>>> Rob into the boat if he is OK with updating all DTS files having such an
>>> EEPROM.
>>>
>>
>> Agreed, are you going to take care of that?
> 
> Nope, sorry, no bandwidth. You might ask Lee, he was very interested in
> getting proper I2C OF support upstream.
>

Ok, understandable. Adding Lee and Kieran to cc who were also interested on this.
 
>> up on this task, it has been a big time sink and I had to explain over and over
>> to different people what the problem is with the I2C modalias uevent reporting.
> 
> Next time, maybe do a wiki page and point people to it? That being said,
> we should probably create a wiki page on the I2C wiki anyhow.
> Documenting the current state of affairs. That I would do when I finally
> get around to brush up I2C wiki.
> 

Agreed, I can help writing such a wiki page if you want. I've already requested for
an account at https://i2c.wiki.kernel.org.

>> But it seems that for many maintainers this is just an unnecessary churn and they
>> don't think there's an issue with the current behaviour. So it feels I'm causing
>> more harm than good by keep pushing this.
> 
> I understand somehow. They probably were reluctant to change something

Yes, I don't blame them. It's kind of corner case that most people don't hit it.

One problem though is that this implementation detail leaks into the DTS and DT
binding documents, as we saw people using compatible strings without a vendor
prefix just because they could.

> that is working, even if it is not pretty. I'm not saying it's not worth
> it, yet one needs energy and motivation to push it through.
> 

Yeah, I think I've the energy and motivation but unfortunately also not enough
time :) And likely to have even less time in the near future.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 4/4] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello,

On 03/16/2017 12:39 PM, Javier Martinez Canillas wrote:
> On 03/16/2017 12:05 PM, Wolfram Sang wrote:

[snip]

>>
>> Next time, maybe do a wiki page and point people to it? That being said,
>> we should probably create a wiki page on the I2C wiki anyhow.
>> Documenting the current state of affairs. That I would do when I finally
>> get around to brush up I2C wiki.
>>
> 
> Agreed, I can help writing such a wiki page if you want. I've already requested for
> an account at https://i2c.wiki.kernel.org.
> 

FYI, I've added a page to the wiki explaining the current issues with the I2C core
MODALIAS uevent reporting and OF match:

https://i2c.wiki.kernel.org/index.php/OF_Modalias

And also linked to the main page in the work-in-progress section:

https://i2c.wiki.kernel.org/index.php/Main_Page#Work_in_progress

Please feel free to add/remove/change anything that you think is missing or isn't
correct for you.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America