> -----Original Message----- > From: coocoo.chan...@googlemail.com [mailto:coocoo.chan...@googlemail.com] > On Behalf Of siegbert.ba...@gmx.de > Sent: Thursday, September 03, 2009 10:25 AM > To: Paulraj, Sandeep > Cc: davinci-linux-open-source@linux.davincidsp.com; > dbrown...@users.sourceforge.net; spi-devel-gene...@lists.sourceforge.net; > a...@linux-foundation.org > Subject: Re: [PATCH] SPI: DaVinci SPI Driver > > Hi Sandeep > > I had another look at your code and am wondering about the correctness > of more comments, and I wonder about error handling in the ISR: > > 2009/9/1 <s-paul...@ti.com>: > > From: Sandeep Paulraj <s-paul...@ti.com> > > + * davinci_spi_bufs - functions which will handle transfer data > > + * @spi: spi device on which data transfer to be done > > + * @t: spi transfer in which transfer info is filled > > + * > > + * This function will put data to be transferred into data register > > + * of SPI controller and then wait untill the completion will be marked > > + * by the IRQ Handler. > > + */ > > Is this true? For me the code seems to be polling in the transmit > case. Look down: [Sandeep] if you look at http://focus.ti.com/lit/ug/sprued4b/sprued4b.pdf page 26 , there is no TXINTFLAG to complement the RXINTFLAG
and if you have a look at page 19, there is no support for transmit interrupt > > > > +static int davinci_spi_bufs_pio(struct spi_device *spi, struct > spi_transfer *t) > > +{ > [snip] > > + /* Determine the command to execute READ or WRITE */ > > + if (t->tx_buf) { > > + clear_io_bits(davinci_spi->base + SPIINT, > SPIINT_MASKALL); > > + > > + while (1) { > > + tx_data = davinci_spi->get_tx(davinci_spi); > > + > > + data1_reg_val &= ~(0xFFFF); > > + data1_reg_val |= (0xFFFF & tx_data); > > + > > + buf_val = ioread32(davinci_spi->base + SPIBUF); > > + if ((buf_val & SPIBUF_TXFULL_MASK) == 0) { > > + iowrite32(data1_reg_val, > > + davinci_spi->base + > SPIDAT1); > > + > > + count--; > > + } > > + while (ioread32(davinci_spi->base + SPIBUF) > > + & SPIBUF_RXEMPTY_MASK) > > + cpu_relax(); > > + > > + /* getting the returned byte */ > > + if (t->rx_buf) { > > + buf_val = ioread32(davinci_spi->base + > SPIBUF); > > + davinci_spi->get_rx(buf_val, > davinci_spi); > > + } > > + if (count <= 0) > > + break; > > + } > > In this case you're polling the SPIBUF_TXFULL_MASK bit before you > write and then the SPIBUF_RXEMPTY_MASK bit before you read. > cpu_relax() is just an alias to barrier(), so there is no wait for an > interrupt, am I right? [Sandeep] yes that is correct > > > > > + * davinci_spi_irq - probe function for SPI Master Controller > > + * @irq: IRQ number for this SPI Master > > + * @context_data: structure for SPI Master controller davinci_spi > > + * > > + * ISR will determine that interrupt arrives either for READ or WRITE > command. > > + * According to command it will do the appropriate action. It will > check > > + * transfer length and if it is not zero then dispatch transfer command > again. > > + * If transfer length is zero then it will indicate the COMPLETION so > that > > + * davinci_spi_bufs function can go ahead. > > + */ > > I can't find anything in the below ISR code with regard to differing > between READ and WRITE or checking transfer lengthes. [Sandeep] as I have mentioned above we do not have a transmit interrupt > > > +static irqreturn_t davinci_spi_irq(s32 irq, void *context_data) > > +{ > > + struct davinci_spi *davinci_spi = context_data; > > + u32 int_status, rx_data = 0; > > + irqreturn_t ret = IRQ_NONE; > > + > > + int_status = ioread32(davinci_spi->base + SPIFLG); > > + while ((int_status & SPIFLG_MASK) != 0) { > > SPIFLG_MASK is many more bits than just the interrupt flag. If you got > an OVRNINTFLG set, you will never end this loop, as you do not reset > this flag in davinci_spi_check_error()! In addition you would > acknowledge the interrupt, even if you just got an error and actually > another interrupt handler would have been responsible for the > interrupt that just occurred. > Because of the last argument I would move down the next line into the > following if-clause. [Sandeep] Yes that's correct > > > + ret = IRQ_HANDLED; > > + > > + if (likely(int_status & SPIFLG_RX_INTR_MASK)) { > > + rx_data = ioread32(davinci_spi->base + SPIBUF); > > + davinci_spi->get_rx(rx_data, davinci_spi); > > + > > + /* Disable Receive Interrupt */ > > + iowrite32(~SPIINT_RX_INTR, > > + davinci_spi->base + SPIINT); > > + } else > > + (void)davinci_spi_check_error(davinci_spi, > int_status); > > + > > + int_status = ioread32(davinci_spi->base + SPIFLG); > > + } > > + > > + return ret; > > +} > > There are also two small typos, "untill" and "bbased" inside comments. [Sandeep] OK Thanks > > Regards > Siegbert _______________________________________________ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source