[PATCH 0/3] mtd: m25p80: fix some module and documentation issues

[PATCH 0/3] mtd: m25p80: fix some module and documentation issues

[Brian Norris]

Hi,

There were a few mistakes and improvements pointed out at various points in
this thread, subject:

    spi: OF module autoloading is still broken
    (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor""
    breaks module autoloading)

    http://lists.infradead.org/pipermail/linux-mtd/2015-October/062369.html
    https://lkml.org/lkml/2015/11/12/574

That thread covers some other interesting issues that are still not solved, but
let's fix the ones we can here in MTD right now.

Regards,
Brian


Brian Norris (3):
  mtd: m25p80: fix module autoloading for "jedec,spi-nor" and "spi-nor"
  mtd: m25p80: replace leftover "nor-jedec" with "spi-nor" in comments
  doc: dt: mtd: stop referring to driver code for spi-nor IDs

 .../devicetree/bindings/mtd/jedec,spi-nor.txt      | 56 ++++++++++++++++++++--
 drivers/mtd/devices/m25p80.c                       | 17 +++++--
 2 files changed, 64 insertions(+), 9 deletions(-)

-- 
2.6.0.rc2.230.g3dd15c0

[PATCH 1/3] mtd: m25p80: fix module autoloading for "jedec, spi-nor" and "spi-nor"

[Brian Norris]

