Re: [PATCH 3/5] spi: spi-nxp-fspi: add DTR mode support

[Michael Walle <mwalle@kernel.org>]

Hi,

>> > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
>> > index 9d6b4d22263c..2562d524149e 100644
>> > --- a/drivers/spi/spi-nxp-fspi.c
>> > +++ b/drivers/spi/spi-nxp-fspi.c
>> > @@ -552,12 +552,22 @@ static void nxp_fspi_prepare_lut(struct nxp_fspi *f,
>> >  	int lutidx = 1, i;
>> >
>> >  	/* cmd */
>> > -	lutval[0] |= LUT_DEF(0, LUT_CMD, LUT_PAD(op->cmd.buswidth),
>> > -			     op->cmd.opcode);
>> > +	if (op->cmd.dtr) {
>> > +		lutval[0] |= LUT_DEF(0, LUT_CMD_DDR,
>> LUT_PAD(op->cmd.buswidth),
>> > +				     op->cmd.opcode >> 8);
>> 
>> Shouldn't we check cmd.nbytes here? You seem to mix dtr with 
>> cmd.nbytes ==
>> 2 here.
> 
> Currently, for DTR mode, all cmd.nbytes == 2. Refer to
> drivers/mtd/spi-nor/core.c : spi_nor_spimem_setup_op()
> But better to check the cmd.nbytes here to make the code more strong.

I'm aware of that, but that might change.

>> > +		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_CMD_DDR,
>> > +					      LUT_PAD(op->cmd.buswidth),
>> > +					      op->cmd.opcode & 0x00ff);
>> 
>> And you seem to assume dtr is always octal mode?
> 
> Currently, I only test the octa dtr mode(8D-8D-8D). but here, we config 
> the
> op->cmd.buswidth, op->addr.buswidth, op->dummy.buswidth, 
> op->data.buswidth.
> So I think current LUT config can cover other dts mode, like 1D-8D-8D,
> 1D-4D-4D, 1D-2D-2D, 1D-1D-1D.

Agreed, for the code that follows this. But the 16bit opcode, ie. the
LUT above only makes sense for 8d8d8d. There you have to have
16bit because thats what transferred in one clock cycle.

You could add that to your .supports_op(). I.e. restrict this
driver to only support 8d8d8d. I know that the default op will
already check that, but better be safe.

-michael