| Message ID | 1410714670.3040.39.camel@decadent.org.uk (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
+ Rafal Rafal has been looking at the same area of code. I'd really like to get this patch into 3.18 if possible, so the more eyes the better. On Sun, Sep 14, 2014 at 06:11:10PM +0100, Ben Hutchings wrote: > m25p80's device ID table is now spi_nor_ids, defined in spi-nor. The > MODULE_DEVICE_TABLE() macro doesn't work with extern definitions, but > its use was also removed at the same time. Now if m25p80 is built as > a module it doesn't get the necessary aliases to be loaded > automatically. > > A clean solution to this will involve defining the list of device > IDs in spi-nor.h and removing struct spi_device_id from the spi-nor > API, but this is quite a large change. > > As a quick fix suitable for stable, copy the device IDs back into > m25p80. > > Fixes: 03e296f613af ("mtd: m25p80: use the SPI nor framework") > Cc: stable <stable@vger.kernel.org> # 3.16 > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > --- > drivers/mtd/devices/m25p80.c | 58 +++++++++++++++++++++++++++++++++++++++++-- > drivers/mtd/spi-nor/spi-nor.c | 5 ++-- > include/linux/mtd/spi-nor.h | 3 +-- > 3 files changed, 59 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index ed7e0a1b..3f0fe86 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -196,6 +196,7 @@ static int m25p_probe(struct spi_device *spi) > struct m25p *flash; > struct spi_nor *nor; > enum read_mode mode = SPI_NOR_NORMAL; > + const struct spi_device_id *id; > int ret; > > flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL); > @@ -223,7 +224,10 @@ static int m25p_probe(struct spi_device *spi) > mode = SPI_NOR_QUAD; > else if (spi->mode & SPI_RX_DUAL) > mode = SPI_NOR_DUAL; > - ret = spi_nor_scan(nor, spi_get_device_id(spi), mode); > + id = spi_nor_match_id(spi_get_device_id(spi)->name); > + if (WARN_ON(!id)) > + return -ENODEV; I'm not confident this logic is sound. And even if it is sound, does it belong in a patch for stable? You might take a look at Rafal's patch here: http://lists.infradead.org/pipermail/linux-mtd/2014-September/055596.html some of the matching logic is actually done via platform data, and I don't think this check for spi_nor_match_id() will catch it. (Honestly, some of this name-matching / ID-matching stuff confuses me; there is at least one too many ways to choose a flash device for this driver.) > + ret = spi_nor_scan(nor, id, mode); > if (ret) > return ret; > > @@ -245,12 +249,62 @@ static int m25p_remove(struct spi_device *spi) > } > > > +/* > + * XXX This needs to be kept in sync with spi_nor_ids. We can't share > + * it with spi-nor, because if this is built as a module then modpost > + * won't be able to read it and add appropriate aliases. > + */ > +static const struct spi_device_id m25p_ids[] = { > + {"at25fs010"}, {"at25fs040"}, {"at25df041a"}, {"at25df321a"}, > + {"at25df641"}, {"at26f004"}, {"at26df081a"}, {"at26df161a"}, > + {"at26df321"}, {"at45db081d"}, > + {"en25f32"}, {"en25p32"}, {"en25q32b"}, {"en25p64"}, > + {"en25q64"}, {"en25qh128"}, {"en25qh256"}, > + {"f25l32pa"}, > + {"mr25h256"}, {"mr25h10"}, > + {"gd25q32"}, {"gd25q64"}, > + {"160s33b"}, {"320s33b"}, {"640s33b"}, > + {"mx25l2005a"}, {"mx25l4005a"}, {"mx25l8005"}, {"mx25l1606e"}, > + {"mx25l3205d"}, {"mx25l3255e"}, {"mx25l6405d"}, {"mx25l12805d"}, > + {"mx25l12855e"},{"mx25l25635e"},{"mx25l25655e"},{"mx66l51235l"}, > + {"mx66l1g55g"}, > + {"n25q064"}, {"n25q128a11"}, {"n25q128a13"}, {"n25q256a"}, > + {"n25q512a"}, {"n25q512ax3"}, {"n25q00"}, > + {"pm25lv512"}, {"pm25lv010"}, {"pm25lq032"}, > + {"s25sl032p"}, {"s25sl064p"}, {"s25fl256s0"}, {"s25fl256s1"}, > + {"s25fl512s"}, {"s70fl01gs"}, {"s25sl12800"}, {"s25sl12801"}, > + {"s25fl129p0"}, {"s25fl129p1"}, {"s25sl004a"}, {"s25sl008a"}, > + {"s25sl016a"}, {"s25sl032a"}, {"s25sl064a"}, {"s25fl008k"}, > + {"s25fl016k"}, {"s25fl064k"}, > + {"sst25vf040b"},{"sst25vf080b"},{"sst25vf016b"},{"sst25vf032b"}, > + {"sst25vf064c"},{"sst25wf512"}, {"sst25wf010"}, {"sst25wf020"}, > + {"sst25wf040"}, > + {"m25p05"}, {"m25p10"}, {"m25p20"}, {"m25p40"}, > + {"m25p80"}, {"m25p16"}, {"m25p32"}, {"m25p64"}, > + {"m25p128"}, {"n25q032"}, > + {"m25p05-nonjedec"}, {"m25p10-nonjedec"}, {"m25p20-nonjedec"}, > + {"m25p40-nonjedec"}, {"m25p80-nonjedec"}, {"m25p16-nonjedec"}, > + {"m25p32-nonjedec"}, {"m25p64-nonjedec"}, {"m25p128-nonjedec"}, > + {"m45pe10"}, {"m45pe80"}, {"m45pe16"}, > + {"m25pe20"}, {"m25pe80"}, {"m25pe16"}, > + {"m25px16"}, {"m25px32"}, {"m25px32-s0"}, {"m25px32-s1"}, > + {"m25px64"}, > + {"w25x10"}, {"w25x20"}, {"w25x40"}, {"w25x80"}, > + {"w25x16"}, {"w25x32"}, {"w25q32"}, {"w25q32dw"}, > + {"w25x64"}, {"w25q64"}, {"w25q128"}, {"w25q80"}, > + {"w25q80bl"}, {"w25q128"}, {"w25q256"}, {"cat25c11"}, > + {"cat25c03"}, {"cat25c09"}, {"cat25c17"}, {"cat25128"}, > + { }, > +}; > +MODULE_DEVICE_TABLE(spi, m25p_ids); > + > + > static struct spi_driver m25p80_driver = { > .driver = { > .name = "m25p80", > .owner = THIS_MODULE, > }, > - .id_table = spi_nor_ids, > + .id_table = m25p_ids, > .probe = m25p_probe, > .remove = m25p_remove, > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 03e0ab8..6b1bda2 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -473,7 +473,7 @@ struct flash_info { > * more nor chips. This current list focusses on newer chips, which > * have been converging on command sets which including JEDEC ID. > */ > -const struct spi_device_id spi_nor_ids[] = { > +static const struct spi_device_id spi_nor_ids[] = { > /* Atmel -- some are (confusingly) marketed as "DataFlash" */ > { "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K) }, > { "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K) }, > @@ -637,7 +637,6 @@ const struct spi_device_id spi_nor_ids[] = { > { "cat25128", CAT25_INFO(2048, 8, 64, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) }, > { }, > }; > -EXPORT_SYMBOL_GPL(spi_nor_ids); > > static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor) > { > @@ -1136,7 +1135,7 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, > } > EXPORT_SYMBOL_GPL(spi_nor_scan); > > -const struct spi_device_id *spi_nor_match_id(char *name) > +const struct spi_device_id *spi_nor_match_id(const char *name) > { > const struct spi_device_id *id = spi_nor_ids; > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index 9e6294f..5ec84cc 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -201,7 +201,6 @@ struct spi_nor { > */ > int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, > enum read_mode mode); > -extern const struct spi_device_id spi_nor_ids[]; > > /** > * spi_nor_match_id() - find the spi_device_id by the name > @@ -213,6 +212,6 @@ extern const struct spi_device_id spi_nor_ids[]; > * Return: returns the right spi_device_id pointer on success, > * and returns NULL on failure. > */ > -const struct spi_device_id *spi_nor_match_id(char *name); > +const struct spi_device_id *spi_nor_match_id(const char *name); > > #endif > > But aside from that, I think this patch is OK. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29 September 2014 00:21, Brian Norris <computersforpeace@gmail.com> wrote: > + Rafal > > Rafal has been looking at the same area of code. I'd really like to get > this patch into 3.18 if possible, so the more eyes the better. Thanks Brian. I took me a while to follow this issue, too bad I wasn't subscribed to the ML earlier. Let me try to sum it up. 1) The main urgent issue: broken auto-loading Tracked in the thread: http://www.spinics.net/lists/linux-spi/msg01726.html Problem: m25p80.c references spi_nor_ids (from external file) Short-term solution: duplicate IDs in the m25p80.c Ben: just like Brian, I think the patch like this one ( [PATCH 1/5] m25p80,spi-nor: Fix module aliases for m25p80 ) is the way to go. However few comments: a) I don't see why you modify m25p_probe in it. b) I don't think the described clean solution (you described it in the commit message): > A clean solution to this will involve defining the list of device > IDs in spi-nor.h and removing struct spi_device_id from the spi-nor > API, but this is quite a large change. is the correct one. I think there should be a single string to trigger m25p80 load and the rest should be handled using JEDEC, with some workarounds for incompatible devices only. One thing I wonder about is sense of pushing this patch to the Linus's tree. Do you think we could prepare a real fix for Linus and leave this patch for stable@ only? I'm far from being an expert on such things, just asking. 2) On going cleanups It seems there are at least 2 ppl (me, Ben) working on some cleanups independently. a) There is (l2-mtd pushed) patch moving flash_platform_data into m25p80: https://patchwork.ozlabs.org/patch/394219/ b) Removing spi_nor::read_id https://patchwork.ozlabs.org/patch/389073/ Ben: I think this one has a NACK from me, because I'm going to use custom read_id in the bcm53xxspiflash driver. See following thread for bcm53xxspiflash description: http://comments.gmane.org/gmane.linux.drivers.mtd/54578 Initial commit (it uses read_id): https://patchwork.ozlabs.org/patch/381902/ c) Obsoleting spi_device_id It seems we both were working on the same thing, me slightly slower however ;) https://patchwork.ozlabs.org/patch/377917/ https://patchwork.ozlabs.org/patch/389074/ https://patchwork.ozlabs.org/patch/389075/ I still have to review calmly your changes, but they need to be rebased anyway. d) Cleaning m25p80 https://patchwork.ozlabs.org/patch/389076/ Ben: As said before, I think we should do smarter in the m25p80. We should get rid of all that duplicated strings. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29 September 2014 00:21, Brian Norris <computersforpeace@gmail.com> wrote: > (Honestly, some of this name-matching / ID-matching stuff confuses me; > there is at least one too many ways to choose a flash device for this > driver.) It's not that complex once you track it. It's just ugly to tracks because of different structs, function calls and code locations. 1) All m25p80 users (arch code, SPI drivers, etc.) register struct spi_board_info that contains "modalias". It it used to match a driver to load. In most (?) cases modalias is set to "m25p80" 2) Some m25p80 users decided to put a flash device name (e.g. "at25fs010", "mx25l2005a", "m25p05", "w25x10") as modalias of the struct spi_board_info. I think they are affected by the recent m25p80 change to use spi-not framework. 3) Other m25p80 users provide flash device name (e.g. "at25fs010", "mx25l2005a", "m25p05", "w25x10") using platform_data in the struct spi_board_info. It points to the struct flash_platform_data with "name" and "type" properties. It seems to me that "name" is never used. My idea for fixing this: 1) Always use "m25p80" modalias (for m25p80 compatible hw). This will make m25p80 driver work without this huge id_table 2) Do not use "name" nor "type" in the struct flash_platform_data if the flash supports JEDEC 3) If the flash is m25p80 compatible, but doesn't support JEDEC, let's use "type" to let m25p80 work. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29 September 2014 08:36, Rafa? Mi?ecki <zajec5@gmail.com> wrote: > 1) The main urgent issue: broken auto-loading > Tracked in the thread: http://www.spinics.net/lists/linux-spi/msg01726.html > Problem: m25p80.c references spi_nor_ids (from external file) > Short-term solution: duplicate IDs in the m25p80.c > > Ben: just like Brian, I think the patch like this one ( > [PATCH 1/5] m25p80,spi-nor: Fix module aliases for m25p80 > ) is the way to go. However few comments: I started wondering... would this be possible to fix all m25p80 users instead? Make them always specify modalias as "m25p80" and then provide platform_data if needed? Has anyone checked in how many places we use "wrong" (or should I say "weird") modaliases? I guess some would need to grep kernel for all possible names. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29 September 2014 11:53, Rafa? Mi?ecki <zajec5@gmail.com> wrote: > On 29 September 2014 08:36, Rafa? Mi?ecki <zajec5@gmail.com> wrote: >> 1) The main urgent issue: broken auto-loading >> Tracked in the thread: http://www.spinics.net/lists/linux-spi/msg01726.html >> Problem: m25p80.c references spi_nor_ids (from external file) >> Short-term solution: duplicate IDs in the m25p80.c >> >> Ben: just like Brian, I think the patch like this one ( >> [PATCH 1/5] m25p80,spi-nor: Fix module aliases for m25p80 >> ) is the way to go. However few comments: > > I started wondering... would this be possible to fix all m25p80 users > instead? Make them always specify modalias as "m25p80" and then > provide platform_data if needed? > > Has anyone checked in how many places we use "wrong" (or should I say > "weird") modaliases? I guess some would need to grep kernel for all > possible names. I did some (e)grepping, it seems we have: 1) Only 6 references (modalias) in arch/mips/ath79/ 2) About 57 references (compatible) in arch/arm/boot/dts/ 3) About 32 references (compatible) in arch/powerpc/boot/dts/ However it seems there is no way to provide platform_data in Device Tree files. So we have to support compatible (modalias) for now I guess. The list (if someone would like to check) of used names in archs: m25p128 m25p16 m25p32 m25p40 m25p64 mx25l12805d mx25l1606e mx25l25635e mx25l4005a mx25l6405d mx66l51235l n25q128a11 n25q128a13 n25q512a s25fl008k s25fl256s1 s25fl512s s25sl064a s25sl12801 sst25vf016b sst25vf032b sst25vf040b sst25wf040 w25q128 w25q256 w25q32 w25q32dw w25q80bl w25x80
On Mon, 2014-09-29 at 08:36 +0200, Rafa? Mi?ecki wrote: > On 29 September 2014 00:21, Brian Norris <computersforpeace@gmail.com> wrote: > > + Rafal > > > > Rafal has been looking at the same area of code. I'd really like to get > > this patch into 3.18 if possible, so the more eyes the better. > > Thanks Brian. > > I took me a while to follow this issue, too bad I wasn't subscribed to > the ML earlier. Let me try to sum it up. > > > > 1) The main urgent issue: broken auto-loading > Tracked in the thread: http://www.spinics.net/lists/linux-spi/msg01726.html > Problem: m25p80.c references spi_nor_ids (from external file) > Short-term solution: duplicate IDs in the m25p80.c > > Ben: just like Brian, I think the patch like this one ( > [PATCH 1/5] m25p80,spi-nor: Fix module aliases for m25p80 > ) is the way to go. However few comments: > > a) I don't see why you modify m25p_probe in it. Because spi_nor_scan() requires a struct spi_device_id with the driver_data field pointing to a struct flash_info. > b) I don't think the described clean solution (you described it in the > commit message): > > A clean solution to this will involve defining the list of device > > IDs in spi-nor.h and removing struct spi_device_id from the spi-nor > > API, but this is quite a large change. > is the correct one. I think there should be a single string to trigger > m25p80 load and the rest should be handled using JEDEC, with some > workarounds for incompatible devices only. That certainly makes sense for Linux-specific platform data, but I don't think it works for Device Tree "compatible" strings (see <http://mid.gmane.org/1410660009.3040.29.camel@decadent.org.uk>). [...] > b) Removing spi_nor::read_id > https://patchwork.ozlabs.org/patch/389073/ > Ben: I think this one has a NACK from me, because I'm going to use > custom read_id in the bcm53xxspiflash driver. > See following thread for bcm53xxspiflash description: > http://comments.gmane.org/gmane.linux.drivers.mtd/54578 > Initial commit (it uses read_id): https://patchwork.ozlabs.org/patch/381902/ [...] But it has to use spi_nor_match_id() because of the driver_data requirement. This just illustrates that the read_id operation doesn't make sense as currently defined. I accept that there will be a need for a read_id operation, but I think it should fill in a struct flash_info rather than requiring every chip to be described and named in spi-nor.c. Ben.
On Tue, Sep 30, 2014 at 03:07:38AM +0100, Ben Hutchings wrote: > On Mon, 2014-09-29 at 08:36 +0200, Rafa? Mi?ecki wrote: > > b) I don't think the described clean solution (you described it in the > > commit message): > > > A clean solution to this will involve defining the list of device > > > IDs in spi-nor.h and removing struct spi_device_id from the spi-nor > > > API, but this is quite a large change. > > is the correct one. I think there should be a single string to trigger > > m25p80 load and the rest should be handled using JEDEC, with some > > workarounds for incompatible devices only. > > That certainly makes sense for Linux-specific platform data, but I don't > think it works for Device Tree "compatible" strings (see > <http://mid.gmane.org/1410660009.3040.29.camel@decadent.org.uk>). I think it might work. Your quote from that thread: "I think that a DT node is always supposed to include a compatible string for the specific device. It could also include a generic compatible string for SPI NOR chips, but the *only* thing a driver can do with that is to use the JEDEC ID command. It can't even generically read a single byte, since addresses may be either 3 or 4 bytes long! So I think that if a generic compatible string is defined, the DT binding must also define the properties that spi-nor currently reads out of its static table." For every current entry (except the "*-nonjedec" entries; I don't think we should be supporting any more entries like this, and I'd like to kill the existing ones if possible), we can do just that; read out the JEDEC ID and go from there. (Rafal is also looking at non-JEDEC RDID commands, but that would just require a different type of binding.) In fact, for most of these entries, we'll read out the ID and override the match provided by the driver anyway. See: int spi_nor_scan(...) { ... [Note: almost all 'info' entries provide a non-zero jedec_id field] if (info->jedec_id) { const struct spi_device_id *jid; jid = nor->read_id(nor); if (IS_ERR(jid)) { return PTR_ERR(jid); } else if (jid != id) { /* * JEDEC knows better, so overwrite platform ID. We * can't trust partitions any longer, but we'll let * mtd apply them anyway, since some partitions may be * marked read-only, and we don't want to lose that * information, even if it's not 100% accurate. */ dev_warn(dev, "found %s, expected %s\n", jid->name, id->name); id = jid; info = (void *)jid->driver_data; } } ... } I think this chunk might be reworked (or at least, modify the comments) to note how we primarily expect to override the input ID. We might even drop the dev_warn() eventually, and make it more informative instead. > [...] > > b) Removing spi_nor::read_id > > https://patchwork.ozlabs.org/patch/389073/ > > Ben: I think this one has a NACK from me, because I'm going to use > > custom read_id in the bcm53xxspiflash driver. > > See following thread for bcm53xxspiflash description: > > http://comments.gmane.org/gmane.linux.drivers.mtd/54578 > > Initial commit (it uses read_id): https://patchwork.ozlabs.org/patch/381902/ > [...] > > But it has to use spi_nor_match_id() because of the driver_data > requirement. This just illustrates that the read_id operation doesn't > make sense as currently defined. Right. I think it may turn out better to drop it and force a redesign for the next user. > I accept that there will be a need for a read_id operation, but I think > it should fill in a struct flash_info rather than requiring every chip > to be described and named in spi-nor.c. Maybe struct flash_info, but this still leaks more spi-nor.c internal info into drivers. Perhaps Rafal's use case could be served by a select-able 'READ ID' opcode, with his flash_info structs still stored in spi_nor_ids[] -- or possibly as a separate ID table within spi-nor.c. But either way, I agree the current read_id() hook is not satisfactory. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30 September 2014 04:07, Ben Hutchings <ben@decadent.org.uk> wrote: > On Mon, 2014-09-29 at 08:36 +0200, Rafa? Mi?ecki wrote: >> On 29 September 2014 00:21, Brian Norris <computersforpeace@gmail.com> wrote: >> > + Rafal >> > >> > Rafal has been looking at the same area of code. I'd really like to get >> > this patch into 3.18 if possible, so the more eyes the better. >> >> Thanks Brian. >> >> I took me a while to follow this issue, too bad I wasn't subscribed to >> the ML earlier. Let me try to sum it up. >> >> >> >> 1) The main urgent issue: broken auto-loading >> Tracked in the thread: http://www.spinics.net/lists/linux-spi/msg01726.html >> Problem: m25p80.c references spi_nor_ids (from external file) >> Short-term solution: duplicate IDs in the m25p80.c >> >> Ben: just like Brian, I think the patch like this one ( >> [PATCH 1/5] m25p80,spi-nor: Fix module aliases for m25p80 >> ) is the way to go. However few comments: >> >> a) I don't see why you modify m25p_probe in it. > > Because spi_nor_scan() requires a struct spi_device_id with the > driver_data field pointing to a struct flash_info. More friendly explanation: because spi_get_device_id uses driver's id_table (that was now changes to pure strings). I see the point. >> b) I don't think the described clean solution (you described it in the >> commit message): >> > A clean solution to this will involve defining the list of device >> > IDs in spi-nor.h and removing struct spi_device_id from the spi-nor >> > API, but this is quite a large change. >> is the correct one. I think there should be a single string to trigger >> m25p80 load and the rest should be handled using JEDEC, with some >> workarounds for incompatible devices only. > > That certainly makes sense for Linux-specific platform data, but I don't > think it works for Device Tree "compatible" strings (see > <http://mid.gmane.org/1410660009.3040.29.camel@decadent.org.uk>). We could simply follow the way Linux-specific platform data works. We could always use compatible = "m25p80"; and then for some rare cases (where JEDEC fails) add something like model = "at25df321a"; >> b) Removing spi_nor::read_id >> https://patchwork.ozlabs.org/patch/389073/ >> Ben: I think this one has a NACK from me, because I'm going to use >> custom read_id in the bcm53xxspiflash driver. >> See following thread for bcm53xxspiflash description: >> http://comments.gmane.org/gmane.linux.drivers.mtd/54578 >> Initial commit (it uses read_id): https://patchwork.ozlabs.org/patch/381902/ > [...] > > But it has to use spi_nor_match_id() because of the driver_data > requirement. This just illustrates that the read_id operation doesn't > make sense as currently defined. I agree, however there is already a patch in progress handling that: https://patchwork.ozlabs.org/patch/377917/ > I accept that there will be a need for a read_id operation, but I think > it should fill in a struct flash_info rather than requiring every chip > to be described and named in spi-nor.c. Flashes not supporting JEDEC are usually some weird versions of normal flashes with JEDEC support. So I think we could still have them in the spi-nor database and let users provide the name instead of filling the whole struct flash_info.
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index ed7e0a1b..3f0fe86 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -196,6 +196,7 @@ static int m25p_probe(struct spi_device *spi) struct m25p *flash; struct spi_nor *nor; enum read_mode mode = SPI_NOR_NORMAL; + const struct spi_device_id *id; int ret; flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL); @@ -223,7 +224,10 @@ static int m25p_probe(struct spi_device *spi) mode = SPI_NOR_QUAD; else if (spi->mode & SPI_RX_DUAL) mode = SPI_NOR_DUAL; - ret = spi_nor_scan(nor, spi_get_device_id(spi), mode); + id = spi_nor_match_id(spi_get_device_id(spi)->name); + if (WARN_ON(!id)) + return -ENODEV; + ret = spi_nor_scan(nor, id, mode); if (ret) return ret; @@ -245,12 +249,62 @@ static int m25p_remove(struct spi_device *spi) } +/* + * XXX This needs to be kept in sync with spi_nor_ids. We can't share + * it with spi-nor, because if this is built as a module then modpost + * won't be able to read it and add appropriate aliases. + */ +static const struct spi_device_id m25p_ids[] = { + {"at25fs010"}, {"at25fs040"}, {"at25df041a"}, {"at25df321a"}, + {"at25df641"}, {"at26f004"}, {"at26df081a"}, {"at26df161a"}, + {"at26df321"}, {"at45db081d"}, + {"en25f32"}, {"en25p32"}, {"en25q32b"}, {"en25p64"}, + {"en25q64"}, {"en25qh128"}, {"en25qh256"}, + {"f25l32pa"}, + {"mr25h256"}, {"mr25h10"}, + {"gd25q32"}, {"gd25q64"}, + {"160s33b"}, {"320s33b"}, {"640s33b"}, + {"mx25l2005a"}, {"mx25l4005a"}, {"mx25l8005"}, {"mx25l1606e"}, + {"mx25l3205d"}, {"mx25l3255e"}, {"mx25l6405d"}, {"mx25l12805d"}, + {"mx25l12855e"},{"mx25l25635e"},{"mx25l25655e"},{"mx66l51235l"}, + {"mx66l1g55g"}, + {"n25q064"}, {"n25q128a11"}, {"n25q128a13"}, {"n25q256a"}, + {"n25q512a"}, {"n25q512ax3"}, {"n25q00"}, + {"pm25lv512"}, {"pm25lv010"}, {"pm25lq032"}, + {"s25sl032p"}, {"s25sl064p"}, {"s25fl256s0"}, {"s25fl256s1"}, + {"s25fl512s"}, {"s70fl01gs"}, {"s25sl12800"}, {"s25sl12801"}, + {"s25fl129p0"}, {"s25fl129p1"}, {"s25sl004a"}, {"s25sl008a"}, + {"s25sl016a"}, {"s25sl032a"}, {"s25sl064a"}, {"s25fl008k"}, + {"s25fl016k"}, {"s25fl064k"}, + {"sst25vf040b"},{"sst25vf080b"},{"sst25vf016b"},{"sst25vf032b"}, + {"sst25vf064c"},{"sst25wf512"}, {"sst25wf010"}, {"sst25wf020"}, + {"sst25wf040"}, + {"m25p05"}, {"m25p10"}, {"m25p20"}, {"m25p40"}, + {"m25p80"}, {"m25p16"}, {"m25p32"}, {"m25p64"}, + {"m25p128"}, {"n25q032"}, + {"m25p05-nonjedec"}, {"m25p10-nonjedec"}, {"m25p20-nonjedec"}, + {"m25p40-nonjedec"}, {"m25p80-nonjedec"}, {"m25p16-nonjedec"}, + {"m25p32-nonjedec"}, {"m25p64-nonjedec"}, {"m25p128-nonjedec"}, + {"m45pe10"}, {"m45pe80"}, {"m45pe16"}, + {"m25pe20"}, {"m25pe80"}, {"m25pe16"}, + {"m25px16"}, {"m25px32"}, {"m25px32-s0"}, {"m25px32-s1"}, + {"m25px64"}, + {"w25x10"}, {"w25x20"}, {"w25x40"}, {"w25x80"}, + {"w25x16"}, {"w25x32"}, {"w25q32"}, {"w25q32dw"}, + {"w25x64"}, {"w25q64"}, {"w25q128"}, {"w25q80"}, + {"w25q80bl"}, {"w25q128"}, {"w25q256"}, {"cat25c11"}, + {"cat25c03"}, {"cat25c09"}, {"cat25c17"}, {"cat25128"}, + { }, +}; +MODULE_DEVICE_TABLE(spi, m25p_ids); + + static struct spi_driver m25p80_driver = { .driver = { .name = "m25p80", .owner = THIS_MODULE, }, - .id_table = spi_nor_ids, + .id_table = m25p_ids, .probe = m25p_probe, .remove = m25p_remove, diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 03e0ab8..6b1bda2 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -473,7 +473,7 @@ struct flash_info { * more nor chips. This current list focusses on newer chips, which * have been converging on command sets which including JEDEC ID. */ -const struct spi_device_id spi_nor_ids[] = { +static const struct spi_device_id spi_nor_ids[] = { /* Atmel -- some are (confusingly) marketed as "DataFlash" */ { "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K) }, { "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K) }, @@ -637,7 +637,6 @@ const struct spi_device_id spi_nor_ids[] = { { "cat25128", CAT25_INFO(2048, 8, 64, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) }, { }, }; -EXPORT_SYMBOL_GPL(spi_nor_ids); static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor) { @@ -1136,7 +1135,7 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, } EXPORT_SYMBOL_GPL(spi_nor_scan); -const struct spi_device_id *spi_nor_match_id(char *name) +const struct spi_device_id *spi_nor_match_id(const char *name) { const struct spi_device_id *id = spi_nor_ids; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 9e6294f..5ec84cc 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -201,7 +201,6 @@ struct spi_nor { */ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, enum read_mode mode); -extern const struct spi_device_id spi_nor_ids[]; /** * spi_nor_match_id() - find the spi_device_id by the name @@ -213,6 +212,6 @@ extern const struct spi_device_id spi_nor_ids[]; * Return: returns the right spi_device_id pointer on success, * and returns NULL on failure. */ -const struct spi_device_id *spi_nor_match_id(char *name); +const struct spi_device_id *spi_nor_match_id(const char *name); #endif
m25p80's device ID table is now spi_nor_ids, defined in spi-nor. The MODULE_DEVICE_TABLE() macro doesn't work with extern definitions, but its use was also removed at the same time. Now if m25p80 is built as a module it doesn't get the necessary aliases to be loaded automatically. A clean solution to this will involve defining the list of device IDs in spi-nor.h and removing struct spi_device_id from the spi-nor API, but this is quite a large change. As a quick fix suitable for stable, copy the device IDs back into m25p80. Fixes: 03e296f613af ("mtd: m25p80: use the SPI nor framework") Cc: stable <stable@vger.kernel.org> # 3.16 Signed-off-by: Ben Hutchings <ben@decadent.org.uk> --- drivers/mtd/devices/m25p80.c | 58 +++++++++++++++++++++++++++++++++++++++++-- drivers/mtd/spi-nor/spi-nor.c | 5 ++-- include/linux/mtd/spi-nor.h | 3 +-- 3 files changed, 59 insertions(+), 7 deletions(-)