Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Stefan,


> >> >> +{
> >> >> +	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
> >> >> +			    COL_ADDR_SHIFT, col);
> >> >> +
> >> >> +	vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
> >> >> +			    ROW_ADDR_SHIFT, row);
> >> >> +
> >> >> +	vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
> >> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
> >> >> +	vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
> >> >> +
> >> >> +	dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
> >> >> +		col, row, cmd1, cmd2, trfr_sz);
> >> >> +
> >> >> +	vf610_nfc_done(nfc);
> >> >> +}
> >> >> +
> >> >> +static inline const struct nand_op_instr *vf610_get_next_instr(
> >> >> +	const struct nand_subop *subop, int *op_id)
> >> >> +{
> >> >> +	if (*op_id + 1 >= subop->ninstrs)
> >> >> +		return NULL;
> >> >> +
> >> >> +	(*op_id)++;
> >> >> +
> >> >> +	return &subop->instrs[*op_id];
> >> >> +}
> >> >> +
> >> >> +static int vf610_nfc_cmd(struct nand_chip *chip,
> >> >> +				const struct nand_subop *subop)
> >> >> +{
> >> >> +	const struct nand_op_instr *instr;
> >> >> +	struct vf610_nfc *nfc = chip_to_nfc(chip);
> >> >> +	struct mtd_info *mtd = nand_to_mtd(chip);
> >> >> +	int op_id = -1, addr = 0, trfr_sz = 0;
> >> >> +	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
> >> >> +
> >> >> +	/* Some ops are optional, but they need to be in order */
> >> >> +	instr = vf610_get_next_instr(subop, &op_id);
> >> >> +	if (!instr)
> >> >> +		return -EINVAL;
> >> >> +
> >> >> +	if (instr && instr->type == NAND_OP_CMD_INSTR) {
> >> >> +		dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode);
> >> >> +		cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
> >> >> +		code |= COMMAND_CMD_BYTE1;
> >> >> +
> >> >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> >> +	}
> >> >> +
> >> >> +	if (instr && instr->type == NAND_OP_ADDR_INSTR) {
> >> >> +  
> >> >
> >> > Hm, you're still checking the sequencing consistency here. This should
> >> > have been taken care by the core parser already, so really, this is not
> >> > needed.
> >> >  
> >>
> >> Since those operations are optional, I have to check...  
> > 
> > Looks like my previous pastebin was not containing all the changes I
> > wanted to share with you, so here is the full version [1].
> > 
> > Hm, what do these checks add to the checks done in the parser? Even if
> > all instructions are optional in your pattern the core parser still
> > check that when they appear in the proper order. So, you can for
> > example never have CMD1+CMD2+ADDR*5 passed to vf610_nfc_cmd().  
> 
> Sure...
> 
> > 
> > The only exception I can think of is when you have DATA_OUT+CMD. In
> > this case, you probably want to skip CMD_BYTE1 and use CMD_BYTE2
> > directly, but that can easily be taken care of in the alternative
> > implementation I proposed:
> > 
> > 		if (instr->type == NAND_OP_DATA_OUT_INSTR) {
> > 			/*
> > 			 * If there was no CMD instruction before the
> > 			 * DATA_OUT one, we must skip CMD_BYTE1 and use
> > 			 * CMD_BYTE2 directly so that the CMD cycle is
> > 			 * really placed after the DATA_OUT one.
> > 			 */
> > 			if (!ncmds)
> > 				ncmds++;
> > 			....
> > 		}
> >  
> >   
> 
> I fully understand, I do not have to enforce order since I can rely on
> the order passed by the stack.
> 
> It is also not what I am after. I just check which operation is going to
> be next. Like your switch statement.
> 
> I just see don't see a real advantage in using a for loop. It makes it
> harder to read, since the order of operations will no more be obvious.
> It makes this ncmd work around necessary.

Well, the aim of ->exec_op() was initially to get rid of the controller
specificities and be free to implement new NAND operations without
having to check every controller driver in the world, so doing it this
way freezes the possibilities to a reduced set of operations (ie. what
we do today). This is wrong from my point of view and I would really
welcome a for-loop approach instead.

Thanks,
Miquèl

> 
> Using a for loop just because we can? I haven't seen a convincing
> argument so far.
> 
> 

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com