| Message ID | 20170113093509.25737-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
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?
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
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
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) }, >
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.
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) }, >> > >
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
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
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
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) },
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(-)