[PATCH 3/3] mtd: rawnand: brcmnand: exec_op implementation

[dregan at mail.com]

Hi Miquel,
>
> Hello,
> 
> dregan at mail.com wrote on Tue, 10 Oct 2023 04:49:41 +0200:
> 
> > exec_op implementation for Broadcom STB, Broadband and iProc SoC
> > This adds exec_op and removes the legacy interface. Based on changes
> > proposed by Boris Brezillon.
> > 
> > https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba
> > https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696
> > 
> > Signed-off-by: David Regan <dregan at mail.com>
> > 

...

> > -		ret = bcmnand_ctrl_poll_status(ctrl,
> > +		ret = bcmnand_ctrl_poll_status(host,
> 
> Please make this change in a separate preparation patch.

I will add a 3/4 patch to the series which separates out this change.

...

> > +
> > +	if (check_only)
> > +		return 0;
> 
> You initially reported (in your first submission I guess) a number of
> limitations regarding the number of successive address and data cycles.
> If these limitations are real, they must checked. If you need to do
> that, please get inspiration from:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/arasan-nand-controller.c#L904

I believe that was a misunderstanding on my part about how the command
parser was matching a command. There should be no such limitation now
as we can handle all commands.

> 
> > +
> > +	if (op->deassert_wp)
> > +		brcmnand_wp(mtd, 0);
> > +
> > +	for (i = 0; i < op->ninstrs; i++) {
> > +		instr = &op->instrs[i];
> > +
> > +		if ((op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> > +			(op->instrs[0].ctx.cmd.opcode == NAND_CMD_STATUS)) {
> > +			if (instr->type == NAND_OP_DATA_IN_INSTR) {
> 
> I would create two helpers: one to check wether it is a full status
> or reset operation and do these checks outside of this for loop.
> And here handle almost all the cases.

I will move the status and reset checks out of the loop.

...

> > 
> >  	chip->controller = &ctrl->controller;
> > 
> This space can go away.

Will do.

> 
> > +	ctrl->controller.controller_wp = 1;
> > +
> >  	/*
> >  	 * The bootloader might have configured 16bit mode but
> >  	 * NAND READID command only works in 8bit mode. We force
> > --
> > 2.37.3
> > 
> > 
> 
> Otherwise looks pretty good.
> 
> Thanks,
> Miquèl
> 

Thanks!

-Dave