Re: [PATCH v1 3/6] mtd: spi-nor: get rid of SPI_NOR_NO_FR

[Pratyush Yadav]

On Wed, Apr 17 2024, Michael Walle wrote:

> Hi,
>
> On Wed Apr 17, 2024 at 3:39 PM CEST, Pratyush Yadav wrote:
>> On Fri, Apr 12 2024, Michael Walle wrote:
>>
>> > The evervision FRAM devices are the only user of the NO_FR flag. Drop
>> > the global flag and instead use a manufacturer fixup for the evervision
>> > flashes to drop the fast read support.
>> >
>> > Signed-off-by: Michael Walle <mwalle@xxxxxxxxxx>
>> > ---
>> > Please note, that the fast read opcode will still be set in
>> > spi_nor_init_default_params(), but the selection of the read opcodes
>> > just depends on the mask.
>>
>> Since that is the case now, might as well drop the
>>
>>     if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)
>>
>> in spi_nor_init_default_params().
>
> I want to address that in another patch where I'll do that for all
> the opcodes. Just doing it for the fast read looks odd.

Okay.

[...]
>> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> > index 072c69b0d06c..9aa7d6399c8a 100644
>> > --- a/drivers/mtd/spi-nor/core.h
>> > +++ b/drivers/mtd/spi-nor/core.h
>> > @@ -479,7 +479,6 @@ struct spi_nor_id {
>> >   *                            Usually these will power-up in a write-protected
>> >   *                            state.
>> >   *   SPI_NOR_NO_ERASE:        no erase command needed.
>> > - *   SPI_NOR_NO_FR:           can't do fastread.
>> >   *   SPI_NOR_QUAD_PP:         flash supports Quad Input Page Program.
>> >   *   SPI_NOR_RWW:             flash supports reads while write.
>> >   *
>> > @@ -528,7 +527,6 @@ struct flash_info {
>> >  #define SPI_NOR_BP3_SR_BIT6		BIT(4)
>> >  #define SPI_NOR_SWP_IS_VOLATILE		BIT(5)
>> >  #define SPI_NOR_NO_ERASE		BIT(6)
>> > -#define SPI_NOR_NO_FR			BIT(7)
>> >  #define SPI_NOR_QUAD_PP			BIT(8)
>> >  #define SPI_NOR_RWW			BIT(9)
>>
>> Move the other bits up since the slot is now free.
>
> Mhh can't decide what's better here. On one hand I'd really like to
> avoid too much code churn because it's already hard enough to follow
> the development using git blame. OTOH, a new flag would need to be
> added in between the existing flags. Not sure.. Or we if we run out
> of free spots at the end we might get rid of the free slots.

Filling this slot with the new deprecated flag should do the trick then.

BTW, -M and -C options for git-blame can help you a bit. They can detect
moved and copied lines, and look for the original one to blame. From man
git-blame:

        -M[<num>]
           Detect moved or copied lines within a file. When a commit
           moves or copies a block of lines (e.g. the original file has
           A and then B, and the commit changes it to B and then A), the
           traditional blame algorithm notices only half of the movement
           and typically blames the lines that were moved up (i.e. B) to
           the parent and assigns blame to the lines that were moved
           down (i.e. A) to the child commit. With this option, both
           groups of lines are blamed on the parent by running extra
           passes of inspection.

           <num> is optional but it is the lower bound on the number of
           alphanumeric characters that Git must detect as
           moving/copying within a file for it to associate those lines
           with the parent commit. The default value is 20.

       -C[<num>]
           In addition to -M, detect lines moved or copied from other
           files that were modified in the same commit. This is useful
           when you reorganize your program and move code around across
           files. When this option is given twice, the command
           additionally looks for copies from other files in the commit
           that creates the file. When this option is given three times,
           the command additionally looks for copies from other files in
           any commit.

           <num> is optional but it is the lower bound on the number of
           alphanumeric characters that Git must detect as
           moving/copying between files for it to associate those lines
           with the parent commit. And the default value is 40. If there
           are more than one -C options given, the <num> argument of the
           last -C will take effect.

[...]

-- 
Regards,
Pratyush Yadav