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

[Fabrizio Castro]

Dear All,

> Subject: Re: [PATCH v2] dt-bindings: mtd: Add sst25vf016b to the list of supported chip names
>
> 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).

that's precisely why we decided to submit the patch, thank you Geert!

>
> 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";

we did submit a patch to fix this ("of: add vendor prefix for Silicon Storage Technology Inc."):
https://patchwork.kernel.org/patch/9946889/

a while ago

>
> > 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.

Totally agree with Geert.

Thanks,
Fab

>
> > 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
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.