[PATCH v4 00/20] eeprom: at24: Add OF device ID table

[PATCH v4 00/20] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello Wolfram,

This series is a follow-up to patch [0] that added an OF device ID table
to the at24 EEPROM driver. As you suggested [1], this version instead of
adding entries for every used <vendor,device> tuple, only adds a single
entry for each chip type using the "atmel" vendor as a generic fallback.

This is a re-spin that addresses some issues pointed out by Rob Herring.

The first patch documents in the DT binding what's the correct vendor to
use and what are the ones that are being deprecated. The second one adds
the OF device ID table for the at24 driver and the next patches use this
vendor in the compatible string to each DTS that defines a compatible I2C
EEPROM device node.

Patches can be applied independently since the DTS changes without driver
changes are no-op and the OF table won't be used without the DTS changes.

[0]: https://lkml.org/lkml/2017/3/14/589
[1]: https://lkml.org/lkml/2017/3/15/99

Best regards,
Javier

Changes in v4:
- Document the manufacturers that have been deprecated (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).
- Only use the atmel manufacturer in the compatible string instead of
  keeping the deprecated ones (Rob Herring).

Changes in v3:
- Fix wrong .data values for "atmel,24c02" and "atmel,24c64" entries.
- Add Geert Uytterhoeven reviewed-by tag.
- Add Geert Uytterhoeven reviewed-by tag.

Changes in v2:
- Only add a single OF device ID entry for each device type (Wolfram Sang).

