linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Siegbert Baude <siegbert.baude-Mmb7MZpHnFY@public.gmane.org>
To: "Paulraj, Sandeep" <s-paulraj-l0cyMroinI0@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, 03 Sep 2009 21:48:30 +0200	[thread overview]
Message-ID: <4AA01D8E.7090609@gmx.de> (raw)
In-Reply-To: <0554BEF07D437848AF01B9C9B5F0BC5D923DA4D0-bGftbgMkZa+IQmiDNMet8wC/G2K4zDHf@public.gmane.org>

Hi Sandeep.


Paulraj, Sandeep schrieb:
>> On Behalf Of siegbert.baude-Mmb7MZpHnFY@public.gmane.org

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

You're right about that. I was not questioning your code. That seems to 
be o.k. for me (without having tested it). I was questioning your 
comment on top of the function, as it describes something, that is not 
implemented in your function. Only one out of three possible code paths 
use interrupts, so the comment should reflect the other two polling 
paths, too.

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

Again, my remark was only about the comment describing the ISR. That one 
is not in accordance with your actual implementation.

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

So here, it's the code that must be changed.

Best regards
Siegbert

  parent reply	other threads:[~2009-09-03 19:48 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
     [not found]         ` <0554BEF07D437848AF01B9C9B5F0BC5D923DA4D0-bGftbgMkZa+IQmiDNMet8wC/G2K4zDHf@public.gmane.org>
2009-09-03 19:48           ` Siegbert Baude [this message]
     [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=4AA01D8E.7090609@gmx.de \
    --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).