[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 at kernel.org> wrote:
> On Mon, May 22, 2017 at 9:46 AM, Javier Martinez Canillas
> <javier at dowhile0.org> wrote:

[snip]

>>>> | >
>>>> | >     at24 at 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