Re: [PATCH v2] dt-bindings: mtd: Add sst25vf016b to the list of supported chip names

[Geert Uytterhoeven]

Hi Cyrille,

On Fri, Nov 17, 2017 at 6:36 PM, Cyrille Pitchen
<cyrille.pitchen@xxxxxxxxxx> wrote:
> sorry but I won't apply this patch.
>
> New values for the 'compatible' DT properties should only be added for
> memory parts not supporting the JEDEC READ ID (0x9F) command.

I tent to disagree. Documenting part numbers in the DT bindings serves two
purposes:
  1. Documenting which parts are supported,
  2. Allowing checkpatch to validate compatible values in DTS files (see
     below).

Unfortunately the current
Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
is useless for the latter, as the values listed don't contain the vendor
prefixes, still causing warnings like

WARNING: DT compatible string "sst,sst25vf016b" appears un-documented
-- check ./Documentation/devicetree/bindings/
#79: FILE: arch/arm/boot/dts/r8a7743-iwg20m.dtsi:79:
+ compatible = "sst,sst25vf016b", "jedec,spi-nor";

So I suggest prepending all part number with the appropriate vendor prefixes
in jedec,spi-nor.txt.

BTW, "sst" (for Silicon Storage Technology) should be added to
Documentation/devicetree/bindings/vendor-prefixes.txt, too, to avoid another
warning:

WARNING: DT compatible string vendor "sst" appears un-documented --
check ./Documentation/devicetree/bindings/vendor-prefixes.txt
#79: FILE: arch/arm/boot/dts/r8a7743-iwg20m.dtsi:79:
+ compatible = "sst,sst25vf016b", "jedec,spi-nor";

> SST25 memories do support this command hence should use the "jedec,spi-nor"
> value alone. For historical reasons, some DT nodes set their 'compatible'
> string to something like "<vendor>,<model>", "jedec,spi-nor".
> It should work as expected in most case however I discourage from doing so
> in new device trees because it may have some side effects especially when
> the m25p80.c driver is used between the spi-nor.c driver and the SPI
> controller driver.

It is considered good practice to always have in DT a compatible value for
the real part number, not just for a generic fallback.
This allows to discriminate using the real part number, in case an anomaly
shows up later.
How else are you gonna handle e.g. a production batch that accidentally
contains a wrong JEDEC ID? And adding the part-specific compatible value
after the discovery won't help, as it won't be present in existing DTSes.

> It's all about setting the 2nd parameter of spi_nor_scan(), the 'name'
> parameter, as NULL when possible. This parameter should be set to a non NULL
> value only for memories not supporting the JEDEC READ ID op code (0x9F).
>
> Best regards,
>
> Cyrille
>
> Le 24/10/2017 Ã 15:50, Fabrizio Castro a Ãcrit :
>> There are a few DT files that make use of sst25vf016b in their
>> compatible strings, and the driver supports this chip already.
>> This patch improves the documentation and therefore the result
>> of ./scripts/checkpatch.pl.
>>
>> Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
>> Signed-off-by: Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>
>> Acked-by: Rob Herring <robh@xxxxxxxxxx>
>> Acked-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>> ---
>> Thank you Rob, thank you Geert, and sorry for the delay on this one.
>> Here is v2.
>>
>> Changes in v2:
>> * fixed subject prefix
>> * added changelog
>>
>>  Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
>> index 4cab5d8..bf56d77 100644
>> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
>> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
>> @@ -31,6 +31,7 @@ Required properties:
>>                   s25sl12801
>>                   s25fl008k
>>                   s25fl064k
>> +                 sst25vf016b
>>                   sst25vf040b
>>                   sst25wf040b
>>                   m25p40

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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