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

[Stefan Agner]

On 21.02.2018 11:09, Miquel Raynal wrote:
> 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.

We can write a driver however we want, the hardware is still strictly
sequentially...

The current implementation should not restrict possible operations in
any way other than it is enforced by hardware anyway...

So the discussion is only about readability/style.

I will make a cleaned up v4 still sequentially and then we can compare
that to a for loop again to make a final decision.

--
Stefan


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