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

Tudor.Ambarus
2021-04-05T09:55:08Z

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.