Re: [PATCH 01/23] mtd: spi-nor: Stop prefixing generic functions with a manufacturer name

[Tudor.Ambarus]

On Friday, March 13, 2020 11:30:07 AM EET Boris Brezillon wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Fri, 13 Mar 2020 11:34:55 +0530
> 
> Vignesh Raghavendra <vigneshr@xxxxxx> wrote:
> > On 02/03/20 11:37 pm, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> > > From: Boris Brezillon <bbrezillon@xxxxxxxxxx>
> > > 
> > > Replace the manufacturer prefix by something describing more precisely
> > > what those functions do.
> > > 
> > > Signed-off-by: Boris Brezillon <bbrezillon@xxxxxxxxxx>
> > > [tudor.ambarus@xxxxxxxxxxxxx: prepend spi_nor_ to all modified methods.]
> > > Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
> > > ---
> > > 
> > >  drivers/mtd/spi-nor/spi-nor.c | 88 ++++++++++++++++++-----------------
> > >  1 file changed, 45 insertions(+), 43 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > b/drivers/mtd/spi-nor/spi-nor.c
> > > index caf0c109cca0..b15e262765e1 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > @@ -568,14 +568,15 @@ static int spi_nor_read_cr(struct spi_nor *nor, u8
> > > *cr)> > 
> > >  }
> > >  
> > >  /**
> > > 
> > > - * macronix_set_4byte() - Set 4-byte address mode for Macronix flashes.
> > > + * spi_nor_en4_ex4_set_4byte() - Enter/Exit 4-byte mode for Macronix
> > > like
> > > + * flashes.
> > > 
> > >   * @nor:   pointer to 'struct spi_nor'.
> > >   * @enable:        true to enter the 4-byte address mode, false to exit
> > >   the 4-byte *         address mode.
> > >   *
> > >   * Return: 0 on success, -errno otherwise.
> > >   */
> > > 
> > > -static int macronix_set_4byte(struct spi_nor *nor, bool enable)
> > > +static int spi_nor_en4_ex4_set_4byte(struct spi_nor *nor, bool enable)
> > 
> > Sounds a bit weird, how about simplifying this to:
> >       spi_nor_set_4byte_addr_mode()
> > 
> > Or if you want to be specific:
> >       spi_nor_en_ex_4byte_addr_mode()
> 
> You're right. Maybe we can simplify things by having a single function
> that does optional steps based on new flags
> 
> SPI_NOR_EN_EX_4B_NEEDS_WEN
> SPI_NOR_CLEAR_EAR_ON_4B_EXIT
> 
> This should probably be done in a separate patch though, so ack on the
> spi_nor_en_ex_4byte_addr_mode() rename, assuming we also change the
> bool argument name to enter.

A single big function will be ugly to handle because of the 
spansion_set_4byte() -> it uses a different opcode.

I like the nor->params>set_4byte hook.

I think that spi_nor_en4_ex4_set_4byte() can be renamed to spi_nor_set_4byte() 
and be the only set_4byte() method exposed to manufacturers. 
spansion_set_4byte() will be static in core.c and the rest will be private in 
the manufacturer drivers.

Here's how the manufacturers enter and exit the 4 byte mode:
-> eon, gidadevice, issi, macronix, xmc use EN4B/EX4B
-> micron-st needs WEN -> private method as they are the only ones that 
require this
-> newer spansion have a 4BAM opcode (new, public command), older spansion 
flashes use BRWR (legacy in core.c, spansion_set_4byte())
-> winbond set_4byte method is hackish and may be reason for just a flash 
fixup hook -> private method

Let me know if you agree with this, so I can respin later today or tomorrow.

cut

> 
> > I expect sending WREN should be harmless even for cmds that don't expect
> > one.

The commands may be ok, but the flash can be corrupted. The WEL bit is NOT 
reset after the completion of the EN4B command on macronix flashes, so the 
gates are opened for some inadvertent commands that may corrupt the memory.

Cheers,
ta