Re: [PATCH v4 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Thu, 16 Dec 2021 18:22:45 +0200
Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> wrote:

> Hi,
> 
> On Thu, Dec 16, 2021 at 11:51:00AM +0100, Boris Brezillon wrote:
> > On Thu, 18 Nov 2021 16:05:42 +0300
> > Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> >   
> > > +static bool intel_spi_cmp_mem_op(const struct intel_spi_mem_op *iop,
> > > +				 const struct spi_mem_op *op)
> > > +{
> > > +	if (iop->mem_op.cmd.nbytes != op->cmd.nbytes ||
> > > +	    iop->mem_op.cmd.buswidth != op->cmd.buswidth ||
> > > +	    iop->mem_op.cmd.dtr != op->cmd.dtr ||
> > > +	    iop->mem_op.cmd.opcode != op->cmd.opcode)
> > > +		return false;
> > > +
> > > +	if (iop->mem_op.addr.nbytes) {
> > > +		if (iop->mem_op.addr.nbytes != op->addr.nbytes ||
> > > +		    iop->mem_op.addr.dtr != op->addr.dtr)
> > > +			return false;
> > > +	}  
> > 
> > Hm, are you sure you want to allow op->addr.nbytes > 0 when
> > iop->mem_op.addr.nbytes == 0? Feels like the command should be reported
> > as unsupported in that case. Unless 0 is a wildcard meaning 'any', but
> > that would be confusing, since operations with 0 address bytes are
> > valid, and I actually expect the number of address cycles to be fixed 
> > or bounded.  
> 
> Indeed. I will change it to:
> 
> 	if (iop->mem_op.addr.nbytes) {
> 		if (iop->mem_op.addr.nbytes != op->addr.nbytes ||
> 		    iop->mem_op.addr.dtr != op->addr.dtr)
> 			return false;
> 	} else if (op->addr.nbytes > 0) {
> 		return false;
> 	}

Why no just

	if (iop->mem_op.addr.nbytes != op->addr.nbytes ||
	    iop->mem_op.addr.dtr != op->addr.dtr)
		return false;

then?

> 
> in v5 if that's what you meant.