On Wednesday 31 July 2013 02:50 PM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Jul 31, 2013 at 02:40:51PM +0530, Sourav Poddar wrote:
>>>> +#define QSPI_FRAME_MAX 0xfff
>>> Frame max is 4096, 0x1000, right ?
>> Yes,
>> this macro was used initially to fill the register bits, where 4095 =
>> 4096 words.
>> Will change it to now.
> you can make this something like:
>
> #define QSPI_FRAME(n) (((n) - 1)& 0xfff)
>
Yes, but now its only used in a clamp function, where I should
provide the exact value 4096. Will use your previous suggestion.
#define QSPI_FRAME_MAX 0x1000
>>>> +static int qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
>>>> +{
>>>> + const u8 *txbuf;
>>>> + int wlen, count, ret;
>>>> +
>>>> + count = t->len;
>>>> + txbuf = t->tx_buf;
>>>> + wlen = t->bits_per_word;
>>>> +
>>>> + while (count--) {
>>> you're decrementing count by one, but in some cases you write 4 bytes or
>>> 2 bytes... This will blow up very soon. I can already see overflows
>>> happening...
>> we write 2 bytes and 4 bytes for 16 bits_per_word and 32 bits_per_word case.
>> count is t->len, which is the total number of words to transfer. This
> t->len is total number of bytes as you can see from the documentation in
> the header:
>
> * @len: size of rx and tx buffers (in bytes)
>
> As I said before, please read the documentation.
>
>> words can be of any length (1, 2 or 4) bytes. So, I think it should be
>> decremented by 1 only.
> this is wrong.
>
hmm..got the point.
I will pass the count address also to ti_qspi_read_data/write_data and make
use of the switch statement to decrement the count.
>>>> +static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + if (t->tx_buf) {
>>>> + ret = qspi_write_msg(qspi, t);
>>>> + if (ret) {
>>>> + dev_dbg(qspi->dev, "Error while writing\n");
>>>> + return -EINVAL;
>>> why do you change the return value from qspi_write_msg() ?
>>>
>> I was not sure about this, I thought I had signals an ETIMEOUT during
>> timeout, So signal a invalid transfer here.
>> Do you suggest keeping ETIMEOUT here also?
> yeah, so we tell whoever called us that the transfer timed out. If you
> return -EINVAL you're telling your clients they gave you an invalid
> spi_transfer, which is not the case.
>
Ok.
>>>> +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 = (m->frame_length<< 3) / spi->bits_per_word;
>>>> +
>>>> + frame_length = clamp(frame_length, 0, QSPI_FRAME_MAX);
>>>> +
>>>> + /* setup command reg */
>>>> + qspi->cmd = 0;
>>>> + qspi->cmd |= QSPI_EN_CS(spi->chip_select);
>>>> + qspi->cmd |= QSPI_FLEN(frame_length);
>>>> + qspi->cmd |= QSPI_WC_CMD_INT_EN;
>>>> +
>>>> + ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
>>>> +
>>>> + list_for_each_entry(t,&m->transfers, transfer_list) {
>>> no locking around list traversal ?
>>>
>> hmm..can put a spin_lock around "qspi_transfer_msg" ?
> no dude, you need to protect the access to the list. So it needs to be
> around list_for_each_entry().
>
Ok.
>>>> +static irqreturn_t ti_qspi_isr(int irq, void *dev_id)
>>>> +{
>>>> + struct ti_qspi *qspi = dev_id;
>>>> + u16 stat;
>>>> +
>>>> + irqreturn_t ret = IRQ_HANDLED;
>>>> +
>>>> + spin_lock(&qspi->lock);
>>>> +
>>>> + stat = ti_qspi_read(qspi, QSPI_INTR_STATUS_ENABLED_CLEAR);
>>>> +
>>>> + if (!stat) {
>>>> + dev_dbg(qspi->dev, "No IRQ triggered\n");
>>>> + return IRQ_NONE;
>>> leaving lock held.
>>>
>> Will add a unlock before returning.
> there's a very nice C statement, goto, which you can use here.
>
Ok.
>>>> +static irqreturn_t ti_qspi_threaded_isr(int this_irq, void *dev_id)
>>>> +{
>>>> + struct ti_qspi *qspi = dev_id;
>>>> + unsigned long flags;
>>>> +
>>>> + spin_lock_irqsave(&qspi->lock, flags);
>>>> +
>>>> + complete(&qspi->transfer_complete);
>>> you need to check if your word completion is actually set. Don't assume
>>> it's set because we might want to change the code later.
>>>
>> hmm..something like this.?
>> if (ti_qspi_read(qspi, QSPI_SPI_STATUS_REG)& WC)
>> complete(&qspi->transfer_complete);
> I rather:
>
> stat = ti_qspi_read(qspi, QSPI_SPI_STATUS_REG);
>
> if (stat& WC)
> complete()
>
> then, if we want to add frame interrupt handling later, we don't need to
> read status register again. In fact, to avoid reading the status
> register here, you could even cache the returned value you read in your
> hardirq handler inside your qspi struct.
>
Ok.
------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent
caught up. So what steps can you take to put your SQL databases under
version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=49501711&iu=/4140/ostg.clktrk
_______________________________________________
spi-devel-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
