Replace C99 type with their kernel counterparts as per request from
Marek Vasut.
No functional change intended.
Cc: [email protected]
Cc: David Woodhouse <[email protected]>
Cc: Brian Norris <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: Cyrille Pitchen <[email protected]>
Cc: [email protected]
Signed-off-by: Andrey Smirnov <[email protected]>
---
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
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: [email protected]
Cc: David Woodhouse <[email protected]>
Cc: Brian Norris <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: Cyrille Pitchen <[email protected]>
Cc: [email protected]
Signed-off-by: Andrey Smirnov <[email protected]>
---
Changes since [v1]:
- Formatting
[v1] http://lkml.kernel.org/r/[email protected]
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
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: [email protected]
Cc: David Woodhouse <[email protected]>
Cc: Brian Norris <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: Cyrille Pitchen <[email protected]>
Cc: [email protected]
Signed-off-by: Andrey Smirnov <[email protected]>
---
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
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: [email protected]
> Cc: David Woodhouse <[email protected]>
> Cc: Brian Norris <[email protected]>
> Cc: Boris Brezillon <[email protected]>
> Cc: Marek Vasut <[email protected]>
> Cc: Richard Weinberger <[email protected]>
> Cc: Cyrille Pitchen <[email protected]>
> Cc: [email protected]
> Signed-off-by: Andrey Smirnov <[email protected]>
Nice, thanks a lot !
Acked-by: Marek Vasut <[email protected]>
--
Best regards,
Marek Vasut
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: [email protected]
> Cc: David Woodhouse <[email protected]>
> Cc: Brian Norris <[email protected]>
> Cc: Boris Brezillon <[email protected]>
> Cc: Marek Vasut <[email protected]>
> Cc: Richard Weinberger <[email protected]>
> Cc: Cyrille Pitchen <[email protected]>
> Cc: [email protected]
> Signed-off-by: Andrey Smirnov <[email protected]>
> ---
>
> Changes since [v1]:
>
> - Formatting
>
> [v1] http://lkml.kernel.org/r/[email protected]
>
>
> 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
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 <[email protected]>
> Cc: [email protected]
> Cc: David Woodhouse <[email protected]>
> Cc: Brian Norris <[email protected]>
> Cc: Boris Brezillon <[email protected]>
> Cc: Marek Vasut <[email protected]>
> Cc: Richard Weinberger <[email protected]>
> Cc: Cyrille Pitchen <[email protected]>
> Cc: [email protected]
> Signed-off-by: Andrey Smirnov <[email protected]>
> ---
> 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
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++) {
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
On Tue, Apr 18, 2017 at 11:31 AM, Marek Vasut <[email protected]> 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: [email protected]
>> Cc: David Woodhouse <[email protected]>
>> Cc: Brian Norris <[email protected]>
>> Cc: Boris Brezillon <[email protected]>
>> Cc: Marek Vasut <[email protected]>
>> Cc: Richard Weinberger <[email protected]>
>> Cc: Cyrille Pitchen <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Andrey Smirnov <[email protected]>
>> ---
>>
>> Changes since [v1]:
>>
>> - Formatting
>>
>> [v1] http://lkml.kernel.org/r/[email protected]
>>
>>
>> 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
On 04/19/2017 04:58 AM, Andrey Smirnov wrote:
> On Tue, Apr 18, 2017 at 11:31 AM, Marek Vasut <[email protected]> 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: [email protected]
>>> Cc: David Woodhouse <[email protected]>
>>> Cc: Brian Norris <[email protected]>
>>> Cc: Boris Brezillon <[email protected]>
>>> Cc: Marek Vasut <[email protected]>
>>> Cc: Richard Weinberger <[email protected]>
>>> Cc: Cyrille Pitchen <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: Andrey Smirnov <[email protected]>
>>> ---
>>>
>>> Changes since [v1]:
>>>
>>> - Formatting
>>>
>>> [v1] http://lkml.kernel.org/r/[email protected]
>>>
>>>
>>> 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
On Wed, Apr 19, 2017 at 1:47 AM, Marek Vasut <[email protected]> wrote:
> On 04/19/2017 04:58 AM, Andrey Smirnov wrote:
>> On Tue, Apr 18, 2017 at 11:31 AM, Marek Vasut <[email protected]> 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: [email protected]
>>>> Cc: David Woodhouse <[email protected]>
>>>> Cc: Brian Norris <[email protected]>
>>>> Cc: Boris Brezillon <[email protected]>
>>>> Cc: Marek Vasut <[email protected]>
>>>> Cc: Richard Weinberger <[email protected]>
>>>> Cc: Cyrille Pitchen <[email protected]>
>>>> Cc: [email protected]
>>>> Signed-off-by: Andrey Smirnov <[email protected]>
>>>> ---
>>>>
>>>> Changes since [v1]:
>>>>
>>>> - Formatting
>>>>
>>>> [v1] http://lkml.kernel.org/r/[email protected]
>>>>
>>>>
>>>> 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
On 04/19/2017 05:07 PM, Andrey Smirnov wrote:
> On Wed, Apr 19, 2017 at 1:47 AM, Marek Vasut <[email protected]> wrote:
>> On 04/19/2017 04:58 AM, Andrey Smirnov wrote:
>>> On Tue, Apr 18, 2017 at 11:31 AM, Marek Vasut <[email protected]> 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: [email protected]
>>>>> Cc: David Woodhouse <[email protected]>
>>>>> Cc: Brian Norris <[email protected]>
>>>>> Cc: Boris Brezillon <[email protected]>
>>>>> Cc: Marek Vasut <[email protected]>
>>>>> Cc: Richard Weinberger <[email protected]>
>>>>> Cc: Cyrille Pitchen <[email protected]>
>>>>> Cc: [email protected]
>>>>> Signed-off-by: Andrey Smirnov <[email protected]>
>>>>> ---
>>>>>
>>>>> Changes since [v1]:
>>>>>
>>>>> - Formatting
>>>>>
>>>>> [v1] http://lkml.kernel.org/r/[email protected]
>>>>>
>>>>>
>>>>> 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