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:
signature.asc
Description: Digital signature
