linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts
@ 2017-04-19 15:23 Andrey Smirnov
  2017-04-19 15:23 ` [PATCH v3 2/6] mtd: dataflash: Improve coding style in jedec_probe() Andrey Smirnov
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Andrey Smirnov @ 2017-04-19 15:23 UTC (permalink / raw)
  To: linux-mtd
  Cc: Andrey Smirnov, cphealy, David Woodhouse, Brian Norris,
	Boris Brezillon, Richard Weinberger, Cyrille Pitchen,
	linux-kernel

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: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Cc: linux-kernel@vger.kernel.org
Acked-by: Marek Vasut <marek.vasut@gmail.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---

Changes since [v2]:

	- Re-worded commit message

	- Collected Acked-by from Marek

Not present in v1

[v2]: http://lkml.kernel.org/r/20170418142127.23301-1-andrew.smirnov@gmail.com


 drivers/mtd/devices/mtd_dataflash.c | 40 ++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index f9e9bd1..a566231 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -84,7 +84,7 @@
 
 
 struct dataflash {
-	uint8_t			command[4];
+	u8			command[4];
 	char			name[24];
 
 	unsigned short		page_offset;	/* offset in flash address */
@@ -153,8 +153,8 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
 	struct spi_transfer	x = { };
 	struct spi_message	msg;
 	unsigned		blocksize = priv->page_size << 3;
-	uint8_t			*command;
-	uint32_t		rem;
+	u8			*command;
+	u32			rem;
 
 	pr_debug("%s: erase addr=0x%llx len 0x%llx\n",
 	      dev_name(&spi->dev), (long long)instr->addr,
@@ -187,8 +187,8 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
 		pageaddr = pageaddr << priv->page_offset;
 
 		command[0] = do_block ? OP_ERASE_BLOCK : OP_ERASE_PAGE;
-		command[1] = (uint8_t)(pageaddr >> 16);
-		command[2] = (uint8_t)(pageaddr >> 8);
+		command[1] = (u8)(pageaddr >> 16);
+		command[2] = (u8)(pageaddr >> 8);
 		command[3] = 0;
 
 		pr_debug("ERASE %s: (%x) %x %x %x [%i]\n",
@@ -239,7 +239,7 @@ static int dataflash_read(struct mtd_info *mtd, loff_t from, size_t len,
 	struct spi_transfer	x[2] = { };
 	struct spi_message	msg;
 	unsigned int		addr;
-	uint8_t			*command;
+	u8			*command;
 	int			status;
 
 	pr_debug("%s: read 0x%x..0x%x\n", dev_name(&priv->spi->dev),
@@ -271,9 +271,9 @@ static int dataflash_read(struct mtd_info *mtd, loff_t from, size_t len,
 	 * fewer "don't care" bytes.  Both buffers stay unchanged.
 	 */
 	command[0] = OP_READ_CONTINUOUS;
-	command[1] = (uint8_t)(addr >> 16);
-	command[2] = (uint8_t)(addr >> 8);
-	command[3] = (uint8_t)(addr >> 0);
+	command[1] = (u8)(addr >> 16);
+	command[2] = (u8)(addr >> 8);
+	command[3] = (u8)(addr >> 0);
 	/* plus 4 "don't care" bytes */
 
 	status = spi_sync(priv->spi, &msg);
@@ -308,7 +308,7 @@ static int dataflash_write(struct mtd_info *mtd, loff_t to, size_t len,
 	size_t			remaining = len;
 	u_char			*writebuf = (u_char *) buf;
 	int			status = -EINVAL;
-	uint8_t			*command;
+	u8			*command;
 
 	pr_debug("%s: write 0x%x..0x%x\n",
 		dev_name(&spi->dev), (unsigned)to, (unsigned)(to + len));
@@ -455,11 +455,11 @@ static int dataflash_get_otp_info(struct mtd_info *mtd, size_t len,
 }
 
 static ssize_t otp_read(struct spi_device *spi, unsigned base,
-		uint8_t *buf, loff_t off, size_t len)
+		u8 *buf, loff_t off, size_t len)
 {
 	struct spi_message	m;
 	size_t			l;
-	uint8_t			*scratch;
+	u8			*scratch;
 	struct spi_transfer	t;
 	int			status;
 
@@ -538,7 +538,7 @@ static int dataflash_write_user_otp(struct mtd_info *mtd,
 {
 	struct spi_message	m;
 	const size_t		l = 4 + 64;
-	uint8_t			*scratch;
+	u8			*scratch;
 	struct spi_transfer	t;
 	struct dataflash	*priv = mtd->priv;
 	int			status;
@@ -689,14 +689,14 @@ struct flash_info {
 	/* JEDEC id has a high byte of zero plus three data bytes:
 	 * the manufacturer id, then a two byte device id.
 	 */
-	uint32_t	jedec_id;
+	u32		jedec_id;
 
 	/* The size listed here is what works with OP_ERASE_PAGE. */
 	unsigned	nr_pages;
-	uint16_t	pagesize;
-	uint16_t	pageoffset;
+	u16		pagesize;
+	u16		pageoffset;
 
-	uint16_t	flags;
+	u16		flags;
 #define SUP_POW2PS	0x0002		/* supports 2^N byte pages */
 #define IS_POW2PS	0x0001		/* uses 2^N byte pages */
 };
@@ -739,9 +739,9 @@ static struct flash_info dataflash_data[] = {
 static struct flash_info *jedec_probe(struct spi_device *spi)
 {
 	int			tmp;
-	uint8_t			code = OP_READ_ID;
-	uint8_t			id[3];
-	uint32_t		jedec;
+	u8			code = OP_READ_ID;
+	u8			id[3];
+	u32			jedec;
 	struct flash_info	*info;
 	int status;
 
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 2/6] mtd: dataflash: Improve coding style in jedec_probe()
  2017-04-19 15:23 [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts Andrey Smirnov
@ 2017-04-19 15:23 ` Andrey Smirnov
  2017-04-19 15:23 ` [PATCH v3 4/6] mtd: dataflash: Get rid of loop counter " Andrey Smirnov
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Andrey Smirnov @ 2017-04-19 15:23 UTC (permalink / raw)
  To: linux-mtd
  Cc: Andrey Smirnov, cphealy, David Woodhouse, Brian Norris,
	Boris Brezillon, Richard Weinberger, Cyrille Pitchen,
	linux-kernel

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: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Cc: linux-kernel@vger.kernel.org
Acked-by: Marek Vasut <marek.vasut@gmail.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---

Changes since [v2]:

	- Re-worded commit message

	- Collected Acked-by from Marek

Not present in v1

[v2] http://lkml.kernel.org/r/20170418142127.23301-2-andrew.smirnov@gmail.com

 drivers/mtd/devices/mtd_dataflash.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index a566231..5b7a8c3 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -82,6 +82,7 @@
 #define OP_WRITE_SECURITY_REVC	0x9A
 #define OP_WRITE_SECURITY	0x9B	/* revision D */
 
+#define CFI_MFR_ATMEL		0x1F
 
 struct dataflash {
 	u8			command[4];
@@ -738,14 +739,15 @@ static struct flash_info dataflash_data[] = {
 
 static struct flash_info *jedec_probe(struct spi_device *spi)
 {
-	int			tmp;
-	u8			code = OP_READ_ID;
-	u8			id[3];
-	u32			jedec;
-	struct flash_info	*info;
+	int ret, i;
+	u8 code = OP_READ_ID;
+	u8 id[3];
+	u32 jedec;
+	struct flash_info *info;
 	int status;
 
-	/* JEDEC also defines an optional "extended device information"
+	/*
+	 * JEDEC also defines an optional "extended device information"
 	 * string for after vendor-specific data, after the three bytes
 	 * we use here.  Supporting some chips might require using it.
 	 *
@@ -753,13 +755,14 @@ static struct flash_info *jedec_probe(struct spi_device *spi)
 	 * That's not an error; only rev C and newer chips handle it, and
 	 * only Atmel sells these chips.
 	 */
-	tmp = spi_write_then_read(spi, &code, 1, id, 3);
-	if (tmp < 0) {
+	ret = spi_write_then_read(spi, &code, 1, id, 3);
+	if (ret < 0) {
 		pr_debug("%s: error %d reading JEDEC ID\n",
-			dev_name(&spi->dev), tmp);
-		return ERR_PTR(tmp);
+			dev_name(&spi->dev), ret);
+		return ERR_PTR(ret);
 	}
-	if (id[0] != 0x1f)
+
+	if (id[0] != CFI_MFR_ATMEL)
 		return NULL;
 
 	jedec = id[0];
@@ -768,9 +771,9 @@ static struct flash_info *jedec_probe(struct spi_device *spi)
 	jedec = jedec << 8;
 	jedec |= id[2];
 
-	for (tmp = 0, info = dataflash_data;
-			tmp < ARRAY_SIZE(dataflash_data);
-			tmp++, info++) {
+	for (i = 0, info = dataflash_data;
+			i < ARRAY_SIZE(dataflash_data);
+			i++, info++) {
 		if (info->jedec_id == jedec) {
 			pr_debug("%s: OTP, sector protect%s\n",
 				dev_name(&spi->dev),
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 4/6] mtd: dataflash: Get rid of loop counter in jedec_probe()
  2017-04-19 15:23 [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts Andrey Smirnov
  2017-04-19 15:23 ` [PATCH v3 2/6] mtd: dataflash: Improve coding style in jedec_probe() Andrey Smirnov