Javier Martinez Canillas (20):
  dt-bindings: i2c: eeprom: Document vendor to be used and deprecated
    ones
  eeprom: at24: Add OF device ID table
  ARM: dts: omap: Add generic compatible string for I2C EEPROM
  ARM: dts: turris-omnia: Add generic compatible string for I2C EEPROM
  ARM: dts: efm32: Add generic compatible string for I2C EEPROM
  ARM: dts: imx: Add generic compatible string for I2C EEPROM
  ARM: dts: keystone: Add generic compatible string for I2C EEPROM
  ARM: dts: lpc18xx: Add generic compatible string for I2C EEPROM
  ARM: dts: r7s72100: Add generic compatible string for I2C EEPROM
  ARM: dts: koelsch: Add generic compatible string for I2C EEPROM
  ARM: dts: socfpga: Add generic compatible string for I2C EEPROM
  ARM: dts: uniphier: Add generic compatible string for I2C EEPROM
  ARM: dts: zynq: Add generic compatible string for I2C EEPROM
  arm64: dts: ls1043a: Add generic compatible string for I2C EEPROM
  arm64: zynqmp: Add generic compatible string for I2C EEPROM
  powerpc/5200: Add generic compatible string for I2C EEPROM
  powerpc/fsl: Add generic compatible string for I2C EEPROM
  powerpc/512x: Add generic compatible string for I2C EEPROM
  powerpc/83xx: Add generic compatible string for I2C EEPROM
  powerpc/44x: Add generic compatible string for I2C EEPROM

 .../devicetree/bindings/eeprom/eeprom.txt          | 14 ++---
 arch/arm/boot/dts/am335x-baltos.dtsi               |  2 +-
 arch/arm/boot/dts/am335x-base0033.dts              |  2 +-
 arch/arm/boot/dts/am335x-bone-common.dtsi          | 10 ++--
 arch/arm/boot/dts/am335x-nano.dts                  |  2 +-
 arch/arm/boot/dts/am335x-pepper.dts                |  2 +-
 arch/arm/boot/dts/am335x-shc.dts                   |  2 +-
 arch/arm/boot/dts/am335x-sl50.dts                  |  2 +-
 arch/arm/boot/dts/am437x-idk-evm.dts               |  2 +-
 arch/arm/boot/dts/am437x-sk-evm.dts                |  2 +-
 arch/arm/boot/dts/am43x-epos-evm.dts               |  2 +-
 arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi    |  2 +-
 arch/arm/boot/dts/armada-385-turris-omnia.dts      |  2 +-
 arch/arm/boot/dts/efm32gg-dk3750.dts               |  2 +-
 arch/arm/boot/dts/imx27-phytec-phycard-s-som.dtsi  |  2 +-
 arch/arm/boot/dts/imx27-phytec-phycore-som.dtsi    |  2 +-
 arch/arm/boot/dts/imx28-evk.dts                    |  2 +-
 arch/arm/boot/dts/imx53-tqma53.dtsi                |  2 +-
 arch/arm/boot/dts/imx6q-cm-fx6.dts                 |  2 +-
 arch/arm/boot/dts/imx6q-utilite-pro.dts            |  2 +-
 arch/arm/boot/dts/keystone-k2e-evm.dts             |  2 +-
 arch/arm/boot/dts/keystone-k2hk-evm.dts            |  2 +-
 arch/arm/boot/dts/keystone-k2l-evm.dts             |  2 +-
 arch/arm/boot/dts/lpc4337-ciaa.dts                 |  6 +-
 arch/arm/boot/dts/lpc4350-hitex-eval.dts           |  2 +-
 arch/arm/boot/dts/lpc4357-ea4357-devkit.dts        |  2 +-
 arch/arm/boot/dts/omap3-cm-t3x.dtsi                |  2 +-
 arch/arm/boot/dts/omap3-gta04.dtsi                 |  2 +-
 arch/arm/boot/dts/omap3-sb-t35.dtsi                |  2 +-
 arch/arm/boot/dts/omap4-var-som-om44.dtsi          |  2 +-
 arch/arm/boot/dts/omap5-cm-t54.dts                 |  2 +-
 arch/arm/boot/dts/omap5-sbc-t54.dts                |  2 +-
 arch/arm/boot/dts/r7s72100-genmai.dts              |  2 +-
 arch/arm/boot/dts/r8a7791-koelsch.dts              |  2 +-
 arch/arm/boot/dts/socfpga_cyclone5_vining_fpga.dts |  2 +-
 arch/arm/boot/dts/uniphier-pro4-ace.dts            |  2 +-
 arch/arm/boot/dts/uniphier-pro4-sanji.dts          |  2 +-
 arch/arm/boot/dts/uniphier-pxs2-gentil.dts         |  2 +-
 arch/arm/boot/dts/zynq-zc702.dts                   |  2 +-
 arch/arm/boot/dts/zynq-zc706.dts                   |  2 +-
 arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts  |  4 +-
 arch/arm64/boot/dts/xilinx/zynqmp-ep108.dts        |  4 +-
 arch/powerpc/boot/dts/digsy_mtc.dts                |  2 +-
 arch/powerpc/boot/dts/fsl/b4qds.dtsi               |  8 +--
 arch/powerpc/boot/dts/fsl/c293pcie.dts             |  2 +-
 arch/powerpc/boot/dts/fsl/p1010rdb.dtsi            |  2 +-
 arch/powerpc/boot/dts/fsl/p1023rdb.dts             |  2 +-
 arch/powerpc/boot/dts/fsl/p2041rdb.dts             |  4 +-
 arch/powerpc/boot/dts/fsl/p3041ds.dts              |  4 +-
 arch/powerpc/boot/dts/fsl/p4080ds.dts              |  4 +-
 arch/powerpc/boot/dts/fsl/p5020ds.dts              |  4 +-
 arch/powerpc/boot/dts/fsl/p5040ds.dts              |  4 +-
 arch/powerpc/boot/dts/fsl/t208xqds.dtsi            |  8 +--
 arch/powerpc/boot/dts/fsl/t4240qds.dts             | 12 ++--
 arch/powerpc/boot/dts/fsl/t4240rdb.dts             |  6 +-
 arch/powerpc/boot/dts/mpc5121ads.dts               |  2 +-
 arch/powerpc/boot/dts/mpc8308_p1m.dts              |  2 +-
 arch/powerpc/boot/dts/mpc8349emitx.dts             |  4 +-
 arch/powerpc/boot/dts/mpc8377_rdb.dts              |  2 +-
 arch/powerpc/boot/dts/mpc8377_wlan.dts             |  2 +-
 arch/powerpc/boot/dts/mpc8378_rdb.dts              |  2 +-
 arch/powerpc/boot/dts/mpc8379_rdb.dts              |  2 +-
 arch/powerpc/boot/dts/pcm030.dts                   |  2 +-
 arch/powerpc/boot/dts/pcm032.dts                   |  2 +-
 arch/powerpc/boot/dts/warp.dts                     |  2 +-
 drivers/misc/eeprom/at24.c                         | 65 +++++++++++++++++++++-
 66 files changed, 159 insertions(+), 102 deletions(-)

