Message ID | AANLkTiksYofs6hhWta6uFX2cla_g8pBu0Mtbtcbv-2tl@mail.gmail.com |
---|---|
State | New, archived |
Headers | show |
On Mon, Jun 21, 2010 at 11:27:31AM +0800, Barry Song wrote: [...] > > How about we add a non_jedec flag in platform_data, if the flag is 1, we > > let the detection pass even though the ID is 0? Otherwise, we need a > > valid ID? > Here i mean: This will break at least OF-enabled platforms (e.g. PowerPC), they assume that the driver will success for non-JEDEC flashes. OF platforms don't pass platform data, and even if they did, device tree doesn't specify if the flash is JEDEC or non-JEDEC. Which is why I think that, by default, the driver should successfully register the flash even if JEDEC probe fails. So, instead of checking for "!non_jedec", I would recommend "force_jedec" check. > Index: drivers/mtd/devices/m25p80.c > =================================================================== > --- drivers/mtd/devices/m25p80.c (revision 8927) > +++ drivers/mtd/devices/m25p80.c (revision 8929) > @@ -795,8 +795,13 @@ > > jid = jedec_probe(spi); > if (!jid) { > - dev_info(&spi->dev, "non-JEDEC variant of %s\n", > - id->name); > + if (!data->non_jedec) { > + dev_err(&spi->dev, "fail to detect%s\n", > + id->name); > + return -ENODEV; > + } else > + dev_info(&spi->dev, "non-JEDEC variant of %s\n", > + id->name); > } else if (jid != id) {
On Mon, Jun 21, 2010 at 3:15 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote: > On Mon, Jun 21, 2010 at 11:27:31AM +0800, Barry Song wrote: > [...] >> > How about we add a non_jedec flag in platform_data, if the flag is 1, we >> > let the detection pass even though the ID is 0? Otherwise, we need a >> > valid ID? >> Here i mean: > > This will break at least OF-enabled platforms (e.g. PowerPC), > they assume that the driver will success for non-JEDEC flashes. > OF platforms don't pass platform data, and even if they did, > device tree doesn't specify if the flash is JEDEC or non-JEDEC. > > Which is why I think that, by default, the driver should > successfully register the flash even if JEDEC probe fails. So, > instead of checking for "!non_jedec", I would recommend > "force_jedec" check. Mike Frysinger suggested to use non_jedec since most devices are standard jedec devices. Only if non_jedec=1, we let the detection pass if ID is 0. > >> Index: drivers/mtd/devices/m25p80.c >> =================================================================== >> --- drivers/mtd/devices/m25p80.c (revision 8927) >> +++ drivers/mtd/devices/m25p80.c (revision 8929) >> @@ -795,8 +795,13 @@ >> >> jid = jedec_probe(spi); >> if (!jid) { >> - dev_info(&spi->dev, "non-JEDEC variant of %s\n", >> - id->name); >> + if (!data->non_jedec) { >> + dev_err(&spi->dev, "fail to detect%s\n", >> + id->name); >> + return -ENODEV; >> + } else >> + dev_info(&spi->dev, "non-JEDEC variant of %s\n", >> + id->name); >> } else if (jid != id) { > > -- > Anton Vorontsov > email: cbouatmailru@gmail.com > irc://irc.freenode.net/bd2 >
On Mon, Jun 21, 2010 at 03:22:48PM +0800, Barry Song wrote: > On Mon, Jun 21, 2010 at 3:15 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote: > > On Mon, Jun 21, 2010 at 11:27:31AM +0800, Barry Song wrote: > > [...] > >> > How about we add a non_jedec flag in platform_data, if the flag is 1, we > >> > let the detection pass even though the ID is 0? Otherwise, we need a > >> > valid ID? > >> Here i mean: > > > > This will break at least OF-enabled platforms (e.g. PowerPC), > > they assume that the driver will success for non-JEDEC flashes. > > OF platforms don't pass platform data, and even if they did, > > device tree doesn't specify if the flash is JEDEC or non-JEDEC. > > > > Which is why I think that, by default, the driver should > > successfully register the flash even if JEDEC probe fails. So, > > instead of checking for "!non_jedec", I would recommend > > "force_jedec" check. > > Mike Frysinger suggested to use non_jedec since most devices are > standard jedec devices. Well, on OF platforms most devices that I'm aware of are non-JEDEC. > Only if non_jedec=1, we let the detection pass > if ID is 0. Then please #ifdef it with CONFIG_OF. Thanks, > >> Index: drivers/mtd/devices/m25p80.c > >> =================================================================== > >> --- drivers/mtd/devices/m25p80.c (revision 8927) > >> +++ drivers/mtd/devices/m25p80.c (revision 8929) > >> @@ -795,8 +795,13 @@ > >> > >> jid = jedec_probe(spi); > >> if (!jid) { > >> - dev_info(&spi->dev, "non-JEDEC variant of %s\n", > >> - id->name); > >> + if (!data->non_jedec) { > >> + dev_err(&spi->dev, "fail to detect%s\n", > >> + id->name); > >> + return -ENODEV; > >> + } else > >> + dev_info(&spi->dev, "non-JEDEC variant of %s\n", > >> + id->name); > >> } else if (jid != id) { > > > > -- > > Anton Vorontsov > > email: cbouatmailru@gmail.com > > irc://irc.freenode.net/bd2 > >
On Mon, Jun 21, 2010 at 3:39 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote: > On Mon, Jun 21, 2010 at 03:22:48PM +0800, Barry Song wrote: >> On Mon, Jun 21, 2010 at 3:15 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote: >> > On Mon, Jun 21, 2010 at 11:27:31AM +0800, Barry Song wrote: >> > [...] >> >> > How about we add a non_jedec flag in platform_data, if the flag is 1, we >> >> > let the detection pass even though the ID is 0? Otherwise, we need a >> >> > valid ID? >> >> Here i mean: >> > >> > This will break at least OF-enabled platforms (e.g. PowerPC), >> > they assume that the driver will success for non-JEDEC flashes. >> > OF platforms don't pass platform data, and even if they did, >> > device tree doesn't specify if the flash is JEDEC or non-JEDEC. >> > >> > Which is why I think that, by default, the driver should >> > successfully register the flash even if JEDEC probe fails. So, >> > instead of checking for "!non_jedec", I would recommend >> > "force_jedec" check. >> >> Mike Frysinger suggested to use non_jedec since most devices are >> standard jedec devices. > > Well, on OF platforms most devices that I'm aware of are non-JEDEC. > >> Only if non_jedec=1, we let the detection pass >> if ID is 0. > > Then please #ifdef it with CONFIG_OF. I think the patch has nothing to do with platform. Here SPI Flash is a peripherals, doesn't depend on any platform. Adding a CONFIG_OF doesn't make sense very much. If you think most devices are non-JEDEC, we can change non_JEDEC to force_JEDEC as you said. But anyway, is that real that most devices are non_JEDEC? If not, I think we should change OF platform codes to fit with this patch. > > Thanks, > >> >> Index: drivers/mtd/devices/m25p80.c >> >> =================================================================== >> >> --- drivers/mtd/devices/m25p80.c (revision 8927) >> >> +++ drivers/mtd/devices/m25p80.c (revision 8929) >> >> @@ -795,8 +795,13 @@ >> >> >> >> jid = jedec_probe(spi); >> >> if (!jid) { >> >> - dev_info(&spi->dev, "non-JEDEC variant of %s\n", >> >> - id->name); >> >> + if (!data->non_jedec) { >> >> + dev_err(&spi->dev, "fail to detect%s\n", >> >> + id->name); >> >> + return -ENODEV; >> >> + } else >> >> + dev_info(&spi->dev, "non-JEDEC variant of %s\n", >> >> + id->name); >> >> } else if (jid != id) { >> > >> > -- >> > Anton Vorontsov >> > email: cbouatmailru@gmail.com >> > irc://irc.freenode.net/bd2 >> > > > -- > Anton Vorontsov > email: cbouatmailru@gmail.com > irc://irc.freenode.net/bd2 >
Index: drivers/mtd/devices/m25p80.c =================================================================== --- drivers/mtd/devices/m25p80.c (revision 8927) +++ drivers/mtd/devices/m25p80.c (revision 8929) @@ -795,8 +795,13 @@ jid = jedec_probe(spi); if (!jid) { - dev_info(&spi->dev, "non-JEDEC variant of %s\n", - id->name); + if (!data->non_jedec) { + dev_err(&spi->dev, "fail to detect%s\n", + id->name); + return -ENODEV; + } else + dev_info(&spi->dev, "non-JEDEC variant of %s\n", + id->name); } else if (jid != id) { /* * JEDEC knows better, so overwrite platform ID. We Index: include/linux/spi/flash.h =================================================================== --- include/linux/spi/flash.h (revision 8927) +++ include/linux/spi/flash.h (revision 8929) @@ -25,6 +25,11 @@ char *type; + /* + * For non-JEDEC, id will be 0. In this case, we can't be sure + * whether the flash exists with runtime probing. + */ + int non_jedec; /* we'll likely add more ... use JEDEC IDs, etc */ };