Re: [PATCH v2 26/35] mtd: spi-nor: core: Introduce spi_nor_init_default_params()

[<Tudor.Ambarus@microchip.com>]

On 10/22/21 3:41 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-10-04 06:17, schrieb Tudor.Ambarus@microchip.com:
>> On 8/24/21 8:30 PM, Pratyush Yadav wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> On 27/07/21 07:52AM, Tudor Ambarus wrote:
>>>> Called for all flashes, regardless if they define SFDP tables or not.
>>>>
>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>> ---
>>>>  drivers/mtd/spi-nor/core.c | 92
>>>> +++++++++++++++++++++-----------------
>>>>  1 file changed, 52 insertions(+), 40 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index b3a01d7d6f0b..9193317f897d 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -2543,6 +2543,56 @@ static int spi_nor_setup(struct spi_nor *nor,
>>>>       return spi_nor_set_addr_width(nor);
>>>>  }
>>>>
>>>> +/**
>>>> + * spi_nor_init_default_params() - Default initialization of flash
>>>> parameters
>>>> + * and settings. Done for all flashes, regardless is they define
>>>> SFDP tables
>>>> + * or not.
>>>> + * @nor:     pointer to a 'struct spi_nor'.
>>>> + */
>>>> +static void spi_nor_init_default_params(struct spi_nor *nor)
>>>> +{
>>>> +     struct spi_nor_flash_parameter *params = nor->params;
>>>> +     const struct flash_info *info = nor->info;
>>>> +     struct device_node *np = spi_nor_get_flash_node(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->otp.org = &info->otp_org;
>>>> +
>>>> +     /* Default to 16-bit Write Status (01h) Command */
>>>> +     nor->flags |= SNOR_F_HAS_16BIT_SR;
>>>> +
>>>> +     /* Set SPI NOR sizes. */
>>>> +     params->writesize = 1;
>>>> +     params->size = (u64)info->sector_size * info->n_sectors;
>>>> +     params->page_size = info->page_size;
>>>
>>> I think these two lines should go in spi_nor_info_init_params() since
>>> you are using the nor info to initialize these parameters. Otherwise,
>>> what even is the difference between these two functions?
>>
>> I think a better name for spi_nor_info_init_params() is
>> spi_nor_nonsfdp_info_init_params(). This method will eventually be
>> called
>> just for non SFDP flashes. Check conversation in 18/35.
>>
>> And maybe I should rename spi_nor_nonsfdp_flags_init to
>> spi_nor_nonsfdp_info_init_snor_f.
> 
> Urgh. I really dislike that suffix. Somehow the flags prefix

:) I'll think of something better then.

> creeps into the function name. If that function is expected
> to only set the SNOR_F_ flags then there should be a comment.
> 
>>>
>>>> +
>>>> +     if (!(info->flags & SPI_NOR_NO_FR)) {
>>>> +             /* Default to Fast Read for DT and non-DT platform
>>>> devices. */
>>>> +             params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
>>>> +
>>>> +             /* Mask out Fast Read if not requested at DT
>>>> instantiation. */
>>>> +             if (np && !of_property_read_bool(np, "m25p,fast-read"))
>>>> +                     params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
>>>> +     }
> 
> Hm, this looks really weird.
> 
> why not:
> 
> /* default to fast read */
> params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
> 
> /* unless the flash doesn't support it */
> if (info->flags & SPI_NOR_NO_FR)
>     params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
> 
> /* or the device tree doesn't explicitly request it */
> if (np && !of_property_read_bool(np, "m25p,fast-read"))
>     params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
> 
> I know the old code was the same, but it might go into another
> patch. The SPI_NOR_NO_FR suggests that fast read was always the

right, dedicated patch. I'll check it.

> default. Otherwise, it would have been SPI_NOR_HAS_FAST_READ.

> 
>>>> +
>>>> +     /* (Fast) Read settings. */
>>>> +     params->hwcaps.mask |= SNOR_HWCAPS_READ;
>>>> +     spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ],
>>>> +                               0, 0, SPINOR_OP_READ,
>>>> +                               SNOR_PROTO_1_1_1);
>>>> +
>>>> +     if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)
>>>> +
>>>> spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST],
>>>> +                                       0, 8, SPINOR_OP_READ_FAST,
>>>> +                                       SNOR_PROTO_1_1_1);
>>>> +     /* Page Program settings. */
>>>> +     params->hwcaps.mask |= SNOR_HWCAPS_PP;
>>>> +     spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP],
>>>> +                             SPINOR_OP_PP, SNOR_PROTO_1_1_1);
>>>> +}
>>>> +
>>>>  /**
>>>>   * spi_nor_manufacturer_init_params() - Initialize the flash's
>>>> parameters and
>>>>   * settings based on MFR register and ->default_init() hook.
>>>> @@ -2609,43 +2659,8 @@ static void spi_nor_info_init_params(struct
>>>> spi_nor *nor)
>>>>       struct spi_nor_flash_parameter *params = nor->params;
>>>>       struct spi_nor_erase_map *map = &params->erase_map;
>>>>       const struct flash_info *info = nor->info;
>>>> -     struct device_node *np = spi_nor_get_flash_node(nor);
>>>>       u8 i, erase_mask;
>>>>
>>>> -     /* Initialize default flash parameters and settings. */
>>>> -     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->otp.org = &info->otp_org;
>>>> -
>>>> -     /* Default to 16-bit Write Status (01h) Command */
>>>> -     nor->flags |= SNOR_F_HAS_16BIT_SR;
>>>> -
>>>> -     /* Set SPI NOR sizes. */
>>>> -     params->writesize = 1;
>>>> -     params->size = (u64)info->sector_size * info->n_sectors;
>>>> -     params->page_size = info->page_size;
>>>> -
>>>> -     if (!(info->flags & SPI_NOR_NO_FR)) {
>>>> -             /* Default to Fast Read for DT and non-DT platform
>>>> devices. */
>>>> -             params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
>>>> -
>>>> -             /* Mask out Fast Read if not requested at DT
>>>> instantiation. */
>>>> -             if (np && !of_property_read_bool(np, "m25p,fast-read"))
>>>> -                     params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
>>>> -     }
>>>> -
>>>> -     /* (Fast) Read settings. */
>>>> -     params->hwcaps.mask |= SNOR_HWCAPS_READ;
>>>> -     spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ],
>>>> -                               0, 0, SPINOR_OP_READ,
>>>> -                               SNOR_PROTO_1_1_1);
>>>> -
>>>> -     if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)
>>>> -
>>>> spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST],
>>>> -                                       0, 8, SPINOR_OP_READ_FAST,
>>>> -                                       SNOR_PROTO_1_1_1);
>>>> -
>>>>       if (info->flags & SPI_NOR_DUAL_READ) {
>>>>               params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2;
>>>>
>>>> spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_1_1_2],
>>>> @@ -2674,11 +2689,6 @@ static void spi_nor_info_init_params(struct
>>>> spi_nor *nor)
>>>>                                         SNOR_PROTO_8_8_8_DTR);
>>>>       }
>>>>
>>>> -     /* Page Program settings. */
>>>> -     params->hwcaps.mask |= SNOR_HWCAPS_PP;
>>>> -     spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP],
>>>> -                             SPINOR_OP_PP, SNOR_PROTO_1_1_1);
>>>> -
>>>>       if (info->flags & SPI_NOR_OCTAL_DTR_PP) {
>>>>               params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;
>>>>               /*
>>>> @@ -2823,6 +2833,8 @@ static int spi_nor_init_params(struct spi_nor
>>>> *nor)
>>>>       if (!nor->params)
>>>>               return -ENOMEM;
>>>>
>>>> +     spi_nor_init_default_params(nor);
>>>> +
>>>>       spi_nor_info_init_params(nor);
>>>>
>>>>       spi_nor_manufacturer_init_params(nor);
>>>
>>> I am neutral towards this patch. I don't think it improves much, but
>>> at
>>> the same time it doesn't make anything worse either.
>>
>> I think it helps readability. It splits spi_nor_init_params() into
>> smaller logical chunks,
>> based on the type of initialization. We should usually avoid long
>> methods where we can split
>> them in logical chunks, it makes the code pleasant to read.
> 
> I'm fine with that. Just want to say, that then we have to think
> about what goes where and where we draw the line.
> 

Okay, I'll propose some guidance in the commit message.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/