Commit 43163022927b ("mtd: m25p80: allow arbitrary OF matching for
"jedec,spi-nor"") moved the "jedec,spi-nor" handling from the
spi_device_id table to the of_match_table, to better handle matching
complex device tree compatible strings. With that patch, device tree
support works as expected when m25p80.c is built into the kernel.

However, that commit ignored the fact that:

 (1) (non-DT) platform devices might want to use the "spi-nor" string
     for matching with this driver, rather than picking an arbitrary one
     like "m25p80"
 (2) the core SPI uevent/modalias code doesn't yet support kernel module
     autoloading via of_match_table strings; so for DT-based devices, it
     will only report (part of) the first compatible string used

Problem (1) has been reported previously, and I forgot to patch it up
afterward.

Problem (2) was noticed recently here:
http://lists.infradead.org/pipermail/linux-mtd/2015-October/062369.html
https://lkml.org/lkml/2015/11/12/574

Specifically, this patch fixes m25p80.ko module autoloading for cases
like this:

	flash@xxx {
		compatible = "jedec,spi-nor";
		...
	};

because modalias of "spi:spi-nor" (the only module loading info provided
by the SPI core for this device) will now be listed as an alias in
m25p80.ko.

Notably, it does *not* help cases like this:

	flash@xxx {
		compatible = "vendor,shiny-new-device", "jedec,spi-nor";
		...
	};

unless we also list "shiny-new-device" in m25p_ids[]. There has been
discussion on future work for this issue here:
https://lkml.org/lkml/2015/11/12/574

Cc: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Tested module autoload with each of the following:

	compatible = "m25p80";
	compatible = "jedec,spi-nor";

 drivers/mtd/devices/m25p80.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index f002a8f75374..151b453d0867 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -179,7 +179,7 @@ static int m25p_probe(struct spi_device *spi)
 	struct m25p *flash;
 	struct spi_nor *nor;
 	enum read_mode mode = SPI_NOR_NORMAL;
-	char *flash_name = NULL;
+	char *flash_name;
 	int ret;
 
 	data = dev_get_platdata(&spi->dev);
@@ -219,6 +219,8 @@ static int m25p_probe(struct spi_device *spi)
 	 */
 	if (data && data->type)
 		flash_name = data->type;
+	else if (!strcmp(spi->modalias, "spi-nor"))
+		flash_name = NULL; /* auto-detect */
 	else
 		flash_name = spi->modalias;
 
@@ -253,6 +255,13 @@ static int m25p_remove(struct spi_device *spi)
  */
 static const struct spi_device_id m25p_ids[] = {
 	/*
+	 * Allow non-DT platform devices to bind to the "spi-nor" modalias, and
+	 * hack around the fact that the SPI core does not provide uevent
+	 * matching for .of_match_table
+	 */
+	{"spi-nor"},
+
+	/*
 	 * Entries not used in DTs that should be safe to drop after replacing
 	 * them with "nor-jedec" in platform data.
 	 */
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH 1/3] mtd: m25p80: fix module autoloading for "jedec,spi-nor" and "spi-nor"

[Javier Martinez Canillas]

Hello Brian,

On 11/16/2015 07:34 PM, Brian Norris wrote:
> Commit 43163022927b ("mtd: m25p80: allow arbitrary OF matching for
> "jedec,spi-nor"") moved the "jedec,spi-nor" handling from the
> spi_device_id table to the of_match_table, to better handle matching
> complex device tree compatible strings. With that patch, device tree
> support works as expected when m25p80.c is built into the kernel.
>
> However, that commit ignored the fact that:
> 
>  (1) (non-DT) platform devices might want to use the "spi-nor" string
>      for matching with this driver, rather than picking an arbitrary one
>      like "m25p80"
>  (2) the core SPI uevent/modalias code doesn't yet support kernel module
>      autoloading via of_match_table strings; so for DT-based devices, it
>      will only report (part of) the first compatible string used
>

Very nice changelog message!

Patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
 
Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[PATCH 2/3] mtd: m25p80: replace leftover "nor-jedec" with "spi-nor" in comments

[Brian Norris]

I overlooked a few comments in commit 8947e396a829 ("Documentation: dt:
mtd: replace "nor-jedec" binding with "jedec, spi-nor""). Fix these up
now.

Suggested-by: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 151b453d0867..6cf5702706a6 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -263,13 +263,13 @@ static const struct spi_device_id m25p_ids[] = {
 
 	/*
 	 * Entries not used in DTs that should be safe to drop after replacing
-	 * them with "nor-jedec" in platform data.
+	 * them with "spi-nor" in platform data.
 	 */
 	{"s25sl064a"},	{"w25x16"},	{"m25p10"},	{"m25px64"},
 
 	/*
-	 * Entries that were used in DTs without "nor-jedec" fallback and should
-	 * be kept for backward compatibility.
+	 * Entries that were used in DTs without "jedec,spi-nor" fallback and
+	 * should be kept for backward compatibility.
 	 */
 	{"at25df321a"},	{"at25df641"},	{"at26df081a"},
 	{"mr25h256"},
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH 2/3] mtd: m25p80: replace leftover "nor-jedec" with "spi-nor" in comments

[Javier Martinez Canillas]

Hello Brian,

On 11/16/2015 07:34 PM, Brian Norris wrote:
> I overlooked a few comments in commit 8947e396a829 ("Documentation: dt:
> mtd: replace "nor-jedec" binding with "jedec, spi-nor""). Fix these up
> now.
> 
> Suggested-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---

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

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

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

[Brian Norris]

Pull the supported chip names from drivers/mtd/devices/m25p80.c and stop
pointing readers to Linux code.

Also (although I see this habit repeated throughout the
Documentation/devicetree/bindings/ tree), stop using the title "driver"
in this file, when we're trying explicitly to describe hardware, not
software.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: <devicetree@vger.kernel.org>
---
 .../devicetree/bindings/mtd/jedec,spi-nor.txt      | 56 ++++++++++++++++++++--
 1 file changed, 51 insertions(+), 5 deletions(-)

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.
                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
+                 at26df081a
+                 mr25h256
+                 mx25l4005a
+                 mx25l1606e
+                 mx25l6405d
+                 mx25l12805d
+                 mx25l25635e
+                 n25q064
+                 n25q128a11
+                 n25q128a13
+                 n25q512a
+                 s25fl256s1
+                 s25fl512s
+                 s25sl12801
+                 s25fl008k
+                 s25fl064k
+                 sst25vf040b
+                 m25p40
+                 m25p80
+                 m25p16
+                 m25p32
+                 m25p64
+                 m25p128
+                 w25x80
+                 w25x32
+                 w25q32
+                 w25q32dw
+                 w25q80bl
+                 w25q128
+                 w25q256
+
+               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
+
 - reg : Chip-Select number
 - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at
 
-- 
2.6.0.rc2.230.g3dd15c0

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

[Rob Herring]

On Mon, Nov 16, 2015 at 02:34:52PM -0800, Brian Norris wrote:
> Pull the supported chip names from drivers/mtd/devices/m25p80.c and stop
> pointing readers to Linux code.
> 
> Also (although I see this habit repeated throughout the
> Documentation/devicetree/bindings/ tree), stop using the title "driver"
> in this file, when we're trying explicitly to describe hardware, not
> software.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: <devicetree@vger.kernel.org>

Thanks for the clean-up.

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

> ---
>  .../devicetree/bindings/mtd/jedec,spi-nor.txt      | 56 ++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 5 deletions(-)
> 
> 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.
>                 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
> +                 at26df081a
> +                 mr25h256
> +                 mx25l4005a
> +                 mx25l1606e
> +                 mx25l6405d
> +                 mx25l12805d
> +                 mx25l25635e
> +                 n25q064
> +                 n25q128a11
> +                 n25q128a13
> +                 n25q512a
> +                 s25fl256s1
> +                 s25fl512s
> +                 s25sl12801
> +                 s25fl008k
> +                 s25fl064k
> +                 sst25vf040b
> +                 m25p40
> +                 m25p80
> +                 m25p16
> +                 m25p32
> +                 m25p64
> +                 m25p128
> +                 w25x80
> +                 w25x32
> +                 w25q32
> +                 w25q32dw
> +                 w25q80bl
> +                 w25q128
> +                 w25q256
> +
> +               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
> +
>  - reg : Chip-Select number
>  - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at
>  
> -- 
> 2.6.0.rc2.230.g3dd15c0
> 
> --
> 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

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

[Javier Martinez Canillas]

Hello Brian,

On 11/16/2015 07:34 PM, Brian Norris wrote:
> Pull the supported chip names from drivers/mtd/devices/m25p80.c and stop
> pointing readers to Linux code.
> 
> Also (although I see this habit repeated throughout the
> Documentation/devicetree/bindings/ tree), stop using the title "driver"

Agreed.

> in this file, when we're trying explicitly to describe hardware, not
> software.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: <devicetree@vger.kernel.org>
> ---
>  .../devicetree/bindings/mtd/jedec,spi-nor.txt      | 56 ++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 5 deletions(-)
> 
> 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
> +                 at26df081a
> +                 mr25h256
> +                 mx25l4005a
> +                 mx25l1606e
> +                 mx25l6405d
> +                 mx25l12805d
> +                 mx25l25635e
> +                 n25q064
> +                 n25q128a11
> +                 n25q128a13
> +                 n25q512a
> +                 s25fl256s1
> +                 s25fl512s
> +                 s25sl12801
> +                 s25fl008k
> +                 s25fl064k
> +                 sst25vf040b
> +                 m25p40
> +                 m25p80
> +                 m25p16
> +                 m25p32
> +                 m25p64
> +                 m25p128
> +                 w25x80
> +                 w25x32
> +                 w25q32
> +                 w25q32dw
> +                 w25q80bl
> +                 w25q128
> +                 w25q256
> +

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

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

>  - reg : Chip-Select number
>  - spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at
>  
> 

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

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

[Brian Norris]

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.

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?

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

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

Regards,
Brian

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

[Javier Martinez Canillas]

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

Re: [PATCH 0/3] mtd: m25p80: fix some module and documentation issues

[Brian Norris]

On Mon, Nov 16, 2015 at 02:34:49PM -0800, Brian Norris wrote:
> Hi,
> 
> There were a few mistakes and improvements pointed out at various points in
> this thread, subject:
> 
>     spi: OF module autoloading is still broken
>     (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor""
>     breaks module autoloading)
> 
>     http://lists.infradead.org/pipermail/linux-mtd/2015-October/062369.html
>     https://lkml.org/lkml/2015/11/12/574
> 
> That thread covers some other interesting issues that are still not solved, but
> let's fix the ones we can here in MTD right now.
> 
> Regards,
> Brian

Pushed all to l2-mtd.git