linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Paulraj, Sandeep" <s-paulraj-l0cyMroinI0@public.gmane.org>
To: "siegbert.baude-Mmb7MZpHnFY@public.gmane.org"
	<siegbert.baude-Mmb7MZpHnFY@public.gmane.org>
Cc: "spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org"
	<spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>,
	"davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org"
	<davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org>,
	"dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org"
	<dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>,
	"akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org"
	<akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Subject: RE: [PATCH] SPI: DaVinci SPI Driver
Date: Thu, 3 Sep 2009 12:36:21 -0500	[thread overview]
Message-ID: <0554BEF07D437848AF01B9C9B5F0BC5D923DA4D0@dlee01.ent.ti.com> (raw)
In-Reply-To: <281fb76e0909030725k1f82b9d5wca1d22e3f3417af4-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>



> -----Original Message-----
> From: coocoo.channel-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org [mailto:coocoo.channel-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org]
> On Behalf Of siegbert.baude-Mmb7MZpHnFY@public.gmane.org
> Sent: Thursday, September 03, 2009 10:25 AM
> To: Paulraj, Sandeep
> Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org;
> dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org;
> akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.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-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: 
[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

  parent reply	other threads:[~2009-09-03 17:36 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
     [not found]     ` <281fb76e0909030725k1f82b9d5wca1d22e3f3417af4-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-09-03 17:36       ` Paulraj, Sandeep [this message]
     [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=0554BEF07D437848AF01B9C9B5F0BC5D923DA4D0@dlee01.ent.ti.com \
    --to=s-paulraj-l0cymroini0@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=siegbert.baude-Mmb7MZpHnFY@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).