linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Paulraj, Sandeep" <s-paulraj-l0cyMroinI0@public.gmane.org>
To: Siegbert Baude <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 16:12:29 -0500	[thread overview]
Message-ID: <0554BEF07D437848AF01B9C9B5F0BC5D923DA969@dlee01.ent.ti.com> (raw)
In-Reply-To: <4AA01D8E.7090609-Mmb7MZpHnFY@public.gmane.org>



> 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.
[Sandeep] I have to get in touch with David Brownell regarding this.
Earlier what I did was when I submitted the patch I took care of some comments pointed out to me but that got a NAK because it was not the original that Dave had sent.

I think these fixes are to be sent as incremental patches.
> 
> Best regards
> Siegbert

  parent reply	other threads:[~2009-09-03 21:12 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
     [not found]             ` <4AA01D8E.7090609-Mmb7MZpHnFY@public.gmane.org>
2009-09-03 21:12               ` Paulraj, Sandeep [this message]
     [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=0554BEF07D437848AF01B9C9B5F0BC5D923DA969@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).