@ 2017-04-19 15:23 ` Andrey Smirnov
  2017-04-19 15:34   ` Marek Vasut
  2017-04-19 15:23 ` [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information" Andrey Smirnov
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Andrey Smirnov @ 2017-04-19 15:23 UTC (permalink / raw)
  To: linux-mtd
  Cc: Andrey Smirnov, cphealy, David Woodhouse, Brian Norris,
	Boris Brezillon, Marek Vasut, Richard Weinberger,
	Cyrille Pitchen, linux-kernel

"For" loop in jedec_probe can be simplified to not need counter
'i'. Convert the code and get rid of the variable.

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

Not present in v2, v1.

 drivers/mtd/devices/mtd_dataflash.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index ccd1e02..2d3e403 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -736,7 +736,7 @@ static struct flash_info dataflash_data[] = {
 
 static struct flash_info *jedec_probe(struct spi_device *spi)
 {
-	int ret, i;
+	int ret;
 	u8 code = OP_READ_ID;
 	u8 id[3];
 	u32 jedec;
@@ -767,9 +767,9 @@ static struct flash_info *jedec_probe(struct spi_device *spi)
 	jedec = jedec << 8;
 	jedec |= id[2];
 
-	for (i = 0, info = dataflash_data;
-			i < ARRAY_SIZE(dataflash_data);
-			i++, info++) {
+	for (info = dataflash_data;
+	     info < dataflash_data + ARRAY_SIZE(dataflash_data);
+	     info++) {
 		if (info->jedec_id == jedec) {
 			dev_dbg(&spi->dev, "OTP, sector protect%s\n",
 				(info->flags & SUP_POW2PS) ?
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"
  2017-04-19 15:23 [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts Andrey Smirnov
  2017-04-19 15:23 ` [PATCH v3 2/6] mtd: dataflash: Improve coding style in jedec_probe() Andrey Smirnov
  2017-04-19 15:23 ` [PATCH v3 4/6] mtd: dataflash: Get rid of loop counter " Andrey Smirnov
@ 2017-04-19 15:23 ` Andrey Smirnov
  2017-04-19 15:42   ` Marek Vasut
  2017-04-19 15:23 ` [PATCH v3 6/6] mtd: dataflash: Add flash_info for AT45DB641E Andrey Smirnov
  2017-04-19 15:35 ` [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts David Woodhouse
  4 siblings, 1 reply; 12+ messages in thread
From: Andrey Smirnov @ 2017-04-19 15:23 UTC (permalink / raw)
  To: linux-mtd
  Cc: Andrey Smirnov, cphealy, David Woodhouse, Brian Norris,
	Boris Brezillon, Marek Vasut, Richard Weinberger,
	Cyrille Pitchen, linux-kernel

In anticipation of supporting chips that need it, extend the size of
struct flash_info's 'jedec_id' field to make room 2 byte of extended
device information as well as add code to fetch this data during
jedec_probe().

Cc: cphealy@gmail.com
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---

Changes since [v2]:

	- Make 'id' have same size as 'jedec'

	- Get rid of eid_mask variable in favour of using GENMASK
          in-place

Changes since [v1]:

	- Formatting

[v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com
[v2] http://lkml.kernel.org/r/20170418142127.23301-3-andrew.smirnov@gmail.com

 drivers/mtd/devices/mtd_dataflash.c | 115 +++++++++++++++++++++---------------
 1 file changed, 68 insertions(+), 47 deletions(-)

diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index 2d3e403..941e8b7 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -687,7 +687,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;
@@ -710,62 +710,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;
-	u8 code = OP_READ_ID;
-	u8 id[3];
-	u32 jedec;
-	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) {
-		dev_dbg(&spi->dev, "error %d reading JEDEC ID\n", 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];
+	struct flash_info *info;
 
 	for (info = dataflash_data;
 	     info < dataflash_data + ARRAY_SIZE(dataflash_data);
@@ -793,12 +765,61 @@ 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;
+	u64 jedec;
+	u8 id[sizeof(jedec)] = {0};
+	const unsigned int id_size = 5;
+	const unsigned int first_byte = sizeof(id) - id_size;
+	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 (bits 0 through 15) and try to find
+	 * a match again in case our chip does not support returning
+	 * extended ID information and we got invalid bits instead
+	 */
+	jedec &= GENMASK_ULL(63, 16);
+
+	info = jedec_lookup(spi, jedec);
+	if (info)
+		return info;
+
 	/*
 	 * Treat other chips as errors ... we won't know the right page
 	 * size (it might be binary) even when we can tell which density
 	 * class is involved (legacy chip id scheme).
 	 */
-	dev_warn(&spi->dev, "JEDEC id %06x not handled\n", jedec);
+	dev_warn(&spi->dev, "JEDEC id %010llx not handled\n", jedec);
 	return ERR_PTR(-ENODEV);
 }
 
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 6/6] mtd: dataflash: Add flash_info for AT45DB641E
  2017-04-19 15:23 [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts Andrey Smirnov
                   ` (2 preceding siblings ...)
  2017-04-19 15:23 ` [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information" Andrey Smirnov
@ 2017-04-19 15:23 ` Andrey Smirnov
  2017-04-19 15:35 ` [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts David Woodhouse
  4 siblings, 0 replies; 12+ messages in thread
From: Andrey Smirnov @ 2017-04-19 15:23 UTC (permalink / raw)
  To: linux-mtd
  Cc: Andrey Smirnov, cphealy, David Woodhouse, Brian Norris,
	Boris Brezillon, Marek Vasut, Richard Weinberger,
	Cyrille Pitchen, linux-kernel

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

No changes between v3 through v1.

 drivers/mtd/devices/mtd_dataflash.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index 941e8b7..c83f341 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -732,6 +732,9 @@ static struct flash_info dataflash_data[] = {
 
 	{ "AT45DB642x",  0x1f28000000, 8192, 1056, 11, SUP_POW2PS},
 	{ "at45db642d",  0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
+
+	{ "AT45DB641E",  0x1f28000100, 32768, 264, 9, SUP_POW2PS},
+	{ "at45db641e",  0x1f28000100, 32768, 256, 8, SUP_POW2PS | IS_POW2PS},
 };
 
 static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec)
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 4/6] mtd: dataflash: Get rid of loop counter in jedec_probe()
  2017-04-19 15:23 ` [PATCH v3 4/6] mtd: dataflash: Get rid of loop counter " Andrey Smirnov
