Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Alexandre Mergnat <amergnat@baylibre.com>
Cc: robh+dt@kernel.org, mark.rutland@arm.com,
	linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
	baylibre-upstreaming@groups.io,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	linux-input@vger.kernel.org
Subject: Re: [PATCH v3 3/3] iio: Add PAT9125 optical tracker sensor
Date: Sun, 14 Jul 2019 11:27:43 +0100
Message-ID: <20190714112743.0b2bfc70@archlinux> (raw)
In-Reply-To: <CAFGrd9ox2FGpL5a26_weZ9tC9XDTL_QnANR5PNTDYXcus2TJGw@mail.gmail.com>

On Thu, 11 Jul 2019 21:39:10 +0200
Alexandre Mergnat <amergnat@baylibre.com> wrote:

> Le dim. 16 juin 2019 à 17:39, Jonathan Cameron <jic23@kernel.org> a écrit :
> >
> > On Mon, 10 Jun 2019 11:29:45 +0200
> > Alexandre Mergnat <amergnat@baylibre.com> wrote:  
>  ...
> >  
> > > +/*
> > > + * To detect if a new value is available, register status is checked. This
> > > + * method is safer than using a flag on GPIO IRQ to track event while sampling
> > > + * because falling edge is missed when device trig just after a read reg value
> > > + * (that happen for fast motions or high CPI setting).  
> >
> > So we have an edge triggered interrupt that doesn't have a 'minimum low'
> > period? If so then the only safe way to handle it would be as a level
> > interrupt. Can you do that here?
> > (I once had the delights of a sensor like this tied to a edge sensitive only
> > interrupt, but thankfully those are a rare thing these days).
> >  
> 
> Trigger level is the first setup I tried (and retried during
> modifications) but it cannot
> works despite of ONESHOT flag. I'm wrong or it's probably due to
> nested_irq because
> it works when I reset interrupt (by reading data) during one of the
> IRQ thread, that what
> I did in my V1 patch. I spent a lot of time to try to use level
> trigger but this is the
> best way I found to do it properly without corner cases.
> The result with nested IRQ and low level trigger is a spamming IRQ
> (probably due to IRQ no more masked during nested IRQ thread) who that stuck the
> board because it hasn't time to make an I2C read to reset interrupt pin.

I'm not totally following the explanation.  The level interrupt
should remain masked until the irq_thread completes, as long
as you have the irq as IRQF_ONESHOT. The nesting shouldn't
matter for this as the interrupt should only be
unmasked once we the poll_func handler has completed.

If the device is sampling fast enough, you might find that it's
taken another sample so you get an immediate new interrupt.
If that's the case, then only option is to reducing the sampling
rate.

Of course, it's always possible we have a bug, but there
are quite a few drivers doing things with level interrupts
with no problem, so I'm a little doubtful.

> 
> 
> > > + * buffer mode and kernel warning due to nested IRQ thread,
> > > + * this function must return 0.
> > > + */
> > > +static int pat9125_trig_try_reenable(struct iio_trigger *trig)
> > > +{
> > > +     struct pat9125_data *data = iio_trigger_get_drvdata(trig);
> > > +     struct regmap *regmap = data->regmap;
> > > +     int status = 0;
> > > +
> > > +     if (data->sampling) {
> > > +             regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
> > > +             if (status & PAT9125_VALID_MOTION_DATA_BIT) {
> > > +                     data->sampling = false;  
> > So we only ever do 2 reads?  Why can't we be unlucky on timing
> > twice in a row?  
> 
> That can works indefinitely, I tested for some retry in a row by
> moving the chip fastly.
> If the method blocked at 2 readings, I should have been stuck during this test.
> 
> If read status return "New data available", a new read value is done
> through the same
> process (that mean data->sampling put to true) thanks to nested IRQ
> thread which will
> call try_reenable again and then re-check pat9125 status.
Hmm. I'm not totally happy with a test that is based on failing to
trigger it.   We need a theoretical argument for why it can't happen
to back up this code.

Jonathan


      reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-10  9:29 [PATCH v3 0/3] Add PAT9125 optical tracker driver Alexandre Mergnat
2019-06-10  9:29 ` [PATCH v3 1/3] dt-bindings: Add pixart vendor Alexandre Mergnat
2019-06-10  9:29 ` [PATCH v3 2/3] dt-bindings: iio: position: Add docs pat9125 Alexandre Mergnat
2019-06-10  9:29 ` [PATCH v3 3/3] iio: Add PAT9125 optical tracker sensor Alexandre Mergnat
2019-06-16 15:39   ` Jonathan Cameron
2019-07-11 19:39     ` Alexandre Mergnat
2019-07-14 10:27       ` Jonathan Cameron [this message]

Reply instructions:

You may reply publically 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=20190714112743.0b2bfc70@archlinux \
    --to=jic23@kernel.org \
    --cc=amergnat@baylibre.com \
    --cc=baylibre-upstreaming@groups.io \
    --cc=dmitry.torokhov@gmail.com \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.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

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox