> -----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-sou...@linux.davincidsp.com;
> dbrown...@users.sourceforge.net; spi-devel-general@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
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
