From: Felipe Balbi <balbi@ti.com> To: Sourav Poddar <sourav.poddar@ti.com> Cc: <broonie@kernel.org>, <spi-devel-general@lists.sourceforge.net>, <grant.likely@linaro.org>, <balbi@ti.com>, <rnayak@ti.com>, <linux-omap@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller Date: Thu, 18 Jul 2013 13:24:04 +0300 [thread overview] Message-ID: <20130718102404.GH11251@arwen.pp.htv.fi> (raw) In-Reply-To: <1374141687-10790-3-git-send-email-sourav.poddar@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 --]
WARNING: multiple messages have this Message-ID (diff)
From: Felipe Balbi <balbi@ti.com> To: Sourav Poddar <sourav.poddar@ti.com> Cc: broonie@kernel.org, spi-devel-general@lists.sourceforge.net, grant.likely@linaro.org, balbi@ti.com, rnayak@ti.com, linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller Date: Thu, 18 Jul 2013 13:24:04 +0300 [thread overview] Message-ID: <20130718102404.GH11251@arwen.pp.htv.fi> (raw) In-Reply-To: <1374141687-10790-3-git-send-email-sourav.poddar@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 --]
next prev parent reply other threads:[~2013-07-18 10:24 UTC|newest] Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-07-18 10:01 [PATCH 0/3] spi changes and ti quad spi controller Sourav Poddar 2013-07-18 10:01 ` Sourav Poddar 2013-07-18 10:01 ` Sourav Poddar 2013-07-18 10:01 ` [RFC/PATCHv2 1/3] driver: spi: Modify core to compute the message length Sourav Poddar 2013-07-18 10:01 ` Sourav Poddar 2013-07-18 10:01 ` Sourav Poddar 2013-07-18 15:22 ` Mark Brown 2013-07-18 10:01 ` [PATCHv4 2/3] drivers: spi: Add qspi flash controller Sourav Poddar 2013-07-18 10:01 ` Sourav Poddar 2013-07-18 10:01 ` Sourav Poddar 2013-07-18 10:24 ` Felipe Balbi [this message] 2013-07-18 10:24 ` Felipe Balbi 2013-07-18 11:18 ` Sourav Poddar 2013-07-18 11:18 ` Sourav Poddar 2013-07-18 11:18 ` Sourav Poddar 2013-07-18 11:24 ` Felipe Balbi 2013-07-18 11:24 ` Felipe Balbi 2013-07-18 12:24 ` Sourav Poddar 2013-07-18 12:24 ` Sourav Poddar 2013-07-18 12:24 ` Sourav Poddar 2013-07-19 11:48 ` Sourav Poddar 2013-07-19 11:48 ` Sourav Poddar 2013-07-19 12:20 ` Felipe Balbi 2013-07-19 12:20 ` Felipe Balbi 2013-07-18 10:42 ` Mark Brown 2013-07-18 11:45 ` Sourav Poddar 2013-07-18 11:45 ` Sourav Poddar 2013-07-18 11:56 ` Felipe Balbi 2013-07-18 11:56 ` Felipe Balbi 2013-07-18 13:18 ` Mark Brown 2013-07-18 13:31 ` Felipe Balbi 2013-07-18 13:31 ` Felipe Balbi 2013-07-18 14:42 ` Mark Brown 2013-07-18 14:55 ` Sourav Poddar 2013-07-18 14:55 ` Sourav Poddar 2013-07-18 15:28 ` Mark Brown 2013-07-19 11:55 ` Sourav Poddar 2013-07-19 11:55 ` Sourav Poddar 2013-07-18 19:08 ` Trent Piepho 2013-07-18 19:08 ` Trent Piepho 2013-07-18 20:39 ` Mark Brown 2013-07-18 20:59 ` Nishanth Menon 2013-07-18 20:59 ` Nishanth Menon 2013-07-19 5:02 ` Sourav Poddar 2013-07-19 5:02 ` Sourav Poddar 2013-07-18 10:01 ` [RFC/PATCHv2 3/3] driver: spi: Add quad spi read support Sourav Poddar 2013-07-18 10:01 ` Sourav Poddar 2013-07-18 10:01 ` Sourav Poddar 2013-07-18 10:44 ` Mark Brown 2013-07-18 11:52 ` Sourav Poddar 2013-07-18 11:52 ` Sourav Poddar
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20130718102404.GH11251@arwen.pp.htv.fi \ --to=balbi@ti.com \ --cc=broonie@kernel.org \ --cc=grant.likely@linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=rnayak@ti.com \ --cc=sourav.poddar@ti.com \ --cc=spi-devel-general@lists.sourceforge.net \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.