Re: [PATCH 1/3] mtd: spi-nor: core: Add manuafacturer/chip specific operations

[Pratyush Yadav <p.yadav@ti.com>]

On 02/04/21 03:50AM, yaliang.wang@windriver.com wrote:
> From: Yaliang Wang <Yaliang.Wang@windriver.com>
> 
> This is quite similar to the patch made by Takahiro Kuwano[1], except
> replacing the bare ->ready() hook with a structure spi_nor_ops.

I don't think this belongs in the commit message. The commit message 
should describe the change in isolation. All the "metadata" like 
"depends on patch X", or "rework of patch Y", etc. should go below the 3 
dashes [2].

> 
> The purpose of this introduction is to provide a more flexible method,
> so that we can expand it when needed. Also Basing on this, the
> manufacturer specific checking ready codes can be shifted into their own
> file.
> 
> [1]http://lists.infradead.org/pipermail/linux-mtd/2021-March/085741.html
> 
> Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
> ---

[2] Here.

>  drivers/mtd/spi-nor/core.c | 8 +++++---
>  drivers/mtd/spi-nor/core.h | 9 +++++++++
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0522304f52fa..6dc8bd0a6bd4 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -785,12 +785,13 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
>  }
>  
>  /**
> - * spi_nor_ready() - Query the flash to see if it is ready for new commands.
> + * spi_nor_default_ready() - Query the flash to see if it is ready for new
> + * commands.
>   * @nor:	pointer to 'struct spi_nor'.
>   *
>   * Return: 1 if ready, 0 if not ready, -errno on errors.
>   */
> -static int spi_nor_ready(struct spi_nor *nor)
> +static int spi_nor_default_ready(struct spi_nor *nor)
>  {
>  	int sr, fsr;
>  
> @@ -826,7 +827,7 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor,
>  		if (time_after_eq(jiffies, deadline))
>  			timeout = 1;
>  
> -		ret = spi_nor_ready(nor);
> +		ret = nor->params->ops.ready(nor);
>  		if (ret < 0)
>  			return ret;
>  		if (ret)
> @@ -2920,6 +2921,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
>  	params->quad_enable = spi_nor_sr2_bit1_quad_enable;
>  	params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
>  	params->setup = spi_nor_default_setup;
> +	params->ops.ready = spi_nor_default_ready;
>  	/* Default to 16-bit Write Status (01h) Command */
>  	nor->flags |= SNOR_F_HAS_16BIT_SR;
>  
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 4a3f7f150b5d..bc042a0ef94e 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -187,6 +187,14 @@ struct spi_nor_locking_ops {
>  	int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>  };
>  
> +/**
> + * struct spi_nor_ops - SPI manuafacturer/chip specific operations
> + * @ready:	query if a chip is ready.
> + */
> +struct spi_nor_ops {
> +	int (*ready)(struct spi_nor *nor);

I don't understand how this is more flexible than just having the 
ready() hook in spi_nor_flash_parameter like Takahiro's patch did. I'm 
not completely opposed to the idea but I'd like to understand your 
thought process first.

Also...

> +};
> +
>  /**
>   * struct spi_nor_flash_parameter - SPI NOR flash parameters and settings.
>   * Includes legacy flash parameters and settings that can be overwritten
> @@ -232,6 +240,7 @@ struct spi_nor_flash_parameter {
>  	struct spi_nor_pp_command	page_programs[SNOR_CMD_PP_MAX];
>  
>  	struct spi_nor_erase_map        erase_map;
> +	struct spi_nor_ops				ops;
>  
>  	int (*octal_dtr_enable)(struct spi_nor *nor, bool enable);
>  	int (*quad_enable)(struct spi_nor *nor);

... shouldn't octal_dtr_enable(), quad_enable(), set_4byte_addr_mode(), 
convert_addr(), and setup() hooks also be moved into spi_nor_ops? Why is 
the ready() hook different from these hooks?

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/