Re: [RFC 02/12] mtd: nand: force drivers to explicitly send READ/PROG commands

[Miquel RAYNAL <miquel.raynal@free-electrons.com>]

Hi Stefan,

> Thanks for the work on this, happy to see the new interface is moving
> forward. Some comments below.

Thank you!

> > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > index c8ceaecd8065..3f2b903158c1 100644
> > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > @@ -1043,6 +1043,8 @@ static int gpmi_ecc_read_page(struct mtd_info
> > *mtd, struct nand_chip *chip,
> >  	unsigned int  max_bitflips = 0;
> >  	int           ret;
> >  
> > +	nand_read_page_op(chip, page, 0, NULL, 0);
> > +
> >  	dev_dbg(this->dev, "page number is : %d\n", page);
> >  	ret = read_page_prepare(this, buf, nfc_geo->payload_size,
> >  					this->payload_virt,
> > this->payload_phys, @@ -1220,12 +1222,13 @@ static int
> > gpmi_ecc_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
> >  	meta = geo->metadata_size;
> >  	if (first) {
> >  		col = meta + (size + ecc_parity_size) * first;
> > -		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, col, -1);  
> 
> Shouldn't we add nand_change_read_column_op here?

The change column operation is done right after with
"nand_read_page_op()" that also set the page. I am pretty sure this
overall operation could be handled in a cleaner way though.

Could you test this driver with ->exec_op() implementation? If you do I
am interested to know if all goes well or not.

> 
> 
> >  
> >  		meta = 0;
> >  		buf = buf + first * size;
> >  	}
> >  
> > +	nand_read_page_op(chip, page, col, NULL, 0);
> > +
> >  	/* Save the old environment */
> >  	r1_old = r1_new = readl(bch_regs + HW_BCH_FLASH0LAYOUT0);
> >  	r2_old = r2_new = readl(bch_regs + HW_BCH_FLASH0LAYOUT1);

[...]

> 
> > diff --git a/drivers/mtd/nand/vf610_nfc.c
> > b/drivers/mtd/nand/vf610_nfc.c index 8037d4b48a05..80d31a58e558
> > 100644 --- a/drivers/mtd/nand/vf610_nfc.c
> > +++ b/drivers/mtd/nand/vf610_nfc.c
> > @@ -560,7 +560,7 @@ static int vf610_nfc_read_page(struct mtd_info
> > *mtd, struct nand_chip *chip,
> >  	int eccsize = chip->ecc.size;
> >  	int stat;
> >  
> > -	vf610_nfc_read_buf(mtd, buf, eccsize);
> > +	nand_read_page_op(chip, page, 0, buf, eccsize);
> >  	if (oob_required)
> >  		vf610_nfc_read_buf(mtd, chip->oob_poi,
> > mtd->oobsize); 
> > @@ -580,7 +580,7 @@ static int vf610_nfc_write_page(struct mtd_info
> > *mtd, struct nand_chip *chip,
> >  {
> >  	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> >  
> > -	vf610_nfc_write_buf(mtd, buf, mtd->writesize);  
> 
> We currently ignore NAND_CMD_SEQIN in ->cmdfunc anyway, so I think
> this change would not even be necessary here.
> 

Boris already answered this one, I will let it as is to remain
consistent but thanks for the remark.

> 
> This is a NAND controller which will benefit from the new interface. I
> plan to convert the driver to the new interface, maybe I manage to do
> that soon so we have a second driver making use of the new interface.

That would be great! I am writing the kernel-doc aside, I hope it will
be ready soon, but in any case do not hesitate if you need help.

Thank you,
Miquèl