Re: [PATCH v4 1/3] spi-nor: Add support for Intel SPI serial flash controller

[Mika Westerberg]

On Wed, Nov 09, 2016 at 02:51:20PM +0100, Cyrille Pitchen wrote:
> > +/* Reads max 64 bytes from the device fifo */
> > +static int intel_spi_read_block(struct intel_spi *ispi, void *buf, size_t size)
> > +{
> > +	size_t bytes;
> > +	int i = 0;
> > +
> > +	if (size > 64)
> 
> This is not a blocking point but just a recommendation: you should define
> and use a macro for this 64 byte FIFO size instead of using this hardcoded
> 64 value here and in intel_spi_write_block(), intel_spi_write(),
> intel_spi_read().

Good point. I'll change that to use a macro.

> > +		return -EINVAL;
> > +
> > +	while (size > 0) {
> > +		bytes = min_t(size_t, size, 4);
> > +		memcpy_fromio(buf, ispi->base + FDATA(i++), bytes);
> Here again another general recommendation: be careful about using
> operators like ++ on macro parameters. In the case of this FDATA() macro
> it will work as expected but unwanted side effect might occur depending on
> the actual macro definition:
> 
> int i = 1;
> 
> #define DOUBLE(n)	((n) + (n))
> DOUBLE(i++); /* here i is incremented twice, not just once. */

Indeed, even though with the current macro it does not happen I'm going
change it to update i separately like:

	memcpy_fromio(buf, ispi->base + FDATA(i), bytes);
	i++

> > +		size -= bytes;
> > +		buf += bytes;
> > +	}
> > +
> > +	return 0;
> > +}

[snip]

> > +static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
> > +			      u_char *read_buf)
> > +{
> > +	struct intel_spi *ispi = nor->priv;
> > +	size_t block_size, retlen = 0;
> > +	u32 val, status;
> > +	ssize_t ret;
> > +
> 
> As I understand some Intel SPI controllers can only use op codes in a fixed
> instruction set, so here you should check the nor->read_opcode.
> Indeed when the support of SFDP tables will be integrated, the
> nor->read_opcode might change between calls of the nor->read() handler.

OK, I did not know that it can change.

> spi_nor_read_sfdp() make use of this nor->read() handler but set
> nor->read_opcode to SPINOR_OP_RDSFDP (5Ah) before calling the handler.
> 
> Then, if intel_spi_read() is called with an unsupported nor->read_opcode,
> it should fail returning -EINVAL. The spi-nor framework will handle this
> failure correctly.
> 
> You can find the SFDP patch here:
> http://patchwork.ozlabs.org/patch/685984/

Thanks for the pointer. I'll update the driver to take read_opcode into
account in intel_spi_read().

Thanks for the comments.