[PATCH v7 1/3] mtd: spi-nor: spansion: Add support for Read/Write Any Register

[Pratyush Yadav]

On 28/01/22 09:43AM, Tudor.Ambarus at microchip.com wrote:
> On 7/19/21 11:03, tkuw584924 at gmail.com wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> > 
> > Some of Spansion/Cypress chips support Read/Write Any Register commands.
> > These commands are mainly used to write volatile registers.
> > 
> > The Read Any Register instruction (65h) is followed by register address
> > and dummy cycles, then the selected register byte is returned.
> > 
> > The Write Any Register instruction (71h) is followed by register address
> > and register byte to write.
> > 
> > Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> > ---
> > Changes in v7:
> >   - No change
> > 
> > Changes in v6:
> >   - Add helper functions for controller_ops
> >   - Add 'reg_addr_width' parameter to spansion_read/write_any_reg()
> >   - Remove spi_nor_write_enable() from spansion_write_any_reg() and modified
> >     function header comment
> > 
> > Changes in v5:
> >   - Fix 'if (ret == 1)' to 'if (ret < 0)' in spansion_read_any_reg()
> > 
> > Changes in v4:
> >   - Fix dummy cycle calculation in spansion_read_any_reg()
> >   - Modify comment for spansion_write_any_reg()
> > 
> > Changes in v3:
> >   - Cleanup implementation
> > 
> >  drivers/mtd/spi-nor/spansion.c | 142 +++++++++++++++++++++++++++++++++
> >  1 file changed, 142 insertions(+)
> > 
> > +/**
> > + * spansion_read_any_reg() - Read Any Register.
> > + * @nor:               pointer to a 'struct spi_nor'
> > + * @reg_addr:          register address
> > + * @reg_addr_width:    number of address bytes
> > + * @reg_dummy:         number of dummy cycles for register read
> > + * @reg_val:           pointer to a buffer where the register value is copied
> > + *
> > + * Return: 0 on success, -errno otherwise.
> > + */
> > +static int spansion_read_any_reg(struct spi_nor *nor, u32 reg_addr,
> > +                                u8 reg_addr_width, u8 reg_dummy, u8 *reg_val)
> 
> how about making this a generic core method? I'm sure there are other SPI NOR
> vendors that use registers indexed by address. How about introducing:
> 
> struct spi_nor_reg {
> 	u32 addr;
> 	u8 addr_width;
> 	u8 dummy;
> 	u8 opcode;
> 	u8 val;
> };
> 
> and passing a pointer to this struct as argument.

Wouldn't it be simpler if they fill it in a struct spi_mem_op, and then 
just pass that in as a "template"?

> 
> > +{
> > +       if (nor->spimem) {
> > +               struct spi_mem_op op =
> > +                       SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RD_ANY_REG, 0),
> > +                                  SPI_MEM_OP_ADDR(reg_addr_width, reg_addr, 0),
> > +                                  SPI_MEM_OP_DUMMY(reg_dummy, 0),
> > +                                  SPI_MEM_OP_DATA_IN(1, reg_val, 0));
> > +
> > +               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> > +
> > +               /* convert the dummy cycles to the number of bytes */
> > +               op.dummy.nbytes = (reg_dummy * op.dummy.buswidth) / 8;
> > +               if (spi_nor_protocol_is_dtr(nor->reg_proto))
> > +                       op.dummy.nbytes *= 2;
> > +
> > +               return spi_mem_exec_op(nor->spimem, &op);
> > +       }
> > +
> else
> 	return -EOPNOTSUPP;
> 
> we'd like to move all the SPI NOR controller drivers under SPI MEM, let's
> not add support for them.

Sounds good to me.

> 
> > +       return controller_ops_read_any_reg(nor, reg_addr, reg_addr_width,
> > +                                          reg_dummy, reg_val);
> > +}

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.