[PATCH] mtd: spi-nor: allow NULL as spi_device_id in spi_nor_scan

[PATCH] mtd: spi-nor: allow NULL as spi_device_id in spi_nor_scan

[Rafał Miłecki]

Now we allow customized read_id handlers we should allow passing NULL
as a struct spi_device_id pointer. In such case we should simply make
use of the read_id callback and let driver read the flash ID.
At some point we may try to remove this argument completely.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
This is based on top of
mtd: move support for struct flash_platform_data's "type" into m25p80
---
 drivers/mtd/spi-nor/spi-nor.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d733b16..45610b2 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -906,29 +906,23 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 	if (ret)
 		return ret;
 
-	info = (void *)id->driver_data;
-
-	if (info->jedec_id) {
-		const struct spi_device_id *jid;
-
-		jid = nor->read_id(nor);
-		if (IS_ERR(jid)) {
-			return PTR_ERR(jid);
-		} else if (jid != id) {
-			/*
-			 * JEDEC knows better, so overwrite platform ID. We
-			 * can't trust partitions any longer, but we'll let
-			 * mtd apply them anyway, since some partitions may be
-			 * marked read-only, and we don't want to lose that
-			 * information, even if it's not 100% accurate.
-			 */
-			dev_warn(dev, "found %s, expected %s\n",
-				 jid->name, id->name);
-			id = jid;
-			info = (void *)jid->driver_data;
+	if (id) {
+		info = (void *)id->driver_data;
+		if (info->jedec_id) {
+			dev_warn(dev,
+				 "passed SPI device ID (%s) contains JEDEC, ignoring it, driver should be fixed!\n",
+				 id->name);
+			id = NULL;
 		}
 	}
 
+	if (!id) {
+		id = nor->read_id(nor);
+		if (IS_ERR(id))
+			return PTR_ERR(id);
+	}
+	info = (void *)id->driver_data;
+
 	mutex_init(&nor->lock);
 
 	/*
-- 
1.8.4.5

Re: [PATCH] mtd: spi-nor: allow NULL as spi_device_id in spi_nor_scan

[Brian Norris]

On Thu, Aug 07, 2014 at 07:13:21PM +0200, Rafał Miłecki wrote:
> Now we allow customized read_id handlers we should allow passing NULL
> as a struct spi_device_id pointer. In such case we should simply make
> use of the read_id callback and let driver read the flash ID.
> At some point we may try to remove this argument completely.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> This is based on top of
> mtd: move support for struct flash_platform_data's "type" into m25p80
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 34 ++++++++++++++--------------------
>  1 file changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d733b16..45610b2 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -906,29 +906,23 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
>  	if (ret)
>  		return ret;
>  
> -	info = (void *)id->driver_data;
> -
> -	if (info->jedec_id) {
> -		const struct spi_device_id *jid;
> -
> -		jid = nor->read_id(nor);
> -		if (IS_ERR(jid)) {
> -			return PTR_ERR(jid);
> -		} else if (jid != id) {
> -			/*
> -			 * JEDEC knows better, so overwrite platform ID. We
> -			 * can't trust partitions any longer, but we'll let
> -			 * mtd apply them anyway, since some partitions may be
> -			 * marked read-only, and we don't want to lose that
> -			 * information, even if it's not 100% accurate.
> -			 */
> -			dev_warn(dev, "found %s, expected %s\n",
> -				 jid->name, id->name);
> -			id = jid;
> -			info = (void *)jid->driver_data;
> +	if (id) {
> +		info = (void *)id->driver_data;
> +		if (info->jedec_id) {
> +			dev_warn(dev,
> +				 "passed SPI device ID (%s) contains JEDEC, ignoring it, driver should be fixed!\n",
> +				 id->name);

I think you want some more code comments in this section, to describe
why we might ignore the driver data, and how the driver could be fixed.

Also, won't this condition be triggered for all m25p80 users right now?

> +			id = NULL;
>  		}
>  	}
>  
> +	if (!id) {
> +		id = nor->read_id(nor);
> +		if (IS_ERR(id))
> +			return PTR_ERR(id);
> +	}
> +	info = (void *)id->driver_data;
> +
>  	mutex_init(&nor->lock);
>  
>  	/*

Brian