[PATCH] mtd: spi-nor: Make octal_dtr_enable() dedicate for enabling Octal DTR

[Tudor Ambarus]

Hi, Takahiro,

On 16.06.2023 08:06, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> 
> The function should be responsible only for enabling Octal DTR. Remove
> 'enable' parameter from octal_dtr_enable() and add octal_dtr_disable()
> that takes care for disabling. This can remove 'enable' flag checks in
> core and manufacturer drivers and improve readability.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> ---
>  drivers/mtd/spi-nor/core.c      | 42 ++++++++++++++++++++++++++-------
>  drivers/mtd/spi-nor/core.h      |  4 +++-
>  drivers/mtd/spi-nor/micron-st.c | 11 +++------
>  drivers/mtd/spi-nor/spansion.c  | 36 ++++++++++++++--------------
>  4 files changed, 57 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 143ca3c9b477..d2571f309f41 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3086,11 +3086,10 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  
>  /** spi_nor_octal_dtr_enable() - enable Octal DTR I/O if needed
>   * @nor:                 pointer to a 'struct spi_nor'
> - * @enable:              whether to enable or disable Octal DTR
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
> +static int spi_nor_octal_dtr_enable(struct spi_nor *nor)
>  {
>  	int ret;
>  
> @@ -3104,14 +3103,39 @@ static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
>  	if (!(nor->flags & SNOR_F_IO_MODE_EN_VOLATILE))
>  		return 0;
>  
> -	ret = nor->params->octal_dtr_enable(nor, enable);
> +	ret = nor->params->octal_dtr_enable(nor);
>  	if (ret)
>  		return ret;
>  
> -	if (enable)
> -		nor->reg_proto = SNOR_PROTO_8_8_8_DTR;
> -	else
> -		nor->reg_proto = SNOR_PROTO_1_1_1;
> +	nor->reg_proto = SNOR_PROTO_8_8_8_DTR;
> +
> +	return 0;
> +}
> +
> +/** spi_nor_octal_dtr_disable() - disable Octal DTR I/O if it is enabled
> + * @nor:                 pointer to a 'struct spi_nor'
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_octal_dtr_disable(struct spi_nor *nor)
> +{
> +	int ret;
> +
> +	if (!nor->params->octal_dtr_disable)
> +		return 0;
> +
> +	if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
> +	      nor->write_proto == SNOR_PROTO_8_8_8_DTR))
> +		return 0;
> +
> +	if (!(nor->flags & SNOR_F_IO_MODE_EN_VOLATILE))
> +		return 0;
> +
> +	ret = nor->params->octal_dtr_disable(nor);
> +	if (ret)
> +		return ret;
> +
> +	nor->reg_proto = SNOR_PROTO_1_1_1;
>  

I gave a little thought on this. Indeed having an
*_enable(..., bool enable) method bends the logic and we have to act on
the name, but from the core's point of view I think we should stick to a
single method, otherwise the duplication of code is obvious. On the
manufacturers drivers side however, we saw that the enable/disable
methods quite differ (see [1]), so I would recommend the have dedicated
methods for the manufacturers. Thus I would do something like this:

in the core:
int (*set_octal_dtr)(struct spi_nor *nor, bool enable);

in the manufacturer drivers:
static int manufacturer_snor_set_octal_dtr(struct spi_nor *nor, bool enable)
{
	return enable ? manufacturer_snor_octal_dtr_enable() :
			manufacturer_snor_octal_dtr_disable();
}

I'll scratch a patch later today.
Cheers,
ta

[1]
https://lore.kernel.org/linux-mtd/cover.1686557139.git.Takahiro.Kuwano@infineon.com/