@ 2017-04-19 15:34   ` Marek Vasut
  0 siblings, 0 replies; 12+ messages in thread
From: Marek Vasut @ 2017-04-19 15:34 UTC (permalink / raw)
  To: Andrey Smirnov, linux-mtd
  Cc: cphealy, David Woodhouse, Brian Norris, Boris Brezillon,
	Richard Weinberger, Cyrille Pitchen, linux-kernel

On 04/19/2017 05:23 PM, Andrey Smirnov wrote:
> "For" loop in jedec_probe can be simplified to not need counter
> 'i'. Convert the code and get rid of the variable.
> 
> 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>
> ---

Reviewed-by: Marek Vasut <marek.vasut@gmail.com>

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts
  2017-04-19 15:23 [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts Andrey Smirnov
                   ` (3 preceding siblings ...)
  2017-04-19 15:23 ` [PATCH v3 6/6] mtd: dataflash: Add flash_info for AT45DB641E Andrey Smirnov
@ 2017-04-19 15:35 ` David Woodhouse
  2017-04-19 16:04   ` Andrey Smirnov
  4 siblings, 1 reply; 12+ messages in thread
From: David Woodhouse @ 2017-04-19 15:35 UTC (permalink / raw)
  To: Andrey Smirnov, linux-mtd
  Cc: cphealy, Brian Norris, Boris Brezillon, Richard Weinberger,
	Cyrille Pitchen, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Wed, 2017-04-19 at 08:23 -0700, Andrey Smirnov wrote:
> No functional change intended.

FWIW I *deliberately* used the proper C datatypes throughout all of the
MTD code instead of the silly kernel-speshul datatypes. To the extent
of *fixing* submissions to be written in C when they were using the
silly kernel types instead.

There is a reason for the __u32 type in userspace APIs. There is
basically no reason for 'u32' et al, just to save a trivial few
characters in the type name. We might as well be using the proper
language types.

I appreciate that not everyone agrees with that, but at least it used
to be consistent throughout the code I was maintaining. I'll defer to
the currently active maintainers though...

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"
  2017-04-19 15:23 ` [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information" Andrey Smirnov
@ 2017-04-19 15:42   ` Marek Vasut
  2017-04-19 16:01     ` Andrey Smirnov
  0 siblings, 1 reply; 12+ messages in thread
From: Marek Vasut @ 2017-04-19 15:42 UTC (permalink / raw)
  To: Andrey Smirnov, linux-mtd
  Cc: cphealy, David Woodhouse, Brian Norris, Boris Brezillon,
	Richard Weinberger, Cyrille Pitchen, linux-kernel

On 04/19/2017 05:23 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 [v2]:
> 
> 	- Make 'id' have same size as 'jedec'
> 
> 	- Get rid of eid_mask variable in favour of using GENMASK
>           in-place
> 
> Changes since [v1]:
> 
> 	- Formatting
> 
> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com
> [v2] http://lkml.kernel.org/r/20170418142127.23301-3-andrew.smirnov@gmail.com
> 
>  drivers/mtd/devices/mtd_dataflash.c | 115 +++++++++++++++++++++---------------
>  1 file changed, 68 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> index 2d3e403..941e8b7 100644
> --- a/drivers/mtd/devices/mtd_dataflash.c
> +++ b/drivers/mtd/devices/mtd_dataflash.c
> @@ -687,7 +687,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;
> @@ -710,62 +710,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;
> -	u8 code = OP_READ_ID;
> -	u8 id[3];
> -	u32 jedec;
> -	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) {
> -		dev_dbg(&spi->dev, "error %d reading JEDEC ID\n", 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];
> +	struct flash_info *info;
>  
>  	for (info = dataflash_data;
>  	     info < dataflash_data + ARRAY_SIZE(dataflash_data);
> @@ -793,12 +765,61 @@ 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;
> +	u64 jedec;
> +	u8 id[sizeof(jedec)] = {0};
> +	const unsigned int id_size = 5;
> +	const unsigned int first_byte = sizeof(id) - id_size;
> +	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);

Hm, looking at this again, what prevents you from reading these bytes
into &id[0] ? Might be easier, ie.

ret = spi_write_then_read(spi, &code, 1, id, 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)

if (id[0] != CFI_MFR_ATMEL)

> +		return NULL;
> +
> +	jedec = be64_to_cpup((__be64 *)id);
> +
> +	info = jedec_lookup(spi, jedec);

... define these somewhere at the beginning of the dataflash code ...
#define DATAFLASH_MASK_STD	GENMASK(63, 40)
#define DATAFLASH_MASK_EXT	GENMASK(63, 16)

... then ...
info = jedec_lookup(spi, jedec & DATAFLASH_MASK_EXT);

> +	if (info)
> +		return info;
> +
> +	/*
> +	 * Clear extended id bits (bits 0 through 15) and try to find
> +	 * a match again in case our chip does not support returning
> +	 * extended ID information and we got invalid bits instead
> +	 */
> +	jedec &= GENMASK_ULL(63, 16);

info = jedec_lookup(spi, jedec & DATAFLASH_MASK_STD);

> +	info = jedec_lookup(spi, jedec);
> +	if (info)
> +		return info;
> +
>  	/*
>  	 * Treat other chips as errors ... we won't know the right page
>  	 * size (it might be binary) even when we can tell which density
>  	 * class is involved (legacy chip id scheme).
>  	 */
> -	dev_warn(&spi->dev, "JEDEC id %06x not handled\n", jedec);
> +	dev_warn(&spi->dev, "JEDEC id %010llx not handled\n", jedec);
>  	return ERR_PTR(-ENODEV);
>  }
>  
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"
  2017-04-19 15:42   ` Marek Vasut
@ 2017-04-19 16:01     ` Andrey Smirnov
  2017-04-19 16:08       ` Marek Vasut
  0 siblings, 1 reply; 12+ messages in thread
From: Andrey Smirnov @ 2017-04-19 16:01 UTC (permalink / raw)
  To: Marek Vasut
  Cc: linux-mtd, Chris Healy, David Woodhouse, Brian Norris,
	Boris Brezillon, Richard Weinberger, Cyrille Pitchen,
	linux-kernel

On Wed, Apr 19, 2017 at 8:42 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 04/19/2017 05:23 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 [v2]:
>>
>>       - Make 'id' have same size as 'jedec'
>>
>>       - Get rid of eid_mask variable in favour of using GENMASK
>>           in-place
>>
>> Changes since [v1]:
>>
>>       - Formatting
>>
>> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com
>> [v2] http://lkml.kernel.org/r/20170418142127.23301-3-andrew.smirnov@gmail.com
>>
>>  drivers/mtd/devices/mtd_dataflash.c | 115 +++++++++++++++++++++---------------
>>  1 file changed, 68 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
>> index 2d3e403..941e8b7 100644
>> --- a/drivers/mtd/devices/mtd_dataflash.c
>> +++ b/drivers/mtd/devices/mtd_dataflash.c
>> @@ -687,7 +687,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;
>> @@ -710,62 +710,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;
>> -     u8 code = OP_READ_ID;
>> -     u8 id[3];
>> -     u32 jedec;
>> -     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) {
>> -             dev_dbg(&spi->dev, "error %d reading JEDEC ID\n", 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];
>> +     struct flash_info *info;
>>
>>       for (info = dataflash_data;
>>            info < dataflash_data + ARRAY_SIZE(dataflash_data);
>> @@ -793,12 +765,61 @@ 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;
>> +     u64 jedec;
>> +     u8 id[sizeof(jedec)] = {0};
>> +     const unsigned int id_size = 5;
>> +     const unsigned int first_byte = sizeof(id) - id_size;
>> +     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);
>
> Hm, looking at this again, what prevents you from reading these bytes
> into &id[0] ? Might be easier, ie.
>
> ret = spi_write_then_read(spi, &code, 1, id, id_size);
>

Then I'd have to add 6 more '0's to each of the IDs in dataflash_data,
which would make that code even less readable.

>> +     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)
>
> if (id[0] != CFI_MFR_ATMEL)
>
>> +             return NULL;
>> +
>> +     jedec = be64_to_cpup((__be64 *)id);
>> +
>> +     info = jedec_lookup(spi, jedec);
>
> ... define these somewhere at the beginning of the dataflash code ...
> #define DATAFLASH_MASK_STD      GENMASK(63, 40)
> #define DATAFLASH_MASK_EXT      GENMASK(63, 16)
>
> ... then ...
> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_EXT);
>
>> +     if (info)
>> +             return info;
>> +
>> +     /*
>> +      * Clear extended id bits (bits 0 through 15) and try to find
>> +      * a match again in case our chip does not support returning
>> +      * extended ID information and we got invalid bits instead
>> +      */
>> +     jedec &= GENMASK_ULL(63, 16);
>
> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_STD);
>

Sure, OK.

Thanks,
Andrey Smirnov

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts
  2017-04-19 15:35 ` [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts David Woodhouse
@ 2017-04-19 16:04   ` Andrey Smirnov
  0 siblings, 0 replies; 12+ messages in thread
From: Andrey Smirnov @ 2017-04-19 16:04 UTC (permalink / raw)
  To: David Woodhouse
  Cc: linux-mtd, Chris Healy, Brian Norris, Boris Brezillon,
	Richard Weinberger, Cyrille Pitchen, linux-kernel, Marek Vasut

[ +cc: Marek, since I accidentally dropped him off CC list ]

On Wed, Apr 19, 2017 at 8:35 AM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Wed, 2017-04-19 at 08:23 -0700, Andrey Smirnov wrote:
>> No functional change intended.
>
> FWIW I *deliberately* used the proper C datatypes throughout all of the
> MTD code instead of the silly kernel-speshul datatypes. To the extent
> of *fixing* submissions to be written in C when they were using the
> silly kernel types instead.
>
> There is a reason for the __u32 type in userspace APIs. There is
> basically no reason for 'u32' et al, just to save a trivial few
> characters in the type name. We might as well be using the proper
> language types.
>
> I appreciate that not everyone agrees with that, but at least it used
> to be consistent throughout the code I was maintaining. I'll defer to
> the currently active maintainers though...

This conversion was an explicit request from Marek, see
https://marc.info/?l=linux-kernel&m=149192751625941

Thanks,
Andrey Smirnov

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"
  2017-04-19 16:01     ` Andrey Smirnov
@ 2017-04-19 16:08       ` Marek Vasut
  2017-04-19 16:12         ` Marek Vasut
  0 siblings, 1 reply; 12+ messages in thread
From: Marek Vasut @ 2017-04-19 16:08 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: linux-mtd, Chris Healy, David Woodhouse, Brian Norris,
	Boris Brezillon, Richard Weinberger, Cyrille Pitchen,
	linux-kernel

On 04/19/2017 06:01 PM, Andrey Smirnov wrote:
> On Wed, Apr 19, 2017 at 8:42 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 04/19/2017 05:23 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 [v2]:
>>>
>>>       - Make 'id' have same size as 'jedec'
>>>
>>>       - Get rid of eid_mask variable in favour of using GENMASK
>>>           in-place
>>>
>>> Changes since [v1]:
>>>
>>>       - Formatting
>>>
>>> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com
>>> [v2] http://lkml.kernel.org/r/20170418142127.23301-3-andrew.smirnov@gmail.com
>>>
>>>  drivers/mtd/devices/mtd_dataflash.c | 115 +++++++++++++++++++++---------------
>>>  1 file changed, 68 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
>>> index 2d3e403..941e8b7 100644
>>> --- a/drivers/mtd/devices/mtd_dataflash.c
>>> +++ b/drivers/mtd/devices/mtd_dataflash.c
>>> @@ -687,7 +687,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;
>>> @@ -710,62 +710,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;
>>> -     u8 code = OP_READ_ID;
>>> -     u8 id[3];
>>> -     u32 jedec;
>>> -     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) {
>>> -             dev_dbg(&spi->dev, "error %d reading JEDEC ID\n", 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];
>>> +     struct flash_info *info;
>>>
>>>       for (info = dataflash_data;
>>>            info < dataflash_data + ARRAY_SIZE(dataflash_data);
>>> @@ -793,12 +765,61 @@ 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;
>>> +     u64 jedec;
>>> +     u8 id[sizeof(jedec)] = {0};
>>> +     const unsigned int id_size = 5;
>>> +     const unsigned int first_byte = sizeof(id) - id_size;
>>> +     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);
>>
>> Hm, looking at this again, what prevents you from reading these bytes
>> into &id[0] ? Might be easier, ie.
>>
>> ret = spi_write_then_read(spi, &code, 1, id, id_size);
>>
> 
> Then I'd have to add 6 more '0's to each of the IDs in dataflash_data,
> which would make that code even less readable.

Or do >>= 24 on the jedec ID :)

