Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
On Mon, 3 Aug 2009 19:54:50 -0700
David Brownell davi...@pacbell.net wrote:
On Thursday 30 July 2009, Anton Vorontsov wrote:
This patch converts the m25p80 driver so that now it uses .id_table
for device matching, making it properly detect devices on OpenFirmware
platforms (prior to this patch the driver misdetected non-JEDEC chips,
seeing all chips as m25p80).
I suspect detect is a misnomer there. It only detects JEDEC chips.
All others got explicit declarations ... so if there's misbehavior for
other chips, it's because those declarations were poorly handled. Maybe
they were not properly flagged as non-JDEC
Also, now jedec_probe() only does jedec probing, nothing else. If it
is not able to detect a chip, NULL is returned and the driver fall
backs to the information specified by the platform (platform_data, or
exact ID).
I'd rather keep the warning, so there's a clue about what's really
going on: JEDEC chip found, but its ID is not handled.
afaik there was no response to David's review comments, so this patch
is in the stuck state.
Signed-off-by: Anton Vorontsov avoront...@ru.mvista.com
---
drivers/mtd/devices/m25p80.c | 146
+++---
1 files changed, 80 insertions(+), 66 deletions(-)
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 10ed195..0d74b38 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
... deletia ...
@@ -481,74 +480,83 @@ struct flash_info {
#defineSECT_4K 0x01/* OPCODE_BE_4K works uniformly
*/
};
+#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
+ ((kernel_ulong_t)(struct flash_info) { \
+ .jedec_id = (_jedec_id),\
+ .ext_id = (_ext_id),\
+ .sector_size = (_sector_size), \
+ .n_sectors = (_n_sectors), \
+ .flags = (_flags), \
+ })
Anonymous inlined structures ... kind of ugly, but I can
understand why you might not want to declare and name a
few dozen single-use structures.
/* NOTE: double check command sets and memory organization when you add
* more flash chips. This current list focusses on newer chips, which
* have been converging on command sets which including JEDEC ID.
*/
-static struct flash_info __devinitdata m25p_data [] = {
-
+static const struct spi_device_id m25p_ids[] = {
/* Atmel -- some are (confusingly) marketed as DataFlash */
- { at25fs010, 0x1f6601, 0, 32 * 1024, 4, SECT_4K, },
- { at25fs040, 0x1f6604, 0, 64 * 1024, 8, SECT_4K, },
+ { at25fs010, INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K) },
+ { at25fs040, INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K) },
... deletia ...
@@ -596,6 +602,7 @@ static struct flash_info *__devinit jedec_probe(struct
spi_device *spi)
*/
static int __devinit m25p_probe(struct spi_device *spi)
{
+ const struct spi_device_id *id;
struct flash_platform_data *data;
struct m25p *flash;
struct flash_info *info;
@@ -608,32 +615,38 @@ static int __devinit m25p_probe(struct spi_device
*spi)
*/
data = spi-dev.platform_data;
if (data data-type) {
At this point I wonder why you're not changing the probe sequence
more. Get id and then id here. If it's for m25p80 assume
it's an old-style board init and do the current logic. Else just
verify info.
There's a new error case of course: new-style but data-type
doesn't match id-name.
- for (i = 0, info = m25p_data;
- i ARRAY_SIZE(m25p_data);
- i++, info++) {
- if (strcmp(data-type, info-name) == 0)
- break;
+ for (i = 0; i ARRAY_SIZE(m25p_ids) - 1; i++) {
+ id = m25p_ids[i];
+ info = (void *)m25p_ids[i].driver_data;
+ if (strcmp(data-type, id-name))
+ continue;
+ break;
}
/* unrecognized chip? */
- if (i == ARRAY_SIZE(m25p_data)) {
+ if (i == ARRAY_SIZE(m25p_ids) - 1) {
Better: if (info == NULL) ... You've got all the pointers
in hand; don't use indices.
DEBUG(MTD_DEBUG_LEVEL0, %s: unrecognized id %s\n,
dev_name(spi-dev), data-type);
info = NULL;
/* recognized; is that chip really what's there? */
} else if (info-jedec_id) {
- struct flash_info *chip = jedec_probe(spi);
+ id =
Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
On Tue, Sep 22, 2009 at 02:17:05PM -0700, Andrew Morton wrote: On Mon, 3 Aug 2009 19:54:50 -0700 David Brownell davi...@pacbell.net wrote: On Thursday 30 July 2009, Anton Vorontsov wrote: This patch converts the m25p80 driver so that now it uses .id_table for device matching, making it properly detect devices on OpenFirmware platforms (prior to this patch the driver misdetected non-JEDEC chips, seeing all chips as m25p80). I suspect detect is a misnomer there. It only detects JEDEC chips. All others got explicit declarations ... so if there's misbehavior for other chips, it's because those declarations were poorly handled. Maybe they were not properly flagged as non-JDEC Also, now jedec_probe() only does jedec probing, nothing else. If it is not able to detect a chip, NULL is returned and the driver fall backs to the information specified by the platform (platform_data, or exact ID). I'd rather keep the warning, so there's a clue about what's really going on: JEDEC chip found, but its ID is not handled. afaik there was no response to David's review comments, so this patch is in the stuck state. Hm? Response: http://lkml.org/lkml/2009/8/18/363 And the two patches I sent on top: http://lkml.org/lkml/2009/8/18/364 http://lkml.org/lkml/2009/8/18/366 -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
On Wed, 2009-09-23 at 03:01 +0400, Anton Vorontsov wrote: And the two patches I sent on top: http://lkml.org/lkml/2009/8/18/364 http://lkml.org/lkml/2009/8/18/366 Got versions of those which apply to the mtd-2.6.git tree (which I'm about to ask Linus to pull)? -- dwmw2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
On Tue, 22 Sep 2009 16:43:47 -0700 David Woodhouse dw...@infradead.org wrote: On Wed, 2009-09-23 at 03:01 +0400, Anton Vorontsov wrote: And the two patches I sent on top: http://lkml.org/lkml/2009/8/18/364 http://lkml.org/lkml/2009/8/18/366 Got versions of those which apply to the mtd-2.6.git tree (which I'm about to ask Linus to pull)? I'll send them in a sec. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
On Tue, Sep 22, 2009 at 04:43:47PM -0700, David Woodhouse wrote: On Wed, 2009-09-23 at 03:01 +0400, Anton Vorontsov wrote: And the two patches I sent on top: http://lkml.org/lkml/2009/8/18/364 http://lkml.org/lkml/2009/8/18/366 Got versions of those which apply to the mtd-2.6.git tree (which I'm about to ask Linus to pull)? I'd love to, but they depend on a bunch of SPI patches that are still in -mm tree. As soon as SPI core changes hit Linus' tree, I think Andrew will send all m25p80 patches to you anyway. Thanks, -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
On Wed, 23 Sep 2009 03:55:34 +0400 Anton Vorontsov cbouatmai...@gmail.com wrote: On Tue, Sep 22, 2009 at 04:43:47PM -0700, David Woodhouse wrote: On Wed, 2009-09-23 at 03:01 +0400, Anton Vorontsov wrote: And the two patches I sent on top: http://lkml.org/lkml/2009/8/18/364 http://lkml.org/lkml/2009/8/18/366 Got versions of those which apply to the mtd-2.6.git tree (which I'm about to ask Linus to pull)? I'd love to, but they depend on a bunch of SPI patches that are still in -mm tree. oh, is that why I queued them up where I did. Sigh. As soon as SPI core changes hit Linus' tree, I think Andrew will send all m25p80 patches to you anyway. Or David can ack them and I'll send 'em up. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
Hi David,
Thanks for the review, and sorry for the delayed response.
On Mon, Aug 03, 2009 at 07:54:50PM -0700, David Brownell wrote:
On Thursday 30 July 2009, Anton Vorontsov wrote:
This patch converts the m25p80 driver so that now it uses .id_table
for device matching, making it properly detect devices on OpenFirmware
platforms (prior to this patch the driver misdetected non-JEDEC chips,
seeing all chips as m25p80).
I suspect detect is a misnomer there. It only detects JEDEC chips.
Currently the driver always tries JEDEC probing, and it wrongly assumed
that all non-JEDEC chips are m25p80, because an entry for m25p80 chips
had 0 in jedec id field, which isn't correct ID, but 0 is returned by
most non-JEDEC chips. Having 0 in the m25p80 entry was a hack.
All others got explicit declarations ... so if there's misbehavior for
other chips, it's because those declarations were poorly handled. Maybe
they were not properly flagged as non-JDEC
Also, now jedec_probe() only does jedec probing, nothing else. If it
is not able to detect a chip, NULL is returned and the driver fall
backs to the information specified by the platform (platform_data, or
exact ID).
I'd rather keep the warning, so there's a clue about what's really
going on: JEDEC chip found, but its ID is not handled.
We can't tell if the chip was actually found.
M25Px0 chips can be JEDEC and non-JEDEC, e.g. Nymonyx manufacturing
M25P80 chips in two variants: The RDID instruction is available only
for parts made with Technology T9HX (0.11μm), ...
Most (but not all) non-JEDEC EEPROMs will return 0 for RDID opcode
though (in that case warning is misleading). And for the chips that
don't return 0, we shouldn't probe them with jedec at all.
[...]
@@ -608,32 +615,38 @@ static int __devinit m25p_probe(struct spi_device
*spi)
*/
data = spi-dev.platform_data;
if (data data-type) {
At this point I wonder why you're not changing the probe sequence
more.
Yep, I was going to do it anyway, but for another reason: to support
CAT25 chips.
There's a new error case of course: new-style but data-type
doesn't match id-name.
[...]
+ if (i == ARRAY_SIZE(m25p_ids) - 1) {
Better: if (info == NULL) ... You've got all the pointers
in hand; don't use indices.
[...]
+ if (id != m25p_ids[i]) {
Again, don't use indices except during the lookup.
Yep, good ideas. Though, the code will vanish anyway.
Patches on the way...
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
Hello Anton, Is m25p_probe now valid with dev.platform_data == NULL, for of platforms? Then shouldn't you have the following change as well, near the end of the function? - } else if (data-nr_parts) + } else if (data data-nr_parts) dev_warn(spi-dev, ignoring %d default partitions on %s\n, data-nr_parts, data-name); Or am I misunderstanding something? -- Michael Barkowski RuggedCom ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
On Wed, Aug 12, 2009 at 04:45:55PM -0400, Michael Barkowski wrote: Hello Anton, Is m25p_probe now valid with dev.platform_data == NULL, for of platforms? Then shouldn't you have the following change as well, near the end of the function? - } else if (data-nr_parts) + } else if (data data-nr_parts) dev_warn(spi-dev, ignoring %d default partitions on %s\n, data-nr_parts, data-name); Or am I misunderstanding something? Yeah, you missed this patch: http://patchwork.kernel.org/patch/38179/ Thanks, -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
Anton Vorontsov wrote: On Wed, Aug 12, 2009 at 04:45:55PM -0400, Michael Barkowski wrote: Hello Anton, Is m25p_probe now valid with dev.platform_data == NULL, for of platforms? Then shouldn't you have the following change as well, near the end of the function? -} else if (data-nr_parts) +} else if (data data-nr_parts) dev_warn(spi-dev, ignoring %d default partitions on %s\n, data-nr_parts, data-name); Or am I misunderstanding something? Yeah, you missed this patch: http://patchwork.kernel.org/patch/38179/ Thanks, Beautiful - thanks - sorry to interrupt -- Michael Barkowski 905-482-4577 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
On Thursday 30 July 2009, Anton Vorontsov wrote:
This patch converts the m25p80 driver so that now it uses .id_table
for device matching, making it properly detect devices on OpenFirmware
platforms (prior to this patch the driver misdetected non-JEDEC chips,
seeing all chips as m25p80).
I suspect detect is a misnomer there. It only detects JEDEC chips.
All others got explicit declarations ... so if there's misbehavior for
other chips, it's because those declarations were poorly handled. Maybe
they were not properly flagged as non-JDEC
Also, now jedec_probe() only does jedec probing, nothing else. If it
is not able to detect a chip, NULL is returned and the driver fall
backs to the information specified by the platform (platform_data, or
exact ID).
I'd rather keep the warning, so there's a clue about what's really
going on: JEDEC chip found, but its ID is not handled.
Signed-off-by: Anton Vorontsov avoront...@ru.mvista.com
---
drivers/mtd/devices/m25p80.c | 146
+++---
1 files changed, 80 insertions(+), 66 deletions(-)
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 10ed195..0d74b38 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
... deletia ...
@@ -481,74 +480,83 @@ struct flash_info {
#define SECT_4K 0x01/* OPCODE_BE_4K works uniformly
*/
};
+#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
+ ((kernel_ulong_t)(struct flash_info) { \
+ .jedec_id = (_jedec_id),\
+ .ext_id = (_ext_id),\
+ .sector_size = (_sector_size), \
+ .n_sectors = (_n_sectors), \
+ .flags = (_flags), \
+ })
Anonymous inlined structures ... kind of ugly, but I can
understand why you might not want to declare and name a
few dozen single-use structures.
/* NOTE: double check command sets and memory organization when you add
* more flash chips. This current list focusses on newer chips, which
* have been converging on command sets which including JEDEC ID.
*/
-static struct flash_info __devinitdata m25p_data [] = {
-
+static const struct spi_device_id m25p_ids[] = {
/* Atmel -- some are (confusingly) marketed as DataFlash */
- { at25fs010, 0x1f6601, 0, 32 * 1024, 4, SECT_4K, },
- { at25fs040, 0x1f6604, 0, 64 * 1024, 8, SECT_4K, },
+ { at25fs010, INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K) },
+ { at25fs040, INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K) },
... deletia ...
@@ -596,6 +602,7 @@ static struct flash_info *__devinit jedec_probe(struct
spi_device *spi)
*/
static int __devinit m25p_probe(struct spi_device *spi)
{
+ const struct spi_device_id *id;
struct flash_platform_data *data;
struct m25p *flash;
struct flash_info *info;
@@ -608,32 +615,38 @@ static int __devinit m25p_probe(struct spi_device *spi)
*/
data = spi-dev.platform_data;
if (data data-type) {
At this point I wonder why you're not changing the probe sequence
more. Get id and then id here. If it's for m25p80 assume
it's an old-style board init and do the current logic. Else just
verify info.
There's a new error case of course: new-style but data-type
doesn't match id-name.
- for (i = 0, info = m25p_data;
- i ARRAY_SIZE(m25p_data);
- i++, info++) {
- if (strcmp(data-type, info-name) == 0)
- break;
+ for (i = 0; i ARRAY_SIZE(m25p_ids) - 1; i++) {
+ id = m25p_ids[i];
+ info = (void *)m25p_ids[i].driver_data;
+ if (strcmp(data-type, id-name))
+ continue;
+ break;
}
/* unrecognized chip? */
- if (i == ARRAY_SIZE(m25p_data)) {
+ if (i == ARRAY_SIZE(m25p_ids) - 1) {
Better: if (info == NULL) ... You've got all the pointers
in hand; don't use indices.
DEBUG(MTD_DEBUG_LEVEL0, %s: unrecognized id %s\n,
dev_name(spi-dev), data-type);
info = NULL;
/* recognized; is that chip really what's there? */
} else if (info-jedec_id) {
- struct flash_info *chip = jedec_probe(spi);
+ id = jedec_probe(spi);
- if (!chip || chip != info) {
+ if (id != m25p_ids[i]) {
Again, don't use indices except during the lookup.
[PATCH 2/6] mtd: m25p80: Convert to device table matching
This patch converts the m25p80 driver so that now it uses .id_table
for device matching, making it properly detect devices on OpenFirmware
platforms (prior to this patch the driver misdetected non-JEDEC chips,
seeing all chips as m25p80).
Also, now jedec_probe() only does jedec probing, nothing else. If it
is not able to detect a chip, NULL is returned and the driver fall
backs to the information specified by the platform (platform_data, or
exact ID).
Signed-off-by: Anton Vorontsov avoront...@ru.mvista.com
---
drivers/mtd/devices/m25p80.c | 146 +++---
1 files changed, 80 insertions(+), 66 deletions(-)
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 10ed195..0d74b38 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -21,6 +21,7 @@
#include linux/interrupt.h
#include linux/mutex.h
#include linux/math64.h
+#include linux/mod_devicetable.h
#include linux/mtd/mtd.h
#include linux/mtd/partitions.h
@@ -462,8 +463,6 @@ static int m25p80_write(struct mtd_info *mtd, loff_t to,
size_t len,
*/
struct flash_info {
- char*name;
-
/* JEDEC id zero means no ID (most older chips); otherwise it has
* a high byte of zero plus three data bytes: the manufacturer id,
* then a two byte device id.
@@ -481,74 +480,83 @@ struct flash_info {
#defineSECT_4K 0x01/* OPCODE_BE_4K works uniformly
*/
};
+#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
+ ((kernel_ulong_t)(struct flash_info) { \
+ .jedec_id = (_jedec_id),\
+ .ext_id = (_ext_id),\
+ .sector_size = (_sector_size), \
+ .n_sectors = (_n_sectors), \
+ .flags = (_flags), \
+ })
/* NOTE: double check command sets and memory organization when you add
* more flash chips. This current list focusses on newer chips, which
* have been converging on command sets which including JEDEC ID.
*/
-static struct flash_info __devinitdata m25p_data [] = {
-
+static const struct spi_device_id m25p_ids[] = {
/* Atmel -- some are (confusingly) marketed as DataFlash */
- { at25fs010, 0x1f6601, 0, 32 * 1024, 4, SECT_4K, },
- { at25fs040, 0x1f6604, 0, 64 * 1024, 8, SECT_4K, },
+ { at25fs010, INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K) },
+ { at25fs040, INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K) },
- { at25df041a, 0x1f4401, 0, 64 * 1024, 8, SECT_4K, },
- { at25df641, 0x1f4800, 0, 64 * 1024, 128, SECT_4K, },
+ { at25df041a, INFO(0x1f4401, 0, 64 * 1024, 8, SECT_4K) },
+ { at25df641, INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
- { at26f004, 0x1f0400, 0, 64 * 1024, 8, SECT_4K, },
- { at26df081a, 0x1f4501, 0, 64 * 1024, 16, SECT_4K, },
- { at26df161a, 0x1f4601, 0, 64 * 1024, 32, SECT_4K, },
- { at26df321, 0x1f4701, 0, 64 * 1024, 64, SECT_4K, },
+ { at26f004, INFO(0x1f0400, 0, 64 * 1024, 8, SECT_4K) },
+ { at26df081a, INFO(0x1f4501, 0, 64 * 1024, 16, SECT_4K) },
+ { at26df161a, INFO(0x1f4601, 0, 64 * 1024, 32, SECT_4K) },
+ { at26df321, INFO(0x1f4701, 0, 64 * 1024, 64, SECT_4K) },
/* Macronix */
- { mx25l12805d, 0xc22018, 0, 64 * 1024, 256, },
+ { mx25l12805d, INFO(0xc22018, 0, 64 * 1024, 256, 0) },
/* Spansion -- single (large) sector size only, at least
* for the chips listed here (without boot sectors).
*/
- { s25sl004a, 0x010212, 0, 64 * 1024, 8, },
- { s25sl008a, 0x010213, 0, 64 * 1024, 16, },
- { s25sl016a, 0x010214, 0, 64 * 1024, 32, },
- { s25sl032a, 0x010215, 0, 64 * 1024, 64, },
- { s25sl064a, 0x010216, 0, 64 * 1024, 128, },
-{ s25sl12800, 0x012018, 0x0300, 256 * 1024, 64, },
- { s25sl12801, 0x012018, 0x0301, 64 * 1024, 256, },
+ { s25sl004a, INFO(0x010212, 0, 64 * 1024, 8, 0) },
+ { s25sl008a, INFO(0x010213, 0, 64 * 1024, 16, 0) },
+ { s25sl016a, INFO(0x010214, 0, 64 * 1024, 32, 0) },
+ { s25sl032a, INFO(0x010215, 0, 64 * 1024, 64, 0) },
+ { s25sl064a, INFO(0x010216, 0, 64 * 1024, 128, 0) },
+ { s25sl12800, INFO(0x012018, 0x0300, 256 * 1024, 64, 0) },
+ { s25sl12801, INFO(0x012018, 0x0301, 64 * 1024, 256, 0) },
/* SST -- large erase sizes are overlays, sectors are 4K */
- { sst25vf040b, 0xbf258d, 0, 64 * 1024, 8, SECT_4K, },
- { sst25vf080b, 0xbf258e, 0, 64 * 1024, 16, SECT_4K, },
- { sst25vf016b, 0xbf2541, 0, 64 * 1024, 32, SECT_4K, },
- { sst25vf032b, 0xbf254a, 0, 64 * 1024, 64, SECT_4K, },
+ { sst25vf040b, INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K) },
+ { sst25vf080b, INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K) },
+
