Received-SPF: pass (google.com: domain of linux-kernel+bounces-148680-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Pratyush Yadav <pratyush@kernel.org>
To: Michael Walle <mwalle@kernel.org>
Cc: Tudor Ambarus <tudor.ambarus@linaro.org>,  Pratyush Yadav
 <pratyush@kernel.org>,  Miquel Raynal <miquel.raynal@bootlin.com>,
  Richard Weinberger <richard@nod.at>,  Vignesh Raghavendra
 <vigneshr@ti.com>,  linux-kernel@vger.kernel.org,
  linux-mtd@lists.infradead.org
Subject: Re: [RFC PATCH v1 6/6] mtd: spi-nor: introduce support for
 displaying deprecation message
In-Reply-To: <20240412134405.381832-7-mwalle@kernel.org> (Michael Walle's
	message of "Fri, 12 Apr 2024 15:44:05 +0200")
References: <20240412134405.381832-1-mwalle@kernel.org>
	<20240412134405.381832-7-mwalle@kernel.org>
Date: Wed, 17 Apr 2024 16:36:06 +0200
Message-ID: <mafs0jzkw6oq1.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Fri, Apr 12 2024, Michael Walle wrote:

> SPI-NOR will automatically detect the attached flash device most of the
> time. We cannot easily find out if boards are using a given flash.
> Therefore, introduce a (temporary) flag to display a message on boot if

Why temporary? There will always be a need to deprecate one flash or
another. Might as well keep the flag around.

Also, this patch/series does not add any users of the deprecated flag.
If you have some flashes in mind, it would be good to add them to the
patch/series.

I like the idea in general. Do you think we should also print a rough
date for the deprecation as well?

> support for a given flash device is scheduled to be removed in the
> future.
>
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
>  drivers/mtd/spi-nor/core.c | 12 ++++++++++++
>  drivers/mtd/spi-nor/core.h |  1 +
>  2 files changed, 13 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 58d310427d35..a294eef2e34a 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3312,6 +3312,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
>  						       const char *name)
>  {
>  	const struct flash_info *jinfo = NULL, *info = NULL;
> +	const char *deprecated = NULL;
>  
>  	if (name)
>  		info = spi_nor_match_name(nor, name);
> @@ -3326,6 +3327,17 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
>  			return jinfo;
>  	}
>  
> +	if (info && (info->flags & SPI_NOR_DEPRECATED))
> +		deprecated = info->name;
> +	else if (jinfo && (jinfo->flags & SPI_NOR_DEPRECATED))
> +		deprecated = jinfo->name;
> +
> +	if (deprecated)
> +		pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n"
> +			"deprecated driver support. It will be removed in future kernel\n"

Nit: "removed in a future kernel version"

> +			"version. If you feel this shouldn't be the case, please contact\n"
> +			"us at linux-mtd@lists.infradead.org\n", deprecated);
> +

Hmm, this isn't so nice. I'd suggest doing something like:

	/*
         * If caller has specified name of flash model that can normally be
         * ...
         */
	info = jinfo ?: info;

	if (info->flags & SPI_NOR_DEPRECATED)
        	pr_warn(...);

	return info;

>  	/*
>  	 * If caller has specified name of flash model that can normally be
>  	 * detected using JEDEC, let's verify it.
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 8552e31b1b07..0317d8e253f4 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -524,6 +524,7 @@ struct flash_info {
>  #define SPI_NOR_NO_ERASE		BIT(6)
>  #define SPI_NOR_QUAD_PP			BIT(8)
>  #define SPI_NOR_RWW			BIT(9)
> +#define SPI_NOR_DEPRECATED		BIT(15)

If you do agree with my suggestion of making it permanent, would it make
more sense to make it BIT(10) instead. Or BIT(9) once you move up the
others because we no longer have BIT(7).

>  
>  	u8 no_sfdp_flags;
>  #define SPI_NOR_SKIP_SFDP		BIT(0)

-- 
Regards,
Pratyush Yadav