Re: [PATCH V4] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-02-11 19:37, Miquel Raynal wrote:
> Hello,
>
> Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> wrote on Wed,
> 10 Feb 2021 14:31:44 +0530:
>
> > On Fri, Jan 29, 2021 at 03:09:19AM +0530, Md Sadre Alam wrote:
> > > From QPIC version 2.0 onwards new register got added to
> > > read last codeword. This change will add the READ_LOCATION_LAST_CW_n
> > > register.
> > >
> > > For first three code word READ_LOCATION_n register will be
> > > use.For last code word READ_LOCATION_LAST_CW_n register will be
> > > use.
>
> Sorry for the late notice, I think the patch is fine but if you don't
> mind I would like to propose a small change that should simplify your
> patch a lot, see below.
>
> > >
> > > Signed-off-by: Md Sadre Alam <mdalam@xxxxxxxxxxxxxx>
> >
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> >
> > Thanks,
> > Mani
> >
> > > ---
> > > [V4]
> > >  * Modified condition for nandc_set_read_loc_last() in qcom_nandc_read_cw_raw().
> > >  * Added one additional argument "last_cw" to the function config_nand_cw_read()
> > >    to handle last code word condition.
> > >  * Changed total number of last code word register "NAND_READ_LOCATION_LAST_CW_0" to 4
> > >    while doing code word configuration.
> > >  drivers/mtd/nand/raw/qcom_nandc.c | 110 +++++++++++++++++++++++++++++---------
> > >  1 file changed, 84 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> > > index 667e4bf..9484be8 100644
> > > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> > > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> > > @@ -48,6 +48,10 @@
> > >  #define	NAND_READ_LOCATION_1		0xf24
> > >  #define	NAND_READ_LOCATION_2		0xf28
> > >  #define	NAND_READ_LOCATION_3		0xf2c
> > > +#define	NAND_READ_LOCATION_LAST_CW_0	0xf40
> > > +#define	NAND_READ_LOCATION_LAST_CW_1	0xf44
> > > +#define	NAND_READ_LOCATION_LAST_CW_2	0xf48
> > > +#define	NAND_READ_LOCATION_LAST_CW_3	0xf4c
> > >
> > >  /* dummy register offsets, used by write_reg_dma */
> > >  #define	NAND_DEV_CMD1_RESTORE		0xdead
> > > @@ -187,6 +191,12 @@ nandc_set_reg(nandc, NAND_READ_LOCATION_##reg,			\
> > >  	      ((size) << READ_LOCATION_SIZE) |			\
> > >  	      ((is_last) << READ_LOCATION_LAST))
> > >
> > > +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last)	\
> > > +nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg,			\
> > > +	      ((offset) << READ_LOCATION_OFFSET) |		\
> > > +	      ((size) << READ_LOCATION_SIZE) |			\
> > > +	      ((is_last) << READ_LOCATION_LAST))
> > > +
>
> You could rename the macro nandc_set_read_loc() into
> nandc_set_read_loc_first() or anything else that make sense, then have
> a helper which does:
>
> nandc_set_read_loc()
> {
> 	if (condition for first)
> 		return nandc_set_read_loc_first();
> 	else
> 		return nandc_set_read_loc_last();
> }

  Yes this is more precise way & simplify the patch a lot.
  But for this i have to change these two macro as a function.

  nandc_set_read_loc() & nandc_set_read_loc_last().

  Since for last code word register we are using Token Pasting 
Operator##.

  So if i am implementing like the below.

  /* helper to configure location register values */
  static void nandc_set_read_loc(struct qcom_nand_controller *nandc, int 
reg,
                  int offset, int size, int is_last, bool last_cw)
  {
          if (last_cw)
                  return nandc_set_read_loc_last(nandc, reg, offset, 
size, is_last);
          else
                  return nandc_set_read_loc_first(nandc, reg, offset, 
size, is_last);
 }

  So here for macro expansion reg should be a value not a variable else 
it will be expended like
  NAND_READ_LOCATION_LAST_CW_reg instead of 
NAND_READ_LOCATION_LAST_CW_0,1,2,3 etc.

 the call for nandc_set_read_loc() as nandc_set_read_loc(nandc, 0, 
read_loc, data_size1, 0, true); ---> for last code word.
 nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0, false); ---> for 
first three code wrod.


 So is this ok for you to convert these two macro into function ?

> And in the rest of your patch you won't have to touch anything else.
>
> Thanks,
> Miquèl