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

[<Tudor.Ambarus@microchip.com>]

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.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/