[PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts

[PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts

[Andrey Smirnov]

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

[PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe()

[Andrey Smirnov]

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

[PATCH v2 3/3] mtd: dataflash: Make use of "extened device information"

[Andrey Smirnov]

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

Re: [PATCH v2 1/3] mtd: dataflash: Replace C99 type with their kernel counterparts

[Marek Vasut]

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

Re: [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe()

[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 <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

Re: [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information"

[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: 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

Re: [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe()

[Joe Perches]

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++) {

Re: [PATCH v2 2/3] mtd: dataflash: Improve coding style in jedec_probe()

[Marek Vasut]

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

Re: [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information"

[Andrey Smirnov]

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

Re: [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information"

[Marek Vasut]

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

Re: [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information"

[Andrey Smirnov]

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

Re: [PATCH v2 3/3] mtd: dataflash: Make use of "extened device information"

[Marek Vasut]

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