>>> +     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)
>>
>> if (id[0] != CFI_MFR_ATMEL)
>>
>>> +             return NULL;
>>> +
>>> +     jedec = be64_to_cpup((__be64 *)id);
>>> +
>>> +     info = jedec_lookup(spi, jedec);
>>
>> ... define these somewhere at the beginning of the dataflash code ...
>> #define DATAFLASH_MASK_STD      GENMASK(63, 40)
>> #define DATAFLASH_MASK_EXT      GENMASK(63, 16)
>>
>> ... then ...
>> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_EXT);
>>
>>> +     if (info)
>>> +             return info;
>>> +
>>> +     /*
>>> +      * Clear extended id bits (bits 0 through 15) and try to find
>>> +      * a match again in case our chip does not support returning
>>> +      * extended ID information and we got invalid bits instead
>>> +      */
>>> +     jedec &= GENMASK_ULL(63, 16);
>>
>> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_STD);
>>
> 
> Sure, OK.
> 
> Thanks,
> Andrey Smirnov
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"
  2017-04-19 16:08       ` Marek Vasut
@ 2017-04-19 16:12         ` Marek Vasut
  0 siblings, 0 replies; 12+ messages in thread
From: Marek Vasut @ 2017-04-19 16:12 UTC (permalink / raw)
  To: Marek Vasut, Andrey Smirnov
  Cc: linux-mtd, Chris Healy, David Woodhouse, Brian Norris,
	Boris Brezillon, Richard Weinberger, Cyrille Pitchen,
	linux-kernel

