[PATCH] mtd: spi-nor: don't claim mr25h40 to be JEDEC compatible

[PATCH] mtd: spi-nor: don't claim mr25h40 to be JEDEC compatible

[Uwe Kleine-König]

Commit edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40") made it
possible to use a mr25h40 by writing

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

in a device tree. This chip however isn't JEDEC compatible however, so
change the chip string and add a compatible entry to bless

	compatible = "mr25h40-nonjedec";

as the right way.

Fixes: edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/devices/m25p80.c  | 1 +
 drivers/mtd/spi-nor/spi-nor.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 9cf7fcd28034..bd0c335692d2 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -304,6 +304,7 @@ static const struct spi_device_id m25p_ids[] = {
 	{"m25p05-nonjedec"},	{"m25p10-nonjedec"},	{"m25p20-nonjedec"},
 	{"m25p40-nonjedec"},	{"m25p80-nonjedec"},	{"m25p16-nonjedec"},
 	{"m25p32-nonjedec"},	{"m25p64-nonjedec"},	{"m25p128-nonjedec"},
+	{"mr25h40-nonjedec"},
 
 	{ },
 };
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index bbdbbd763c9d..3a8042fe44f0 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -825,7 +825,7 @@ static const struct flash_info spi_nor_ids[] = {
 	/* Everspin */
 	{ "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
 	{ "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
-	{ "mr25h40",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
+	{ "mr25h40-nonjedec",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
 
 	/* Fujitsu */
 	{ "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },
-- 
2.11.0

Re: [PATCH] mtd: spi-nor: don't claim mr25h40 to be JEDEC compatible

[Rafał Miłecki]

On 13 January 2017 at 10:35, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Commit edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40") made it
> possible to use a mr25h40 by writing
>
>         compatible = "mr25h40", "jedec,spi-nor";
>
> in a device tree. This chip however isn't JEDEC compatible however, so
> change the chip string and add a compatible entry to bless
>
>         compatible = "mr25h40-nonjedec";
>
> as the right way.
>
> Fixes: edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/mtd/devices/m25p80.c  | 1 +
>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 9cf7fcd28034..bd0c335692d2 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -304,6 +304,7 @@ static const struct spi_device_id m25p_ids[] = {
>         {"m25p05-nonjedec"},    {"m25p10-nonjedec"},    {"m25p20-nonjedec"},
>         {"m25p40-nonjedec"},    {"m25p80-nonjedec"},    {"m25p16-nonjedec"},
>         {"m25p32-nonjedec"},    {"m25p64-nonjedec"},    {"m25p128-nonjedec"},
> +       {"mr25h40-nonjedec"},
>
>         { },
>  };
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index bbdbbd763c9d..3a8042fe44f0 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -825,7 +825,7 @@ static const struct flash_info spi_nor_ids[] = {
>         /* Everspin */
>         { "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>         { "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> -       { "mr25h40",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> +       { "mr25h40-nonjedec",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>
>         /* Fujitsu */
>         { "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },

It seems every flash for which we use CAT25_INFO should be named with
-nonjedec suffix.

Names mr25h10, mr25h40, cat25c11, cat25c03 cat25c09, cat25c17 and
cat25128 are not even part of m25p80 (or any other driver). Is it
possible to use them at all? Maybe we should just rename all these
entries?

-- 
Rafał

Re: [PATCH] mtd: spi-nor: don't claim mr25h40 to be JEDEC compatible

[Uwe Kleine-König]

On Fri, Jan 13, 2017 at 10:51:47AM +0100, Rafał Miłecki wrote:
> On 13 January 2017 at 10:35, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > Commit edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40") made it
> > possible to use a mr25h40 by writing
> >
> >         compatible = "mr25h40", "jedec,spi-nor";
> >
> > in a device tree. This chip however isn't JEDEC compatible however, so
> > change the chip string and add a compatible entry to bless
> >
> >         compatible = "mr25h40-nonjedec";
> >
> > as the right way.
> >
> > Fixes: edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/mtd/devices/m25p80.c  | 1 +
> >  drivers/mtd/spi-nor/spi-nor.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 9cf7fcd28034..bd0c335692d2 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -304,6 +304,7 @@ static const struct spi_device_id m25p_ids[] = {
> >         {"m25p05-nonjedec"},    {"m25p10-nonjedec"},    {"m25p20-nonjedec"},
> >         {"m25p40-nonjedec"},    {"m25p80-nonjedec"},    {"m25p16-nonjedec"},
> >         {"m25p32-nonjedec"},    {"m25p64-nonjedec"},    {"m25p128-nonjedec"},
> > +       {"mr25h40-nonjedec"},
> >
> >         { },
> >  };
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index bbdbbd763c9d..3a8042fe44f0 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -825,7 +825,7 @@ static const struct flash_info spi_nor_ids[] = {
> >         /* Everspin */
> >         { "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> >         { "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> > -       { "mr25h40",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> > +       { "mr25h40-nonjedec",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
                                  ^------.
I missed to remove the double space here |.

> >         /* Fujitsu */
> >         { "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },
> 
> It seems every flash for which we use CAT25_INFO should be named with
> -nonjedec suffix.
> 
> Names mr25h10, mr25h40, cat25c11, cat25c03 cat25c09, cat25c17 and
> cat25128 are not even part of m25p80 (or any other driver). Is it
> possible to use them at all? Maybe we should just rename all these
> entries?

I suspect they are usable (as was mr25h40 without my patch) using 

	compatible = "$name-here", "jedec,spi-nor";

. For each chip this is either wrong (because it doesn't support JEDEC)
or the listing is not necessary because it can be determined using
JEDEC.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] mtd: spi-nor: don't claim mr25h40 to be JEDEC compatible

[Rafał Miłecki]

On 13 January 2017 at 10:58, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Fri, Jan 13, 2017 at 10:51:47AM +0100, Rafał Miłecki wrote:
>> On 13 January 2017 at 10:35, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> > Commit edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40") made it
>> > possible to use a mr25h40 by writing
>> >
>> >         compatible = "mr25h40", "jedec,spi-nor";
>> >
>> > in a device tree. This chip however isn't JEDEC compatible however, so
>> > change the chip string and add a compatible entry to bless
>> >
>> >         compatible = "mr25h40-nonjedec";
>> >
>> > as the right way.
>> >
>> > Fixes: edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40")
>> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> > ---
>> >  drivers/mtd/devices/m25p80.c  | 1 +
>> >  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>> >  2 files changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> > index 9cf7fcd28034..bd0c335692d2 100644
>> > --- a/drivers/mtd/devices/m25p80.c
>> > +++ b/drivers/mtd/devices/m25p80.c
>> > @@ -304,6 +304,7 @@ static const struct spi_device_id m25p_ids[] = {
>> >         {"m25p05-nonjedec"},    {"m25p10-nonjedec"},    {"m25p20-nonjedec"},
>> >         {"m25p40-nonjedec"},    {"m25p80-nonjedec"},    {"m25p16-nonjedec"},
>> >         {"m25p32-nonjedec"},    {"m25p64-nonjedec"},    {"m25p128-nonjedec"},
>> > +       {"mr25h40-nonjedec"},
>> >
>> >         { },
>> >  };
>> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> > index bbdbbd763c9d..3a8042fe44f0 100644
>> > --- a/drivers/mtd/spi-nor/spi-nor.c
>> > +++ b/drivers/mtd/spi-nor/spi-nor.c
>> > @@ -825,7 +825,7 @@ static const struct flash_info spi_nor_ids[] = {
>> >         /* Everspin */
>> >         { "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>> >         { "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>> > -       { "mr25h40",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>> > +       { "mr25h40-nonjedec",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>                                   ^------.
> I missed to remove the double space here |.
>
>> >         /* Fujitsu */
>> >         { "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },
>>
>> It seems every flash for which we use CAT25_INFO should be named with
>> -nonjedec suffix.
>>
>> Names mr25h10, mr25h40, cat25c11, cat25c03 cat25c09, cat25c17 and
>> cat25128 are not even part of m25p80 (or any other driver). Is it
>> possible to use them at all? Maybe we should just rename all these
>> entries?
>
> I suspect they are usable (as was mr25h40 without my patch) using
>
>         compatible = "$name-here", "jedec,spi-nor";
>
> . For each chip this is either wrong (because it doesn't support JEDEC)
> or the listing is not necessary because it can be determined using
> JEDEC.

"mr25h40" is not inside m25p80.c (or any other), so it won't trigger
driver probe function. It's the "jedec,spi-nor" (incorrectly used)
that was triggering m25p80 probe for you.

-- 
Rafał

Re: [PATCH] mtd: spi-nor: don't claim mr25h40 to be JEDEC compatible

[Uwe Kleine-König]

On Fri, Jan 13, 2017 at 10:35:09AM +0100, Uwe Kleine-König wrote:
> Commit edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40") made it
> possible to use a mr25h40 by writing
> 
> 	compatible = "mr25h40", "jedec,spi-nor";
> 
> in a device tree. This chip however isn't JEDEC compatible however, so
> change the chip string and add a compatible entry to bless
> 
> 	compatible = "mr25h40-nonjedec";
> 
> as the right way.
> 
> Fixes: edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---

I intended to write a note here, but sent out the mail before :w :-|

I wrote:

I wonder a bit about the "-nonjedec" suffix and only added it for
consistency. AFAICT all mr25h40 don't support JEDEC, so "-nonjedec"
doesn't really qualify as hardware description. Thoughts?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] mtd: spi-nor: don't claim mr25h40 to be JEDEC compatible

[Cyrille Pitchen]

Hi all,

Le 13/01/2017 à 10:35, Uwe Kleine-König a écrit :
> Commit edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40") made it
> possible to use a mr25h40 by writing
> 
> 	compatible = "mr25h40", "jedec,spi-nor";
> 
> in a device tree. This chip however isn't JEDEC compatible however, so

That's true but it should not be the only one working combination. Indeed
the m25p80.c driver doesn't care about the manufacturer prefix and only
checks the device name to match some DT compatible string. Hence
technically you could use something like:

compatible = "everspin,mr25h40", "nonjedec,spi-nor";


The "*,spi-nor" strings match the "spi-nor" entry of the m25p_ids[] array
in m25p80.c so based on the DT compatible string, the kernel knows that the
memory device can be handled by this m25p80.c driver.

Then from m25p80.c, flash_name should point to the "mr25h40" string, which
will be matched in spi_nor_scan() using the "mr25h40" entry of the
spi_nor_ids[] array.

The "nonjedec,spi-nor" DT compatible string in not documented yet but we
could document it if this solution is chosen. This would be a first solution.

Another solution is, like you did in this patch, to add the relevant
entries in the m25p_ids[] array in m25p80.c. I have no problem with that.
However I would just want some consistent naming scheme. I mean a
"mr25h256" entry already exists in the m25p_ids[] array. We must keep this
entry to be backward compatible with existing .dtb files. Then I think news
entries should simply be named "mr25h10" and "mr25h40", without the
"-nonjedec" suffix. This way, the naming scheme for Everspin memory would
be consistent in both the m25p_ids[] and spi_nor_ids[] arrays.
No change is needed in spi-nor.c

Then the 3rd solution: adding a "-nonjedec" suffix to the "mr25h10" and
"mr25h40" entries but keep the "mr25h256" name unchanged. Then add the
"mr25h10-nonjedec" and "mr25h40-nonjedec" entries in m25p_ids[].
Indeed, this is what you propose with your patch, except that you didn't
handle the case of the mr25h10 devices then ;) , but honestly this is not
the solution I prefer.

I don't want to force anything, I just want this to be discussed a little
bit more but maybe not so long because at some point we have to make a
decision otherwise the patch would be forgotten.

Marek, what do you think about that?

> change the chip string and add a compatible entry to bless
> 
> 	compatible = "mr25h40-nonjedec";
> 
> as the right way.
> 
> Fixes: edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40")

I'm not sure whether this could really be considered as a bug since it is
already possible to use this memory even with device trees.

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/mtd/devices/m25p80.c  | 1 +
>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 9cf7fcd28034..bd0c335692d2 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -304,6 +304,7 @@ static const struct spi_device_id m25p_ids[] = {
>  	{"m25p05-nonjedec"},	{"m25p10-nonjedec"},	{"m25p20-nonjedec"},
>  	{"m25p40-nonjedec"},	{"m25p80-nonjedec"},	{"m25p16-nonjedec"},
>  	{"m25p32-nonjedec"},	{"m25p64-nonjedec"},	{"m25p128-nonjedec"},
> +	{"mr25h40-nonjedec"},
>  
>  	{ },
>  };
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index bbdbbd763c9d..3a8042fe44f0 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -825,7 +825,7 @@ static const struct flash_info spi_nor_ids[] = {
>  	/* Everspin */
>  	{ "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>  	{ "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> -	{ "mr25h40",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> +	{ "mr25h40-nonjedec",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },

Removing the old entry might create regressions if people have already
started to use the "mr25h40" name in their .dtb files or in some platform
device structures. It is possible even if this entry is quite recent.

>  
>  	/* Fujitsu */
>  	{ "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },
> 

Re: [PATCH] mtd: spi-nor: don't claim mr25h40 to be JEDEC compatible

[Cyrille Pitchen]

Le 13/01/2017 à 15:30, Cyrille Pitchen a écrit :
> Hi all,
> 
> Le 13/01/2017 à 10:35, Uwe Kleine-König a écrit :
>> Commit edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40") made it
>> possible to use a mr25h40 by writing
>>
>> 	compatible = "mr25h40", "jedec,spi-nor";
>>
>> in a device tree. This chip however isn't JEDEC compatible however, so
> 
> That's true but it should not be the only one working combination. Indeed
> the m25p80.c driver doesn't care about the manufacturer prefix and only
> checks the device name to match some DT compatible string. Hence
> technically you could use something like:
> 
> compatible = "everspin,mr25h40", "nonjedec,spi-nor";
> 
> 
> The "*,spi-nor" strings match the "spi-nor" entry of the m25p_ids[] array
> in m25p80.c so based on the DT compatible string, the kernel knows that the
> memory device can be handled by this m25p80.c driver.
> 

I think I made a mistake since I forgot about the m25p_of_table[] array:
the only string is "jedec,spi-nor" so "nonjedec,spi-nor" may not work with
DT platform.

> Then from m25p80.c, flash_name should point to the "mr25h40" string, which
> will be matched in spi_nor_scan() using the "mr25h40" entry of the
> spi_nor_ids[] array.
> 
> The "nonjedec,spi-nor" DT compatible string in not documented yet but we
> could document it if this solution is chosen. This would be a first solution.
> 
> Another solution is, like you did in this patch, to add the relevant
> entries in the m25p_ids[] array in m25p80.c. I have no problem with that.
> However I would just want some consistent naming scheme. I mean a
> "mr25h256" entry already exists in the m25p_ids[] array. We must keep this
> entry to be backward compatible with existing .dtb files. Then I think news
> entries should simply be named "mr25h10" and "mr25h40", without the
> "-nonjedec" suffix. This way, the naming scheme for Everspin memory would
> be consistent in both the m25p_ids[] and spi_nor_ids[] arrays.
> No change is needed in spi-nor.c
> 
> Then the 3rd solution: adding a "-nonjedec" suffix to the "mr25h10" and
> "mr25h40" entries but keep the "mr25h256" name unchanged. Then add the
> "mr25h10-nonjedec" and "mr25h40-nonjedec" entries in m25p_ids[].
> Indeed, this is what you propose with your patch, except that you didn't
> handle the case of the mr25h10 devices then ;) , but honestly this is not
> the solution I prefer.
> 
> I don't want to force anything, I just want this to be discussed a little
> bit more but maybe not so long because at some point we have to make a
> decision otherwise the patch would be forgotten.
> 
> Marek, what do you think about that?
> 
>> change the chip string and add a compatible entry to bless
>>
>> 	compatible = "mr25h40-nonjedec";
>>
>> as the right way.
>>
>> Fixes: edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40")
> 
> I'm not sure whether this could really be considered as a bug since it is
> already possible to use this memory even with device trees.
> 
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>>  drivers/mtd/devices/m25p80.c  | 1 +
>>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 9cf7fcd28034..bd0c335692d2 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -304,6 +304,7 @@ static const struct spi_device_id m25p_ids[] = {
>>  	{"m25p05-nonjedec"},	{"m25p10-nonjedec"},	{"m25p20-nonjedec"},
>>  	{"m25p40-nonjedec"},	{"m25p80-nonjedec"},	{"m25p16-nonjedec"},
>>  	{"m25p32-nonjedec"},	{"m25p64-nonjedec"},	{"m25p128-nonjedec"},
>> +	{"mr25h40-nonjedec"},
>>  
>>  	{ },
>>  };
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index bbdbbd763c9d..3a8042fe44f0 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -825,7 +825,7 @@ static const struct flash_info spi_nor_ids[] = {
>>  	/* Everspin */
>>  	{ "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>>  	{ "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>> -	{ "mr25h40",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>> +	{ "mr25h40-nonjedec",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> 
> Removing the old entry might create regressions if people have already
> started to use the "mr25h40" name in their .dtb files or in some platform
> device structures. It is possible even if this entry is quite recent.
> 
>>  
>>  	/* Fujitsu */
>>  	{ "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },
>>
> 
> 

Re: [PATCH] mtd: spi-nor: don't claim mr25h40 to be JEDEC compatible

[Geert Uytterhoeven]

Hi Uwe,

CC devicetree

On Fri, Jan 13, 2017 at 10:35 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Commit edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40") made it
> possible to use a mr25h40 by writing
>
>         compatible = "mr25h40", "jedec,spi-nor";

No vendor prefix?

>
> in a device tree. This chip however isn't JEDEC compatible however, so
> change the chip string and add a compatible entry to bless
>
>         compatible = "mr25h40-nonjedec";
>
> as the right way.

This whole "-nonjedec" business looks wrong to me.
If the device is called "mr25h40", its compatible value should be
"everspin,mr25h40". Adding some (in)compatibility indicator violates the
spirit of compatible values, IMHO.

> Fixes: edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/mtd/devices/m25p80.c  | 1 +
>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 9cf7fcd28034..bd0c335692d2 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -304,6 +304,7 @@ static const struct spi_device_id m25p_ids[] = {
>         {"m25p05-nonjedec"},    {"m25p10-nonjedec"},    {"m25p20-nonjedec"},
>         {"m25p40-nonjedec"},    {"m25p80-nonjedec"},    {"m25p16-nonjedec"},
>         {"m25p32-nonjedec"},    {"m25p64-nonjedec"},    {"m25p128-nonjedec"},
> +       {"mr25h40-nonjedec"},
>
>         { },
>  };
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index bbdbbd763c9d..3a8042fe44f0 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -825,7 +825,7 @@ static const struct flash_info spi_nor_ids[] = {
>         /* Everspin */
>         { "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>         { "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> -       { "mr25h40",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> +       { "mr25h40-nonjedec",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>
>         /* Fujitsu */
>         { "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },
> --
> 2.11.0

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] mtd: spi-nor: don't claim mr25h40 to be JEDEC compatible

[Mark Rutland]

On Fri, Jan 13, 2017 at 07:42:34PM +0100, Geert Uytterhoeven wrote:
> Hi Uwe,
> 
> CC devicetree
> 
> On Fri, Jan 13, 2017 at 10:35 AM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > Commit edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40") made it
> > possible to use a mr25h40 by writing
> >
> >         compatible = "mr25h40", "jedec,spi-nor";
> 
> No vendor prefix?
> 
> >
> > in a device tree. This chip however isn't JEDEC compatible however, so
> > change the chip string and add a compatible entry to bless
> >
> >         compatible = "mr25h40-nonjedec";
> >
> > as the right way.
> 
> This whole "-nonjedec" business looks wrong to me.
> If the device is called "mr25h40", its compatible value should be
> "everspin,mr25h40". Adding some (in)compatibility indicator violates the
> spirit of compatible values, IMHO.

Agreed on all counts.

The compatible string should specify the vendor and device, any
compliance details should either be known for that string or derived
from other properties.

IIUC this is following an existing pattern, which we should deprecate
(retaining support for those strings so old DTBs work).

Thanks,
Mark.

> 
> > Fixes: edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/mtd/devices/m25p80.c  | 1 +
> >  drivers/mtd/spi-nor/spi-nor.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 9cf7fcd28034..bd0c335692d2 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -304,6 +304,7 @@ static const struct spi_device_id m25p_ids[] = {
> >         {"m25p05-nonjedec"},    {"m25p10-nonjedec"},    {"m25p20-nonjedec"},
> >         {"m25p40-nonjedec"},    {"m25p80-nonjedec"},    {"m25p16-nonjedec"},
> >         {"m25p32-nonjedec"},    {"m25p64-nonjedec"},    {"m25p128-nonjedec"},
> > +       {"mr25h40-nonjedec"},
> >
> >         { },
> >  };
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index bbdbbd763c9d..3a8042fe44f0 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -825,7 +825,7 @@ static const struct flash_info spi_nor_ids[] = {
> >         /* Everspin */
> >         { "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> >         { "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> > -       { "mr25h40",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> > +       { "mr25h40-nonjedec",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> >
> >         /* Fujitsu */
> >         { "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },
> > --
> > 2.11.0
> 
> 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

Re: [PATCH] mtd: spi-nor: don't claim mr25h40 to be JEDEC compatible

[Uwe Kleine-König]

Hello,

On Fri, Jan 13, 2017 at 07:42:27PM +0000, Mark Rutland wrote:
> On Fri, Jan 13, 2017 at 07:42:34PM +0100, Geert Uytterhoeven wrote:
> > CC devicetree
thanks

> > 
> > On Fri, Jan 13, 2017 at 10:35 AM, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > Commit edd0c8f4932d ("mtd: spi-nor: Add support for mr25h40") made it
> > > possible to use a mr25h40 by writing
> > >
> > >         compatible = "mr25h40", "jedec,spi-nor";
> > 
> > No vendor prefix?
> > 
> > >
> > > in a device tree. This chip however isn't JEDEC compatible however, so
> > > change the chip string and add a compatible entry to bless
> > >
> > >         compatible = "mr25h40-nonjedec";
> > >
> > > as the right way.
> > 
> > This whole "-nonjedec" business looks wrong to me.
> > If the device is called "mr25h40", its compatible value should be
> > "everspin,mr25h40". Adding some (in)compatibility indicator violates the
> > spirit of compatible values, IMHO.
> 
> Agreed on all counts.
> 
> The compatible string should specify the vendor and device, any
> compliance details should either be known for that string or derived
> from other properties.
> 
> IIUC this is following an existing pattern, which we should deprecate
> (retaining support for those strings so old DTBs work).

Looking at drivers/mtd/spi-nor/spi-nor.c there is in the spi_nor_ids
array:

	...
        { "m25p05",  INFO(0x202010,  0,  32 * 1024,   2, 0) },
	...
        { "m25p05-nonjedec",  INFO(0, 0,  32 * 1024,   2, 0) },

and similar entries for the other M25P members. So I guess these chips
couldn't do JEDEC at the beginning, then got feature updates but no new
name. So "m25p05-nonjedec" is fine as compatibility string?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |