linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: siegbert.baude-Mmb7MZpHnFY@public.gmane.org
To: s-paulraj-l0cyMroinI0@public.gmane.org
Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org,
	dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org
Subject: Re: [PATCH] SPI: DaVinci SPI Driver
Date: Thu, 3 Sep 2009 16:25:14 +0200	[thread overview]
Message-ID: <281fb76e0909030725k1f82b9d5wca1d22e3f3417af4@mail.gmail.com> (raw)
In-Reply-To: <1251834668-32246-1-git-send-email-s-paulraj-l0cyMroinI0@public.gmane.org>

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-paulraj-l0cyMroinI0@public.gmane.org>:
> From: Sandeep Paulraj <s-paulraj-l0cyMroinI0@public.gmane.org>
> + * 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:


> +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?



> + * 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.

> +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.

> +               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.

Regards
Siegbert

  parent reply	other threads:[~2009-09-03 14:25 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-09-01 19:51 [PATCH] SPI: DaVinci SPI Driver s-paulraj-l0cyMroinI0
     [not found] ` <1251834668-32246-1-git-send-email-s-paulraj-l0cyMroinI0@public.gmane.org>
2009-09-02  2:07   ` David Brownell
2009-09-03 14:25   ` siegbert.baude-Mmb7MZpHnFY [this message]
     [not found]     ` <281fb76e0909030725k1f82b9d5wca1d22e3f3417af4-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-09-03 17:36       ` Paulraj, Sandeep
     [not found]         ` <0554BEF07D437848AF01B9C9B5F0BC5D923DA4D0-bGftbgMkZa+IQmiDNMet8wC/G2K4zDHf@public.gmane.org>
2009-09-03 19:48           ` Siegbert Baude
     [not found]             ` <4AA01D8E.7090609-Mmb7MZpHnFY@public.gmane.org>
2009-09-03 21:12               ` Paulraj, Sandeep
     [not found]                 ` <0554BEF07D437848AF01B9C9B5F0BC5D923DA969-bGftbgMkZa+IQmiDNMet8wC/G2K4zDHf@public.gmane.org>
2009-09-03 21:38                   ` David Brownell

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=281fb76e0909030725k1f82b9d5wca1d22e3f3417af4@mail.gmail.com \
    --to=siegbert.baude-mmb7mzphnfy@public.gmane.org \
    --cc=akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org \
    --cc=davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org \
    --cc=dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org \
    --cc=s-paulraj-l0cyMroinI0@public.gmane.org \
    --cc=spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org \
    /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: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).