Re: [PATCH v7 07/14] spi: spi-mem: Add an ecc_en parameter to the spi_mem_op structure

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Pratyush,

p.yadav@xxxxxx wrote on Tue, 21 Dec 2021 00:32:29 +0530:

> On 17/12/21 05:16PM, Miquel Raynal wrote:
> > Soon the SPI-NAND core will need a way to request a SPI controller to
> > enable ECC support for a given operation. This is because of the
> > pipelined integration of certain ECC engines, which are directly managed
> > by the SPI controller itself.
> > 
> > Introduce a spi_mem_op additional field for this purpose: ecc_en.
> > 
> > So far this field is left unset and checked to be false by all
> > the SPI controller drivers in their ->supports_op() hook, as they all
> > call spi_mem_default_supports_op().
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > ---
> >  drivers/spi/spi-mem.c       | 5 +++++
> >  include/linux/spi/spi-mem.h | 5 +++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > index cfe1c99db5f3..94758e7e747d 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -178,6 +178,11 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
> >  			return false;
> >  	}
> >  
> > +	if (op->ecc_en) {
> > +		if (!spi_mem_controller_is_capable(ctlr, ecc))
> > +			return false;
> > +	}
> > +
> >  	return spi_mem_check_buswidth(mem, op);
> >  }
> >  EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
> > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> > index d7787c8f3746..e9238a858109 100644
> > --- a/include/linux/spi/spi-mem.h
> > +++ b/include/linux/spi/spi-mem.h
> > @@ -94,6 +94,7 @@ enum spi_mem_data_dir {
> >   *		 operation does not involve transferring data
> >   * @data.buf.in: input buffer (must be DMA-able)
> >   * @data.buf.out: output buffer (must be DMA-able)
> > + * @ecc_en: error correction is required
> >   */
> >  struct spi_mem_op {
> >  	struct {
> > @@ -126,6 +127,8 @@ struct spi_mem_op {
> >  			const void *out;
> >  		} buf;
> >  	} data;
> > +
> > +	bool ecc_en;  
> 
> ECC should only concern the data phase right? Would it make more sense 
> to move this field under data?
> 
> Anyway, I don't know much about NAND or ECC so either way,

I've moved the field to the data struct because it makes indeed more
sense. I've also changed the type and name to "u8 ecc : 1;" to match
the style used for the dtr parameter.

> 
> Acked-by: Pratyush Yadav <p.yadav@xxxxxx>
> 
> >  };
> >  
> >  #define SPI_MEM_OP(__cmd, __addr, __dummy, __data)		\
> > @@ -223,9 +226,11 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
> >  /**
> >   * struct spi_controller_mem_caps - SPI memory controller capabilities
> >   * @dtr: Supports DTR operations
> > + * @ecc: Supports operations with error correction
> >   */
> >  struct spi_controller_mem_caps {
> >  	bool dtr;
> > +	bool ecc;
> >  };
> >  
> >  #define spi_mem_controller_is_capable(ctlr, cap)		\
> > -- 
> > 2.27.0
> >   
> 


Thanks,
Miquèl