Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3390 bytes --]

Hi,

it might be just me, but ...

On Thu, Jul 18, 2013 at 03:31:26PM +0530, Sourav Poddar wrote:
> +static inline unsigned long ti_qspi_readl_data(struct ti_qspi *qspi,
> +		unsigned long reg, int wlen)
> +{
> +	switch (wlen) {
> +	case 8:
> +		return readw(qspi->base + reg);
> +		break;
> +	case 16:
> +		return readb(qspi->base + reg);
> +		break;
> +	case 32:
> +		return readl(qspi->base + reg);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static inline void ti_qspi_writel_data(struct ti_qspi *qspi,
> +		unsigned long val, unsigned long reg, int wlen)
> +{
> +	switch (wlen) {
> +	case 8:
> +		writew(val, qspi->base + reg);
> +		break;
> +	case 16:
> +		writeb(val, qspi->base + reg);
> +		break;
> +	case 32:
> +		writeb(val, qspi->base + reg);
> +		break;
> +	default:
> +		dev_dbg(qspi->dev, "word lenght out of range");
> +		break;
> +	}
> +}

because of these two functions you have the hability to read/write
*more* than one byte, and yet ...

> +static void qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
> +{
> +	const u8 *txbuf;
> +	int wlen, count;
> +
> +	count = t->len;
> +	txbuf = t->tx_buf;
> +	wlen = t->bits_per_word;
> +
> +	while (count--) {
> +		dev_dbg(qspi->dev, "tx cmd %08x dc %08x data %02x\n",
> +			qspi->cmd | QSPI_WR_SNGL, qspi->dc, *txbuf);
> +		ti_qspi_writel_data(qspi, *txbuf++, QSPI_SPI_DATA_REG, wlen);

you always increment by each byte. Here, if you used writel(), you wrote
4 bytes and should increment txbuf by 4. Same goes for read_data(),
below. Another thing. Even though your wlen might be 8 bits, if you
write 4 bytes to write, you can save a few CPU cycles by using writel().

You only use writew() if you have exactly 2 bytes to write and writeb()
if you have exactly 1 byte to write. 3 bytes we'll be left as an
exercise.

> +static int ti_qspi_start_transfer_one(struct spi_master *master,
> +		struct spi_message *m)
> +{
> +	struct ti_qspi *qspi = spi_master_get_devdata(master);
> +	struct spi_device *spi = m->spi;
> +	struct spi_transfer *t;
> +	int status = 0, ret;
> +	int frame_length;
> +
> +	/* setup device control reg */
> +	qspi->dc = 0;
> +
> +	if (spi->mode & SPI_CPHA)
> +		qspi->dc |= QSPI_CKPHA(spi->chip_select);
> +	if (spi->mode & SPI_CPOL)
> +		qspi->dc |= QSPI_CKPOL(spi->chip_select);
> +	if (spi->mode & SPI_CS_HIGH)
> +		qspi->dc |= QSPI_CSPOL(spi->chip_select);
> +
> +	frame_length = DIV_ROUND_UP(m->frame_length * spi->bits_per_word,
> +				spi->bits_per_word);

this calculation doesn't look correct.

	(m->frame_length * spi->bits_per_word) /
		spi->bits_per_word = m->frame_length

What are you trying to achieve here ? frame_length should be counted in
words right ? And we get that value in bytes. So what's the best
calculation to convert bytes into words ? If you have 8 bits_per_word
you don't need any calculation, but if you have 32 bits_per_word, you
_do_ need something.

How will you achieve the number you want ? (hint: 1 byte == 8 bits)

And btw, all of these mistakes pretty much tell me that this driver
hasn't been tested. How have you tested this driver ? Is your spansion
memory accessed with 8 bits_per_word only ? Is there anyway to use
32 bits_per_word with that device ? That would uncover quite a few
mistakes in $subject.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]