On 04/19/2017 06:08 PM, Marek Vasut wrote:
> On 04/19/2017 06:01 PM, Andrey Smirnov wrote:
>> On Wed, Apr 19, 2017 at 8:42 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 04/19/2017 05:23 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 [v2]:
>>>>
>>>>       - Make 'id' have same size as 'jedec'
>>>>
>>>>       - Get rid of eid_mask variable in favour of using GENMASK
>>>>           in-place
>>>>
>>>> Changes since [v1]:
>>>>
>>>>       - Formatting
>>>>
>>>> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com
>>>> [v2] http://lkml.kernel.org/r/20170418142127.23301-3-andrew.smirnov@gmail.com
>>>>
>>>>  drivers/mtd/devices/mtd_dataflash.c | 115 +++++++++++++++++++++---------------
>>>>  1 file changed, 68 insertions(+), 47 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
>>>> index 2d3e403..941e8b7 100644
>>>> --- a/drivers/mtd/devices/mtd_dataflash.c
>>>> +++ b/drivers/mtd/devices/mtd_dataflash.c
>>>> @@ -687,7 +687,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;
>>>> @@ -710,62 +710,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;
>>>> -     u8 code = OP_READ_ID;
>>>> -     u8 id[3];
>>>> -     u32 jedec;
>>>> -     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) {
>>>> -             dev_dbg(&spi->dev, "error %d reading JEDEC ID\n", 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];
>>>> +     struct flash_info *info;
>>>>
>>>>       for (info = dataflash_data;
>>>>            info < dataflash_data + ARRAY_SIZE(dataflash_data);
>>>> @@ -793,12 +765,61 @@ 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;
>>>> +     u64 jedec;
>>>> +     u8 id[sizeof(jedec)] = {0};
>>>> +     const unsigned int id_size = 5;
>>>> +     const unsigned int first_byte = sizeof(id) - id_size;
>>>> +     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);
>>>
>>> Hm, looking at this again, what prevents you from reading these bytes
>>> into &id[0] ? Might be easier, ie.
>>>
>>> ret = spi_write_then_read(spi, &code, 1, id, id_size);
>>>
>>
>> Then I'd have to add 6 more '0's to each of the IDs in dataflash_data,
>> which would make that code even less readable.
> 
> Or do >>= 24 on the jedec ID :)

