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

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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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

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