Re: [PATCH 3/3] doc: dt: mtd: stop referring to driver code for spi-nor IDs

[Javier Martinez Canillas <javier@osg.samsung.com>]

Hello Brian,

On 11/18/2015 04:43 PM, Brian Norris wrote:
> Hi,
> 
> On Tue, Nov 17, 2015 at 11:04:55AM -0300, Javier Martinez Canillas wrote:
>> On 11/16/2015 07:34 PM, Brian Norris wrote:
>>> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
>>> index 2bee68103b01..2c91c03e7eb0 100644
>>> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
>>> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
>>> @@ -1,15 +1,61 @@
>>> -* MTD SPI driver for ST M25Pxx (and similar) serial flash chips
>>> +* SPI NOR flash: ST M25Pxx (and similar) serial flash chips
>>>  
>>>  Required properties:
>>>  - #address-cells, #size-cells : Must be present if the device has sub-nodes
>>>    representing partitions.
>>>  - compatible : May include a device-specific string consisting of the
>>> -               manufacturer and name of the chip. Bear in mind the DT binding
>>> -               is not Linux-only, but in case of Linux, see the "m25p_ids"
>>> -               table in drivers/mtd/devices/m25p80.c for the list of supported
>>> -               chips.
>>> +               manufacturer and name of the chip. A list of supported chip
>>> +               names follows.
>>
>> Here says that the compatible string consists of manufacturer and name...
>>
>>>                 Must also include "jedec,spi-nor" for any SPI NOR flash that can
>>>                 be identified by the JEDEC READ ID opcode (0x9F).
>>> +
>>> +               Supported chip names:
>>> +                 at25df321a
>>> +                 at25df641
> [...]
>> +
>>
>> ... but the entries in the list don't have a manufacturer. I know this is
>> due backward compatibility because as we discussed in the thread mentioned
>> in the cover letter, the SPI core didn't use the manufacturer and that
>> implementation detail leaked into the DTBs.
>>
>> But I think that either only the correct list with vendor should be added
>> to the DT binding docs (but make sure that backward compatibility in the
>> driver and SPI core is maintained) or both the wrong and correct list should
>> be documented and the former be marked as deprecated.
> 
> First, note that the list says "Supported chip *names*" (not "Supported
> compatible values"). It does not attempt to specify the full compatible
> value, and that's intentional.
> 

Right, sorry I missed that subtlety.

> Second, I believe it is hard to determine after-the-fact what all the
> reasonable pairings of vendors might be. For some of these parts,
> various companies have produced parts under the same chip ID -- usually
> because one company bought another. For most chips though, this probably
> isn't a problem, so I could probably pick reasonable vendor pairings.
>
> IOW, there isn't just "a wrong" and "a correct" list; there's a
> (probably?) correct list and an enormous space of "I don't know what
> people might have put here" list. It's nearly unbounded, but even a
> reasonable list might get pretty large. So in practice, we'd essentially
> be sacrificing completeness for...what reason?
>

I see.

>>> +               The following chip names have been used historically to
>>> +               designate quirky versions of flash chips that do not support the
>>> +               JEDEC READ ID opcode (0x9F):
>>> +                 m25p05-nonjedec
>>> +                 m25p10-nonjedec
>>> +                 m25p20-nonjedec
>>> +                 m25p40-nonjedec
>>> +                 m25p80-nonjedec
>>> +                 m25p16-nonjedec
>>> +                 m25p32-nonjedec
>>> +                 m25p64-nonjedec
>>> +                 m25p128-nonjedec
>>> +
>>
>> Same here, I would prefer if the DT binding make it clear that not having
>> a vendor is wrong and is only documented to maintain backward compatibility.
> 
> The doc never says anything about not including the vendor. It says
> 
>   "May include a device-specific string consisting of the manufacturer
>   and name of the chip"
> 
> and it lists the chip names. So if someone is actually following the
> documentation, they will include a vendor. The vendor names are not
> listed because they're really not relevant to the implementation. But I
> could try to include them.
>

My goal was to start forcing people to use a complete compatible string
to avoid the "garbage,chip-name" that you mentioned in the other thread.

The vendor are not relevant in the current implementation because how the
SPI core is implemented but I think that shouldn't affect the accuracy on
which hardware is described in the DT.
 
>>>  - reg : Chip-Select number
>>>  - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at
>>>  
>>>
> 
> So, what makes sense? I can make a separate list of vendors (my
> preference), or even try to pair up vendors with chip names (even if
> it's sometimes an N:1 relationship). But I don't see that really buying
> us much, since the implementation never has (and probably never will)
> enforce this. What do you think?
>

Yes, you are right that is hard to come with a reasonable list specially since
the vendor and chip relation could be N:1 as you said.

$SUBJECT is definitely an improvement over the current doc that mentions the
"m25p_ids" table in the driver though. So we can update later the DT binding
once / if the SPI core is modified to report proper OF modalias so OF-only
drivers can get rid of their SPI id table.

So for this patch:

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

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