* [PATCH v2 0/3] Enhancements to at91-sama5d2_adc driver
@ 2020-01-13 12:07 Eugen.Hristev
2020-01-13 12:07 ` [PATCH v2 1/3] iio: adc: at91-sama5d2_adc: fix differential channels in triggered mode Eugen.Hristev
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Eugen.Hristev @ 2020-01-13 12:07 UTC (permalink / raw)
To: jic23, linux-iio, linux-arm-kernel, linux-kernel
Cc: Eugen.Hristev, Ludovic.Desroches
From: Eugen Hristev <eugen.hristev@microchip.com>
This is a rework of the first patches from
[PATCH 00/10] Enhancements to at91-sama5d2_adc and rtc trigger
I reworked according to Jonathan's review.
The RTC triggering part is still work in progress and I will send it separately
Eugen Hristev (3):
iio: adc: at91-sama5d2_adc: fix differential channels in triggered
mode
iio: adc: at91-sama5d2_adc: handle unfinished conversions
iio: adc: at91-sama5d2_adc: update for other trigger usage
drivers/iio/adc/at91-sama5d2_adc.c | 171 ++++++++++++++++++++++++-------------
1 file changed, 111 insertions(+), 60 deletions(-)
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/3] iio: adc: at91-sama5d2_adc: fix differential channels in triggered mode
2020-01-13 12:07 [PATCH v2 0/3] Enhancements to at91-sama5d2_adc driver Eugen.Hristev
@ 2020-01-13 12:07 ` Eugen.Hristev
2020-01-13 12:07 ` [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: handle unfinished conversions Eugen.Hristev
2020-01-13 12:07 ` [PATCH v2 3/3] iio: adc: at91-sama5d2_adc: update for other trigger usage Eugen.Hristev
2 siblings, 0 replies; 10+ messages in thread
From: Eugen.Hristev @ 2020-01-13 12:07 UTC (permalink / raw)
To: jic23, linux-iio, linux-arm-kernel, linux-kernel
Cc: Eugen.Hristev, Ludovic.Desroches
From: Eugen Hristev <eugen.hristev@microchip.com>
The differential channels require writing the channel offset register (COR).
Otherwise they do not work in differential mode.
The configuration of COR is missing in triggered mode.
Fixes: 5e1a1da0f8c9 ("iio: adc: at91-sama5d2_adc: add hw trigger and buffer support")
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
Changes in v2:
- moved to the start of the list
drivers/iio/adc/at91-sama5d2_adc.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index e1850f3..2a6950a 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -723,6 +723,7 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
struct iio_chan_spec const *chan = at91_adc_chan_get(indio, bit);
+ u32 cor;
if (!chan)
continue;
@@ -732,6 +733,20 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
continue;
if (state) {
+ cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
+
+ if (chan->differential)
+ cor |= (BIT(chan->channel) |
+ BIT(chan->channel2)) <<
+ AT91_SAMA5D2_COR_DIFF_OFFSET;
+ else
+ cor &= ~(BIT(chan->channel) <<
+ AT91_SAMA5D2_COR_DIFF_OFFSET);
+
+ at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
+ }
+
+ if (state) {
at91_adc_writel(st, AT91_SAMA5D2_CHER,
BIT(chan->channel));
/* enable irq only if not using DMA */
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: handle unfinished conversions
2020-01-13 12:07 [PATCH v2 0/3] Enhancements to at91-sama5d2_adc driver Eugen.Hristev
2020-01-13 12:07 ` [PATCH v2 1/3] iio: adc: at91-sama5d2_adc: fix differential channels in triggered mode Eugen.Hristev
@ 2020-01-13 12:07 ` Eugen.Hristev
2020-01-17 17:34 ` Jonathan Cameron
2020-01-13 12:07 ` [PATCH v2 3/3] iio: adc: at91-sama5d2_adc: update for other trigger usage Eugen.Hristev
2 siblings, 1 reply; 10+ messages in thread
From: Eugen.Hristev @ 2020-01-13 12:07 UTC (permalink / raw)
To: jic23, linux-iio, linux-arm-kernel, linux-kernel
Cc: Eugen.Hristev, Ludovic.Desroches
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).
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, use usleep
to sleep until the conversion is done or we timeout.
Normally, it should happen that in a short time fashion, all channels are
ready, since the first IRQ triggered.
If a hardware fault happens (for example the clock suddently dissappears),
the handler will not be completed, in which case we do not report anything to
userspace anymore.
Also, change from using the EOC interrupts to DRDY interrupt.
This helps with the fact that not 'n' interrupt statuses are enabled,
each being able to trigger an interrupt, and instead only data ready
interrupt can wake up the CPU. Like this, when data is ready, check in
handler which and how many channels are done. While the DRDY is raised,
other IRQs cannot occur. Once the channel data is being read, we ack the
IRQ and finish the conversion.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
Changes in v2:
- move start of conversion to threaded irq, removed specific at91 pollfunc
- add timeout to channel mask readiness check in trigger handler
- use DRDY irq instead of EOC irqs.
- move enable irq after DRDY has been acked in reenable_trigger
drivers/iio/adc/at91-sama5d2_adc.c | 62 ++++++++++++++++++++++++++++----------
1 file changed, 46 insertions(+), 16 deletions(-)
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 2a6950a..454a493 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>
@@ -100,6 +101,8 @@
#define AT91_SAMA5D2_IER_YRDY BIT(21)
/* Interrupt Enable Register - TS pressure measurement ready */
#define AT91_SAMA5D2_IER_PRDY BIT(22)
+/* Interrupt Enable Register - Data ready */
+#define AT91_SAMA5D2_IER_DRDY BIT(24)
/* Interrupt Enable Register - general overrun error */
#define AT91_SAMA5D2_IER_GOVRE BIT(25)
/* Interrupt Enable Register - Pen detect */
@@ -486,6 +489,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 */
@@ -746,24 +764,19 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
}
- if (state) {
+ if (state)
at91_adc_writel(st, AT91_SAMA5D2_CHER,
BIT(chan->channel));
- /* enable irq only if not using DMA */
- if (!st->dma_st.dma_chan) {
- at91_adc_writel(st, AT91_SAMA5D2_IER,
- BIT(chan->channel));
- }
- } else {
- /* disable irq only if not using DMA */
- if (!st->dma_st.dma_chan) {
- at91_adc_writel(st, AT91_SAMA5D2_IDR,
- BIT(chan->channel));
- }
+ else
at91_adc_writel(st, AT91_SAMA5D2_CHDR,
BIT(chan->channel));
- }
}
+ /* enable irq only if not using DMA */
+ if (state && !st->dma_st.dma_chan)
+ at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
+ /* disable irq only if not using DMA */
+ if (!state && !st->dma_st.dma_chan)
+ at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
return 0;
}
@@ -777,10 +790,10 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
if (st->dma_st.dma_chan)
return 0;
- enable_irq(st->irq);
-
/* Needed to ACK the DRDY interruption */
at91_adc_readl(st, AT91_SAMA5D2_LCDR);
+
+ enable_irq(st->irq);
return 0;
}
@@ -1015,6 +1028,22 @@ static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
int i = 0;
int val;
u8 bit;
+ u32 mask = at91_adc_active_scan_mask_to_reg(indio_dev);
+ unsigned int timeout = 50;
+
+ /*
+ * Check if the conversion is ready. If not, wait a little bit, and
+ * in case of timeout exit with an error.
+ */
+ while ((at91_adc_readl(st, AT91_SAMA5D2_ISR) & mask) != mask &&
+ timeout) {
+ usleep_range(50, 100);
+ timeout--;
+ }
+
+ /* Cannot read data, not ready. Continue without reporting data */
+ if (!timeout)
+ return;
for_each_set_bit(bit, indio_dev->active_scan_mask,
indio_dev->num_channels) {
@@ -1281,7 +1310,8 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
status = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
status = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
status = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
- } else if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
+ } else if (iio_buffer_enabled(indio) &&
+ (status & AT91_SAMA5D2_IER_DRDY)) {
/* triggered buffer without DMA */
disable_irq_nosync(irq);
iio_trigger_poll(indio->trig);
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] iio: adc: at91-sama5d2_adc: update for other trigger usage
2020-01-13 12:07 [PATCH v2 0/3] Enhancements to at91-sama5d2_adc driver Eugen.Hristev
2020-01-13 12:07 ` [PATCH v2 1/3] iio: adc: at91-sama5d2_adc: fix differential channels in triggered mode Eugen.Hristev
2020-01-13 12:07 ` [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: handle unfinished conversions Eugen.Hristev
@ 2020-01-13 12:07 ` Eugen.Hristev
2020-01-17 17:42 ` Jonathan Cameron
2 siblings, 1 reply; 10+ messages in thread
From: Eugen.Hristev @ 2020-01-13 12:07 UTC (permalink / raw)
To: jic23, linux-iio, linux-arm-kernel, linux-kernel
Cc: Eugen.Hristev, Ludovic.Desroches
From: Eugen Hristev <eugen.hristev@microchip.com>
This change will allow the at91-sama5d2_adc driver to use other triggers
than it's own.
In particular, tested with the sysfs trigger.
To be able to achieve this functionality, some changes were required:
1) Do not enable/disable channels when enabling/disabling the trigger.
This is because the trigger is enabled/disabled only for our trigger
(obviously). We need channels enabled/disabled regardless of what trigger is
being used.
2) Cope with DMA : DMA cannot be used when using another type of trigger.
Other triggers work through pollfunc, so we get polled anyway on every trigger.
Thus we have to obtain data at every trigger.
3) When to start conversion? The usual pollfunc (store time from subsystem)
would be in hard irq and this would be a good way, but current iio subsystem
recommends to have it in the threaded irq. Thus adding software start
code in this handler.
4) Buffer config: we need to setup buffer regardless of our own device's
trigger. We may get one attached later.
5) IRQ handling: we use our own device IRQ only if it's our own trigger
and we do not use DMA . If we use DMA, we use the DMA controller's IRQ.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
Changes in v2:
- adapt to the situation of having the previous two patches ahead in the series
drivers/iio/adc/at91-sama5d2_adc.c | 140 +++++++++++++++++++------------------
1 file changed, 73 insertions(+), 67 deletions(-)
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 454a493..34df043 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -728,7 +728,6 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
struct iio_dev *indio = iio_trigger_get_drvdata(trig);
struct at91_adc_state *st = iio_priv(indio);
u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
- u8 bit;
/* clear TRGMOD */
status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
@@ -739,45 +738,6 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
/* set/unset hw trigger */
at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
- for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
- struct iio_chan_spec const *chan = at91_adc_chan_get(indio, bit);
- u32 cor;
-
- if (!chan)
- continue;
- /* these channel types cannot be handled by this trigger */
- if (chan->type == IIO_POSITIONRELATIVE ||
- chan->type == IIO_PRESSURE)
- continue;
-
- if (state) {
- cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
-
- if (chan->differential)
- cor |= (BIT(chan->channel) |
- BIT(chan->channel2)) <<
- AT91_SAMA5D2_COR_DIFF_OFFSET;
- else
- cor &= ~(BIT(chan->channel) <<
- AT91_SAMA5D2_COR_DIFF_OFFSET);
-
- at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
- }
-
- if (state)
- at91_adc_writel(st, AT91_SAMA5D2_CHER,
- BIT(chan->channel));
- else
- at91_adc_writel(st, AT91_SAMA5D2_CHDR,
- BIT(chan->channel));
- }
- /* enable irq only if not using DMA */
- if (state && !st->dma_st.dma_chan)
- at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
- /* disable irq only if not using DMA */
- if (!state && !st->dma_st.dma_chan)
- at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
-
return 0;
}
@@ -901,9 +861,22 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
return 0;
}
+static bool at91_adc_buffer_check_use_irq(struct iio_dev *indio,
+ struct at91_adc_state *st)
+{
+ /* if using DMA, we do not use our own IRQ (we use DMA-controller) */
+ if (st->dma_st.dma_chan)
+ return false;
+ /* if the trigger is not ours, then it has its own IRQ */
+ if (iio_trigger_validate_own_device(indio->trig, indio))
+ return false;
+ return true;
+}
+
static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
{
int ret;
+ u8 bit;
struct at91_adc_state *st = iio_priv(indio_dev);
/* check if we are enabling triggered buffer or the touchscreen */
@@ -921,9 +894,40 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
ret = at91_adc_dma_start(indio_dev);
if (ret) {
dev_err(&indio_dev->dev, "buffer postenable failed\n");
+ iio_triggered_buffer_predisable(indio_dev);
return ret;
}
+ 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);
+ u32 cor;
+
+ if (!chan)
+ continue;
+ /* these channel types cannot be handled by this trigger */
+ if (chan->type == IIO_POSITIONRELATIVE ||
+ chan->type == IIO_PRESSURE)
+ continue;
+
+ cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
+
+ if (chan->differential)
+ cor |= (BIT(chan->channel) | BIT(chan->channel2)) <<
+ AT91_SAMA5D2_COR_DIFF_OFFSET;
+ else
+ cor &= ~(BIT(chan->channel) <<
+ AT91_SAMA5D2_COR_DIFF_OFFSET);
+
+ at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
+
+ at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
+ }
+
+ if (at91_adc_buffer_check_use_irq(indio_dev, st))
+ at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
+
return iio_triggered_buffer_postenable(indio_dev);
}
@@ -944,21 +948,11 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
return -EINVAL;
- /* continue with the triggered buffer */
- ret = iio_triggered_buffer_predisable(indio_dev);
- if (ret < 0)
- dev_err(&indio_dev->dev, "buffer predisable failed\n");
-
- if (!st->dma_st.dma_chan)
- return ret;
-
- /* if we are using DMA we must clear registers and end DMA */
- dmaengine_terminate_sync(st->dma_st.dma_chan);
-
/*
- * For each enabled channel we must read the last converted value
+ * For each enable channel we must disable it in hardware.
+ * In the case of DMA, we must read the last converted value
* to clear EOC status and not get a possible interrupt later.
- * This value is being read by DMA from LCDR anyway
+ * This value is being read by DMA from LCDR anyway, so it's not lost.
*/
for_each_set_bit(bit, indio_dev->active_scan_mask,
indio_dev->num_channels) {
@@ -971,12 +965,28 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
if (chan->type == IIO_POSITIONRELATIVE ||
chan->type == IIO_PRESSURE)
continue;
+
+ at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
+
if (st->dma_st.dma_chan)
at91_adc_readl(st, chan->address);
}
+ if (at91_adc_buffer_check_use_irq(indio_dev, st))
+ at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
+
/* read overflow register to clear possible overflow status */
at91_adc_readl(st, AT91_SAMA5D2_OVER);
+
+ /* continue with the triggered buffer */
+ ret = iio_triggered_buffer_predisable(indio_dev);
+ if (ret < 0)
+ dev_err(&indio_dev->dev, "buffer predisable failed\n");
+
+ /* if we are using DMA we must clear registers and end DMA */
+ if (st->dma_st.dma_chan)
+ dmaengine_terminate_sync(st->dma_st.dma_chan);
+
return ret;
}
@@ -1131,6 +1141,13 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
struct iio_dev *indio_dev = pf->indio_dev;
struct at91_adc_state *st = iio_priv(indio_dev);
+ /*
+ * If it's not our trigger, start a conversion now, as we are
+ * actually polling the trigger now.
+ */
+ if (iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
+ at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
+
if (st->dma_st.dma_chan)
at91_adc_trigger_handler_dma(indio_dev);
else
@@ -1143,20 +1160,9 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
static int at91_adc_buffer_init(struct iio_dev *indio)
{
- struct at91_adc_state *st = iio_priv(indio);
-
- if (st->selected_trig->hw_trig) {
- return devm_iio_triggered_buffer_setup(&indio->dev, indio,
- &iio_pollfunc_store_time,
- &at91_adc_trigger_handler, &at91_buffer_setup_ops);
- }
- /*
- * we need to prepare the buffer ops in case we will get
- * another buffer attached (like a callback buffer for the touchscreen)
- */
- indio->setup_ops = &at91_buffer_setup_ops;
-
- return 0;
+ return devm_iio_triggered_buffer_setup(&indio->dev, indio,
+ &iio_pollfunc_store_time,
+ &at91_adc_trigger_handler, &at91_buffer_setup_ops);
}
static unsigned at91_adc_startup_time(unsigned startup_time_min,
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: handle unfinished conversions
2020-01-13 12:07 ` [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: handle unfinished conversions Eugen.Hristev
@ 2020-01-17 17:34 ` Jonathan Cameron
2020-01-20 7:27 ` Eugen.Hristev
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2020-01-17 17:34 UTC (permalink / raw)
To: Eugen.Hristev
Cc: linux-iio, Ludovic.Desroches, linux-arm-kernel, jic23, linux-kernel
On Mon, 13 Jan 2020 12:07:09 +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).
> 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, use usleep
> to sleep until the conversion is done or we timeout.
> Normally, it should happen that in a short time fashion, all channels are
> ready, since the first IRQ triggered.
> If a hardware fault happens (for example the clock suddently dissappears),
> the handler will not be completed, in which case we do not report anything to
> userspace anymore.
> Also, change from using the EOC interrupts to DRDY interrupt.
> This helps with the fact that not 'n' interrupt statuses are enabled,
> each being able to trigger an interrupt, and instead only data ready
> interrupt can wake up the CPU. Like this, when data is ready, check in
> handler which and how many channels are done. While the DRDY is raised,
> other IRQs cannot occur. Once the channel data is being read, we ack the
> IRQ and finish the conversion.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
> Changes in v2:
> - move start of conversion to threaded irq, removed specific at91 pollfunc
> - add timeout to channel mask readiness check in trigger handler
> - use DRDY irq instead of EOC irqs.
> - move enable irq after DRDY has been acked in reenable_trigger
>
> drivers/iio/adc/at91-sama5d2_adc.c | 62 ++++++++++++++++++++++++++++----------
> 1 file changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 2a6950a..454a493 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>
> @@ -100,6 +101,8 @@
> #define AT91_SAMA5D2_IER_YRDY BIT(21)
> /* Interrupt Enable Register - TS pressure measurement ready */
> #define AT91_SAMA5D2_IER_PRDY BIT(22)
> +/* Interrupt Enable Register - Data ready */
> +#define AT91_SAMA5D2_IER_DRDY BIT(24)
> /* Interrupt Enable Register - general overrun error */
> #define AT91_SAMA5D2_IER_GOVRE BIT(25)
> /* Interrupt Enable Register - Pen detect */
> @@ -486,6 +489,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 */
> @@ -746,24 +764,19 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> }
>
> - if (state) {
> + if (state)
> at91_adc_writel(st, AT91_SAMA5D2_CHER,
> BIT(chan->channel));
> - /* enable irq only if not using DMA */
> - if (!st->dma_st.dma_chan) {
> - at91_adc_writel(st, AT91_SAMA5D2_IER,
> - BIT(chan->channel));
> - }
> - } else {
> - /* disable irq only if not using DMA */
> - if (!st->dma_st.dma_chan) {
> - at91_adc_writel(st, AT91_SAMA5D2_IDR,
> - BIT(chan->channel));
> - }
> + else
> at91_adc_writel(st, AT91_SAMA5D2_CHDR,
> BIT(chan->channel));
> - }
> }
> + /* enable irq only if not using DMA */
> + if (state && !st->dma_st.dma_chan)
> + at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
> + /* disable irq only if not using DMA */
> + if (!state && !st->dma_st.dma_chan)
> + at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
Hmm. Would have been nicer to avoid the refactor and just have the change to
what was written. If you want to keep it, perhaps:
/* Nothing to do if using DMA */
if (!st->dma_st.dma_chan)
return 0;
if (state)
at91...
else
at91...
>
> return 0;
> }
> @@ -777,10 +790,10 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
> if (st->dma_st.dma_chan)
> return 0;
>
> - enable_irq(st->irq);
> -
> /* Needed to ACK the DRDY interruption */
> at91_adc_readl(st, AT91_SAMA5D2_LCDR);
> +
> + enable_irq(st->irq);
Why this change? I'm not totally following the description above.
> return 0;
> }
>
> @@ -1015,6 +1028,22 @@ static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
> int i = 0;
> int val;
> u8 bit;
> + u32 mask = at91_adc_active_scan_mask_to_reg(indio_dev);
> + unsigned int timeout = 50;
> +
> + /*
> + * Check if the conversion is ready. If not, wait a little bit, and
> + * in case of timeout exit with an error.
> + */
> + while ((at91_adc_readl(st, AT91_SAMA5D2_ISR) & mask) != mask &&
> + timeout) {
> + usleep_range(50, 100);
> + timeout--;
> + }
> +
> + /* Cannot read data, not ready. Continue without reporting data */
> + if (!timeout)
> + return;
>
> for_each_set_bit(bit, indio_dev->active_scan_mask,
> indio_dev->num_channels) {
> @@ -1281,7 +1310,8 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
> status = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
> status = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
> status = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
> - } else if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
> + } else if (iio_buffer_enabled(indio) &&
> + (status & AT91_SAMA5D2_IER_DRDY)) {
> /* triggered buffer without DMA */
> disable_irq_nosync(irq);
> iio_trigger_poll(indio->trig);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] iio: adc: at91-sama5d2_adc: update for other trigger usage
2020-01-13 12:07 ` [PATCH v2 3/3] iio: adc: at91-sama5d2_adc: update for other trigger usage Eugen.Hristev
@ 2020-01-17 17:42 ` Jonathan Cameron
2020-01-20 7:24 ` Eugen.Hristev
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2020-01-17 17:42 UTC (permalink / raw)
To: Eugen.Hristev
Cc: linux-iio, linux-kernel, Ludovic.Desroches, jic23,
Alexandru Ardelean, linux-arm-kernel
On Mon, 13 Jan 2020 12:07:10 +0000
<Eugen.Hristev@microchip.com> wrote:
> From: Eugen Hristev <eugen.hristev@microchip.com>
>
> This change will allow the at91-sama5d2_adc driver to use other triggers
> than it's own.
> In particular, tested with the sysfs trigger.
> To be able to achieve this functionality, some changes were required:
> 1) Do not enable/disable channels when enabling/disabling the trigger.
> This is because the trigger is enabled/disabled only for our trigger
> (obviously). We need channels enabled/disabled regardless of what trigger is
> being used.
> 2) Cope with DMA : DMA cannot be used when using another type of trigger.
> Other triggers work through pollfunc, so we get polled anyway on every trigger.
> Thus we have to obtain data at every trigger.
> 3) When to start conversion? The usual pollfunc (store time from subsystem)
> would be in hard irq and this would be a good way, but current iio subsystem
> recommends to have it in the threaded irq. Thus adding software start
> code in this handler.
> 4) Buffer config: we need to setup buffer regardless of our own device's
> trigger. We may get one attached later.
> 5) IRQ handling: we use our own device IRQ only if it's our own trigger
> and we do not use DMA . If we use DMA, we use the DMA controller's IRQ.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
+CC Alexandru as he's doing a lot of cleanup around the buffer functions.
I'd like Alex to take a look at this.
A few comments inline from me.
Thanks,
Jonathan
> ---
> Changes in v2:
> - adapt to the situation of having the previous two patches ahead in the series
>
> drivers/iio/adc/at91-sama5d2_adc.c | 140 +++++++++++++++++++------------------
> 1 file changed, 73 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 454a493..34df043 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -728,7 +728,6 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> struct at91_adc_state *st = iio_priv(indio);
> u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
> - u8 bit;
>
> /* clear TRGMOD */
> status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
> @@ -739,45 +738,6 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> /* set/unset hw trigger */
> at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
>
> - for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> - struct iio_chan_spec const *chan = at91_adc_chan_get(indio, bit);
> - u32 cor;
> -
> - if (!chan)
> - continue;
> - /* these channel types cannot be handled by this trigger */
> - if (chan->type == IIO_POSITIONRELATIVE ||
> - chan->type == IIO_PRESSURE)
> - continue;
> -
> - if (state) {
> - cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
> -
> - if (chan->differential)
> - cor |= (BIT(chan->channel) |
> - BIT(chan->channel2)) <<
> - AT91_SAMA5D2_COR_DIFF_OFFSET;
> - else
> - cor &= ~(BIT(chan->channel) <<
> - AT91_SAMA5D2_COR_DIFF_OFFSET);
> -
> - at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> - }
> -
> - if (state)
> - at91_adc_writel(st, AT91_SAMA5D2_CHER,
> - BIT(chan->channel));
> - else
> - at91_adc_writel(st, AT91_SAMA5D2_CHDR,
> - BIT(chan->channel));
> - }
> - /* enable irq only if not using DMA */
> - if (state && !st->dma_st.dma_chan)
> - at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
> - /* disable irq only if not using DMA */
> - if (!state && !st->dma_st.dma_chan)
> - at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
> -
> return 0;
> }
>
> @@ -901,9 +861,22 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
> return 0;
> }
>
> +static bool at91_adc_buffer_check_use_irq(struct iio_dev *indio,
> + struct at91_adc_state *st)
> +{
> + /* if using DMA, we do not use our own IRQ (we use DMA-controller) */
> + if (st->dma_st.dma_chan)
> + return false;
> + /* if the trigger is not ours, then it has its own IRQ */
> + if (iio_trigger_validate_own_device(indio->trig, indio))
> + return false;
> + return true;
> +}
> +
> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
> {
> int ret;
> + u8 bit;
> struct at91_adc_state *st = iio_priv(indio_dev);
>
> /* check if we are enabling triggered buffer or the touchscreen */
> @@ -921,9 +894,40 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
> ret = at91_adc_dma_start(indio_dev);
> if (ret) {
> dev_err(&indio_dev->dev, "buffer postenable failed\n");
> + iio_triggered_buffer_predisable(indio_dev);
This seems odd given you have called the iio_triggered_buffer_postenable yet..
That is below.
> return ret;
> }
>
> + 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);
> + u32 cor;
> +
> + if (!chan)
> + continue;
> + /* these channel types cannot be handled by this trigger */
> + if (chan->type == IIO_POSITIONRELATIVE ||
> + chan->type == IIO_PRESSURE)
> + continue;
> +
> + cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
> +
> + if (chan->differential)
> + cor |= (BIT(chan->channel) | BIT(chan->channel2)) <<
> + AT91_SAMA5D2_COR_DIFF_OFFSET;
> + else
> + cor &= ~(BIT(chan->channel) <<
> + AT91_SAMA5D2_COR_DIFF_OFFSET);
> +
> + at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> +
> + at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
> + }
> +
> + if (at91_adc_buffer_check_use_irq(indio_dev, st))
> + at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
> +
> return iio_triggered_buffer_postenable(indio_dev);
> }
>
> @@ -944,21 +948,11 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> return -EINVAL;
>
> - /* continue with the triggered buffer */
> - ret = iio_triggered_buffer_predisable(indio_dev);
> - if (ret < 0)
> - dev_err(&indio_dev->dev, "buffer predisable failed\n");
> -
> - if (!st->dma_st.dma_chan)
> - return ret;
> -
> - /* if we are using DMA we must clear registers and end DMA */
> - dmaengine_terminate_sync(st->dma_st.dma_chan);
> -
> /*
> - * For each enabled channel we must read the last converted value
> + * For each enable channel we must disable it in hardware.
> + * In the case of DMA, we must read the last converted value
> * to clear EOC status and not get a possible interrupt later.
> - * This value is being read by DMA from LCDR anyway
> + * This value is being read by DMA from LCDR anyway, so it's not lost.
> */
> for_each_set_bit(bit, indio_dev->active_scan_mask,
> indio_dev->num_channels) {
> @@ -971,12 +965,28 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> if (chan->type == IIO_POSITIONRELATIVE ||
> chan->type == IIO_PRESSURE)
> continue;
> +
> + at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
> +
> if (st->dma_st.dma_chan)
> at91_adc_readl(st, chan->address);
> }
>
> + if (at91_adc_buffer_check_use_irq(indio_dev, st))
> + at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
> +
> /* read overflow register to clear possible overflow status */
> at91_adc_readl(st, AT91_SAMA5D2_OVER);
> +
> + /* continue with the triggered buffer */
> + ret = iio_triggered_buffer_predisable(indio_dev);
> + if (ret < 0)
> + dev_err(&indio_dev->dev, "buffer predisable failed\n");
> +
> + /* if we are using DMA we must clear registers and end DMA */
> + if (st->dma_st.dma_chan)
> + dmaengine_terminate_sync(st->dma_st.dma_chan);
This ordering is going to stop Alex doing his rework to remove the need
to manually call iio_triggered_buffer_predisable. Why does it make
sense to do the dma stuff after that?
Ah I see it always did and the postenable is the opposite of what Alex
has been moving to as well.
> +
> return ret;
> }
>
> @@ -1131,6 +1141,13 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> struct iio_dev *indio_dev = pf->indio_dev;
> struct at91_adc_state *st = iio_priv(indio_dev);
>
> + /*
> + * If it's not our trigger, start a conversion now, as we are
> + * actually polling the trigger now.
> + */
> + if (iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
> + at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
> +
> if (st->dma_st.dma_chan)
> at91_adc_trigger_handler_dma(indio_dev);
> else
> @@ -1143,20 +1160,9 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>
> static int at91_adc_buffer_init(struct iio_dev *indio)
> {
> - struct at91_adc_state *st = iio_priv(indio);
> -
> - if (st->selected_trig->hw_trig) {
> - return devm_iio_triggered_buffer_setup(&indio->dev, indio,
> - &iio_pollfunc_store_time,
> - &at91_adc_trigger_handler, &at91_buffer_setup_ops);
> - }
> - /*
> - * we need to prepare the buffer ops in case we will get
> - * another buffer attached (like a callback buffer for the touchscreen)
> - */
> - indio->setup_ops = &at91_buffer_setup_ops;
> -
> - return 0;
> + return devm_iio_triggered_buffer_setup(&indio->dev, indio,
> + &iio_pollfunc_store_time,
> + &at91_adc_trigger_handler, &at91_buffer_setup_ops);
> }
>
> static unsigned at91_adc_startup_time(unsigned startup_time_min,
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] iio: adc: at91-sama5d2_adc: update for other trigger usage
2020-01-17 17:42 ` Jonathan Cameron
@ 2020-01-20 7:24 ` Eugen.Hristev
2020-01-27 12:13 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Eugen.Hristev @ 2020-01-20 7:24 UTC (permalink / raw)
To: Jonathan.Cameron
Cc: linux-iio, linux-kernel, Ludovic.Desroches, jic23,
alexandru.ardelean, linux-arm-kernel
On 17.01.2020 19:42, Jonathan Cameron wrote:
> On Mon, 13 Jan 2020 12:07:10 +0000
> <Eugen.Hristev@microchip.com> wrote:
>
>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>
>> This change will allow the at91-sama5d2_adc driver to use other triggers
>> than it's own.
>> In particular, tested with the sysfs trigger.
>> To be able to achieve this functionality, some changes were required:
>> 1) Do not enable/disable channels when enabling/disabling the trigger.
>> This is because the trigger is enabled/disabled only for our trigger
>> (obviously). We need channels enabled/disabled regardless of what trigger is
>> being used.
>> 2) Cope with DMA : DMA cannot be used when using another type of trigger.
>> Other triggers work through pollfunc, so we get polled anyway on every trigger.
>> Thus we have to obtain data at every trigger.
>> 3) When to start conversion? The usual pollfunc (store time from subsystem)
>> would be in hard irq and this would be a good way, but current iio subsystem
>> recommends to have it in the threaded irq. Thus adding software start
>> code in this handler.
>> 4) Buffer config: we need to setup buffer regardless of our own device's
>> trigger. We may get one attached later.
>> 5) IRQ handling: we use our own device IRQ only if it's our own trigger
>> and we do not use DMA . If we use DMA, we use the DMA controller's IRQ.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>
> +CC Alexandru as he's doing a lot of cleanup around the buffer functions.
> I'd like Alex to take a look at this.
>
> A few comments inline from me.
>
> Thanks,
>
> Jonathan
>
>
>
>> ---
>> Changes in v2:
>> - adapt to the situation of having the previous two patches ahead in the series
>>
>> drivers/iio/adc/at91-sama5d2_adc.c | 140 +++++++++++++++++++------------------
>> 1 file changed, 73 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 454a493..34df043 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -728,7 +728,6 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>> struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>> struct at91_adc_state *st = iio_priv(indio);
>> u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
>> - u8 bit;
>>
>> /* clear TRGMOD */
>> status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
>> @@ -739,45 +738,6 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>> /* set/unset hw trigger */
>> at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
>>
>> - for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
>> - struct iio_chan_spec const *chan = at91_adc_chan_get(indio, bit);
>> - u32 cor;
>> -
>> - if (!chan)
>> - continue;
>> - /* these channel types cannot be handled by this trigger */
>> - if (chan->type == IIO_POSITIONRELATIVE ||
>> - chan->type == IIO_PRESSURE)
>> - continue;
>> -
>> - if (state) {
>> - cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
>> -
>> - if (chan->differential)
>> - cor |= (BIT(chan->channel) |
>> - BIT(chan->channel2)) <<
>> - AT91_SAMA5D2_COR_DIFF_OFFSET;
>> - else
>> - cor &= ~(BIT(chan->channel) <<
>> - AT91_SAMA5D2_COR_DIFF_OFFSET);
>> -
>> - at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
>> - }
>> -
>> - if (state)
>> - at91_adc_writel(st, AT91_SAMA5D2_CHER,
>> - BIT(chan->channel));
>> - else
>> - at91_adc_writel(st, AT91_SAMA5D2_CHDR,
>> - BIT(chan->channel));
>> - }
>> - /* enable irq only if not using DMA */
>> - if (state && !st->dma_st.dma_chan)
>> - at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
>> - /* disable irq only if not using DMA */
>> - if (!state && !st->dma_st.dma_chan)
>> - at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
>> -
>> return 0;
>> }
>>
>> @@ -901,9 +861,22 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
>> return 0;
>> }
>>
>> +static bool at91_adc_buffer_check_use_irq(struct iio_dev *indio,
>> + struct at91_adc_state *st)
>> +{
>> + /* if using DMA, we do not use our own IRQ (we use DMA-controller) */
>> + if (st->dma_st.dma_chan)
>> + return false;
>> + /* if the trigger is not ours, then it has its own IRQ */
>> + if (iio_trigger_validate_own_device(indio->trig, indio))
>> + return false;
>> + return true;
>> +}
>> +
>> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
>> {
>> int ret;
>> + u8 bit;
>> struct at91_adc_state *st = iio_priv(indio_dev);
>>
>> /* check if we are enabling triggered buffer or the touchscreen */
>> @@ -921,9 +894,40 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
>> ret = at91_adc_dma_start(indio_dev);
>> if (ret) {
>> dev_err(&indio_dev->dev, "buffer postenable failed\n");
>> + iio_triggered_buffer_predisable(indio_dev);
>
> This seems odd given you have called the iio_triggered_buffer_postenable yet..
> That is below.
Hi Jonathan,
You are right, I will remove this.
>
>> return ret;
>> }
>>
>> + 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);
>> + u32 cor;
>> +
>> + if (!chan)
>> + continue;
>> + /* these channel types cannot be handled by this trigger */
>> + if (chan->type == IIO_POSITIONRELATIVE ||
>> + chan->type == IIO_PRESSURE)
>> + continue;
>> +
>> + cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
>> +
>> + if (chan->differential)
>> + cor |= (BIT(chan->channel) | BIT(chan->channel2)) <<
>> + AT91_SAMA5D2_COR_DIFF_OFFSET;
>> + else
>> + cor &= ~(BIT(chan->channel) <<
>> + AT91_SAMA5D2_COR_DIFF_OFFSET);
>> +
>> + at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
>> +
>> + at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
>> + }
>> +
>> + if (at91_adc_buffer_check_use_irq(indio_dev, st))
>> + at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
>> +
>> return iio_triggered_buffer_postenable(indio_dev);
>> }
>>
>> @@ -944,21 +948,11 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
>> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
>> return -EINVAL;
>>
>> - /* continue with the triggered buffer */
>> - ret = iio_triggered_buffer_predisable(indio_dev);
>> - if (ret < 0)
>> - dev_err(&indio_dev->dev, "buffer predisable failed\n");
>> -
>> - if (!st->dma_st.dma_chan)
>> - return ret;
>> -
>> - /* if we are using DMA we must clear registers and end DMA */
>> - dmaengine_terminate_sync(st->dma_st.dma_chan);
>> -
>> /*
>> - * For each enabled channel we must read the last converted value
>> + * For each enable channel we must disable it in hardware.
>> + * In the case of DMA, we must read the last converted value
>> * to clear EOC status and not get a possible interrupt later.
>> - * This value is being read by DMA from LCDR anyway
>> + * This value is being read by DMA from LCDR anyway, so it's not lost.
>> */
>> for_each_set_bit(bit, indio_dev->active_scan_mask,
>> indio_dev->num_channels) {
>> @@ -971,12 +965,28 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
>> if (chan->type == IIO_POSITIONRELATIVE ||
>> chan->type == IIO_PRESSURE)
>> continue;
>> +
>> + at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
>> +
>> if (st->dma_st.dma_chan)
>> at91_adc_readl(st, chan->address);
>> }
>>
>> + if (at91_adc_buffer_check_use_irq(indio_dev, st))
>> + at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
>> +
>> /* read overflow register to clear possible overflow status */
>> at91_adc_readl(st, AT91_SAMA5D2_OVER);
>> +
>> + /* continue with the triggered buffer */
>> + ret = iio_triggered_buffer_predisable(indio_dev);
>> + if (ret < 0)
>> + dev_err(&indio_dev->dev, "buffer predisable failed\n");
>> +
>> + /* if we are using DMA we must clear registers and end DMA */
>> + if (st->dma_st.dma_chan)
>> + dmaengine_terminate_sync(st->dma_st.dma_chan);
>
> This ordering is going to stop Alex doing his rework to remove the need
> to manually call iio_triggered_buffer_predisable. Why does it make
> sense to do the dma stuff after that?
>
> Ah I see it always did and the postenable is the opposite of what Alex
> has been moving to as well.
Ok, so keep it like this ?
>
>> +
>> return ret;
>> }
>>
>> @@ -1131,6 +1141,13 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>> struct iio_dev *indio_dev = pf->indio_dev;
>> struct at91_adc_state *st = iio_priv(indio_dev);
>>
>> + /*
>> + * If it's not our trigger, start a conversion now, as we are
>> + * actually polling the trigger now.
>> + */
>> + if (iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
>> + at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
>> +
>> if (st->dma_st.dma_chan)
>> at91_adc_trigger_handler_dma(indio_dev);
>> else
>> @@ -1143,20 +1160,9 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>>
>> static int at91_adc_buffer_init(struct iio_dev *indio)
>> {
>> - struct at91_adc_state *st = iio_priv(indio);
>> -
>> - if (st->selected_trig->hw_trig) {
>> - return devm_iio_triggered_buffer_setup(&indio->dev, indio,
>> - &iio_pollfunc_store_time,
>> - &at91_adc_trigger_handler, &at91_buffer_setup_ops);
>> - }
>> - /*
>> - * we need to prepare the buffer ops in case we will get
>> - * another buffer attached (like a callback buffer for the touchscreen)
>> - */
>> - indio->setup_ops = &at91_buffer_setup_ops;
>> -
>> - return 0;
>> + return devm_iio_triggered_buffer_setup(&indio->dev, indio,
>> + &iio_pollfunc_store_time,
>> + &at91_adc_trigger_handler, &at91_buffer_setup_ops);
>> }
>>
>> static unsigned at91_adc_startup_time(unsigned startup_time_min,
>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: handle unfinished conversions
2020-01-17 17:34 ` Jonathan Cameron
@ 2020-01-20 7:27 ` Eugen.Hristev
2020-01-27 12:13 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Eugen.Hristev @ 2020-01-20 7:27 UTC (permalink / raw)
To: Jonathan.Cameron
Cc: linux-iio, Ludovic.Desroches, linux-arm-kernel, jic23, linux-kernel
On 17.01.2020 19:34, Jonathan Cameron wrote:
> On Mon, 13 Jan 2020 12:07:09 +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).
>> 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, use usleep
>> to sleep until the conversion is done or we timeout.
>> Normally, it should happen that in a short time fashion, all channels are
>> ready, since the first IRQ triggered.
>> If a hardware fault happens (for example the clock suddently dissappears),
>> the handler will not be completed, in which case we do not report anything to
>> userspace anymore.
>> Also, change from using the EOC interrupts to DRDY interrupt.
>> This helps with the fact that not 'n' interrupt statuses are enabled,
>> each being able to trigger an interrupt, and instead only data ready
>> interrupt can wake up the CPU. Like this, when data is ready, check in
>> handler which and how many channels are done. While the DRDY is raised,
>> other IRQs cannot occur. Once the channel data is being read, we ack the
>> IRQ and finish the conversion.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>> Changes in v2:
>> - move start of conversion to threaded irq, removed specific at91 pollfunc
>> - add timeout to channel mask readiness check in trigger handler
>> - use DRDY irq instead of EOC irqs.
>> - move enable irq after DRDY has been acked in reenable_trigger
>>
>> drivers/iio/adc/at91-sama5d2_adc.c | 62 ++++++++++++++++++++++++++++----------
>> 1 file changed, 46 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 2a6950a..454a493 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>
>> @@ -100,6 +101,8 @@
>> #define AT91_SAMA5D2_IER_YRDY BIT(21)
>> /* Interrupt Enable Register - TS pressure measurement ready */
>> #define AT91_SAMA5D2_IER_PRDY BIT(22)
>> +/* Interrupt Enable Register - Data ready */
>> +#define AT91_SAMA5D2_IER_DRDY BIT(24)
>> /* Interrupt Enable Register - general overrun error */
>> #define AT91_SAMA5D2_IER_GOVRE BIT(25)
>> /* Interrupt Enable Register - Pen detect */
>> @@ -486,6 +489,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 */
>> @@ -746,24 +764,19 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>> at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
>> }
>>
>> - if (state) {
>> + if (state)
>> at91_adc_writel(st, AT91_SAMA5D2_CHER,
>> BIT(chan->channel));
>> - /* enable irq only if not using DMA */
>> - if (!st->dma_st.dma_chan) {
>> - at91_adc_writel(st, AT91_SAMA5D2_IER,
>> - BIT(chan->channel));
>> - }
>> - } else {
>> - /* disable irq only if not using DMA */
>> - if (!st->dma_st.dma_chan) {
>> - at91_adc_writel(st, AT91_SAMA5D2_IDR,
>> - BIT(chan->channel));
>> - }
>> + else
>> at91_adc_writel(st, AT91_SAMA5D2_CHDR,
>> BIT(chan->channel));
>> - }
>> }
>> + /* enable irq only if not using DMA */
>> + if (state && !st->dma_st.dma_chan)
>> + at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
>> + /* disable irq only if not using DMA */
>> + if (!state && !st->dma_st.dma_chan)
>> + at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
> Hmm. Would have been nicer to avoid the refactor and just have the change to
> what was written. If you want to keep it, perhaps:
>
> /* Nothing to do if using DMA */
> if (!st->dma_st.dma_chan)
> return 0;
>
> if (state)
> at91...
> else
> at91...
>
Hi Jonathan,
Ok I will rework it in next version
>>
>> return 0;
>> }
>> @@ -777,10 +790,10 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
>> if (st->dma_st.dma_chan)
>> return 0;
>>
>> - enable_irq(st->irq);
>> -
>> /* Needed to ACK the DRDY interruption */
>> at91_adc_readl(st, AT91_SAMA5D2_LCDR);
>> +
>> + enable_irq(st->irq);
>
> Why this change? I'm not totally following the description above.
>
The ' reading of the LCDR register ' makes the DRDY bit in ISR
(interrupt status register) to be cleared.
So, reading that clears the IRQ, but, if we enable the IRQs before this
clearance, there is a race chance that we get the DRDY IRQ triggered again.
It is best to clear the DRDY first, and then re enable the IRQs.
Does it make sense ?
>> return 0;
>> }
>>
>> @@ -1015,6 +1028,22 @@ static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
>> int i = 0;
>> int val;
>> u8 bit;
>> + u32 mask = at91_adc_active_scan_mask_to_reg(indio_dev);
>> + unsigned int timeout = 50;
>> +
>> + /*
>> + * Check if the conversion is ready. If not, wait a little bit, and
>> + * in case of timeout exit with an error.
>> + */
>> + while ((at91_adc_readl(st, AT91_SAMA5D2_ISR) & mask) != mask &&
>> + timeout) {
>> + usleep_range(50, 100);
>> + timeout--;
>> + }
>> +
>> + /* Cannot read data, not ready. Continue without reporting data */
>> + if (!timeout)
>> + return;
>>
>> for_each_set_bit(bit, indio_dev->active_scan_mask,
>> indio_dev->num_channels) {
>> @@ -1281,7 +1310,8 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>> status = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
>> status = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
>> status = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
>> - } else if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
>> + } else if (iio_buffer_enabled(indio) &&
>> + (status & AT91_SAMA5D2_IER_DRDY)) {
>> /* triggered buffer without DMA */
>> disable_irq_nosync(irq);
>> iio_trigger_poll(indio->trig);
>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: handle unfinished conversions
2020-01-20 7:27 ` Eugen.Hristev
@ 2020-01-27 12:13 ` Jonathan Cameron
0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2020-01-27 12:13 UTC (permalink / raw)
To: Eugen.Hristev
Cc: linux-iio, Ludovic.Desroches, linux-arm-kernel, jic23, linux-kernel
On Mon, 20 Jan 2020 07:27:00 +0000
<Eugen.Hristev@microchip.com> wrote:
> On 17.01.2020 19:34, Jonathan Cameron wrote:
>
> > On Mon, 13 Jan 2020 12:07:09 +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).
> >> 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, use usleep
> >> to sleep until the conversion is done or we timeout.
> >> Normally, it should happen that in a short time fashion, all channels are
> >> ready, since the first IRQ triggered.
> >> If a hardware fault happens (for example the clock suddently dissappears),
> >> the handler will not be completed, in which case we do not report anything to
> >> userspace anymore.
> >> Also, change from using the EOC interrupts to DRDY interrupt.
> >> This helps with the fact that not 'n' interrupt statuses are enabled,
> >> each being able to trigger an interrupt, and instead only data ready
> >> interrupt can wake up the CPU. Like this, when data is ready, check in
> >> handler which and how many channels are done. While the DRDY is raised,
> >> other IRQs cannot occur. Once the channel data is being read, we ack the
> >> IRQ and finish the conversion.
> >>
> >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> >> ---
> >> Changes in v2:
> >> - move start of conversion to threaded irq, removed specific at91 pollfunc
> >> - add timeout to channel mask readiness check in trigger handler
> >> - use DRDY irq instead of EOC irqs.
> >> - move enable irq after DRDY has been acked in reenable_trigger
> >>
> >> drivers/iio/adc/at91-sama5d2_adc.c | 62 ++++++++++++++++++++++++++++----------
> >> 1 file changed, 46 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> >> index 2a6950a..454a493 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>
> >> @@ -100,6 +101,8 @@
> >> #define AT91_SAMA5D2_IER_YRDY BIT(21)
> >> /* Interrupt Enable Register - TS pressure measurement ready */
> >> #define AT91_SAMA5D2_IER_PRDY BIT(22)
> >> +/* Interrupt Enable Register - Data ready */
> >> +#define AT91_SAMA5D2_IER_DRDY BIT(24)
> >> /* Interrupt Enable Register - general overrun error */
> >> #define AT91_SAMA5D2_IER_GOVRE BIT(25)
> >> /* Interrupt Enable Register - Pen detect */
> >> @@ -486,6 +489,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 */
> >> @@ -746,24 +764,19 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> >> at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> >> }
> >>
> >> - if (state) {
> >> + if (state)
> >> at91_adc_writel(st, AT91_SAMA5D2_CHER,
> >> BIT(chan->channel));
> >> - /* enable irq only if not using DMA */
> >> - if (!st->dma_st.dma_chan) {
> >> - at91_adc_writel(st, AT91_SAMA5D2_IER,
> >> - BIT(chan->channel));
> >> - }
> >> - } else {
> >> - /* disable irq only if not using DMA */
> >> - if (!st->dma_st.dma_chan) {
> >> - at91_adc_writel(st, AT91_SAMA5D2_IDR,
> >> - BIT(chan->channel));
> >> - }
> >> + else
> >> at91_adc_writel(st, AT91_SAMA5D2_CHDR,
> >> BIT(chan->channel));
> >> - }
> >> }
> >> + /* enable irq only if not using DMA */
> >> + if (state && !st->dma_st.dma_chan)
> >> + at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
> >> + /* disable irq only if not using DMA */
> >> + if (!state && !st->dma_st.dma_chan)
> >> + at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
> > Hmm. Would have been nicer to avoid the refactor and just have the change to
> > what was written. If you want to keep it, perhaps:
> >
> > /* Nothing to do if using DMA */
> > if (!st->dma_st.dma_chan)
> > return 0;
> >
> > if (state)
> > at91...
> > else
> > at91...
> >
>
> Hi Jonathan,
>
> Ok I will rework it in next version
>
> >>
> >> return 0;
> >> }
> >> @@ -777,10 +790,10 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
> >> if (st->dma_st.dma_chan)
> >> return 0;
> >>
> >> - enable_irq(st->irq);
> >> -
> >> /* Needed to ACK the DRDY interruption */
> >> at91_adc_readl(st, AT91_SAMA5D2_LCDR);
> >> +
> >> + enable_irq(st->irq);
> >
> > Why this change? I'm not totally following the description above.
> >
>
> The ' reading of the LCDR register ' makes the DRDY bit in ISR
> (interrupt status register) to be cleared.
> So, reading that clears the IRQ, but, if we enable the IRQs before this
> clearance, there is a race chance that we get the DRDY IRQ triggered again.
> It is best to clear the DRDY first, and then re enable the IRQs.
>
> Does it make sense ?
Why is it an issue if dataready is triggered again?
That should only happen if there really is new data. In that case we
want to call the interrupt handler again.
Normally hardware will only generate a new interrupt after the status
register is cleared. If that's not the case here than the hardware is
racy whatever order we do it in. If not, the old order is correct as
we don't want to potentially miss the interrupt.
Chances are this is a level interrupt so it won't make any difference
either way, we either trigger at or after the readl (original code)
or it's still set when we hit the enable_irq and trigger then
(with reordered code).
So unless I'm missing something, original code order was more 'standard'
but it may make not difference.
Jonathan
>
> >> return 0;
> >> }
> >>
> >> @@ -1015,6 +1028,22 @@ static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
> >> int i = 0;
> >> int val;
> >> u8 bit;
> >> + u32 mask = at91_adc_active_scan_mask_to_reg(indio_dev);
> >> + unsigned int timeout = 50;
> >> +
> >> + /*
> >> + * Check if the conversion is ready. If not, wait a little bit, and
> >> + * in case of timeout exit with an error.
> >> + */
> >> + while ((at91_adc_readl(st, AT91_SAMA5D2_ISR) & mask) != mask &&
> >> + timeout) {
> >> + usleep_range(50, 100);
> >> + timeout--;
> >> + }
> >> +
> >> + /* Cannot read data, not ready. Continue without reporting data */
> >> + if (!timeout)
> >> + return;
> >>
> >> for_each_set_bit(bit, indio_dev->active_scan_mask,
> >> indio_dev->num_channels) {
> >> @@ -1281,7 +1310,8 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
> >> status = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
> >> status = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
> >> status = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
> >> - } else if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
> >> + } else if (iio_buffer_enabled(indio) &&
> >> + (status & AT91_SAMA5D2_IER_DRDY)) {
> >> /* triggered buffer without DMA */
> >> disable_irq_nosync(irq);
> >> iio_trigger_poll(indio->trig);
> >
> >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] iio: adc: at91-sama5d2_adc: update for other trigger usage
2020-01-20 7:24 ` Eugen.Hristev
@ 2020-01-27 12:13 ` Jonathan Cameron
0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2020-01-27 12:13 UTC (permalink / raw)
To: Eugen.Hristev
Cc: linux-iio, linux-kernel, Ludovic.Desroches, jic23,
alexandru.ardelean, linux-arm-kernel
On Mon, 20 Jan 2020 07:24:50 +0000
<Eugen.Hristev@microchip.com> wrote:
> On 17.01.2020 19:42, Jonathan Cameron wrote:
>
> > On Mon, 13 Jan 2020 12:07:10 +0000
> > <Eugen.Hristev@microchip.com> wrote:
> >
> >> From: Eugen Hristev <eugen.hristev@microchip.com>
> >>
> >> This change will allow the at91-sama5d2_adc driver to use other triggers
> >> than it's own.
> >> In particular, tested with the sysfs trigger.
> >> To be able to achieve this functionality, some changes were required:
> >> 1) Do not enable/disable channels when enabling/disabling the trigger.
> >> This is because the trigger is enabled/disabled only for our trigger
> >> (obviously). We need channels enabled/disabled regardless of what trigger is
> >> being used.
> >> 2) Cope with DMA : DMA cannot be used when using another type of trigger.
> >> Other triggers work through pollfunc, so we get polled anyway on every trigger.
> >> Thus we have to obtain data at every trigger.
> >> 3) When to start conversion? The usual pollfunc (store time from subsystem)
> >> would be in hard irq and this would be a good way, but current iio subsystem
> >> recommends to have it in the threaded irq. Thus adding software start
> >> code in this handler.
> >> 4) Buffer config: we need to setup buffer regardless of our own device's
> >> trigger. We may get one attached later.
> >> 5) IRQ handling: we use our own device IRQ only if it's our own trigger
> >> and we do not use DMA . If we use DMA, we use the DMA controller's IRQ.
> >>
> >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> >
> > +CC Alexandru as he's doing a lot of cleanup around the buffer functions.
> > I'd like Alex to take a look at this.
> >
> > A few comments inline from me.
> >
> > Thanks,
> >
> > Jonathan
> >
> >
> >
> >> ---
> >> Changes in v2:
> >> - adapt to the situation of having the previous two patches ahead in the series
> >>
> >> drivers/iio/adc/at91-sama5d2_adc.c | 140 +++++++++++++++++++------------------
> >> 1 file changed, 73 insertions(+), 67 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> >> index 454a493..34df043 100644
> >> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> >> @@ -728,7 +728,6 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> >> struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> >> struct at91_adc_state *st = iio_priv(indio);
> >> u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
> >> - u8 bit;
> >>
> >> /* clear TRGMOD */
> >> status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
> >> @@ -739,45 +738,6 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> >> /* set/unset hw trigger */
> >> at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
> >>
> >> - for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> >> - struct iio_chan_spec const *chan = at91_adc_chan_get(indio, bit);
> >> - u32 cor;
> >> -
> >> - if (!chan)
> >> - continue;
> >> - /* these channel types cannot be handled by this trigger */
> >> - if (chan->type == IIO_POSITIONRELATIVE ||
> >> - chan->type == IIO_PRESSURE)
> >> - continue;
> >> -
> >> - if (state) {
> >> - cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
> >> -
> >> - if (chan->differential)
> >> - cor |= (BIT(chan->channel) |
> >> - BIT(chan->channel2)) <<
> >> - AT91_SAMA5D2_COR_DIFF_OFFSET;
> >> - else
> >> - cor &= ~(BIT(chan->channel) <<
> >> - AT91_SAMA5D2_COR_DIFF_OFFSET);
> >> -
> >> - at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> >> - }
> >> -
> >> - if (state)
> >> - at91_adc_writel(st, AT91_SAMA5D2_CHER,
> >> - BIT(chan->channel));
> >> - else
> >> - at91_adc_writel(st, AT91_SAMA5D2_CHDR,
> >> - BIT(chan->channel));
> >> - }
> >> - /* enable irq only if not using DMA */
> >> - if (state && !st->dma_st.dma_chan)
> >> - at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
> >> - /* disable irq only if not using DMA */
> >> - if (!state && !st->dma_st.dma_chan)
> >> - at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
> >> -
> >> return 0;
> >> }
> >>
> >> @@ -901,9 +861,22 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
> >> return 0;
> >> }
> >>
> >> +static bool at91_adc_buffer_check_use_irq(struct iio_dev *indio,
> >> + struct at91_adc_state *st)
> >> +{
> >> + /* if using DMA, we do not use our own IRQ (we use DMA-controller) */
> >> + if (st->dma_st.dma_chan)
> >> + return false;
> >> + /* if the trigger is not ours, then it has its own IRQ */
> >> + if (iio_trigger_validate_own_device(indio->trig, indio))
> >> + return false;
> >> + return true;
> >> +}
> >> +
> >> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
> >> {
> >> int ret;
> >> + u8 bit;
> >> struct at91_adc_state *st = iio_priv(indio_dev);
> >>
> >> /* check if we are enabling triggered buffer or the touchscreen */
> >> @@ -921,9 +894,40 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
> >> ret = at91_adc_dma_start(indio_dev);
> >> if (ret) {
> >> dev_err(&indio_dev->dev, "buffer postenable failed\n");
> >> + iio_triggered_buffer_predisable(indio_dev);
> >
> > This seems odd given you have called the iio_triggered_buffer_postenable yet..
> > That is below.
>
> Hi Jonathan,
>
> You are right, I will remove this.
>
> >
> >> return ret;
> >> }
> >>
> >> + 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);
> >> + u32 cor;
> >> +
> >> + if (!chan)
> >> + continue;
> >> + /* these channel types cannot be handled by this trigger */
> >> + if (chan->type == IIO_POSITIONRELATIVE ||
> >> + chan->type == IIO_PRESSURE)
> >> + continue;
> >> +
> >> + cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
> >> +
> >> + if (chan->differential)
> >> + cor |= (BIT(chan->channel) | BIT(chan->channel2)) <<
> >> + AT91_SAMA5D2_COR_DIFF_OFFSET;
> >> + else
> >> + cor &= ~(BIT(chan->channel) <<
> >> + AT91_SAMA5D2_COR_DIFF_OFFSET);
> >> +
> >> + at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> >> +
> >> + at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
> >> + }
> >> +
> >> + if (at91_adc_buffer_check_use_irq(indio_dev, st))
> >> + at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
> >> +
> >> return iio_triggered_buffer_postenable(indio_dev);
> >> }
> >>
> >> @@ -944,21 +948,11 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> >> return -EINVAL;
> >>
> >> - /* continue with the triggered buffer */
> >> - ret = iio_triggered_buffer_predisable(indio_dev);
> >> - if (ret < 0)
> >> - dev_err(&indio_dev->dev, "buffer predisable failed\n");
> >> -
> >> - if (!st->dma_st.dma_chan)
> >> - return ret;
> >> -
> >> - /* if we are using DMA we must clear registers and end DMA */
> >> - dmaengine_terminate_sync(st->dma_st.dma_chan);
> >> -
> >> /*
> >> - * For each enabled channel we must read the last converted value
> >> + * For each enable channel we must disable it in hardware.
> >> + * In the case of DMA, we must read the last converted value
> >> * to clear EOC status and not get a possible interrupt later.
> >> - * This value is being read by DMA from LCDR anyway
> >> + * This value is being read by DMA from LCDR anyway, so it's not lost.
> >> */
> >> for_each_set_bit(bit, indio_dev->active_scan_mask,
> >> indio_dev->num_channels) {
> >> @@ -971,12 +965,28 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> >> if (chan->type == IIO_POSITIONRELATIVE ||
> >> chan->type == IIO_PRESSURE)
> >> continue;
> >> +
> >> + at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
> >> +
> >> if (st->dma_st.dma_chan)
> >> at91_adc_readl(st, chan->address);
> >> }
> >>
> >> + if (at91_adc_buffer_check_use_irq(indio_dev, st))
> >> + at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
> >> +
> >> /* read overflow register to clear possible overflow status */
> >> at91_adc_readl(st, AT91_SAMA5D2_OVER);
> >> +
> >> + /* continue with the triggered buffer */
> >> + ret = iio_triggered_buffer_predisable(indio_dev);
> >> + if (ret < 0)
> >> + dev_err(&indio_dev->dev, "buffer predisable failed\n");
> >> +
> >> + /* if we are using DMA we must clear registers and end DMA */
> >> + if (st->dma_st.dma_chan)
> >> + dmaengine_terminate_sync(st->dma_st.dma_chan);
> >
> > This ordering is going to stop Alex doing his rework to remove the need
> > to manually call iio_triggered_buffer_predisable. Why does it make
> > sense to do the dma stuff after that?
> >
> > Ah I see it always did and the postenable is the opposite of what Alex
> > has been moving to as well.
>
> Ok, so keep it like this ?
For this series yes. But I'd like input from Alex on this more generally!
Thanks,
Jonathan
>
> >
> >> +
> >> return ret;
> >> }
> >>
> >> @@ -1131,6 +1141,13 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> >> struct iio_dev *indio_dev = pf->indio_dev;
> >> struct at91_adc_state *st = iio_priv(indio_dev);
> >>
> >> + /*
> >> + * If it's not our trigger, start a conversion now, as we are
> >> + * actually polling the trigger now.
> >> + */
> >> + if (iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
> >> + at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
> >> +
> >> if (st->dma_st.dma_chan)
> >> at91_adc_trigger_handler_dma(indio_dev);
> >> else
> >> @@ -1143,20 +1160,9 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> >>
> >> static int at91_adc_buffer_init(struct iio_dev *indio)
> >> {
> >> - struct at91_adc_state *st = iio_priv(indio);
> >> -
> >> - if (st->selected_trig->hw_trig) {
> >> - return devm_iio_triggered_buffer_setup(&indio->dev, indio,
> >> - &iio_pollfunc_store_time,
> >> - &at91_adc_trigger_handler, &at91_buffer_setup_ops);
> >> - }
> >> - /*
> >> - * we need to prepare the buffer ops in case we will get
> >> - * another buffer attached (like a callback buffer for the touchscreen)
> >> - */
> >> - indio->setup_ops = &at91_buffer_setup_ops;
> >> -
> >> - return 0;
> >> + return devm_iio_triggered_buffer_setup(&indio->dev, indio,
> >> + &iio_pollfunc_store_time,
> >> + &at91_adc_trigger_handler, &at91_buffer_setup_ops);
> >> }
> >>
> >> static unsigned at91_adc_startup_time(unsigned startup_time_min,
> >
> >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-27 12:14 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-13 12:07 [PATCH v2 0/3] Enhancements to at91-sama5d2_adc driver Eugen.Hristev
2020-01-13 12:07 ` [PATCH v2 1/3] iio: adc: at91-sama5d2_adc: fix differential channels in triggered mode Eugen.Hristev
2020-01-13 12:07 ` [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: handle unfinished conversions Eugen.Hristev
2020-01-17 17:34 ` Jonathan Cameron
2020-01-20 7:27 ` Eugen.Hristev
2020-01-27 12:13 ` Jonathan Cameron
2020-01-13 12:07 ` [PATCH v2 3/3] iio: adc: at91-sama5d2_adc: update for other trigger usage Eugen.Hristev
2020-01-17 17:42 ` Jonathan Cameron
2020-01-20 7:24 ` Eugen.Hristev
2020-01-27 12:13 ` Jonathan Cameron
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).