Message-ID: <1576062159-4832-1-git-send-email-eugen.hristev@microchip.com>
Date:   Wed, 11 Dec 2019 11:03:14 +0000
From:   <Eugen.Hristev@...rochip.com>
To:     <jic23@...nel.org>
CC:     <Ludovic.Desroches@...rochip.com>, <linux-iio@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <Eugen.Hristev@...rochip.com>
Subject: [PATCH] iio: adc: at91-sama5d2_adc: update for other trigger usage

From: Eugen Hristev <eugen.hristev@...rochip.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)
is replaced with specific at91 code, that will start the software conversion
on the poll action(if it's not our trigger).
4) When is the conversion done ? Usually it should be done at EOC (end of
channel) interrupt. But we start the conversion in pollfunc. So, in the handler
for this pollfunc, check if data is ready. If not ready, cannot busywait, so,
start the workq to get the data later.
5) Buffer config: we need to setup buffer regardless of our own device's
trigger. We may get one attached later.
6) 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.
7) Touchscreen workq: the workq is now also used with other triggers. So, move
this from the touchscreen state struct to the at91_adc_state.
8) Timestamp: the timestamp is kept in the pollfunc. However if in the handler
we start a workq, the timestamp is no longer accessible. Copy it to our state
struct.

Signed-off-by: Eugen Hristev <eugen.hristev@...rochip.com>
---

Hi Jonathan,

This patch is quite a leap, hopefully I am on the right track to do this
right.
What I am not very sure about are number 8) and number 3)

Here is how it works with sysfs trigger:

 # echo 1 > /sys/bus/iio/devices/iio_sysfs_trigger/add_trigger
 # echo 1 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltage4_en
 #
 #
 # iio_generic_buffer -n fc030000.adc -t sysfstrig1 -c 5  &
 # iio device number being used is 0
 iio trigger number being used is 1
 /sys/bus/iio/devices/iio:device0 sysfstrig1
 # echo 1 > /sys/bus/iio/devices/iio_sysfs_trigger/trigger1/trigger_now
 # 3293.554688
 #
 # echo 1 > /sys/bus/iio/devices/iio_sysfs_trigger/trigger1/trigger_now
 # 3297.583008
 # echo 1 > /sys/bus/iio/devices/iio_sysfs_trigger/trigger1/trigger_now
 # 3296.777344
 # echo 1 > /sys/bus/iio/devices/iio_sysfs_trigger/trigger1/trigger_now
 # 3297.583008


Thanks,
Eugen

 drivers/iio/adc/at91-sama5d2_adc.c | 212 ++++++++++++++++++++++---------------
 1 file changed, 127 insertions(+), 85 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index e1850f3..c575970 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -378,7 +378,6 @@ struct at91_adc_touch {
 	bool				touching;
 	u16				x_pos;
 	unsigned long			channels_bitmask;
-	struct work_struct		workq;
 };
 
 struct at91_adc_state {
@@ -405,6 +404,8 @@ struct at91_adc_state {
 	 * sysfs.
 	 */
 	struct mutex			lock;
+	struct work_struct		workq;
+	s64				timestamp;
 };
 
 static const struct at91_adc_trigger at91_adc_trigger_list[] = {
@@ -710,7 +711,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;
@@ -721,35 +721,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);
-
-		if (!chan)
-			continue;
-		/* these channel types cannot be handled by this trigger */
-		if (chan->type == IIO_POSITIONRELATIVE ||
-		    chan->type == IIO_PRESSURE)
-			continue;
-
-		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));
-			}
-			at91_adc_writel(st, AT91_SAMA5D2_CHDR,
-					BIT(chan->channel));
-		}
-	}
-
 	return 0;
 }
 
@@ -873,69 +844,90 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
 	return 0;
 }
 