In fact, you can use the flags field to introduce a flag to indicate
that the flash uses ext ID . Then the lookup function can do first try
the ext IDs (with correct shift) and then standard IDs (with correct
bitshift again).

Upside is, you don't need to patch the table above at all, you just add
your new flash with another flag , ie. SUP_EXTID .

>>>> +     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)
>>>
>>> if (id[0] != CFI_MFR_ATMEL)
>>>
>>>> +             return NULL;
>>>> +
>>>> +     jedec = be64_to_cpup((__be64 *)id);
>>>> +
>>>> +     info = jedec_lookup(spi, jedec);
>>>
>>> ... define these somewhere at the beginning of the dataflash code ...
>>> #define DATAFLASH_MASK_STD      GENMASK(63, 40)
>>> #define DATAFLASH_MASK_EXT      GENMASK(63, 16)
>>>
>>> ... then ...
>>> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_EXT);
>>>
>>>> +     if (info)
>>>> +             return info;
>>>> +
>>>> +     /*
>>>> +      * Clear extended id bits (bits 0 through 15) and try to find
>>>> +      * a match again in case our chip does not support returning
>>>> +      * extended ID information and we got invalid bits instead
>>>> +      */
>>>> +     jedec &= GENMASK_ULL(63, 16);
>>>
>>> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_STD);
>>>
>>
>> Sure, OK.
>>
>> Thanks,
>> Andrey Smirnov
>>
> 
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-04-19 16:13 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-19 15:23 [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts Andrey Smirnov
2017-04-19 15:23 ` [PATCH v3 2/6] mtd: dataflash: Improve coding style in jedec_probe() Andrey Smirnov
2017-04-19 15:23 ` [PATCH v3 4/6] mtd: dataflash: Get rid of loop counter " Andrey Smirnov
2017-04-19 15:34   ` Marek Vasut
2017-04-19 15:23 ` [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information" Andrey Smirnov
2017-04-19 15:42   ` Marek Vasut
2017-04-19 16:01     ` Andrey Smirnov
2017-04-19 16:08       ` Marek Vasut
2017-04-19 16:12         ` Marek Vasut
2017-04-19 15:23 ` [PATCH v3 6/6] mtd: dataflash: Add flash_info for AT45DB641E Andrey Smirnov
2017-04-19 15:35 ` [PATCH v3 1/6] mtd: dataflash: Replace C99 types with their kernel counterparts David Woodhouse
2017-04-19 16:04   ` Andrey Smirnov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).