Re: [PATCH v7 01/14] spi: spi-mem: Fix a DTR related check in spi_mem_dtr_supports_op()

[Pratyush Yadav <p.yadav@xxxxxx>]

On 21/12/21 10:50AM, Miquel Raynal wrote:
> Hi Pratyush,
> 
> p.yadav@xxxxxx wrote on Tue, 21 Dec 2021 00:09:19 +0530:
> 
> > On 17/12/21 05:16PM, Miquel Raynal wrote:
> > > It seems that the number of command bytes must be "2" only when the
> > > command itself is sent in DTR mode. The current logic checks if the
> > > number of command bytes is "2" when any of the cycles is a DTR cycle. It
> > > is likely that so far no device was actually mixing DTR/non-DTR cycles
> > > in the same operation, explaining why this was left undetected until
> > > now.  
> > 
> > This was intentional. spi_mem_dtr_supports_op() must only be called when 
> > the operation is DTR in all phases so I did not add any sanity checks if 
> > someone was using it for non-DTR ops.
> 
> Maybe that was the original intention but since then the Macronix
> driver has been merged and supports (at lest does not reject) these
> modes.

But I don't see any code that actually creates mixed DTR ops. SPI NOR 
does not do it for sure, and SPI NAND does not have DTR support yet. 
Those are the only two users of SPI MEM I can see. So we are solving at 
a problem that doesn't exist yet. Which is fine of course, I just want 
to point it out.

> 
> > In fact, I added on to this 
> > function in [0] to check nbytes for other phases as well. The patch fell 
> > off my radar unfortunately, and it didn't get merged.
> > 
> > I would like to keep this as it is since we have no user of mixed 
> > DTR/non-DTR modes yet.
> 
> I don't know if the Macronix driver really supports it or if it is the
> driver that is doing the wrong checks but in appearance such mixed mode
> could be used.
> 
> > But if you really want to support it, please 
> > apply my patch first to make sure we check for every phase, not just 
> > command.
> > 
> > [0] https://lore.kernel.org/all/20210531181757.19458-5-p.yadav@xxxxxx/
> 
> Nice, I might take that as well indeed in order to make the checks a
> little bit more robust.

Thanks.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.