-static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
+#define AT91_ADC_BUFFER_CHECK_USE_IRQ(use_irq)  { \
+	use_irq = true; \
+	/* if using DMA, we do not use our own IRQ (we use DMA-controller) */ \
+	if (st->dma_st.dma_chan) \
+		use_irq = false; \
+	/* if the trigger is not ours, then it has its own IRQ */ \
+	if (iio_trigger_validate_own_device(indio->trig, indio)) \
+		use_irq = false; \
+	}
+
+static int at91_adc_buffer_postenable(struct iio_dev *indio)
 {
 	int ret;
-	struct at91_adc_state *st = iio_priv(indio_dev);
+	u8 bit;
+	bool use_irq;
+	struct at91_adc_state *st = iio_priv(indio);
 
 	/* check if we are enabling triggered buffer or the touchscreen */
-	if (bitmap_subset(indio_dev->active_scan_mask,
+	if (bitmap_subset(indio->active_scan_mask,
 			  &st->touch_st.channels_bitmask,
 			  AT91_SAMA5D2_MAX_CHAN_IDX + 1)) {
 		/* touchscreen enabling */
 		return at91_adc_configure_touch(st, true);
 	}
 	/* if we are not in triggered mode, we cannot enable the buffer. */
-	if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
+	if (!(indio->currentmode & INDIO_ALL_TRIGGERED_MODES))
 		return -EINVAL;
 
 	/* we continue with the triggered buffer */
-	ret = at91_adc_dma_start(indio_dev);
+	ret = at91_adc_dma_start(indio);
 	if (ret) {
-		dev_err(&indio_dev->dev, "buffer postenable failed\n");
+		dev_err(&indio->dev, "buffer postenable failed\n");
+		iio_triggered_buffer_predisable(indio);
 		return ret;
 	}
 
-	return iio_triggered_buffer_postenable(indio_dev);
+	AT91_ADC_BUFFER_CHECK_USE_IRQ(use_irq);
+
+	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
+		struct iio_chan_spec const *chan = at91_adc_chan_get(indio, bit);
+
+		if (!chan)
+			continue;
+		/* these channel types cannot be handled by this trigger */
+		if (chan->type == IIO_POSITIONRELATIVE ||
+		    chan->type == IIO_PRESSURE)
+			continue;
+
+		at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
+		if (use_irq) {
+			at91_adc_writel(st, AT91_SAMA5D2_IER,
+					BIT(chan->channel));
+		}
+	}
+	return iio_triggered_buffer_postenable(indio);
 }
 
-static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
+static int at91_adc_buffer_predisable(struct iio_dev *indio)
 {
-	struct at91_adc_state *st = iio_priv(indio_dev);
+	struct at91_adc_state *st = iio_priv(indio);
 	int ret;
 	u8 bit;
+	bool use_irq;
 
 	/* check if we are disabling triggered buffer or the touchscreen */
-	if (bitmap_subset(indio_dev->active_scan_mask,
+	if (bitmap_subset(indio->active_scan_mask,
 			  &st->touch_st.channels_bitmask,
 			  AT91_SAMA5D2_MAX_CHAN_IDX + 1)) {
 		/* touchscreen disable */
 		return at91_adc_configure_touch(st, false);
 	}
 	/* if we are not in triggered mode, nothing to do here */
-	if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
+	if (!(indio->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);
-
+	AT91_ADC_BUFFER_CHECK_USE_IRQ(use_irq);
 	/*
-	 * 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) {
-		struct iio_chan_spec const *chan =
-					at91_adc_chan_get(indio_dev, bit);
+	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
+		struct iio_chan_spec const *chan = at91_adc_chan_get(indio, bit);
 
 		if (!chan)
 			continue;
@@ -943,12 +935,29 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
 		if (chan->type == IIO_POSITIONRELATIVE ||
 		    chan->type == IIO_PRESSURE)
 			continue;
+
+		if (use_irq) {
+			at91_adc_writel(st, AT91_SAMA5D2_IDR,
+					BIT(chan->channel));
+		}
+		at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
+
 		if (st->dma_st.dma_chan)
 			at91_adc_readl(st, chan->address);
 	}
 
 	/* 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);
+	if (ret < 0)
+		dev_err(&indio->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;
 }
 
@@ -993,8 +1002,8 @@ static int at91_adc_trigger_init(struct iio_dev *indio)
 	return 0;
 }
 
-static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
-					   struct iio_poll_func *pf)
+static void at91_adc_read_and_push_channels(struct iio_dev *indio_dev,
+					    s64 timestamp)
 {
 	struct at91_adc_state *st = iio_priv(indio_dev);
 	int i = 0;
@@ -1028,11 +1037,30 @@ static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
 		}
 		i++;
 	}
-	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer,
-					   pf->timestamp);
+	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer, timestamp);
+}
+
+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);
+
+	/*
+	 * 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))) {
+		schedule_work(&st->workq);
+		return -EINPROGRESS;
+	}
+
+	/* we have data, so let's extract and push it */
+	at91_adc_read_and_push_channels(indio_dev, pf->timestamp);
+
+	return 0;
 }
 
-static void at91_adc_trigger_handler_dma(struct iio_dev *indio_dev)
+static int at91_adc_trigger_handler_dma(struct iio_dev *indio_dev)
 {
 	struct at91_adc_state *st = iio_priv(indio_dev);
 	int transferred_len = at91_adc_dma_size_done(st);
@@ -1079,6 +1107,8 @@ static void at91_adc_trigger_handler_dma(struct iio_dev *indio_dev)
 	}
 	/* adjust saved time for next transfer handling */
 	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
+
+	return 0;
 }
 
 static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
@@ -1086,33 +1116,41 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct at91_adc_state *st = iio_priv(indio_dev);
+	int ret;
 
+	st->timestamp = pf->timestamp;
 	if (st->dma_st.dma_chan)
-		at91_adc_trigger_handler_dma(indio_dev);
+		ret = at91_adc_trigger_handler_dma(indio_dev);
 	else
-		at91_adc_trigger_handler_nodma(indio_dev, pf);
+		ret = at91_adc_trigger_handler_nodma(indio_dev, pf);
 
-	iio_trigger_notify_done(indio_dev->trig);
+	if (!ret)
+		iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
 }
 
-static int at91_adc_buffer_init(struct iio_dev *indio)
+irqreturn_t at91_adc_pollfunc(int irq, void *p)
 {
-	struct at91_adc_state *st = iio_priv(indio);
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct at91_adc_state *st = iio_priv(indio_dev);
 
-	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)
+	 * If it's not our trigger, start a conversion now, as we are
+	 * actually polling the trigger now.
 	 */
-	indio->setup_ops = &at91_buffer_setup_ops;
+	if (iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
+		at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
 
-	return 0;
+	return iio_pollfunc_store_time(irq, p);
+}
+
+static int at91_adc_buffer_init(struct iio_dev *indio)
+{
+	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
+		&at91_adc_pollfunc,
+		&at91_adc_trigger_handler, &at91_buffer_setup_ops);
 }
 
 static unsigned at91_adc_startup_time(unsigned startup_time_min,
@@ -1195,7 +1233,7 @@ static void at91_adc_touch_data_handler(struct iio_dev *indio_dev)
 	 * from our IRQ context. Which is something we better avoid.
 	 * Let's schedule it after our IRQ is completed.
 	 */
-	schedule_work(&st->touch_st.workq);
+	schedule_work(&st->workq);
 }
 
 static void at91_adc_pen_detect_interrupt(struct at91_adc_state *st)
@@ -1228,13 +1266,17 @@ static void at91_adc_no_pen_detect_interrupt(struct at91_adc_state *st)
 
 static void at91_adc_workq_handler(struct work_struct *workq)
 {
-	struct at91_adc_touch *touch_st = container_of(workq,
-					struct at91_adc_touch, workq);
-	struct at91_adc_state *st = container_of(touch_st,
-					struct at91_adc_state, touch_st);
+	struct at91_adc_state *st = container_of(workq,
+					struct at91_adc_state, workq);
 	struct iio_dev *indio_dev = iio_priv_to_dev(st);
 
-	iio_push_to_buffers(indio_dev, st->buffer);
+	if ((indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES) &&
+	    iio_trigger_validate_own_device(indio_dev->trig, indio_dev)) {
+		at91_adc_read_and_push_channels(indio_dev, st->timestamp);
+		iio_trigger_notify_done(indio_dev->trig);
+	} else {
+		iio_push_to_buffers(indio_dev, st->buffer);
+	}
 }
 
 static irqreturn_t at91_adc_interrupt(int irq, void *private)
@@ -1711,7 +1753,7 @@ static int at91_adc_probe(struct platform_device *pdev)
 
 	init_waitqueue_head(&st->wq_data_available);
 	mutex_init(&st->lock);
-	INIT_WORK(&st->touch_st.workq, at91_adc_workq_handler);
+	INIT_WORK(&st->workq, at91_adc_workq_handler);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
-- 
2.7.4

Powered by blists - more mailing lists