diff mbox series

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

Message ID 20210401195012.4009166-1-yaliang.wang@windriver.com
State Changes Requested
Delegated to: Ambarus Tudor
Headers show
Series [1/3] mtd: spi-nor: core: Add manuafacturer/chip specific operations | expand

Commit Message

Yaliang Wang April 1, 2021, 7:50 p.m. UTC 
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.

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>
---
 drivers/mtd/spi-nor/core.c | 8 +++++---
 drivers/mtd/spi-nor/core.h | 9 +++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

Comments

Pratyush Yadav April 5, 2021, 8:54 a.m. UTC | #1 
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?
Tudor Ambarus April 5, 2021, 9:53 a.m. UTC | #2 
On 4/5/21 11:54 AM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you 
> know the content is safe
> 
> 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.
> 

When I proposed the introduction of spi_nor_ops I thought about having one
place for storing different register operations or command opcodes. For example,
macronix uses for SPINOR_OP_RDCR a 0x15 value instead of 0x35. But maybe we'll
find a better place for these when parsing SCCR SFDP table. I'm ok with dropping
spi_nor_ops.

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

Right, maybe ready() resembles more the ones that you indicated. As the
spi_nor_flash_parameter is not yet big, we can keep all scattered for now.
diff mbox series

Patch

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);
+};
+
 /**
  * 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);