[PATCH] doc: dt: mtd: Add sst25vf016b to the list of supported chip names

[PATCH] doc: dt: mtd: Add sst25vf016b to the list of supported chip names

[Fabrizio Castro]

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
 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 9ce35af..5cdd527 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
@@ -30,6 +30,7 @@ Required properties:
                  s25sl12801
                  s25fl008k
                  s25fl064k
+                 sst25vf016b
                  sst25vf040b
                  m25p40
                  m25p80
-- 
2.7.4

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

[Rob Herring]

On Mon, Sep 11, 2017 at 09:35:28AM +0100, Fabrizio Castro wrote:
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

"dt-bindings: mtd: " for the subject prefix and you need a changelog.

> ---
>  Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 1 +
>  1 file changed, 1 insertion(+)

With that fixed,

Acked-by: Rob Herring <robh@kernel.org>

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

[Geert Uytterhoeven]

On Mon, Sep 11, 2017 at 10:35 AM, Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

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

[Fabrizio Castro]

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@bp.renesas.com>
Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
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
-- 
2.7.4

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

[Fabrizio Castro]

Dear All,

how does this patch look like?

Thanks,
Fabrizio

> Subject: [PATCH v2] dt-bindings: mtd: Add sst25vf016b to the list of supported chip names
>
> 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@bp.renesas.com>
> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> 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
> --
> 2.7.4




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

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

[Cyrille Pitchen]

Hi Fabrizio,

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.

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'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@bp.renesas.com>
> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> 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
> 

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

[Fabrizio Castro]

Hello Cyrille,

it's ok, thank you for the feedback.

Best regards,
Fab

> Hi Fabrizio,
>
> 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.
>
> 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'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@bp.renesas.com>
> > Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
> > Acked-by: Rob Herring <robh@kernel.org>
> > Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > 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
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> 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.

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

[Marek Vasut]

On 11/17/2017 06:36 PM, Cyrille Pitchen wrote:
> Hi Fabrizio,
> 
> sorry but I won't apply this patch.

ACK on that, if it can be detected with READID or SFDP, use that and
don't add redundant stuff into the DT bindings.

btw which renesas board has this SPI NOR on it ?

> New values for the 'compatible' DT properties should only be added for
> memory parts not supporting the JEDEC READ ID (0x9F) command.
> 
> 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'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@bp.renesas.com>
>> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> 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
>>
> 


-- 
Best regards,
Marek Vasut

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@wedev4u.fr> 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@bp.renesas.com>
>> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> 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@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 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@wedev4u.fr> 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@bp.renesas.com>
> >> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
> >> Acked-by: Rob Herring <robh@kernel.org>
> >> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> ---
> >> 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@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
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> 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.

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

[Geert Uytterhoeven]

Hi Fabrizio,

On Mon, Nov 20, 2017 at 10:43 AM, Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
>> On Fri, Nov 17, 2017 at 6:36 PM, Cyrille Pitchen
>> <cyrille.pitchen@wedev4u.fr> wrote:
>> 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

And it is queued in dt-rh/for-next (I thought I had that tree included, but
apparently I hadn't. Fixed ;-)

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 v2] dt-bindings: mtd: Add sst25vf016b to the list of supported chip names

[Cyrille Pitchen]

Hi Geert,

Le 20/11/2017 à 09:49, Geert Uytterhoeven a écrit :
> Hi Cyrille,
> 
> On Fri, Nov 17, 2017 at 6:36 PM, Cyrille Pitchen
> <cyrille.pitchen@wedev4u.fr> 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,

Documenting supported memory parts is not the same as documenting chip names
that can be used in the 'compatible' DT property. Here we are talking about
DT bindings.

>   2. Allowing checkpatch to validate compatible values in DTS files (see
>      below).
>

I you want to pass the checkpatch test then use the "jedec,spi-nor" string
alone. Please have a look at the drives/mtd/devices/m25p80.c file in the
m25p_of_table[] array: this is the value to be set in the 'compatible' DT
property.

SST25 memories do support the JEDEC READ ID op code (0x9F), so there is no
reason to use any other value but "jedec,spi-nor" for the 'compatible' DT
property.

Adding useless values would go to the wrong direction:
3094fe121e75 ("mtd: m25p80: remove unused flash entries from id_table")

As I've explained, the only new values that should be added in the m25p80.c
driver and the jedec,spi-nor.txt files are those for memory parts not
supporting the JEDEC READ ID op code (0x9F). No exception to that rule.

Also please note that there are far more entries in the spi_nor_ids[] array
from drivers/mtd/spi-nor/spi-nor.c than in the m25p_ids[] array from
drivers/mtd/devices/m25p80.c. Indeed, names in the spi_nor_ids[] array are
mostly informative and should not be considered as values for the
'compatible' DT properties.
For 'compatible' values, you should consider the m25p80.c driver only.

Moreover, we should refer to the comment in the m25p_ids[] array, applying
to the sst25vf016b memory part:

"""
Entries that were used in DTs without "jedec,spi-nor" fallback and
should be kept for backward compatibility.
"""

This is for backward compatibility purpose only, with some existing DTs,
likely before the "jedec,spi-nor" string was introduced.
However new DTs using values like "sst25vf016b" are _wrong_ and that's
why this is not documented is the jedec,spi-nor.txt file.

Best regards,

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

This is a deprecated usage. I agree some DT use such patterns; this is
legacy and we have to deal with it however it doesn't mean that it is the
correct binding. New device trees should use "jedec,spi-nor" alone.

That's something I plan to clarify in the jedec,spi-nor.txt file once
4.15-rc1 will be released.

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

And which prefix should be use ? I see both "microchip,sst25vf016b" and
"sst,sst25vf016b" in device trees.

> 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@bp.renesas.com>
>>> Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>> 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@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
>