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

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Wed, 21 Feb 2018 00:15:18 +0100
Stefan Agner <stefan@agner.ch> wrote:

> >> +		}
> >> +
> >> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
> >> +		code |= COMMAND_RAR_BYTE1;
> >> +		if (addr < instr->ctx.addr.naddrs) {
> >> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
> >> +			code |= COMMAND_RAR_BYTE2;
> >> +		}
> >> +		if (addr < instr->ctx.addr.naddrs) {
> >> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
> >> +			code |= COMMAND_RAR_BYTE3;
> >> +		}
> >> +
> >> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
> >> +
> >> +		instr = vf610_get_next_instr(subop, &op_id);
> >> +	}
> >> +
> >> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
> >> +		int len = nand_subop_get_data_len(subop, op_id);
> >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> >> +
> >> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
> >> +
> >> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
> >> +				 instr->ctx.data.buf.in + offset,
> >> +				 len);  
> > 
> > I think you have the same endianness problem you have for the READ
> > path. For example, I doubt SET_FEATURES will work properly if you're
> > in LE. So I repeat my initial suggestion: always do the byte swapping
> > when you're transfering data to/from the SRAM from vf610_nfc_cmd()
> > and use vf610_nfc_memcpy() only in the ->read/write_page()
> > implementations. 
> >   
> 
> Hm, but doesn't that leads to wrong order of data when using e.g. raw
> read/write page...?

Yep you'll have to implement ->{read,write}_{page,oob}[_raw](), but I
prefer that to having an ->exec_op() implementation that tries to guess
what the core is trying to do.

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com