-- 
2.9.3

[PATCH v4 02/20] 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.

Suggested-by: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Javier Martinez Canillas <javier@dowhile0.org>

---

Changes in v4: None
Changes in v3:
- Fix wrong .data values for "atmel,24c02" and "atmel,24c64" entries.

Changes in v2:
- Only add a single OF device ID entry for each device type (Wolfram Sang).

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

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 764ff5df0dbc..79c5c39be29c 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,64 @@ 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 = "atmel,24c02",
+		.data = (void *)AT24_DEVICE_MAGIC(2048 / 8, 0)
+	},
+	{
+		.compatible = "atmel,spd",
+		.data = (void *)AT24_DEVICE_MAGIC(2048 / 8,
+				AT24_FLAG_READONLY | AT24_FLAG_IRUGO)
+	},
+	{
+		.compatible = "atmel,24c04",
+		.data = (void *)AT24_DEVICE_MAGIC(4096 / 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 = "atmel,24c32",
+		.data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "atmel,24c64",
+		.data = (void *)AT24_DEVICE_MAGIC(65536 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "atmel,24c128",
+		.data = (void *)AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "atmel,24c256",
+		.data = (void *)AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "atmel,24c512",
+		.data = (void *)AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16)
+	},
+	{
+		.compatible = "atmel,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 +657,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 +876,7 @@ static int at24_remove(struct i2c_client *client)
 static struct i2c_driver at24_driver = {
 	.driver = {
 		.name = "at24",
+		.of_match_table = at24_of_match,
 		.acpi_match_table = ACPI_PTR(at24_acpi_ids),
 	},
 	.probe = at24_probe,
-- 
2.9.3

Re: [PATCH v4 00/20] eeprom: at24: Add OF device ID table

[Geert Uytterhoeven]

Hi Javier,

On Mon, May 22, 2017 at 4:01 PM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> This series is a follow-up to patch [0] that added an OF device ID table
> to the at24 EEPROM driver. As you suggested [1], this version instead of
> adding entries for every used <vendor,device> tuple, only adds a single
> entry for each chip type using the "atmel" vendor as a generic fallback.
>
> This is a re-spin that addresses some issues pointed out by Rob Herring.
>
> The first patch documents in the DT binding what's the correct vendor to
> use and what are the ones that are being deprecated. The second one adds
> the OF device ID table for the at24 driver and the next patches use this
> vendor in the compatible string to each DTS that defines a compatible I2C
> EEPROM device node.
>
> Patches can be applied independently since the DTS changes without driver
> changes are no-op and the OF table won't be used without the DTS changes.
>
> [0]: https://lkml.org/lkml/2017/3/14/589
> [1]: https://lkml.org/lkml/2017/3/15/99
>
> Best regards,
> Javier
>
> Changes in v4:
> - Document the manufacturers that have been deprecated (Rob Herring).
> - Only use the atmel manufacturer in the compatible string instead of
>   keeping the deprecated ones (Rob Herring).

I think you're referring to this (https://lkml.org/lkml/2017/4/19/1136)?

| > --- a/arch/arm/boot/dts/am335x-baltos.dtsi
| > +++ b/arch/arm/boot/dts/am335x-baltos.dtsi
| > @@ -255,7 +255,7 @@
| >     };
| >
| >     at24@50 {
| > -           compatible = "at24,24c02";
| > +           compatible = "at24,24c02", "atmel,24c02";
|
| I think you can just drop the at24 compatibles. A new kernel doesn't
| need it. An old kernel ignores the manufacturer. I checked that u-boot
| only matches on "atmel,*", so okay there. Don't know about the *BSDs. I
| couldn't find anything.

I think you misunderstood what Rob means.

In the case above it makes sense to drop the first compatible, as "at24" is
not a manufacturer, but refers to ATMEL's "AT24" line of i2c FLASH ROMs.

However, in cases where a real vendor/part combo is specified, like on
r8a7791-koelsch.dts:

-               compatible = "renesas,24c02";
+               compatible = "atmel,24c02";

you do want to keep the real vendor/part combo, i.e.

     compatible = "renesas,24c02", "atmel,24c02";

like in v3, which is what I gave my Reviewed-by for.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 00/20] eeprom: at24: Add OF device ID table

[Rob Herring]

On Mon, May 22, 2017 at 9:23 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Javier,
>
> On Mon, May 22, 2017 at 4:01 PM, Javier Martinez Canillas
> <javier@dowhile0.org> wrote:
>> This series is a follow-up to patch [0] that added an OF device ID table
>> to the at24 EEPROM driver. As you suggested [1], this version instead of
>> adding entries for every used <vendor,device> tuple, only adds a single
>> entry for each chip type using the "atmel" vendor as a generic fallback.
>>
>> This is a re-spin that addresses some issues pointed out by Rob Herring.
>>
>> The first patch documents in the DT binding what's the correct vendor to
>> use and what are the ones that are being deprecated. The second one adds
>> the OF device ID table for the at24 driver and the next patches use this
>> vendor in the compatible string to each DTS that defines a compatible I2C
>> EEPROM device node.
>>
>> Patches can be applied independently since the DTS changes without driver
>> changes are no-op and the OF table won't be used without the DTS changes.
>>
>> [0]: https://lkml.org/lkml/2017/3/14/589
>> [1]: https://lkml.org/lkml/2017/3/15/99
>>
>> Best regards,
>> Javier
>>
>> Changes in v4:
>> - Document the manufacturers that have been deprecated (Rob Herring).
>> - Only use the atmel manufacturer in the compatible string instead of
>>   keeping the deprecated ones (Rob Herring).
>
> I think you're referring to this (https://lkml.org/lkml/2017/4/19/1136)?
>
> | > --- a/arch/arm/boot/dts/am335x-baltos.dtsi
> | > +++ b/arch/arm/boot/dts/am335x-baltos.dtsi
> | > @@ -255,7 +255,7 @@
> | >     };
> | >
> | >     at24@50 {
> | > -           compatible = "at24,24c02";
> | > +           compatible = "at24,24c02", "atmel,24c02";
> |
> | I think you can just drop the at24 compatibles. A new kernel doesn't
> | need it. An old kernel ignores the manufacturer. I checked that u-boot
> | only matches on "atmel,*", so okay there. Don't know about the *BSDs. I
> | couldn't find anything.
>
> I think you misunderstood what Rob means.
>
> In the case above it makes sense to drop the first compatible, as "at24" is
> not a manufacturer, but refers to ATMEL's "AT24" line of i2c FLASH ROMs.
>
> However, in cases where a real vendor/part combo is specified, like on
> r8a7791-koelsch.dts:
>
> -               compatible = "renesas,24c02";
> +               compatible = "atmel,24c02";
>
> you do want to keep the real vendor/part combo, i.e.
>
>      compatible = "renesas,24c02", "atmel,24c02";

Yes, Geert is correct.

Rob

Re: [PATCH v4 00/20] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello Geert and Rob,

On Mon, May 22, 2017 at 4:26 PM, Rob Herring <robh@kernel.org> wrote:
> On Mon, May 22, 2017 at 9:23 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> Hi Javier,
>>
>> On Mon, May 22, 2017 at 4:01 PM, Javier Martinez Canillas
>> <javier@dowhile0.org> wrote:
>>> This series is a follow-up to patch [0] that added an OF device ID table
>>> to the at24 EEPROM driver. As you suggested [1], this version instead of
>>> adding entries for every used <vendor,device> tuple, only adds a single
>>> entry for each chip type using the "atmel" vendor as a generic fallback.
>>>
>>> This is a re-spin that addresses some issues pointed out by Rob Herring.
>>>
>>> The first patch documents in the DT binding what's the correct vendor to
>>> use and what are the ones that are being deprecated. The second one adds
>>> the OF device ID table for the at24 driver and the next patches use this
>>> vendor in the compatible string to each DTS that defines a compatible I2C
>>> EEPROM device node.
>>>
>>> Patches can be applied independently since the DTS changes without driver
>>> changes are no-op and the OF table won't be used without the DTS changes.
>>>
>>> [0]: https://lkml.org/lkml/2017/3/14/589
>>> [1]: https://lkml.org/lkml/2017/3/15/99
>>>
>>> Best regards,
>>> Javier
>>>
>>> Changes in v4:
>>> - Document the manufacturers that have been deprecated (Rob Herring).
>>> - Only use the atmel manufacturer in the compatible string instead of
>>>   keeping the deprecated ones (Rob Herring).
>>
>> I think you're referring to this (https://lkml.org/lkml/2017/4/19/1136)?
>>
>> | > --- a/arch/arm/boot/dts/am335x-baltos.dtsi
>> | > +++ b/arch/arm/boot/dts/am335x-baltos.dtsi
>> | > @@ -255,7 +255,7 @@
>> | >     };
>> | >
>> | >     at24@50 {
>> | > -           compatible = "at24,24c02";
>> | > +           compatible = "at24,24c02", "atmel,24c02";
>> |
>> | I think you can just drop the at24 compatibles. A new kernel doesn't
>> | need it. An old kernel ignores the manufacturer. I checked that u-boot
>> | only matches on "atmel,*", so okay there. Don't know about the *BSDs. I
>> | couldn't find anything.
>>
>> I think you misunderstood what Rob means.
>>
>> In the case above it makes sense to drop the first compatible, as "at24" is
>> not a manufacturer, but refers to ATMEL's "AT24" line of i2c FLASH ROMs.
>>
>> However, in cases where a real vendor/part combo is specified, like on
>> r8a7791-koelsch.dts:
>>
>> -               compatible = "renesas,24c02";
>> +               compatible = "atmel,24c02";
>>
>> you do want to keep the real vendor/part combo, i.e.
>>
>>      compatible = "renesas,24c02", "atmel,24c02";
>
> Yes, Geert is correct.
>

Sorry for misunderstanding your previous comment...

How this should be documented in the DT binding? Should I include
"renesas" as a valid manufacturer or only list the used
<vendor,device> pairs (i.e: "renesas,24c02")?

I also wonder why this is really needed if AFAIU "renesas,24c02" is
compatible with "atmel,24c02". IOW, the driver doesn't need to
differentiate between the two since the devices are the same and will
always match using "atmel,24c02".

> Rob

Best regards,
Javier

Re: [PATCH v4 00/20] eeprom: at24: Add OF device ID table

[Rob Herring]

On Mon, May 22, 2017 at 9:46 AM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> Hello Geert and Rob,
>
> On Mon, May 22, 2017 at 4:26 PM, Rob Herring <robh@kernel.org> wrote:
>> On Mon, May 22, 2017 at 9:23 AM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>> Hi Javier,
>>>
>>> On Mon, May 22, 2017 at 4:01 PM, Javier Martinez Canillas
>>> <javier@dowhile0.org> wrote:
>>>> This series is a follow-up to patch [0] that added an OF device ID table
>>>> to the at24 EEPROM driver. As you suggested [1], this version instead of
>>>> adding entries for every used <vendor,device> tuple, only adds a single
>>>> entry for each chip type using the "atmel" vendor as a generic fallback.
>>>>
>>>> This is a re-spin that addresses some issues pointed out by Rob Herring.
>>>>
>>>> The first patch documents in the DT binding what's the correct vendor to
>>>> use and what are the ones that are being deprecated. The second one adds
>>>> the OF device ID table for the at24 driver and the next patches use this
>>>> vendor in the compatible string to each DTS that defines a compatible I2C
>>>> EEPROM device node.
>>>>
>>>> Patches can be applied independently since the DTS changes without driver
>>>> changes are no-op and the OF table won't be used without the DTS changes.
>>>>
>>>> [0]: https://lkml.org/lkml/2017/3/14/589
>>>> [1]: https://lkml.org/lkml/2017/3/15/99
>>>>
>>>> Best regards,
>>>> Javier
>>>>
>>>> Changes in v4:
>>>> - Document the manufacturers that have been deprecated (Rob Herring).
>>>> - Only use the atmel manufacturer in the compatible string instead of
>>>>   keeping the deprecated ones (Rob Herring).
>>>
>>> I think you're referring to this (https://lkml.org/lkml/2017/4/19/1136)?
>>>
>>> | > --- a/arch/arm/boot/dts/am335x-baltos.dtsi
>>> | > +++ b/arch/arm/boot/dts/am335x-baltos.dtsi
>>> | > @@ -255,7 +255,7 @@
>>> | >     };
>>> | >
>>> | >     at24@50 {
>>> | > -           compatible = "at24,24c02";
>>> | > +           compatible = "at24,24c02", "atmel,24c02";
>>> |
>>> | I think you can just drop the at24 compatibles. A new kernel doesn't
>>> | need it. An old kernel ignores the manufacturer. I checked that u-boot
>>> | only matches on "atmel,*", so okay there. Don't know about the *BSDs. I
>>> | couldn't find anything.
>>>
>>> I think you misunderstood what Rob means.
>>>
>>> In the case above it makes sense to drop the first compatible, as "at24" is
>>> not a manufacturer, but refers to ATMEL's "AT24" line of i2c FLASH ROMs.
>>>
>>> However, in cases where a real vendor/part combo is specified, like on
>>> r8a7791-koelsch.dts:
>>>
>>> -               compatible = "renesas,24c02";
>>> +               compatible = "atmel,24c02";
>>>
>>> you do want to keep the real vendor/part combo, i.e.
>>>
>>>      compatible = "renesas,24c02", "atmel,24c02";
>>
>> Yes, Geert is correct.
>>
>
> Sorry for misunderstanding your previous comment...
>
> How this should be documented in the DT binding? Should I include
> "renesas" as a valid manufacturer or only list the used
> <vendor,device> pairs (i.e: "renesas,24c02")?

However you are handling any of the vendors. I'll have to go look.

> I also wonder why this is really needed if AFAIU "renesas,24c02" is
> compatible with "atmel,24c02". IOW, the driver doesn't need to
> differentiate between the two since the devices are the same and will
> always match using "atmel,24c02".

It is needed, so that when a difference is found, it can be handled
without updating the DT.

Rob

Re: [PATCH v4 00/20] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello Rob,

Thanks a lot for your feedback.

On Mon, May 22, 2017 at 6:52 PM, Rob Herring <robh@kernel.org> wrote:
> On Mon, May 22, 2017 at 9:46 AM, Javier Martinez Canillas
> <javier@dowhile0.org> wrote:

[snip]

>>>> | >
>>>> | >     at24@50 {
>>>> | > -           compatible = "at24,24c02";
>>>> | > +           compatible = "at24,24c02", "atmel,24c02";
>>>> |
>>>> | I think you can just drop the at24 compatibles. A new kernel doesn't
>>>> | need it. An old kernel ignores the manufacturer. I checked that u-boot
>>>> | only matches on "atmel,*", so okay there. Don't know about the *BSDs. I
>>>> | couldn't find anything.
>>>>
>>>> I think you misunderstood what Rob means.
>>>>
>>>> In the case above it makes sense to drop the first compatible, as "at24" is
>>>> not a manufacturer, but refers to ATMEL's "AT24" line of i2c FLASH ROMs.
>>>>
>>>> However, in cases where a real vendor/part combo is specified, like on
>>>> r8a7791-koelsch.dts:
>>>>
>>>> -               compatible = "renesas,24c02";
>>>> +               compatible = "atmel,24c02";
>>>>
>>>> you do want to keep the real vendor/part combo, i.e.
>>>>
>>>>      compatible = "renesas,24c02", "atmel,24c02";
>>>
>>> Yes, Geert is correct.
>>>
>>
>> Sorry for misunderstanding your previous comment...
>>
>> How this should be documented in the DT binding? Should I include
>> "renesas" as a valid manufacturer or only list the used
>> <vendor,device> pairs (i.e: "renesas,24c02")?
>
> However you are handling any of the vendors. I'll have to go look.
>

The current DT binding is quite lax when describing this. From
Documentation/devicetree/bindings/eeprom/eeprom.txt:

----------------------------------
EEPROMs (I2C)

Required properties:

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

"atmel,24c00", "atmel,24c01", "atmel,24c02", "atmel,24c04",
"atmel,24c08", "atmel,24c16", "atmel,24c32", "atmel,24c64",
"atmel,24c128", "atmel,24c256", "atmel,24c512", "atmel,24c1024"

"catalyst,24c32"

"microchip,24c128"

"ramtron,24c64"

"renesas,r1ex24002"

If there is no specific driver for <manufacturer>, a generic
driver based on <type> is selected. Possible types are:
"24c00", "24c01", "24c02", "24c04", "24c08", "24c16", "24c32", "24c64",
"24c128", "24c256", "24c512", "24c1024", "spd"

  - reg : the I2C address of the EEPROM
----------------------------------

I think though that this is one of the cases where the Linux I2C
subsystem matching logic is leaking into the DT binding doc, since the
manufacturer prefix is ignored by the I2C core (the I2C device ID
table is used to match and to report a MODALIAS).

But I'll keep the description generic as it is now and only mention
the atmel variants (at and at24) as deprecated then.

>> I also wonder why this is really needed if AFAIU "renesas,24c02" is
>> compatible with "atmel,24c02". IOW, the driver doesn't need to
>> differentiate between the two since the devices are the same and will
>> always match using "atmel,24c02".
>
> It is needed, so that when a difference is found, it can be handled
> without updating the DT.
>

Yes, I understand this. What I tried to ask is if there could really
be a difference for the same chip type between different vendors, or
is just that people were using other manufacturers in the compatible
string as a consequence of the DT binding doc and the I2C core
ignoring the vendor prefix.

I don't mind though, I will leave the manufacturers that are different
than the atmel variants in the mainline DTS as you and Geert asked.

> Rob

Best regards,
Javier

Re: [PATCH v4 00/20] eeprom: at24: Add OF device ID table

[Geert Uytterhoeven]

Hi Javier,

On Mon, May 22, 2017 at 7:15 PM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
>>> I also wonder why this is really needed if AFAIU "renesas,24c02" is
>>> compatible with "atmel,24c02". IOW, the driver doesn't need to
>>> differentiate between the two since the devices are the same and will
>>> always match using "atmel,24c02".
>>
>> It is needed, so that when a difference is found, it can be handled
>> without updating the DT.
>
> Yes, I understand this. What I tried to ask is if there could really
> be a difference for the same chip type between different vendors, or
> is just that people were using other manufacturers in the compatible
> string as a consequence of the DT binding doc and the I2C core
> ignoring the vendor prefix.

The devices from different vendors are not the same. They contain FLASH
ROM of a specific size, and glue logic to expose an i2c slave
interface providing
an AT24-compatible command set.  They should behave similar within
the limits of the AT24 "spec".  But the actual implementation may be different.

> I don't mind though, I will leave the manufacturers that are different
> than the atmel variants in the mainline DTS as you and Geert asked.

OK, thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 00/20] eeprom: at24: Add OF device ID table

[Javier Martinez Canillas]

Hello Geert,

On Mon, May 22, 2017 at 9:30 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Javier,
>
> On Mon, May 22, 2017 at 7:15 PM, Javier Martinez Canillas
> <javier@dowhile0.org> wrote:
>>>> I also wonder why this is really needed if AFAIU "renesas,24c02" is
>>>> compatible with "atmel,24c02". IOW, the driver doesn't need to
>>>> differentiate between the two since the devices are the same and will
>>>> always match using "atmel,24c02".
>>>
>>> It is needed, so that when a difference is found, it can be handled
>>> without updating the DT.
>>
>> Yes, I understand this. What I tried to ask is if there could really
>> be a difference for the same chip type between different vendors, or
>> is just that people were using other manufacturers in the compatible
>> string as a consequence of the DT binding doc and the I2C core
>> ignoring the vendor prefix.
>
> The devices from different vendors are not the same. They contain FLASH
> ROM of a specific size, and glue logic to expose an i2c slave
> interface providing
> an AT24-compatible command set.  They should behave similar within
> the limits of the AT24 "spec".  But the actual implementation may be different.
>

I see, really appreciate your explanation. I'm not familiar with these
devices and driver but the patch-series are needed in order to make
sure that no regressions will happen once the I2C core reports a
proper OF modalias.

>> I don't mind though, I will leave the manufacturers that are different
>> than the atmel variants in the mainline DTS as you and Geert asked.
>
> OK, thanks!
>

Thanks a lot for your feedback!

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --

Best regards,
Javier