* [PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts
@ 2017-04-18 14:21 Andrey Smirnov
2017-04-18 14:21 ` [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe() Andrey Smirnov
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Andrey Smirnov @ 2017-04-18 14:21 UTC (permalink / raw)
To: linux-mtd
Cc: Andrey Smirnov, cphealy, David Woodhouse, Brian Norris,
Boris Brezillon, Marek Vasut, Richard Weinberger,
Cyrille Pitchen, linux-kernel
Replace C99 type with their kernel counterparts as per request from
Marek Vasut.
No functional change intended.
Cc: cphealy@gmail.com
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
drivers/mtd/devices/mtd_dataflash.c | 40 ++++++++++++++++++-------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index f9e9bd1..a566231 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -84,7 +84,7 @@
struct dataflash {
- uint8_t command[4];
+ u8 command[4];
char name[24];
unsigned short page_offset; /* offset in flash address */
@@ -153,8 +153,8 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
struct spi_transfer x = { };
struct spi_message msg;
unsigned blocksize = priv->page_size << 3;
- uint8_t *command;
- uint32_t rem;
+ u8 *command;
+ u32 rem;
pr_debug("%s: erase addr=0x%llx len 0x%llx\n",
dev_name(&spi->dev), (long long)instr->addr,
@@ -187,8 +187,8 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
pageaddr = pageaddr << priv->page_offset;
command[0] = do_block ? OP_ERASE_BLOCK : OP_ERASE_PAGE;
- command[1] = (uint8_t)(pageaddr >> 16);
- command[2] = (uint8_t)(pageaddr >> 8);
+ command[1] = (u8)(pageaddr >> 16);
+ command[2] = (u8)(pageaddr >> 8);
command[3] = 0;
pr_debug("ERASE %s: (%x) %x %x %x [%i]\n",
@@ -239,7 +239,7 @@ static int dataflash_read(struct mtd_info *mtd, loff_t from, size_t len,
struct spi_transfer x[2] = { };
struct spi_message msg;
unsigned int addr;
- uint8_t *command;
+ u8 *command;
int status;
pr_debug("%s: read 0x%x..0x%x\n", dev_name(&priv->spi->dev),
@@ -271,9 +271,9 @@ static int dataflash_read(struct mtd_info *mtd, loff_t from, size_t len,
* fewer "don't care" bytes. Both buffers stay unchanged.
*/
command[0] = OP_READ_CONTINUOUS;
- command[1] = (uint8_t)(addr >> 16);
- command[2] = (uint8_t)(addr >> 8);
- command[3] = (uint8_t)(addr >> 0);
+ command[1] = (u8)(addr >> 16);
+ command[2] = (u8)(addr >> 8);
+ command[3] = (u8)(addr >> 0);
/* plus 4 "don't care" bytes */
status = spi_sync(priv->spi, &msg);
@@ -308,7 +308,7 @@ static int dataflash_write(struct mtd_info *mtd, loff_t to, size_t len,
size_t remaining = len;
u_char *writebuf = (u_char *) buf;
int status = -EINVAL;
- uint8_t *command;
+ u8 *command;
pr_debug("%s: write 0x%x..0x%x\n",
dev_name(&spi->dev), (unsigned)to, (unsigned)(to + len));
@@ -455,11 +455,11 @@ static int dataflash_get_otp_info(struct mtd_info *mtd, size_t len,
}
static ssize_t otp_read(struct spi_device *spi, unsigned base,
- uint8_t *buf, loff_t off, size_t len)
+ u8 *buf, loff_t off, size_t len)
{
struct spi_message m;
size_t l;
- uint8_t *scratch;
+ u8 *scratch;
struct spi_transfer t;
int status;
@@ -538,7 +538,7 @@ static int dataflash_write_user_otp(struct mtd_info *mtd,
{
struct spi_message m;
const size_t l = 4 + 64;
- uint8_t *scratch;
+ u8 *scratch;
struct spi_transfer t;
struct dataflash *priv = mtd->priv;
int status;
@@ -689,14 +689,14 @@ struct flash_info {
/* JEDEC id has a high byte of zero plus three data bytes:
* the manufacturer id, then a two byte device id.
*/
- uint32_t jedec_id;
+ u32 jedec_id;
/* The size listed here is what works with OP_ERASE_PAGE. */
unsigned nr_pages;
- uint16_t pagesize;
- uint16_t pageoffset;
+ u16 pagesize;
+ u16 pageoffset;
- uint16_t flags;
+ u16 flags;
#define SUP_POW2PS 0x0002 /* supports 2^N byte pages */
#define IS_POW2PS 0x0001 /* uses 2^N byte pages */
};
@@ -739,9 +739,9 @@ static struct flash_info dataflash_data[] = {
static struct flash_info *jedec_probe(struct spi_device *spi)
{
int tmp;
- uint8_t code = OP_READ_ID;
- uint8_t id[3];
- uint32_t jedec;
+ u8 code = OP_READ_ID;
+ u8 id[3];
+ u32 jedec;
struct flash_info *info;
int status;
--
2.9.3
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe() 2017-04-18 14:21 [PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts Andrey Smirnov @ 2017-04-18 14:21 ` Andrey Smirnov 2017-04-18 18:25 ` Marek Vasut 2017-04-18 14:21 ` [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information" Andrey Smirnov 2017-04-18 18:24 ` [PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts Marek Vasut 2 siblings, 1 reply; 12+ messages in thread From: Andrey Smirnov @ 2017-04-18 14:21 UTC (permalink / raw) To: linux-mtd Cc: Andrey Smirnov, cphealy, David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-kernel As per request from Marek Vasut, change the following: - Replace indentation between type and name of local variable from tabs to spaces - Replace magic number 0x1F with CFI_MFR_ATMEL macro - Replace variable 'tmp' with 'ret' and 'i' where appropriate - Reformat multi-line comments and add newlines where appropriate No functional change intended. Cc: cphealy@gmail.com Cc: David Woodhouse <dwmw2@infradead.org> Cc: Brian Norris <computersforpeace@gmail.com> Cc: Boris Brezillon <boris.brezillon@free-electrons.com> Cc: Marek Vasut <marek.vasut@gmail.com> Cc: Richard Weinberger <richard@nod.at> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com> Cc: linux-kernel@vger.kernel.org Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- drivers/mtd/devices/mtd_dataflash.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c index a566231..5b7a8c3 100644 --- a/drivers/mtd/devices/mtd_dataflash.c +++ b/drivers/mtd/devices/mtd_dataflash.c @@ -82,6 +82,7 @@ #define OP_WRITE_SECURITY_REVC 0x9A #define OP_WRITE_SECURITY 0x9B /* revision D */ +#define CFI_MFR_ATMEL 0x1F struct dataflash { u8 command[4]; @@ -738,14 +739,15 @@ static struct flash_info dataflash_data[] = { static struct flash_info *jedec_probe(struct spi_device *spi) { - int tmp; - u8 code = OP_READ_ID; - u8 id[3]; - u32 jedec; - struct flash_info *info; + int ret, i; + u8 code = OP_READ_ID; + u8 id[3]; + u32 jedec; + struct flash_info *info; int status; - /* JEDEC also defines an optional "extended device information" + /* + * JEDEC also defines an optional "extended device information" * string for after vendor-specific data, after the three bytes * we use here. Supporting some chips might require using it. * @@ -753,13 +755,14 @@ static struct flash_info *jedec_probe(struct spi_device *spi) * That's not an error; only rev C and newer chips handle it, and * only Atmel sells these chips. */ - tmp = spi_write_then_read(spi, &code, 1, id, 3); - if (tmp < 0) { + ret = spi_write_then_read(spi, &code, 1, id, 3); + if (ret < 0) { pr_debug("%s: error %d reading JEDEC ID\n", - dev_name(&spi->dev), tmp); - return ERR_PTR(tmp); + dev_name(&spi->dev), ret); + return ERR_PTR(ret); } - if (id[0] != 0x1f) + + if (id[0] != CFI_MFR_ATMEL) return NULL; jedec = id[0]; @@ -768,9 +771,9 @@ static struct flash_info *jedec_probe(struct spi_device *spi) jedec = jedec << 8; jedec |= id[2]; - for (tmp = 0, info = dataflash_data; - tmp < ARRAY_SIZE(dataflash_data); - tmp++, info++) { + for (i = 0, info = dataflash_data; + i < ARRAY_SIZE(dataflash_data); + i++, info++) { if (info->jedec_id == jedec) { pr_debug("%s: OTP, sector protect%s\n", dev_name(&spi->dev), -- 2.9.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe() 2017-04-18 14:21 ` [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe() Andrey Smirnov @ 2017-04-18 18:25 ` Marek Vasut 2017-04-18 22:36 ` Joe Perches 0 siblings, 1 reply; 12+ messages in thread From: Marek Vasut @ 2017-04-18 18:25 UTC (permalink / raw) To: Andrey Smirnov, linux-mtd Cc: cphealy, David Woodhouse, Brian Norris, Boris Brezillon, Richard Weinberger, Cyrille Pitchen, linux-kernel On 04/18/2017 04:21 PM, Andrey Smirnov wrote: > As per request from Marek Vasut, change the following: Does that really have to be in the commit message ? ^_^' > - Replace indentation between type and name of local variable from > tabs to spaces > > - Replace magic number 0x1F with CFI_MFR_ATMEL macro > > - Replace variable 'tmp' with 'ret' and 'i' where appropriate > > - Reformat multi-line comments and add newlines where appropriate > > No functional change intended. Appreciated, thanks! Acked-by: Marek Vasut <marek.vasut@gmail.com> > Cc: cphealy@gmail.com > Cc: David Woodhouse <dwmw2@infradead.org> > Cc: Brian Norris <computersforpeace@gmail.com> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Marek Vasut <marek.vasut@gmail.com> > Cc: Richard Weinberger <richard@nod.at> > Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com> > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > drivers/mtd/devices/mtd_dataflash.c | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c > index a566231..5b7a8c3 100644 > --- a/drivers/mtd/devices/mtd_dataflash.c > +++ b/drivers/mtd/devices/mtd_dataflash.c > @@ -82,6 +82,7 @@ > #define OP_WRITE_SECURITY_REVC 0x9A > #define OP_WRITE_SECURITY 0x9B /* revision D */ > > +#define CFI_MFR_ATMEL 0x1F > > struct dataflash { > u8 command[4]; > @@ -738,14 +739,15 @@ static struct flash_info dataflash_data[] = { > > static struct flash_info *jedec_probe(struct spi_device *spi) > { > - int tmp; > - u8 code = OP_READ_ID; > - u8 id[3]; > - u32 jedec; > - struct flash_info *info; > + int ret, i; > + u8 code = OP_READ_ID; > + u8 id[3]; > + u32 jedec; > + struct flash_info *info; > int status; > > - /* JEDEC also defines an optional "extended device information" > + /* > + * JEDEC also defines an optional "extended device information" > * string for after vendor-specific data, after the three bytes > * we use here. Supporting some chips might require using it. > * > @@ -753,13 +755,14 @@ static struct flash_info *jedec_probe(struct spi_device *spi) > * That's not an error; only rev C and newer chips handle it, and > * only Atmel sells these chips. > */ > - tmp = spi_write_then_read(spi, &code, 1, id, 3); > - if (tmp < 0) { > + ret = spi_write_then_read(spi, &code, 1, id, 3); > + if (ret < 0) { > pr_debug("%s: error %d reading JEDEC ID\n", > - dev_name(&spi->dev), tmp); > - return ERR_PTR(tmp); > + dev_name(&spi->dev), ret); > + return ERR_PTR(ret); > } > - if (id[0] != 0x1f) > + > + if (id[0] != CFI_MFR_ATMEL) > return NULL; > > jedec = id[0]; > @@ -768,9 +771,9 @@ static struct flash_info *jedec_probe(struct spi_device *spi) > jedec = jedec << 8; > jedec |= id[2]; > > - for (tmp = 0, info = dataflash_data; > - tmp < ARRAY_SIZE(dataflash_data); > - tmp++, info++) { > + for (i = 0, info = dataflash_data; > + i < ARRAY_SIZE(dataflash_data); > + i++, info++) { > if (info->jedec_id == jedec) { > pr_debug("%s: OTP, sector protect%s\n", > dev_name(&spi->dev), > -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe() 2017-04-18 18:25 ` Marek Vasut @ 2017-04-18 22:36 ` Joe Perches 2017-04-18 22:38 ` Marek Vasut 0 siblings, 1 reply; 12+ messages in thread From: Joe Perches @ 2017-04-18 22:36 UTC (permalink / raw) To: Marek Vasut, Andrey Smirnov, linux-mtd Cc: cphealy, David Woodhouse, Brian Norris, Boris Brezillon, Richard Weinberger, Cyrille Pitchen, linux-kernel On Tue, 2017-04-18 at 20:25 +0200, Marek Vasut wrote: > On 04/18/2017 04:21 PM, Andrey Smirnov wrote: > > As per request from Marek Vasut, change the following: > > Does that really have to be in the commit message ? ^_^' > > > - Replace indentation between type and name of local variable from > > tabs to spaces > > > > - Replace magic number 0x1F with CFI_MFR_ATMEL macro > > > > - Replace variable 'tmp' with 'ret' and 'i' where appropriate > > > > - Reformat multi-line comments and add newlines where appropriate > > > > No functional change intended. > > Appreciated, thanks! trivia: > > diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c [] > > @@ -768,9 +771,9 @@ static struct flash_info *jedec_probe(struct spi_device *spi) > > jedec = jedec << 8; > > jedec |= id[2]; > > > > - for (tmp = 0, info = dataflash_data; > > - tmp < ARRAY_SIZE(dataflash_data); > > - tmp++, info++) { > > + for (i = 0, info = dataflash_data; > > + i < ARRAY_SIZE(dataflash_data); > > + i++, info++) { > > if (info->jedec_id == jedec) { > > pr_debug("%s: OTP, sector protect%s\n", > > dev_name(&spi->dev), This loop could be written without the i variable. for (info = dataflash_data; info < dataflash_data + ARRAY_SIZE(dataflash_data); info++) { ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe() 2017-04-18 22:36 ` Joe Perches @ 2017-04-18 22:38 ` Marek Vasut 0 siblings, 0 replies; 12+ messages in thread From: Marek Vasut @ 2017-04-18 22:38 UTC (permalink / raw) To: Joe Perches, Andrey Smirnov, linux-mtd Cc: cphealy, David Woodhouse, Brian Norris, Boris Brezillon, Richard Weinberger, Cyrille Pitchen, linux-kernel On 04/19/2017 12:36 AM, Joe Perches wrote: > On Tue, 2017-04-18 at 20:25 +0200, Marek Vasut wrote: >> On 04/18/2017 04:21 PM, Andrey Smirnov wrote: >>> As per request from Marek Vasut, change the following: >> >> Does that really have to be in the commit message ? ^_^' >> >>> - Replace indentation between type and name of local variable from >>> tabs to spaces >>> >>> - Replace magic number 0x1F with CFI_MFR_ATMEL macro >>> >>> - Replace variable 'tmp' with 'ret' and 'i' where appropriate >>> >>> - Reformat multi-line comments and add newlines where appropriate >>> >>> No functional change intended. >> >> Appreciated, thanks! > > trivia: > >>> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c > [] >>> @@ -768,9 +771,9 @@ static struct flash_info *jedec_probe(struct spi_device *spi) >>> jedec = jedec << 8; >>> jedec |= id[2]; >>> >>> - for (tmp = 0, info = dataflash_data; >>> - tmp < ARRAY_SIZE(dataflash_data); >>> - tmp++, info++) { >>> + for (i = 0, info = dataflash_data; >>> + i < ARRAY_SIZE(dataflash_data); >>> + i++, info++) { >>> if (info->jedec_id == jedec) { >>> pr_debug("%s: OTP, sector protect%s\n", >>> dev_name(&spi->dev), > > This loop could be written without the i variable. > > for (info = dataflash_data; > info < dataflash_data + ARRAY_SIZE(dataflash_data); > info++) { > But in a separate patch please, so it's bisectable. I don't want to slap such change into this patch. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information" 2017-04-18 14:21 [PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts Andrey Smirnov 2017-04-18 14:21 ` [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe() Andrey Smirnov @ 2017-04-18 14:21 ` Andrey Smirnov 2017-04-18 18:31 ` Marek Vasut 2017-04-18 18:24 ` [PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts Marek Vasut 2 siblings, 1 reply; 12+ messages in thread From: Andrey Smirnov @ 2017-04-18 14:21 UTC (permalink / raw) To: linux-mtd Cc: Andrey Smirnov, cphealy, David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-kernel In anticipation of supporting chips that need it, extend the size of struct flash_info's 'jedec_id' field to make room 2 byte of extended device information as well as add code to fetch this data during jedec_probe(). Cc: cphealy@gmail.com Cc: David Woodhouse <dwmw2@infradead.org> Cc: Brian Norris <computersforpeace@gmail.com> Cc: Boris Brezillon <boris.brezillon@free-electrons.com> Cc: Marek Vasut <marek.vasut@gmail.com> Cc: Richard Weinberger <richard@nod.at> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com> Cc: linux-kernel@vger.kernel.org Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- Changes since [v1]: - Formatting [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com drivers/mtd/devices/mtd_dataflash.c | 113 +++++++++++++++++++++--------------- 1 file changed, 65 insertions(+), 48 deletions(-) diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c index 5b7a8c3..5d694a4 100644 --- a/drivers/mtd/devices/mtd_dataflash.c +++ b/drivers/mtd/devices/mtd_dataflash.c @@ -690,7 +690,7 @@ struct flash_info { /* JEDEC id has a high byte of zero plus three data bytes: * the manufacturer id, then a two byte device id. */ - u32 jedec_id; + u64 jedec_id; /* The size listed here is what works with OP_ERASE_PAGE. */ unsigned nr_pages; @@ -713,63 +713,34 @@ static struct flash_info dataflash_data[] = { * These newer chips also support 128-byte security registers (with * 64 bytes one-time-programmable) and software write-protection. */ - { "AT45DB011B", 0x1f2200, 512, 264, 9, SUP_POW2PS}, - { "at45db011d", 0x1f2200, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, + { "AT45DB011B", 0x1f22000000, 512, 264, 9, SUP_POW2PS}, + { "at45db011d", 0x1f22000000, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, - { "AT45DB021B", 0x1f2300, 1024, 264, 9, SUP_POW2PS}, - { "at45db021d", 0x1f2300, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, + { "AT45DB021B", 0x1f23000000, 1024, 264, 9, SUP_POW2PS}, + { "at45db021d", 0x1f23000000, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, - { "AT45DB041x", 0x1f2400, 2048, 264, 9, SUP_POW2PS}, - { "at45db041d", 0x1f2400, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, + { "AT45DB041x", 0x1f24000000, 2048, 264, 9, SUP_POW2PS}, + { "at45db041d", 0x1f24000000, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, - { "AT45DB081B", 0x1f2500, 4096, 264, 9, SUP_POW2PS}, - { "at45db081d", 0x1f2500, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, + { "AT45DB081B", 0x1f25000000, 4096, 264, 9, SUP_POW2PS}, + { "at45db081d", 0x1f25000000, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, - { "AT45DB161x", 0x1f2600, 4096, 528, 10, SUP_POW2PS}, - { "at45db161d", 0x1f2600, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, + { "AT45DB161x", 0x1f26000000, 4096, 528, 10, SUP_POW2PS}, + { "at45db161d", 0x1f26000000, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, - { "AT45DB321x", 0x1f2700, 8192, 528, 10, 0}, /* rev C */ + { "AT45DB321x", 0x1f27000000, 8192, 528, 10, 0}, /* rev C */ - { "AT45DB321x", 0x1f2701, 8192, 528, 10, SUP_POW2PS}, - { "at45db321d", 0x1f2701, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, + { "AT45DB321x", 0x1f27010000, 8192, 528, 10, SUP_POW2PS}, + { "at45db321d", 0x1f27010000, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, - { "AT45DB642x", 0x1f2800, 8192, 1056, 11, SUP_POW2PS}, - { "at45db642d", 0x1f2800, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, + { "AT45DB642x", 0x1f28000000, 8192, 1056, 11, SUP_POW2PS}, + { "at45db642d", 0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, }; -static struct flash_info *jedec_probe(struct spi_device *spi) +static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec) { - int ret, i; - u8 code = OP_READ_ID; - u8 id[3]; - u32 jedec; + int status, i; struct flash_info *info; - int status; - - /* - * JEDEC also defines an optional "extended device information" - * string for after vendor-specific data, after the three bytes - * we use here. Supporting some chips might require using it. - * - * If the vendor ID isn't Atmel's (0x1f), assume this call failed. - * That's not an error; only rev C and newer chips handle it, and - * only Atmel sells these chips. - */ - ret = spi_write_then_read(spi, &code, 1, id, 3); - if (ret < 0) { - pr_debug("%s: error %d reading JEDEC ID\n", - dev_name(&spi->dev), ret); - return ERR_PTR(ret); - } - - if (id[0] != CFI_MFR_ATMEL) - return NULL; - - jedec = id[0]; - jedec = jedec << 8; - jedec |= id[1]; - jedec = jedec << 8; - jedec |= id[2]; for (i = 0, info = dataflash_data; i < ARRAY_SIZE(dataflash_data); @@ -799,12 +770,58 @@ static struct flash_info *jedec_probe(struct spi_device *spi) } } + return NULL; +} + +static struct flash_info *jedec_probe(struct spi_device *spi) +{ + int ret; + u8 code = OP_READ_ID; + u8 id[8] = {0}; + const unsigned int id_size = 5; + const unsigned int first_byte = sizeof(id) - id_size; + const u64 eid_mask = GENMASK_ULL(63, 16); + u64 jedec; + struct flash_info *info; + + /* + * JEDEC also defines an optional "extended device information" + * string for after vendor-specific data, after the three bytes + * we use here. Supporting some chips might require using it. + * + * If the vendor ID isn't Atmel's (0x1f), assume this call failed. + * That's not an error; only rev C and newer chips handle it, and + * only Atmel sells these chips. + */ + ret = spi_write_then_read(spi, &code, 1, &id[first_byte], id_size); + if (ret < 0) { + pr_debug("%s: error %d reading JEDEC ID\n", + dev_name(&spi->dev), ret); + return ERR_PTR(ret); + } + + if (id[first_byte] != CFI_MFR_ATMEL) + return NULL; + + jedec = be64_to_cpup((__be64 *)id); + + info = jedec_lookup(spi, jedec); + if (info) + return info; + + /* Clear extended id bits and try to find a match again */ + jedec &= eid_mask; + + info = jedec_lookup(spi, jedec); + if (info) + return info; + /* * Treat other chips as errors ... we won't know the right page * size (it might be binary) even when we can tell which density * class is involved (legacy chip id scheme). */ - dev_warn(&spi->dev, "JEDEC id %06x not handled\n", jedec); + dev_warn(&spi->dev, "JEDEC id %010llx not handled\n", jedec); return ERR_PTR(-ENODEV); } -- 2.9.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information" 2017-04-18 14:21 ` [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information" Andrey Smirnov @ 2017-04-18 18:31 ` Marek Vasut 2017-04-19 2:58 ` Andrey Smirnov 0 siblings, 1 reply; 12+ messages in thread From: Marek Vasut @ 2017-04-18 18:31 UTC (permalink / raw) To: Andrey Smirnov, linux-mtd Cc: cphealy, David Woodhouse, Brian Norris, Boris Brezillon, Richard Weinberger, Cyrille Pitchen, linux-kernel On 04/18/2017 04:21 PM, Andrey Smirnov wrote: > In anticipation of supporting chips that need it, extend the size of > struct flash_info's 'jedec_id' field to make room 2 byte of extended > device information as well as add code to fetch this data during > jedec_probe(). > > Cc: cphealy@gmail.com > Cc: David Woodhouse <dwmw2@infradead.org> > Cc: Brian Norris <computersforpeace@gmail.com> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Marek Vasut <marek.vasut@gmail.com> > Cc: Richard Weinberger <richard@nod.at> > Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com> > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > > Changes since [v1]: > > - Formatting > > [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com > > > drivers/mtd/devices/mtd_dataflash.c | 113 +++++++++++++++++++++--------------- > 1 file changed, 65 insertions(+), 48 deletions(-) > > diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c > index 5b7a8c3..5d694a4 100644 > --- a/drivers/mtd/devices/mtd_dataflash.c > +++ b/drivers/mtd/devices/mtd_dataflash.c > @@ -690,7 +690,7 @@ struct flash_info { > /* JEDEC id has a high byte of zero plus three data bytes: > * the manufacturer id, then a two byte device id. > */ > - u32 jedec_id; > + u64 jedec_id; > > /* The size listed here is what works with OP_ERASE_PAGE. */ > unsigned nr_pages; > @@ -713,63 +713,34 @@ static struct flash_info dataflash_data[] = { > * These newer chips also support 128-byte security registers (with > * 64 bytes one-time-programmable) and software write-protection. > */ > - { "AT45DB011B", 0x1f2200, 512, 264, 9, SUP_POW2PS}, > - { "at45db011d", 0x1f2200, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, > + { "AT45DB011B", 0x1f22000000, 512, 264, 9, SUP_POW2PS}, > + { "at45db011d", 0x1f22000000, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, > > - { "AT45DB021B", 0x1f2300, 1024, 264, 9, SUP_POW2PS}, > - { "at45db021d", 0x1f2300, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, > + { "AT45DB021B", 0x1f23000000, 1024, 264, 9, SUP_POW2PS}, > + { "at45db021d", 0x1f23000000, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, > > - { "AT45DB041x", 0x1f2400, 2048, 264, 9, SUP_POW2PS}, > - { "at45db041d", 0x1f2400, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, > + { "AT45DB041x", 0x1f24000000, 2048, 264, 9, SUP_POW2PS}, > + { "at45db041d", 0x1f24000000, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, > > - { "AT45DB081B", 0x1f2500, 4096, 264, 9, SUP_POW2PS}, > - { "at45db081d", 0x1f2500, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, > + { "AT45DB081B", 0x1f25000000, 4096, 264, 9, SUP_POW2PS}, > + { "at45db081d", 0x1f25000000, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, > > - { "AT45DB161x", 0x1f2600, 4096, 528, 10, SUP_POW2PS}, > - { "at45db161d", 0x1f2600, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, > + { "AT45DB161x", 0x1f26000000, 4096, 528, 10, SUP_POW2PS}, > + { "at45db161d", 0x1f26000000, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, > > - { "AT45DB321x", 0x1f2700, 8192, 528, 10, 0}, /* rev C */ > + { "AT45DB321x", 0x1f27000000, 8192, 528, 10, 0}, /* rev C */ > > - { "AT45DB321x", 0x1f2701, 8192, 528, 10, SUP_POW2PS}, > - { "at45db321d", 0x1f2701, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, > + { "AT45DB321x", 0x1f27010000, 8192, 528, 10, SUP_POW2PS}, > + { "at45db321d", 0x1f27010000, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, > > - { "AT45DB642x", 0x1f2800, 8192, 1056, 11, SUP_POW2PS}, > - { "at45db642d", 0x1f2800, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, > + { "AT45DB642x", 0x1f28000000, 8192, 1056, 11, SUP_POW2PS}, > + { "at45db642d", 0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, > }; > > -static struct flash_info *jedec_probe(struct spi_device *spi) > +static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec) > { > - int ret, i; > - u8 code = OP_READ_ID; > - u8 id[3]; > - u32 jedec; > + int status, i; > struct flash_info *info; > - int status; > - > - /* > - * JEDEC also defines an optional "extended device information" > - * string for after vendor-specific data, after the three bytes > - * we use here. Supporting some chips might require using it. > - * > - * If the vendor ID isn't Atmel's (0x1f), assume this call failed. > - * That's not an error; only rev C and newer chips handle it, and > - * only Atmel sells these chips. > - */ > - ret = spi_write_then_read(spi, &code, 1, id, 3); > - if (ret < 0) { > - pr_debug("%s: error %d reading JEDEC ID\n", > - dev_name(&spi->dev), ret); > - return ERR_PTR(ret); > - } > - > - if (id[0] != CFI_MFR_ATMEL) > - return NULL; > - > - jedec = id[0]; > - jedec = jedec << 8; > - jedec |= id[1]; > - jedec = jedec << 8; > - jedec |= id[2]; Hm, the diff again screwed this up, oh well ... > for (i = 0, info = dataflash_data; > i < ARRAY_SIZE(dataflash_data); > @@ -799,12 +770,58 @@ static struct flash_info *jedec_probe(struct spi_device *spi) > } > } > > + return NULL; > +} > + > +static struct flash_info *jedec_probe(struct spi_device *spi) > +{ > + int ret; > + u8 code = OP_READ_ID; > + u8 id[8] = {0}; > + const unsigned int id_size = 5; > + const unsigned int first_byte = sizeof(id) - id_size; > + const u64 eid_mask = GENMASK_ULL(63, 16); > + u64 jedec; > + struct flash_info *info; > + > + /* > + * JEDEC also defines an optional "extended device information" > + * string for after vendor-specific data, after the three bytes > + * we use here. Supporting some chips might require using it. > + * > + * If the vendor ID isn't Atmel's (0x1f), assume this call failed. > + * That's not an error; only rev C and newer chips handle it, and > + * only Atmel sells these chips. > + */ > + ret = spi_write_then_read(spi, &code, 1, &id[first_byte], id_size); > + if (ret < 0) { > + pr_debug("%s: error %d reading JEDEC ID\n", > + dev_name(&spi->dev), ret); I think you can turn this into: dev_err(&spi->dev, "error %d reading JEDEC ID\n", ret); > + return ERR_PTR(ret); > + } > + > + if (id[first_byte] != CFI_MFR_ATMEL) > + return NULL; > + > + jedec = be64_to_cpup((__be64 *)id); > + > + info = jedec_lookup(spi, jedec); > + if (info) > + return info; > + > + /* Clear extended id bits and try to find a match again */ > + jedec &= eid_mask; Wouldn't that be jedec &= ~eid_mask; (with a tilde) if you want to clear them ? Except for those two nits, looks fine, thanks! > + info = jedec_lookup(spi, jedec); > + if (info) > + return info; > + > /* > * Treat other chips as errors ... we won't know the right page > * size (it might be binary) even when we can tell which density > * class is involved (legacy chip id scheme). > */ > - dev_warn(&spi->dev, "JEDEC id %06x not handled\n", jedec); > + dev_warn(&spi->dev, "JEDEC id %010llx not handled\n", jedec); > return ERR_PTR(-ENODEV); > } > > -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information" 2017-04-18 18:31 ` Marek Vasut @ 2017-04-19 2:58 ` Andrey Smirnov 2017-04-19 8:47 ` Marek Vasut 0 siblings, 1 reply; 12+ messages in thread From: Andrey Smirnov @ 2017-04-19 2:58 UTC (permalink / raw) To: Marek Vasut Cc: linux-mtd, Chris Healy, David Woodhouse, Brian Norris, Boris Brezillon, Richard Weinberger, Cyrille Pitchen, linux-kernel On Tue, Apr 18, 2017 at 11:31 AM, Marek Vasut <marek.vasut@gmail.com> wrote: > On 04/18/2017 04:21 PM, Andrey Smirnov wrote: >> In anticipation of supporting chips that need it, extend the size of >> struct flash_info's 'jedec_id' field to make room 2 byte of extended >> device information as well as add code to fetch this data during >> jedec_probe(). >> >> Cc: cphealy@gmail.com >> Cc: David Woodhouse <dwmw2@infradead.org> >> Cc: Brian Norris <computersforpeace@gmail.com> >> Cc: Boris Brezillon <boris.brezillon@free-electrons.com> >> Cc: Marek Vasut <marek.vasut@gmail.com> >> Cc: Richard Weinberger <richard@nod.at> >> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com> >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >> --- >> >> Changes since [v1]: >> >> - Formatting >> >> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com >> >> >> drivers/mtd/devices/mtd_dataflash.c | 113 +++++++++++++++++++++--------------- >> 1 file changed, 65 insertions(+), 48 deletions(-) >> >> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c >> index 5b7a8c3..5d694a4 100644 >> --- a/drivers/mtd/devices/mtd_dataflash.c >> +++ b/drivers/mtd/devices/mtd_dataflash.c >> @@ -690,7 +690,7 @@ struct flash_info { >> /* JEDEC id has a high byte of zero plus three data bytes: >> * the manufacturer id, then a two byte device id. >> */ >> - u32 jedec_id; >> + u64 jedec_id; >> >> /* The size listed here is what works with OP_ERASE_PAGE. */ >> unsigned nr_pages; >> @@ -713,63 +713,34 @@ static struct flash_info dataflash_data[] = { >> * These newer chips also support 128-byte security registers (with >> * 64 bytes one-time-programmable) and software write-protection. >> */ >> - { "AT45DB011B", 0x1f2200, 512, 264, 9, SUP_POW2PS}, >> - { "at45db011d", 0x1f2200, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, >> + { "AT45DB011B", 0x1f22000000, 512, 264, 9, SUP_POW2PS}, >> + { "at45db011d", 0x1f22000000, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, >> >> - { "AT45DB021B", 0x1f2300, 1024, 264, 9, SUP_POW2PS}, >> - { "at45db021d", 0x1f2300, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, >> + { "AT45DB021B", 0x1f23000000, 1024, 264, 9, SUP_POW2PS}, >> + { "at45db021d", 0x1f23000000, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, >> >> - { "AT45DB041x", 0x1f2400, 2048, 264, 9, SUP_POW2PS}, >> - { "at45db041d", 0x1f2400, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, >> + { "AT45DB041x", 0x1f24000000, 2048, 264, 9, SUP_POW2PS}, >> + { "at45db041d", 0x1f24000000, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, >> >> - { "AT45DB081B", 0x1f2500, 4096, 264, 9, SUP_POW2PS}, >> - { "at45db081d", 0x1f2500, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, >> + { "AT45DB081B", 0x1f25000000, 4096, 264, 9, SUP_POW2PS}, >> + { "at45db081d", 0x1f25000000, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, >> >> - { "AT45DB161x", 0x1f2600, 4096, 528, 10, SUP_POW2PS}, >> - { "at45db161d", 0x1f2600, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, >> + { "AT45DB161x", 0x1f26000000, 4096, 528, 10, SUP_POW2PS}, >> + { "at45db161d", 0x1f26000000, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, >> >> - { "AT45DB321x", 0x1f2700, 8192, 528, 10, 0}, /* rev C */ >> + { "AT45DB321x", 0x1f27000000, 8192, 528, 10, 0}, /* rev C */ >> >> - { "AT45DB321x", 0x1f2701, 8192, 528, 10, SUP_POW2PS}, >> - { "at45db321d", 0x1f2701, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, >> + { "AT45DB321x", 0x1f27010000, 8192, 528, 10, SUP_POW2PS}, >> + { "at45db321d", 0x1f27010000, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, >> >> - { "AT45DB642x", 0x1f2800, 8192, 1056, 11, SUP_POW2PS}, >> - { "at45db642d", 0x1f2800, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, >> + { "AT45DB642x", 0x1f28000000, 8192, 1056, 11, SUP_POW2PS}, >> + { "at45db642d", 0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, >> }; >> >> -static struct flash_info *jedec_probe(struct spi_device *spi) >> +static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec) >> { >> - int ret, i; >> - u8 code = OP_READ_ID; >> - u8 id[3]; >> - u32 jedec; >> + int status, i; >> struct flash_info *info; >> - int status; >> - >> - /* >> - * JEDEC also defines an optional "extended device information" >> - * string for after vendor-specific data, after the three bytes >> - * we use here. Supporting some chips might require using it. >> - * >> - * If the vendor ID isn't Atmel's (0x1f), assume this call failed. >> - * That's not an error; only rev C and newer chips handle it, and >> - * only Atmel sells these chips. >> - */ >> - ret = spi_write_then_read(spi, &code, 1, id, 3); >> - if (ret < 0) { >> - pr_debug("%s: error %d reading JEDEC ID\n", >> - dev_name(&spi->dev), ret); >> - return ERR_PTR(ret); >> - } >> - >> - if (id[0] != CFI_MFR_ATMEL) >> - return NULL; >> - >> - jedec = id[0]; >> - jedec = jedec << 8; >> - jedec |= id[1]; >> - jedec = jedec << 8; >> - jedec |= id[2]; > > Hm, the diff again screwed this up, oh well ... > >> for (i = 0, info = dataflash_data; >> i < ARRAY_SIZE(dataflash_data); >> @@ -799,12 +770,58 @@ static struct flash_info *jedec_probe(struct spi_device *spi) >> } >> } >> >> + return NULL; >> +} >> + >> +static struct flash_info *jedec_probe(struct spi_device *spi) >> +{ >> + int ret; >> + u8 code = OP_READ_ID; >> + u8 id[8] = {0}; >> + const unsigned int id_size = 5; >> + const unsigned int first_byte = sizeof(id) - id_size; >> + const u64 eid_mask = GENMASK_ULL(63, 16); >> + u64 jedec; >> + struct flash_info *info; >> + >> + /* >> + * JEDEC also defines an optional "extended device information" >> + * string for after vendor-specific data, after the three bytes >> + * we use here. Supporting some chips might require using it. >> + * >> + * If the vendor ID isn't Atmel's (0x1f), assume this call failed. >> + * That's not an error; only rev C and newer chips handle it, and >> + * only Atmel sells these chips. >> + */ >> + ret = spi_write_then_read(spi, &code, 1, &id[first_byte], id_size); >> + if (ret < 0) { >> + pr_debug("%s: error %d reading JEDEC ID\n", >> + dev_name(&spi->dev), ret); > > I think you can turn this into: > > dev_err(&spi->dev, "error %d reading JEDEC ID\n", ret); OK, will do it in a separate patch, since this is original code. > >> + return ERR_PTR(ret); >> + } >> + >> + if (id[first_byte] != CFI_MFR_ATMEL) >> + return NULL; >> + >> + jedec = be64_to_cpup((__be64 *)id); >> + >> + info = jedec_lookup(spi, jedec); >> + if (info) >> + return info; >> + >> + /* Clear extended id bits and try to find a match again */ >> + jedec &= eid_mask; > > Wouldn't that be jedec &= ~eid_mask; (with a tilde) if you want to clear > them ? eid_mask is generated as bits 63 to 16 with GENMASK_ULL, so bits 15 to 0 should already be zeroed out. I can rename it to eid_mask_inverted so it's less confusing. Thanks, Andrey Smirnov ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information" 2017-04-19 2:58 ` Andrey Smirnov @ 2017-04-19 8:47 ` Marek Vasut 2017-04-19 15:07 ` Andrey Smirnov 0 siblings, 1 reply; 12+ messages in thread From: Marek Vasut @ 2017-04-19 8:47 UTC (permalink / raw) To: Andrey Smirnov Cc: linux-mtd, Chris Healy, David Woodhouse, Brian Norris, Boris Brezillon, Richard Weinberger, Cyrille Pitchen, linux-kernel On 04/19/2017 04:58 AM, Andrey Smirnov wrote: > On Tue, Apr 18, 2017 at 11:31 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >> On 04/18/2017 04:21 PM, Andrey Smirnov wrote: >>> In anticipation of supporting chips that need it, extend the size of >>> struct flash_info's 'jedec_id' field to make room 2 byte of extended >>> device information as well as add code to fetch this data during >>> jedec_probe(). >>> >>> Cc: cphealy@gmail.com >>> Cc: David Woodhouse <dwmw2@infradead.org> >>> Cc: Brian Norris <computersforpeace@gmail.com> >>> Cc: Boris Brezillon <boris.brezillon@free-electrons.com> >>> Cc: Marek Vasut <marek.vasut@gmail.com> >>> Cc: Richard Weinberger <richard@nod.at> >>> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com> >>> Cc: linux-kernel@vger.kernel.org >>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >>> --- >>> >>> Changes since [v1]: >>> >>> - Formatting >>> >>> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com >>> >>> >>> drivers/mtd/devices/mtd_dataflash.c | 113 +++++++++++++++++++++--------------- >>> 1 file changed, 65 insertions(+), 48 deletions(-) >>> >>> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c >>> index 5b7a8c3..5d694a4 100644 >>> --- a/drivers/mtd/devices/mtd_dataflash.c >>> +++ b/drivers/mtd/devices/mtd_dataflash.c >>> @@ -690,7 +690,7 @@ struct flash_info { >>> /* JEDEC id has a high byte of zero plus three data bytes: >>> * the manufacturer id, then a two byte device id. >>> */ >>> - u32 jedec_id; >>> + u64 jedec_id; >>> >>> /* The size listed here is what works with OP_ERASE_PAGE. */ >>> unsigned nr_pages; >>> @@ -713,63 +713,34 @@ static struct flash_info dataflash_data[] = { >>> * These newer chips also support 128-byte security registers (with >>> * 64 bytes one-time-programmable) and software write-protection. >>> */ >>> - { "AT45DB011B", 0x1f2200, 512, 264, 9, SUP_POW2PS}, >>> - { "at45db011d", 0x1f2200, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, >>> + { "AT45DB011B", 0x1f22000000, 512, 264, 9, SUP_POW2PS}, >>> + { "at45db011d", 0x1f22000000, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, >>> >>> - { "AT45DB021B", 0x1f2300, 1024, 264, 9, SUP_POW2PS}, >>> - { "at45db021d", 0x1f2300, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, >>> + { "AT45DB021B", 0x1f23000000, 1024, 264, 9, SUP_POW2PS}, >>> + { "at45db021d", 0x1f23000000, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, >>> >>> - { "AT45DB041x", 0x1f2400, 2048, 264, 9, SUP_POW2PS}, >>> - { "at45db041d", 0x1f2400, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, >>> + { "AT45DB041x", 0x1f24000000, 2048, 264, 9, SUP_POW2PS}, >>> + { "at45db041d", 0x1f24000000, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, >>> >>> - { "AT45DB081B", 0x1f2500, 4096, 264, 9, SUP_POW2PS}, >>> - { "at45db081d", 0x1f2500, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, >>> + { "AT45DB081B", 0x1f25000000, 4096, 264, 9, SUP_POW2PS}, >>> + { "at45db081d", 0x1f25000000, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, >>> >>> - { "AT45DB161x", 0x1f2600, 4096, 528, 10, SUP_POW2PS}, >>> - { "at45db161d", 0x1f2600, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, >>> + { "AT45DB161x", 0x1f26000000, 4096, 528, 10, SUP_POW2PS}, >>> + { "at45db161d", 0x1f26000000, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, >>> >>> - { "AT45DB321x", 0x1f2700, 8192, 528, 10, 0}, /* rev C */ >>> + { "AT45DB321x", 0x1f27000000, 8192, 528, 10, 0}, /* rev C */ >>> >>> - { "AT45DB321x", 0x1f2701, 8192, 528, 10, SUP_POW2PS}, >>> - { "at45db321d", 0x1f2701, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, >>> + { "AT45DB321x", 0x1f27010000, 8192, 528, 10, SUP_POW2PS}, >>> + { "at45db321d", 0x1f27010000, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, >>> >>> - { "AT45DB642x", 0x1f2800, 8192, 1056, 11, SUP_POW2PS}, >>> - { "at45db642d", 0x1f2800, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, >>> + { "AT45DB642x", 0x1f28000000, 8192, 1056, 11, SUP_POW2PS}, >>> + { "at45db642d", 0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, >>> }; >>> >>> -static struct flash_info *jedec_probe(struct spi_device *spi) >>> +static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec) >>> { >>> - int ret, i; >>> - u8 code = OP_READ_ID; >>> - u8 id[3]; >>> - u32 jedec; >>> + int status, i; >>> struct flash_info *info; >>> - int status; >>> - >>> - /* >>> - * JEDEC also defines an optional "extended device information" >>> - * string for after vendor-specific data, after the three bytes >>> - * we use here. Supporting some chips might require using it. >>> - * >>> - * If the vendor ID isn't Atmel's (0x1f), assume this call failed. >>> - * That's not an error; only rev C and newer chips handle it, and >>> - * only Atmel sells these chips. >>> - */ >>> - ret = spi_write_then_read(spi, &code, 1, id, 3); >>> - if (ret < 0) { >>> - pr_debug("%s: error %d reading JEDEC ID\n", >>> - dev_name(&spi->dev), ret); >>> - return ERR_PTR(ret); >>> - } >>> - >>> - if (id[0] != CFI_MFR_ATMEL) >>> - return NULL; >>> - >>> - jedec = id[0]; >>> - jedec = jedec << 8; >>> - jedec |= id[1]; >>> - jedec = jedec << 8; >>> - jedec |= id[2]; >> >> Hm, the diff again screwed this up, oh well ... >> >>> for (i = 0, info = dataflash_data; >>> i < ARRAY_SIZE(dataflash_data); >>> @@ -799,12 +770,58 @@ static struct flash_info *jedec_probe(struct spi_device *spi) >>> } >>> } >>> >>> + return NULL; >>> +} >>> + >>> +static struct flash_info *jedec_probe(struct spi_device *spi) >>> +{ >>> + int ret; >>> + u8 code = OP_READ_ID; >>> + u8 id[8] = {0}; >>> + const unsigned int id_size = 5; >>> + const unsigned int first_byte = sizeof(id) - id_size; >>> + const u64 eid_mask = GENMASK_ULL(63, 16); >>> + u64 jedec; >>> + struct flash_info *info; >>> + >>> + /* >>> + * JEDEC also defines an optional "extended device information" >>> + * string for after vendor-specific data, after the three bytes >>> + * we use here. Supporting some chips might require using it. >>> + * >>> + * If the vendor ID isn't Atmel's (0x1f), assume this call failed. >>> + * That's not an error; only rev C and newer chips handle it, and >>> + * only Atmel sells these chips. >>> + */ >>> + ret = spi_write_then_read(spi, &code, 1, &id[first_byte], id_size); >>> + if (ret < 0) { >>> + pr_debug("%s: error %d reading JEDEC ID\n", >>> + dev_name(&spi->dev), ret); >> >> I think you can turn this into: >> >> dev_err(&spi->dev, "error %d reading JEDEC ID\n", ret); > > OK, will do it in a separate patch, since this is original code. OK >>> + return ERR_PTR(ret); >>> + } >>> + >>> + if (id[first_byte] != CFI_MFR_ATMEL) >>> + return NULL; >>> + >>> + jedec = be64_to_cpup((__be64 *)id); >>> + >>> + info = jedec_lookup(spi, jedec); >>> + if (info) >>> + return info; >>> + >>> + /* Clear extended id bits and try to find a match again */ >>> + jedec &= eid_mask; >> >> Wouldn't that be jedec &= ~eid_mask; (with a tilde) if you want to clear >> them ? > > eid_mask is generated as bits 63 to 16 with GENMASK_ULL, so bits 15 to > 0 should already be zeroed out. I can rename it to eid_mask_inverted > so it's less confusing. I think that's a good idea and you can make the mask u32 instead of u64. But isn't eid_mask_inverted the same as id_mask ? -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information" 2017-04-19 8:47 ` Marek Vasut @ 2017-04-19 15:07 ` Andrey Smirnov 2017-04-19 15:32 ` Marek Vasut 0 siblings, 1 reply; 12+ messages in thread From: Andrey Smirnov @ 2017-04-19 15:07 UTC (permalink / raw) To: Marek Vasut Cc: linux-mtd, Chris Healy, David Woodhouse, Brian Norris, Boris Brezillon, Richard Weinberger, Cyrille Pitchen, linux-kernel On Wed, Apr 19, 2017 at 1:47 AM, Marek Vasut <marek.vasut@gmail.com> wrote: > On 04/19/2017 04:58 AM, Andrey Smirnov wrote: >> On Tue, Apr 18, 2017 at 11:31 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >>> On 04/18/2017 04:21 PM, Andrey Smirnov wrote: >>>> In anticipation of supporting chips that need it, extend the size of >>>> struct flash_info's 'jedec_id' field to make room 2 byte of extended >>>> device information as well as add code to fetch this data during >>>> jedec_probe(). >>>> >>>> Cc: cphealy@gmail.com >>>> Cc: David Woodhouse <dwmw2@infradead.org> >>>> Cc: Brian Norris <computersforpeace@gmail.com> >>>> Cc: Boris Brezillon <boris.brezillon@free-electrons.com> >>>> Cc: Marek Vasut <marek.vasut@gmail.com> >>>> Cc: Richard Weinberger <richard@nod.at> >>>> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com> >>>> Cc: linux-kernel@vger.kernel.org >>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >>>> --- >>>> >>>> Changes since [v1]: >>>> >>>> - Formatting >>>> >>>> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com >>>> >>>> >>>> drivers/mtd/devices/mtd_dataflash.c | 113 +++++++++++++++++++++--------------- >>>> 1 file changed, 65 insertions(+), 48 deletions(-) >>>> >>>> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c >>>> index 5b7a8c3..5d694a4 100644 >>>> --- a/drivers/mtd/devices/mtd_dataflash.c >>>> +++ b/drivers/mtd/devices/mtd_dataflash.c >>>> @@ -690,7 +690,7 @@ struct flash_info { >>>> /* JEDEC id has a high byte of zero plus three data bytes: >>>> * the manufacturer id, then a two byte device id. >>>> */ >>>> - u32 jedec_id; >>>> + u64 jedec_id; >>>> >>>> /* The size listed here is what works with OP_ERASE_PAGE. */ >>>> unsigned nr_pages; >>>> @@ -713,63 +713,34 @@ static struct flash_info dataflash_data[] = { >>>> * These newer chips also support 128-byte security registers (with >>>> * 64 bytes one-time-programmable) and software write-protection. >>>> */ >>>> - { "AT45DB011B", 0x1f2200, 512, 264, 9, SUP_POW2PS}, >>>> - { "at45db011d", 0x1f2200, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>> + { "AT45DB011B", 0x1f22000000, 512, 264, 9, SUP_POW2PS}, >>>> + { "at45db011d", 0x1f22000000, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>> >>>> - { "AT45DB021B", 0x1f2300, 1024, 264, 9, SUP_POW2PS}, >>>> - { "at45db021d", 0x1f2300, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>> + { "AT45DB021B", 0x1f23000000, 1024, 264, 9, SUP_POW2PS}, >>>> + { "at45db021d", 0x1f23000000, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>> >>>> - { "AT45DB041x", 0x1f2400, 2048, 264, 9, SUP_POW2PS}, >>>> - { "at45db041d", 0x1f2400, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>> + { "AT45DB041x", 0x1f24000000, 2048, 264, 9, SUP_POW2PS}, >>>> + { "at45db041d", 0x1f24000000, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>> >>>> - { "AT45DB081B", 0x1f2500, 4096, 264, 9, SUP_POW2PS}, >>>> - { "at45db081d", 0x1f2500, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>> + { "AT45DB081B", 0x1f25000000, 4096, 264, 9, SUP_POW2PS}, >>>> + { "at45db081d", 0x1f25000000, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>> >>>> - { "AT45DB161x", 0x1f2600, 4096, 528, 10, SUP_POW2PS}, >>>> - { "at45db161d", 0x1f2600, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, >>>> + { "AT45DB161x", 0x1f26000000, 4096, 528, 10, SUP_POW2PS}, >>>> + { "at45db161d", 0x1f26000000, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, >>>> >>>> - { "AT45DB321x", 0x1f2700, 8192, 528, 10, 0}, /* rev C */ >>>> + { "AT45DB321x", 0x1f27000000, 8192, 528, 10, 0}, /* rev C */ >>>> >>>> - { "AT45DB321x", 0x1f2701, 8192, 528, 10, SUP_POW2PS}, >>>> - { "at45db321d", 0x1f2701, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, >>>> + { "AT45DB321x", 0x1f27010000, 8192, 528, 10, SUP_POW2PS}, >>>> + { "at45db321d", 0x1f27010000, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, >>>> >>>> - { "AT45DB642x", 0x1f2800, 8192, 1056, 11, SUP_POW2PS}, >>>> - { "at45db642d", 0x1f2800, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, >>>> + { "AT45DB642x", 0x1f28000000, 8192, 1056, 11, SUP_POW2PS}, >>>> + { "at45db642d", 0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, >>>> }; >>>> >>>> -static struct flash_info *jedec_probe(struct spi_device *spi) >>>> +static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec) >>>> { >>>> - int ret, i; >>>> - u8 code = OP_READ_ID; >>>> - u8 id[3]; >>>> - u32 jedec; >>>> + int status, i; >>>> struct flash_info *info; >>>> - int status; >>>> - >>>> - /* >>>> - * JEDEC also defines an optional "extended device information" >>>> - * string for after vendor-specific data, after the three bytes >>>> - * we use here. Supporting some chips might require using it. >>>> - * >>>> - * If the vendor ID isn't Atmel's (0x1f), assume this call failed. >>>> - * That's not an error; only rev C and newer chips handle it, and >>>> - * only Atmel sells these chips. >>>> - */ >>>> - ret = spi_write_then_read(spi, &code, 1, id, 3); >>>> - if (ret < 0) { >>>> - pr_debug("%s: error %d reading JEDEC ID\n", >>>> - dev_name(&spi->dev), ret); >>>> - return ERR_PTR(ret); >>>> - } >>>> - >>>> - if (id[0] != CFI_MFR_ATMEL) >>>> - return NULL; >>>> - >>>> - jedec = id[0]; >>>> - jedec = jedec << 8; >>>> - jedec |= id[1]; >>>> - jedec = jedec << 8; >>>> - jedec |= id[2]; >>> >>> Hm, the diff again screwed this up, oh well ... >>> >>>> for (i = 0, info = dataflash_data; >>>> i < ARRAY_SIZE(dataflash_data); >>>> @@ -799,12 +770,58 @@ static struct flash_info *jedec_probe(struct spi_device *spi) >>>> } >>>> } >>>> >>>> + return NULL; >>>> +} >>>> + >>>> +static struct flash_info *jedec_probe(struct spi_device *spi) >>>> +{ >>>> + int ret; >>>> + u8 code = OP_READ_ID; >>>> + u8 id[8] = {0}; >>>> + const unsigned int id_size = 5; >>>> + const unsigned int first_byte = sizeof(id) - id_size; >>>> + const u64 eid_mask = GENMASK_ULL(63, 16); >>>> + u64 jedec; >>>> + struct flash_info *info; >>>> + >>>> + /* >>>> + * JEDEC also defines an optional "extended device information" >>>> + * string for after vendor-specific data, after the three bytes >>>> + * we use here. Supporting some chips might require using it. >>>> + * >>>> + * If the vendor ID isn't Atmel's (0x1f), assume this call failed. >>>> + * That's not an error; only rev C and newer chips handle it, and >>>> + * only Atmel sells these chips. >>>> + */ >>>> + ret = spi_write_then_read(spi, &code, 1, &id[first_byte], id_size); >>>> + if (ret < 0) { >>>> + pr_debug("%s: error %d reading JEDEC ID\n", >>>> + dev_name(&spi->dev), ret); >>> >>> I think you can turn this into: >>> >>> dev_err(&spi->dev, "error %d reading JEDEC ID\n", ret); >> >> OK, will do it in a separate patch, since this is original code. > > OK > >>>> + return ERR_PTR(ret); >>>> + } >>>> + >>>> + if (id[first_byte] != CFI_MFR_ATMEL) >>>> + return NULL; >>>> + >>>> + jedec = be64_to_cpup((__be64 *)id); >>>> + >>>> + info = jedec_lookup(spi, jedec); >>>> + if (info) >>>> + return info; >>>> + >>>> + /* Clear extended id bits and try to find a match again */ >>>> + jedec &= eid_mask; >>> >>> Wouldn't that be jedec &= ~eid_mask; (with a tilde) if you want to clear >>> them ? >> >> eid_mask is generated as bits 63 to 16 with GENMASK_ULL, so bits 15 to >> 0 should already be zeroed out. I can rename it to eid_mask_inverted >> so it's less confusing. > > I think that's a good idea and you can make the mask u32 instead of u64. > But isn't eid_mask_inverted the same as id_mask ? I just realized that "eid_mask" is used only in one place and I don't gain anything by having that variable, so I think what I'd do is use GENMASK in-place, then it should be clear why there's no bit inversion and there's no dilemma on how to name the variable. Thanks, Andrey Smirnov ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information" 2017-04-19 15:07 ` Andrey Smirnov @ 2017-04-19 15:32 ` Marek Vasut 0 siblings, 0 replies; 12+ messages in thread From: Marek Vasut @ 2017-04-19 15:32 UTC (permalink / raw) To: Andrey Smirnov Cc: linux-mtd, Chris Healy, David Woodhouse, Brian Norris, Boris Brezillon, Richard Weinberger, Cyrille Pitchen, linux-kernel On 04/19/2017 05:07 PM, Andrey Smirnov wrote: > On Wed, Apr 19, 2017 at 1:47 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >> On 04/19/2017 04:58 AM, Andrey Smirnov wrote: >>> On Tue, Apr 18, 2017 at 11:31 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >>>> On 04/18/2017 04:21 PM, Andrey Smirnov wrote: >>>>> In anticipation of supporting chips that need it, extend the size of >>>>> struct flash_info's 'jedec_id' field to make room 2 byte of extended >>>>> device information as well as add code to fetch this data during >>>>> jedec_probe(). >>>>> >>>>> Cc: cphealy@gmail.com >>>>> Cc: David Woodhouse <dwmw2@infradead.org> >>>>> Cc: Brian Norris <computersforpeace@gmail.com> >>>>> Cc: Boris Brezillon <boris.brezillon@free-electrons.com> >>>>> Cc: Marek Vasut <marek.vasut@gmail.com> >>>>> Cc: Richard Weinberger <richard@nod.at> >>>>> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com> >>>>> Cc: linux-kernel@vger.kernel.org >>>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >>>>> --- >>>>> >>>>> Changes since [v1]: >>>>> >>>>> - Formatting >>>>> >>>>> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com >>>>> >>>>> >>>>> drivers/mtd/devices/mtd_dataflash.c | 113 +++++++++++++++++++++--------------- >>>>> 1 file changed, 65 insertions(+), 48 deletions(-) >>>>> >>>>> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c >>>>> index 5b7a8c3..5d694a4 100644 >>>>> --- a/drivers/mtd/devices/mtd_dataflash.c >>>>> +++ b/drivers/mtd/devices/mtd_dataflash.c >>>>> @@ -690,7 +690,7 @@ struct flash_info { >>>>> /* JEDEC id has a high byte of zero plus three data bytes: >>>>> * the manufacturer id, then a two byte device id. >>>>> */ >>>>> - u32 jedec_id; >>>>> + u64 jedec_id; >>>>> >>>>> /* The size listed here is what works with OP_ERASE_PAGE. */ >>>>> unsigned nr_pages; >>>>> @@ -713,63 +713,34 @@ static struct flash_info dataflash_data[] = { >>>>> * These newer chips also support 128-byte security registers (with >>>>> * 64 bytes one-time-programmable) and software write-protection. >>>>> */ >>>>> - { "AT45DB011B", 0x1f2200, 512, 264, 9, SUP_POW2PS}, >>>>> - { "at45db011d", 0x1f2200, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>>> + { "AT45DB011B", 0x1f22000000, 512, 264, 9, SUP_POW2PS}, >>>>> + { "at45db011d", 0x1f22000000, 512, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>>> >>>>> - { "AT45DB021B", 0x1f2300, 1024, 264, 9, SUP_POW2PS}, >>>>> - { "at45db021d", 0x1f2300, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>>> + { "AT45DB021B", 0x1f23000000, 1024, 264, 9, SUP_POW2PS}, >>>>> + { "at45db021d", 0x1f23000000, 1024, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>>> >>>>> - { "AT45DB041x", 0x1f2400, 2048, 264, 9, SUP_POW2PS}, >>>>> - { "at45db041d", 0x1f2400, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>>> + { "AT45DB041x", 0x1f24000000, 2048, 264, 9, SUP_POW2PS}, >>>>> + { "at45db041d", 0x1f24000000, 2048, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>>> >>>>> - { "AT45DB081B", 0x1f2500, 4096, 264, 9, SUP_POW2PS}, >>>>> - { "at45db081d", 0x1f2500, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>>> + { "AT45DB081B", 0x1f25000000, 4096, 264, 9, SUP_POW2PS}, >>>>> + { "at45db081d", 0x1f25000000, 4096, 256, 8, SUP_POW2PS | IS_POW2PS}, >>>>> >>>>> - { "AT45DB161x", 0x1f2600, 4096, 528, 10, SUP_POW2PS}, >>>>> - { "at45db161d", 0x1f2600, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, >>>>> + { "AT45DB161x", 0x1f26000000, 4096, 528, 10, SUP_POW2PS}, >>>>> + { "at45db161d", 0x1f26000000, 4096, 512, 9, SUP_POW2PS | IS_POW2PS}, >>>>> >>>>> - { "AT45DB321x", 0x1f2700, 8192, 528, 10, 0}, /* rev C */ >>>>> + { "AT45DB321x", 0x1f27000000, 8192, 528, 10, 0}, /* rev C */ >>>>> >>>>> - { "AT45DB321x", 0x1f2701, 8192, 528, 10, SUP_POW2PS}, >>>>> - { "at45db321d", 0x1f2701, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, >>>>> + { "AT45DB321x", 0x1f27010000, 8192, 528, 10, SUP_POW2PS}, >>>>> + { "at45db321d", 0x1f27010000, 8192, 512, 9, SUP_POW2PS | IS_POW2PS}, >>>>> >>>>> - { "AT45DB642x", 0x1f2800, 8192, 1056, 11, SUP_POW2PS}, >>>>> - { "at45db642d", 0x1f2800, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, >>>>> + { "AT45DB642x", 0x1f28000000, 8192, 1056, 11, SUP_POW2PS}, >>>>> + { "at45db642d", 0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS}, >>>>> }; >>>>> >>>>> -static struct flash_info *jedec_probe(struct spi_device *spi) >>>>> +static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec) >>>>> { >>>>> - int ret, i; >>>>> - u8 code = OP_READ_ID; >>>>> - u8 id[3]; >>>>> - u32 jedec; >>>>> + int status, i; >>>>> struct flash_info *info; >>>>> - int status; >>>>> - >>>>> - /* >>>>> - * JEDEC also defines an optional "extended device information" >>>>> - * string for after vendor-specific data, after the three bytes >>>>> - * we use here. Supporting some chips might require using it. >>>>> - * >>>>> - * If the vendor ID isn't Atmel's (0x1f), assume this call failed. >>>>> - * That's not an error; only rev C and newer chips handle it, and >>>>> - * only Atmel sells these chips. >>>>> - */ >>>>> - ret = spi_write_then_read(spi, &code, 1, id, 3); >>>>> - if (ret < 0) { >>>>> - pr_debug("%s: error %d reading JEDEC ID\n", >>>>> - dev_name(&spi->dev), ret); >>>>> - return ERR_PTR(ret); >>>>> - } >>>>> - >>>>> - if (id[0] != CFI_MFR_ATMEL) >>>>> - return NULL; >>>>> - >>>>> - jedec = id[0]; >>>>> - jedec = jedec << 8; >>>>> - jedec |= id[1]; >>>>> - jedec = jedec << 8; >>>>> - jedec |= id[2]; >>>> >>>> Hm, the diff again screwed this up, oh well ... >>>> >>>>> for (i = 0, info = dataflash_data; >>>>> i < ARRAY_SIZE(dataflash_data); >>>>> @@ -799,12 +770,58 @@ static struct flash_info *jedec_probe(struct spi_device *spi) >>>>> } >>>>> } >>>>> >>>>> + return NULL; >>>>> +} >>>>> + >>>>> +static struct flash_info *jedec_probe(struct spi_device *spi) >>>>> +{ >>>>> + int ret; >>>>> + u8 code = OP_READ_ID; >>>>> + u8 id[8] = {0}; >>>>> + const unsigned int id_size = 5; >>>>> + const unsigned int first_byte = sizeof(id) - id_size; >>>>> + const u64 eid_mask = GENMASK_ULL(63, 16); >>>>> + u64 jedec; >>>>> + struct flash_info *info; >>>>> + >>>>> + /* >>>>> + * JEDEC also defines an optional "extended device information" >>>>> + * string for after vendor-specific data, after the three bytes >>>>> + * we use here. Supporting some chips might require using it. >>>>> + * >>>>> + * If the vendor ID isn't Atmel's (0x1f), assume this call failed. >>>>> + * That's not an error; only rev C and newer chips handle it, and >>>>> + * only Atmel sells these chips. >>>>> + */ >>>>> + ret = spi_write_then_read(spi, &code, 1, &id[first_byte], id_size); >>>>> + if (ret < 0) { >>>>> + pr_debug("%s: error %d reading JEDEC ID\n", >>>>> + dev_name(&spi->dev), ret); >>>> >>>> I think you can turn this into: >>>> >>>> dev_err(&spi->dev, "error %d reading JEDEC ID\n", ret); >>> >>> OK, will do it in a separate patch, since this is original code. >> >> OK >> >>>>> + return ERR_PTR(ret); >>>>> + } >>>>> + >>>>> + if (id[first_byte] != CFI_MFR_ATMEL) >>>>> + return NULL; >>>>> + >>>>> + jedec = be64_to_cpup((__be64 *)id); >>>>> + >>>>> + info = jedec_lookup(spi, jedec); >>>>> + if (info) >>>>> + return info; >>>>> + >>>>> + /* Clear extended id bits and try to find a match again */ >>>>> + jedec &= eid_mask; >>>> >>>> Wouldn't that be jedec &= ~eid_mask; (with a tilde) if you want to clear >>>> them ? >>> >>> eid_mask is generated as bits 63 to 16 with GENMASK_ULL, so bits 15 to >>> 0 should already be zeroed out. I can rename it to eid_mask_inverted >>> so it's less confusing. >> >> I think that's a good idea and you can make the mask u32 instead of u64. >> But isn't eid_mask_inverted the same as id_mask ? > > I just realized that "eid_mask" is used only in one place and I don't > gain anything by having that variable, so I think what I'd do is use > GENMASK in-place, then it should be clear why there's no bit inversion > and there's no dilemma on how to name the variable. The upside of defining mask as a macro is that you can derive the meaning of the macro from it's name . If you use ad-hoc constant in the code, you cannot do that. And there might be spots in the code which will re-use that macro in the future. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts 2017-04-18 14:21 [PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts Andrey Smirnov 2017-04-18 14:21 ` [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe() Andrey Smirnov 2017-04-18 14:21 ` [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information" Andrey Smirnov @ 2017-04-18 18:24 ` Marek Vasut 2 siblings, 0 replies; 12+ messages in thread From: Marek Vasut @ 2017-04-18 18:24 UTC (permalink / raw) To: Andrey Smirnov, linux-mtd Cc: cphealy, David Woodhouse, Brian Norris, Boris Brezillon, Richard Weinberger, Cyrille Pitchen, linux-kernel On 04/18/2017 04:21 PM, Andrey Smirnov wrote: > Replace C99 type with their kernel counterparts as per request from > Marek Vasut. > > No functional change intended. > > Cc: cphealy@gmail.com > Cc: David Woodhouse <dwmw2@infradead.org> > Cc: Brian Norris <computersforpeace@gmail.com> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Marek Vasut <marek.vasut@gmail.com> > Cc: Richard Weinberger <richard@nod.at> > Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com> > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> Nice, thanks a lot ! Acked-by: Marek Vasut <marek.vasut@gmail.com> -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-04-19 15:32 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-18 14:21 [PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts Andrey Smirnov 2017-04-18 14:21 ` [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe() Andrey Smirnov 2017-04-18 18:25 ` Marek Vasut 2017-04-18 22:36 ` Joe Perches 2017-04-18 22:38 ` Marek Vasut 2017-04-18 14:21 ` [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information" Andrey Smirnov 2017-04-18 18:31 ` Marek Vasut 2017-04-19 2:58 ` Andrey Smirnov 2017-04-19 8:47 ` Marek Vasut 2017-04-19 15:07 ` Andrey Smirnov 2017-04-19 15:32 ` Marek Vasut 2017-04-18 18:24 ` [PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts Marek Vasut
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.