From: Jonathan Cameron <jic23@kernel.org>
To: <Eugen.Hristev@microchip.com>
Cc: <robh+dt@kernel.org>, <alexandre.belloni@bootlin.com>,
<Nicolas.Ferre@microchip.com>, <linux-iio@vger.kernel.org>,
<devicetree@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-kernel@vger.kernel.org>, <linux-rtc@vger.kernel.org>,
<a.zummo@towertech.it>, <Ludovic.Desroches@microchip.com>
Subject: Re: [PATCH 06/10] iio: adc: at91-sama5d2_adc: handle unfinished conversions
Date: Mon, 23 Dec 2019 12:20:27 +0000 [thread overview]
Message-ID: <20191223122027.7b93129f@archlinux> (raw)
In-Reply-To: <1576686157-11939-7-git-send-email-eugen.hristev@microchip.com>
On Wed, 18 Dec 2019 16:24:01 +0000
<Eugen.Hristev@microchip.com> wrote:
> From: Eugen Hristev <eugen.hristev@microchip.com>
>
> It can happen that on IRQ trigger, not all conversions are done if
> we are enabling multiple channels.
> The IRQ is triggered on first EOC (end of channel), but it can happen
> that not all channels are done. This leads into erroneous reports to
> userspace (zero values or previous values).
Ouch. That's an unfortunate hardware design.
> To solve this, in trigger handler, check if the mask of done channels
> is the same as the mask of active scan channels.
> If it's the same, proceed and push to buffers. Otherwise, to avoid sleeping
> in trigger handler, start a workq that will wait until all channels are
> ready.
You are fine sleeping in that handler. It's an interrupt thread ;)
> Normally, it should happen that in a short time fashion, all channels are
> ready, since the first IRQ triggered.
> The workq can stall in a loop if a hardware fault happens (for example
> the clock suddently dissappears), but if it's a hardware fault then
> even exiting the workq won't fix the hardware.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
> drivers/iio/adc/at91-sama5d2_adc.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index c575970..a6b4dff 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -8,6 +8,7 @@
>
> #include <linux/bitops.h>
> #include <linux/clk.h>
> +#include <linux/delay.h>
> #include <linux/dma-mapping.h>
> #include <linux/dmaengine.h>
> #include <linux/interrupt.h>
> @@ -487,6 +488,21 @@ static inline int at91_adc_of_xlate(struct iio_dev *indio_dev,
> return at91_adc_chan_xlate(indio_dev, iiospec->args[0]);
> }
>
> +static unsigned int at91_adc_active_scan_mask_to_reg(struct iio_dev *indio_dev)
> +{
> + u32 mask = 0;
> + u8 bit;
> +
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> + indio_dev->num_channels) {
> + struct iio_chan_spec const *chan =
> + at91_adc_chan_get(indio_dev, bit);
> + mask |= BIT(chan->channel);
> + }
> +
> + return mask & GENMASK(11, 0);
> +}
> +
> static void at91_adc_config_emr(struct at91_adc_state *st)
> {
> /* configure the extended mode register */
> @@ -1044,12 +1060,13 @@ static int at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
> struct iio_poll_func *pf)
> {
> struct at91_adc_state *st = iio_priv(indio_dev);
> + u32 mask = at91_adc_active_scan_mask_to_reg(indio_dev);
>
> /*
> * Check if the conversion is ready. If not, schedule a work to
> * check again later.
> */
> - if (!(at91_adc_readl(st, AT91_SAMA5D2_ISR) & GENMASK(11, 0))) {
> + if ((at91_adc_readl(st, AT91_SAMA5D2_ISR) & mask) != mask) {
> schedule_work(&st->workq);
> return -EINPROGRESS;
> }
> @@ -1269,9 +1286,13 @@ static void at91_adc_workq_handler(struct work_struct *workq)
> struct at91_adc_state *st = container_of(workq,
> struct at91_adc_state, workq);
> struct iio_dev *indio_dev = iio_priv_to_dev(st);
> + u32 mask = at91_adc_active_scan_mask_to_reg(indio_dev);
>
> if ((indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES) &&
> iio_trigger_validate_own_device(indio_dev->trig, indio_dev)) {
> + while ((at91_adc_readl(st, AT91_SAMA5D2_ISR) & mask) != mask)
> + udelay(1);
> +
ok. This fixes the issue raised earlier. Please reorganize the series
so we never introduce this code in a broken fashion. + look at doing it
in the interrupt thread anyway.
Jonathan
> at91_adc_read_and_push_channels(indio_dev, st->timestamp);
> iio_trigger_notify_done(indio_dev->trig);
> } else {
next prev parent reply other threads:[~2019-12-23 12:20 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-18 16:23 [PATCH 00/10] Enhancements to at91-sama5d2_adc and rtc trigger Eugen.Hristev
2019-12-18 16:23 ` [PATCH 02/10] dt-bindings: iio: adc: at91-sama5d2: add rtc-trigger optional property Eugen.Hristev
2019-12-23 11:58 ` Jonathan Cameron
2019-12-18 16:23 ` [PATCH 01/10] iio: adc: at91-sama5d2_adc: update for other trigger usage Eugen.Hristev
2019-12-23 11:56 ` Jonathan Cameron
2019-12-18 16:23 ` [PATCH 03/10] dt-bindings: iio: trigger: at91-rtc-trigger: add bindings Eugen.Hristev
2019-12-23 12:01 ` Jonathan Cameron
2019-12-18 16:24 ` [PATCH 04/10] rtc: at91rm9200: use of_platform_populate as return value Eugen.Hristev
2019-12-18 16:43 ` Alexandre Belloni
2019-12-18 16:52 ` Eugen.Hristev
2019-12-18 16:58 ` Alexandre Belloni
2019-12-19 9:15 ` Eugen.Hristev
2019-12-19 10:23 ` Alexandre Belloni
2019-12-23 11:16 ` Jonathan Cameron
2020-01-09 11:19 ` Eugen.Hristev
2020-01-09 11:52 ` Alexandre Belloni
2019-12-18 16:24 ` [PATCH 06/10] iio: adc: at91-sama5d2_adc: handle unfinished conversions Eugen.Hristev
2019-12-23 12:20 ` Jonathan Cameron [this message]
2019-12-18 16:24 ` [PATCH 05/10] iio: trigger: at91-rtc-trigger: introduce at91 rtc adc trigger driver Eugen.Hristev
2019-12-23 12:17 ` Jonathan Cameron
2019-12-18 16:24 ` [PATCH 08/10] iio: adc: at91-sama5d2_adc: implement RTC triggering Eugen.Hristev
2019-12-23 12:28 ` Jonathan Cameron
2019-12-18 16:24 ` [PATCH 07/10] iio: adc: at91-sama5d2_adc: fix differential channels in triggered mode Eugen.Hristev
2019-12-23 12:23 ` Jonathan Cameron
2019-12-18 16:24 ` [PATCH 10/10] ARM: dts: at91: sama5d2_xplained: enable rtc_adc_trigger Eugen.Hristev
2019-12-18 16:24 ` [PATCH 09/10] ARM: dts: at91: sama5d2: add rtc_adc_trigger node Eugen.Hristev
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=20191223122027.7b93129f@archlinux \
--to=jic23@kernel.org \
--cc=Eugen.Hristev@microchip.com \
--cc=Ludovic.Desroches@microchip.com \
--cc=Nicolas.Ferre@microchip.com \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--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
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).