[PATCH 00/10] Enhancements to at91-sama5d2_adc and rtc trigger

[PATCH 00/10] Enhancements to at91-sama5d2_adc and rtc trigger

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

This series includes support for having the Real Time Clock trigger
capability for the Analog to Digital Converter in sama5d2-based SoCs
(RTC ADC Trigger)

The first patch of the series has been already submitted on the iio mailing list
as
[PATCH] iio: adc: at91-sama5d2_adc: update for other trigger usage
But I also include here for reference since the other commits on the driver
use this as a base commit.

In short, the RTC block can trigger the ADC block to perform a conversion.
To solve this, I created a driver named rtc-adc-trigger that shares the
register map with the RTC. It's done in devicetree as a subnode.
This driver will register a separate trigger inside the iio subsystem.
This trigger can then be associated to the ADC driver (sysfs current_trigger).
However, this is not enough. The ADC has to be aware that it;s being
triggered by the RTC (TRGMOD and TRGSEL). So, this hardware link between
the two IPs has been described as a phandle reference in the ADC node to the
RTC trigger node.
At runtime (trigger selection), the ADC will check if the assigned trigger
is the RTC one given by the phandle link. If so, it will configure
accordingly.
The RTC trigger driver will also register to sysfs two attributes for
selecting the desired trigger frequency.
One attribute is RO : list of possible frequencies.
Another attribute is RW: current set frequency.

To achieve all this, had to make a small patch on the RTC to populate
child nodes platform data to probe them.

Fixed other issues with the adc driver: unfinished conversions on IRQ in
triggered mode, and differential channels missing configurations in
triggered mode.

For exercising this, created DT patches for sama5d2/sama5d2_xplained.

Here is a sample of how it works in sysfs:

 # cat /sys/bus/iio/devices/trigger0/trigger_frequency_hz
 1
 # echo 1 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltage4_en
 # cat /sys/bus/iio/devices/
 iio:device0/        iio_sysfs_trigger/  trigger0/           trigger1/
 # cat /sys/bus/iio/devices/trigger0/name
 f80480b0.at91_rtc_adc
 # iio_generic_buffer -n fc030000.adc -t f80480b0.at91_rtc_adc -c 5
 iio device number being used is 0
 iio trigger number being used is 0
 /sys/bus/iio/devices/iio:device0 f80480b0.at91_rtc_adc
 3298.388672
 3294.360352
 3291.943359
 3294.360352
 3291.943359


Future work:
In the future the node would have to be enabled for other sama5d2 based
boards as well, and MAINTAINERS to be updated if this driver is accepted.


Eugen Hristev (10):
  iio: adc: at91-sama5d2_adc: update for other trigger usage
  dt-bindings: iio: adc: at91-sama5d2: add rtc-trigger optional property
  dt-bindings: iio: trigger: at91-rtc-trigger: add bindings
  rtc: at91rm9200: use of_platform_populate as return value
  iio: trigger: at91-rtc-trigger: introduce at91 rtc adc trigger driver
  iio: adc: at91-sama5d2_adc: handle unfinished conversions
  iio: adc: at91-sama5d2_adc: fix differential channels in triggered
    mode
  iio: adc: at91-sama5d2_adc: implement RTC triggering
  ARM: dts: at91: sama5d2: add rtc_adc_trigger node
  ARM: dts: at91: sama5d2_xplained: enable rtc_adc_trigger

 .../bindings/iio/adc/at91-sama5d2_adc.txt          |   4 +
 .../bindings/iio/trigger/at91-rtc-trigger.yaml     |  44 +++
 arch/arm/boot/dts/at91-sama5d2_xplained.dts        |   4 +
 arch/arm/boot/dts/sama5d2.dtsi                     |  11 +
 drivers/iio/adc/at91-sama5d2_adc.c                 | 336 ++++++++++++++++-----
 drivers/iio/trigger/Kconfig                        |  10 +
 drivers/iio/trigger/Makefile                       |   1 +
 drivers/iio/trigger/at91-rtc-trigger.c             | 213 +++++++++++++
 drivers/rtc/rtc-at91rm9200.c                       |   2 +-
 9 files changed, 543 insertions(+), 82 deletions(-)
 create mode 040000 Documentation/devicetree/bindings/iio/trigger
 create mode 100644 Documentation/devicetree/bindings/iio/trigger/at91-rtc-trigger.yaml
 create mode 100644 drivers/iio/trigger/at91-rtc-trigger.c

-- 
2.7.4

[PATCH 01/10] iio: adc: at91-sama5d2_adc: update for other trigger usage

[Eugen.Hristev]

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)
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@microchip.com>
---
 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

[PATCH 02/10] dt-bindings: iio: adc: at91-sama5d2: add rtc-trigger optional property

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

Add property to connect RTC-type trigger to the ADC block.
The ADC is connected internally with a line to the RTC block.
The RTC can provide a trigger signal to the ADC to start conversions.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
index 4a3c1d4..1980f0e 100644
--- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
@@ -23,6 +23,9 @@ Optional properties:
   See ../../dma/dma.txt for details.
   - #io-channel-cells: in case consumer drivers are attached, this must be 1.
   See <Documentation/devicetree/bindings/iio/iio-bindings.txt> for details.
+  - atmel,rtc-trigger: The ADC IP block can be triggered by the RTC block
+inside the SoC. This property is a phandle to a node that provides a
+trigger device, if the ADC block supports it.
 
 Properties for consumer drivers:
   - Consumer drivers can be connected to this producer device, as specified
@@ -44,6 +47,7 @@ adc: adc@fc030000 {
 	vddana-supply = <&vdd_3v3_lp_reg>;
 	vref-supply = <&vdd_3v3_lp_reg>;
 	atmel,trigger-edge-type = <IRQ_TYPE_EDGE_BOTH>;
+	atmel,rtc-trigger = <&rtc_adc_trigger>;
 	dmas = <&dma0 (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) | AT91_XDMAC_DT_PERID(25))>;
 	dma-names = "rx";
 	#io-channel-cells = <1>;
-- 
2.7.4

[PATCH 03/10] dt-bindings: iio: trigger: at91-rtc-trigger: add bindings

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

Add bindings for AT91 RTC ADC Trigger hardware node.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 .../bindings/iio/trigger/at91-rtc-trigger.yaml     | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 040000 Documentation/devicetree/bindings/iio/trigger
 create mode 100644 Documentation/devicetree/bindings/iio/trigger/at91-rtc-trigger.yaml

diff --git a/Documentation/devicetree/bindings/iio/trigger/at91-rtc-trigger.yaml b/Documentation/devicetree/bindings/iio/trigger/at91-rtc-trigger.yaml
new file mode 100644
index 0000000..c8c5886
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/trigger/at91-rtc-trigger.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2019 Eugen Hristev <eugen.hristev@gmail.com>
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/bindings/iio/trigger/microchip,rtc-adc-trigger.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Microchip AT91 RTC ADC Trigger (Real Time Clock to Analog to Digital Converter)
+
+maintainers:
+  - Eugen Hristev <eugen.hristev@microchip.com>
+
+description: |
+  Bindings for the Microchip AT91 RTC ADC Trigger.
+  The Real Time Clock block inside AT91 SoCs can be connected with a direct
+  hardware line to the ADC. This line can be raised at a specific time
+  interval in order to trigger the ADC to perform conversions.
+  Datasheet can be found here: http://ww1.microchip.com/downloads/en/devicedoc/ds60001476b.pdf
+
+properties:
+  compatible:
+    enum:
+      - microchip,rtc-adc-trigger
+
+  reg:
+    description: |
+      Register map address (start address, size).
+    maxItems: 2
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    {
+      #address_cells = <1>;
+      #size-cells = <1>;
+
+      rtc_adc_trigger: rtc-adc-trigger {
+        reg = <0x0 0x10>;
+        compatible = "microchip,rtc-adc-trigger";
+      };
+    };
-- 
2.7.4

[PATCH 04/10] rtc: at91rm9200: use of_platform_populate as return value

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

This allows the RTC node to have child nodes in DT.
This allows subnodes to be probed.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/rtc/rtc-at91rm9200.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 3b833e0..f1b5b3d 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -421,7 +421,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 	at91_rtc_write_ier(AT91_RTC_SECEV);
 
 	dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n");
-	return 0;
+	return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
 
 err_clk:
 	clk_disable_unprepare(sclk);
-- 
2.7.4

[PATCH 06/10] iio: adc: at91-sama5d2_adc: handle unfinished conversions

[Eugen.Hristev]

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, to avoid sleeping
in trigger handler, start a workq that will wait until all channels are
ready.
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);
+
 		at91_adc_read_and_push_channels(indio_dev, st->timestamp);
 		iio_trigger_notify_done(indio_dev->trig);
 	} else {
-- 
2.7.4

[PATCH 05/10] iio: trigger: at91-rtc-trigger: introduce at91 rtc adc trigger driver

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

The AT91 RTC ADC trigger driver registers a trigger into the iio subsystem,
which can be associated with at91_sama5d2-adc compatible drivers.
The SAMA5D2 SoC contains a hardware link between the RTC block and the ADC
block that will allow the RTC to trigger the ADC to perform conversions.
The at91-rtc-trigger will allow the customisation of the trigger frequency
from sysfs:
trigger_frequency_hz_available will display possible frequencies in Hz
trigger_frequency_hz will allow configuring this frequency.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/trigger/Kconfig            |  10 ++
 drivers/iio/trigger/Makefile           |   1 +
 drivers/iio/trigger/at91-rtc-trigger.c | 213 +++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/iio/trigger/at91-rtc-trigger.c

diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
index 8cef2f7..dc7e6f4 100644
--- a/drivers/iio/trigger/Kconfig
+++ b/drivers/iio/trigger/Kconfig
@@ -68,4 +68,14 @@ config IIO_SYSFS_TRIGGER
 	  To compile this driver as a module, choose M here: the
 	  module will be called iio-trig-sysfs.
 
+config IIO_AT91_RTC_TRIGGER
+	tristate "AT91 RTC trigger"
+	help
+	  Provides support for using AT91 RTC IP block to generate trigger
+	  events for ADC devices.
+	  If unsure, say N (but it's safe to say "Y").
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called at91-rtc-trigger.
+
 endmenu
diff --git a/drivers/iio/trigger/Makefile b/drivers/iio/trigger/Makefile
index f3d11ac..6512436 100644
--- a/drivers/iio/trigger/Makefile
+++ b/drivers/iio/trigger/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_IIO_STM32_LPTIMER_TRIGGER) += stm32-lptimer-trigger.o
 obj-$(CONFIG_IIO_STM32_TIMER_TRIGGER) += stm32-timer-trigger.o
 obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
 obj-$(CONFIG_IIO_TIGHTLOOP_TRIGGER) += iio-trig-loop.o
+obj-$(CONFIG_IIO_AT91_RTC_TRIGGER) += at91-rtc-trigger.o
diff --git a/drivers/iio/trigger/at91-rtc-trigger.c b/drivers/iio/trigger/at91-rtc-trigger.c
new file mode 100644
index 0000000..8cdcfeb
--- /dev/null
+++ b/drivers/iio/trigger/at91-rtc-trigger.c
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2019 Microchip Technology, Inc. and its subsidiaries
+ *
+ * Author: Eugen Hristev <eugen.hristev@microchip.com>
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/irq_work.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+
+#define at91_adc_readl(t, reg)		readl_relaxed((t)->base + (reg))
+#define at91_adc_writel(t, reg, val)	writel_relaxed(val, (t)->base + (reg))
+
+#define AT91_RTC_MR		0x4
+#define AT91_RTC_OUT0_MASK	GENMASK(18, 16)
+#define AT91_RTC_OUT0_NO_WAVE	(0x0 << 16)
+#define AT91_RTC_OUT0_1HZ	(0x1 << 16)
+#define AT91_RTC_OUT0_32HZ	(0x2 << 16)
+#define AT91_RTC_OUT0_64HZ	(0x3 << 16)
+#define AT91_RTC_OUT0_512HZ	(0x4 << 16)
+
+/* attribute pack list */
+#define AT91_RTC_1HZ		1
+#define AT91_RTC_32HZ		32
+#define AT91_RTC_64HZ		64
+#define AT91_RTC_512HZ		512
+
+struct at91_rtc_adc_trig {
+	struct iio_trigger	*trig;
+	void __iomem		*base;
+	unsigned int		hz_config;
+};
+
+static int at91_hz_config_sysfs_to_reg(unsigned int hz_config)
+{
+	switch (hz_config) {
+	case AT91_RTC_1HZ:
+		return AT91_RTC_OUT0_1HZ;
+	case AT91_RTC_32HZ:
+		return AT91_RTC_OUT0_32HZ;
+	case AT91_RTC_64HZ:
+		return AT91_RTC_OUT0_64HZ;
+	case AT91_RTC_512HZ:
+		return AT91_RTC_OUT0_512HZ;
+	}
+	return AT91_RTC_OUT0_1HZ;
+}
+
+static int at91_configure_trigger(struct iio_trigger *trig, bool state)
+{
+	struct at91_rtc_adc_trig *t = iio_trigger_get_drvdata(trig);
+	u32 mr = at91_adc_readl(t, AT91_RTC_MR);
+
+	mr &= ~AT91_RTC_OUT0_MASK;
+
+	if (state)
+		mr |= at91_hz_config_sysfs_to_reg(t->hz_config);
+
+	at91_adc_writel(t, AT91_RTC_MR, mr);
+
+	return 0;
+}
+
+static const struct iio_trigger_ops at91_rtc_adc_trigger_ops = {
+	.set_trigger_state = &at91_configure_trigger,
+};
+
+static ssize_t at91_rtc_trigger_frequency_get(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct iio_trigger *trig = to_iio_trigger(dev);
+	struct at91_rtc_adc_trig *t = iio_trigger_get_drvdata(trig);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", t->hz_config);
+}
+
+static ssize_t at91_rtc_trigger_frequency_set(struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf, size_t count)
+{
+	struct iio_trigger *trig = to_iio_trigger(dev);
+	struct at91_rtc_adc_trig *t = iio_trigger_get_drvdata(trig);
+	int ret;
+	unsigned int val;
+
+	ret = kstrtou32(buf, 10, &val);
+
+	if (ret)
+		return ret;
+
+	if (val != AT91_RTC_1HZ && val != AT91_RTC_32HZ &&
+	    val != AT91_RTC_64HZ && val != AT91_RTC_512HZ)
+		return -EINVAL;
+
+	t->hz_config = val;
+
+	return count;
+}
+
+static IIO_DEVICE_ATTR(trigger_frequency_hz, 0644,
+		       at91_rtc_trigger_frequency_get,
+		       at91_rtc_trigger_frequency_set, 0);
+
+static IIO_CONST_ATTR(trigger_frequency_hz_available,
+		      __stringify(AT91_RTC_1HZ) " "
+		      __stringify(AT91_RTC_32HZ) " "
+		      __stringify(AT91_RTC_64HZ) " "
+		      __stringify(AT91_RTC_512HZ));
+
+static struct attribute *at91_rtc_adc_trigger_attributes[] = {
+	&iio_const_attr_trigger_frequency_hz_available.dev_attr.attr,
+	&iio_dev_attr_trigger_frequency_hz.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group at91_rtc_adc_trigger_attribute_group = {
+	.attrs = at91_rtc_adc_trigger_attributes,
+};
+
+static const struct attribute_group *at91_rtc_adc_trigger_attr_groups[] = {
+	&at91_rtc_adc_trigger_attribute_group,
+	NULL
+};
+
+static void at91_rtc_adc_trigger_remove(void *priv)
+{
+	struct at91_rtc_adc_trig *t = priv;
+
+	iio_trigger_unregister(t->trig);
+	iio_trigger_free(t->trig);
+}
+
+static int at91_rtc_adc_trigger_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource	*res;
+	struct at91_rtc_adc_trig *t;
+	int ret = 0;
+
+	t = devm_kzalloc(dev, sizeof(*t), GFP_KERNEL);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
+
+	t->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(t->base))
+		return PTR_ERR(t->base);
+
+	t->trig = iio_trigger_alloc("%x.at91_rtc_adc", res->start);
+	if (!t->trig) {
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	t->hz_config = AT91_RTC_1HZ;
+
+	t->trig->ops = &at91_rtc_adc_trigger_ops;
+	t->trig->dev.parent = dev;
+	t->trig->dev.groups = at91_rtc_adc_trigger_attr_groups;
+
+	iio_trigger_set_drvdata(t->trig, t);
+
+	ret = iio_trigger_register(t->trig);
+	if (ret) {
+		dev_err(dev, "failed to register trigger.\n");
+		goto at91_rtc_adc_trigger_probe_fail_register;
+	}
+
+	ret = devm_add_action_or_reset(dev, at91_rtc_adc_trigger_remove, t);
+	if (ret) {
+		dev_err(dev, "failed to add disable action.\n");
+		goto at91_rtc_adc_trigger_probe_fail_add_action;
+	}
+
+	return 0;
+
+at91_rtc_adc_trigger_probe_fail_add_action:
+	iio_trigger_unregister(t->trig);
+at91_rtc_adc_trigger_probe_fail_register:
+	iio_trigger_free(t->trig);
+	return ret;
+}
+
+static const struct of_device_id at91_rtc_adc_trigger_of_match[] = {
+	{
+		.compatible = "microchip,rtc-adc-trigger",
+	},
+	{ /* end node */ },
+};
+MODULE_DEVICE_TABLE(of, at91_rtc_adc_trigger_of_match);
+
+static struct platform_driver at91_rtc_adc_trigger_driver = {
+	.probe = at91_rtc_adc_trigger_probe,
+	.driver = {
+		.name = "at91-rtc-adc-trigger",
+		.of_match_table = at91_rtc_adc_trigger_of_match,
+	},
+};
+module_platform_driver(at91_rtc_adc_trigger_driver);
+
+MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");
+MODULE_DESCRIPTION("AT91 RTC ADC trigger driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:iio-at91-rtc-trigger");
-- 
2.7.4

[PATCH 08/10] iio: adc: at91-sama5d2_adc: implement RTC triggering

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

Implement the property atmel,rtc-trigger which provides a phandle
to a RTC trigger.
To make it work, one has to check at buffer_postenable if the trigger
the device is using is the one we provide using the phandle link.
The trigger mode must be selected accordingly in the trigger mode selection
register.
The RTC trigger will use our IRQ. Dedicated hardware line inside the SoC
will actually trigger the ADC to make the conversion, and EOC irqs are fired
when conversion is done.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 109 +++++++++++++++++++++++++++++++++++--
 1 file changed, 104 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index ccffa48..ac97f4a 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -58,6 +58,8 @@
 #define	AT91_SAMA5D2_MR_TRGSEL_TRIG6	6
 /* RTCOUT0 */
 #define	AT91_SAMA5D2_MR_TRGSEL_TRIG7	7
+/* TRGSEL mask */
+#define AT91_SAMA5D2_MR_TRGSEL_MASK	GENMASK(3, 1)
 /* Sleep Mode */
 #define	AT91_SAMA5D2_MR_SLEEP		BIT(5)
 /* Fast Wake Up */
@@ -195,6 +197,8 @@
 #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
 /* Trigger Mode external trigger any edge */
 #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
+/* Trigger Mode RTC - must be any of the above 3 values */
+#define AT91_SAMA5D2_TRGR_TRGMOD_RTC AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE
 /* Trigger Mode internal periodic */
 #define AT91_SAMA5D2_TRGR_TRGMOD_PERIODIC 5
 /* Trigger Mode - trigger period mask */
@@ -407,6 +411,8 @@ struct at91_adc_state {
 	struct mutex			lock;
 	struct work_struct		workq;
 	s64				timestamp;
+	struct device			*rtc_trig_dev;
+	bool				rtc_triggered;
 };
 
 static const struct at91_adc_trigger at91_adc_trigger_list[] = {
@@ -737,6 +743,42 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
 	/* set/unset hw trigger */
 	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
 
+	status = at91_adc_readl(st, AT91_SAMA5D2_MR);
+
+	status &= ~AT91_SAMA5D2_MR_TRGSEL_MASK;
+
+	/* set/unset TRGSEL to ADTRG */
+	if (state)
+		status |= AT91_SAMA5D2_MR_TRGSEL(AT91_SAMA5D2_MR_TRGSEL_TRIG0);
+
+	at91_adc_writel(st, AT91_SAMA5D2_MR, status);
+
+	return 0;
+}
+
+static int at91_adc_rtc_configure_trigger(struct at91_adc_state *st, bool state)
+{
+	u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
+
+	/* clear TRGMOD */
+	status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
+
+	if (state)
+		status |= AT91_SAMA5D2_TRGR_TRGMOD_RTC;
+
+	/* set/unset hw trigger */
+	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
+
+	status = at91_adc_readl(st, AT91_SAMA5D2_MR);
+
+	status &= ~AT91_SAMA5D2_MR_TRGSEL_MASK;
+
+	/* set/unset TRGSEL to RTCOUT0 */
+	if (state)
+		status |= AT91_SAMA5D2_MR_TRGSEL(AT91_SAMA5D2_MR_TRGSEL_TRIG7);
+
+	at91_adc_writel(st, AT91_SAMA5D2_MR, status);
+
 	return 0;
 }
 
@@ -866,7 +908,8 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
 	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)) \
+	if (iio_trigger_validate_own_device(indio->trig, indio) && \
+		!st->rtc_triggered) \
 		use_irq = false; \
 	}
 
@@ -884,6 +927,18 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio)
 		/* touchscreen enabling */
 		return at91_adc_configure_touch(st, true);
 	}
+
+	/*
+	 * If our rtc trigger link is identical to the current trigger,
+	 * then we are rtc-triggered.
+	 * Configure accordingly.
+	 */
+	if (!IS_ERR_OR_NULL(st->rtc_trig_dev) &&
+	    st->rtc_trig_dev == indio->trig->dev.parent) {
+		at91_adc_rtc_configure_trigger(st, true);
+		st->rtc_triggered = true;
+	}
+
 	/* if we are not in triggered mode, we cannot enable the buffer. */
 	if (!(indio->currentmode & INDIO_ALL_TRIGGERED_MODES))
 		return -EINVAL;
@@ -947,6 +1002,17 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio)
 	if (!(indio->currentmode & INDIO_ALL_TRIGGERED_MODES))
 		return -EINVAL;
 
+	/*
+	 * If our rtc trigger link is identical to the current trigger,
+	 * then we are rtc-triggered.
+	 * Unconfigure accordingly.
+	 */
+	if (!IS_ERR_OR_NULL(st->rtc_trig_dev) &&
+	    st->rtc_trig_dev == indio->trig->dev.parent) {
+		at91_adc_rtc_configure_trigger(st, false);
+		st->rtc_triggered = false;
+	}
+
 	AT91_ADC_BUFFER_CHECK_USE_IRQ(use_irq);
 	/*
 	 * For each enable channel we must disable it in hardware.
@@ -1153,8 +1219,15 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
 	else
 		ret = at91_adc_trigger_handler_nodma(indio_dev, pf);
 
-	if (!ret)
+	if (!ret) {
 		iio_trigger_notify_done(indio_dev->trig);
+		/*
+		 * RTC trigger does not know how to reenable our IRQ.
+		 * So, we must do it.
+		 */
+		if (st->rtc_triggered)
+			enable_irq(st->irq);
+	}
 
 	return IRQ_HANDLED;
 }
@@ -1166,10 +1239,13 @@ irqreturn_t at91_adc_pollfunc(int irq, void *p)
 	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.
+	 * We need to start a software trigger if we are not using a trigger
+	 * that uses our own IRQ.
+	 * External trigger and RTC trigger do not not need software start
+	 * However the other triggers do.
 	 */
-	if (iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
+	if (iio_trigger_validate_own_device(indio_dev->trig, indio_dev) &&
+	    !st->rtc_triggered)
 		at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
 
 	return iio_pollfunc_store_time(irq, p);
@@ -1307,6 +1383,12 @@ static void at91_adc_workq_handler(struct work_struct *workq)
 
 		at91_adc_read_and_push_channels(indio_dev, st->timestamp);
 		iio_trigger_notify_done(indio_dev->trig);
+		/*
+		 * RTC trigger does not know how to reenable our IRQ.
+		 * So, we must do it.
+		 */
+		if (st->rtc_triggered)
+			enable_irq(st->irq);
 	} else {
 		iio_push_to_buffers(indio_dev, st->buffer);
 	}
@@ -1712,6 +1794,7 @@ static int at91_adc_probe(struct platform_device *pdev)
 	struct iio_dev *indio_dev;
 	struct at91_adc_state *st;
 	struct resource	*res;
+	struct device_node *rtc_trig_np;
 	int ret, i;
 	u32 edge_type = IRQ_TYPE_NONE;
 
@@ -1737,6 +1820,8 @@ static int at91_adc_probe(struct platform_device *pdev)
 
 	st->oversampling_ratio = AT91_OSR_1SAMPLES;
 
+	st->rtc_trig_dev = ERR_PTR(-EINVAL);
+
 	ret = of_property_read_u32(pdev->dev.of_node,
 				   "atmel,min-sample-rate-hz",
 				   &st->soc_info.min_sample_rate);
@@ -1784,6 +1869,20 @@ static int at91_adc_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	rtc_trig_np = of_parse_phandle(pdev->dev.of_node, "atmel,rtc-trigger",
+				       0);
+	if (rtc_trig_np) {
+		struct platform_device *rtc_trig_plat_dev;
+
+		rtc_trig_plat_dev = of_find_device_by_node(rtc_trig_np);
+		if (rtc_trig_plat_dev) {
+			st->rtc_trig_dev = &rtc_trig_plat_dev->dev;
+			dev_info(&pdev->dev,
+				 "RTC trigger link set-up with %s\n",
+				 dev_name(st->rtc_trig_dev));
+		}
+	}
+
 	init_waitqueue_head(&st->wq_data_available);
 	mutex_init(&st->lock);
 	INIT_WORK(&st->workq, at91_adc_workq_handler);
-- 
2.7.4

[PATCH 07/10] iio: adc: at91-sama5d2_adc: fix differential channels in triggered mode

[Eugen.Hristev]

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.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index a6b4dff..ccffa48 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -900,6 +900,7 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio)
 
 	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;
@@ -908,6 +909,17 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio)
 		    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 (use_irq) {
 			at91_adc_writel(st, AT91_SAMA5D2_IER,
-- 
2.7.4

[PATCH 10/10] ARM: dts: at91: sama5d2_xplained: enable rtc_adc_trigger

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

Enable the rtc_adc_trigger node.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 arch/arm/boot/dts/at91-sama5d2_xplained.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
index 9d0a7fb..606ca70 100644
--- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
@@ -645,3 +645,7 @@
 		};
 	};
 };
+
+&rtc_adc_trigger {
+	status = "okay";
+};
-- 
2.7.4

[PATCH 09/10] ARM: dts: at91: sama5d2: add rtc_adc_trigger node

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

Add node for the AT91 RTC ADC Trigger.
This is a child node of the RTC and uses the same register map.
Add a link in the ADC node to this new node. This represents the internal
hardware line that is connected from the ADC to the RTC device.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 arch/arm/boot/dts/sama5d2.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index 5652048..c2df369 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -694,6 +694,16 @@
 				reg = <0xf80480b0 0x30>;
 				interrupts = <74 IRQ_TYPE_LEVEL_HIGH 7>;
 				clocks = <&clk32k>;
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0 0xf80480b0 0x10>;
+
+				rtc_adc_trigger: rtc-adc-trigger {
+					reg = <0x0 0x10>;
+					compatible = "microchip,rtc-adc-trigger";
+					status = "disabled";
+				};
+
 			};
 
 			i2s0: i2s@f8050000 {
@@ -856,6 +866,7 @@
 				atmel,max-sample-rate-hz = <20000000>;
 				atmel,startup-time-ms = <4>;
 				atmel,trigger-edge-type = <IRQ_TYPE_EDGE_RISING>;
+				atmel,rtc-trigger = <&rtc_adc_trigger>;
 				#io-channel-cells = <1>;
 				status = "disabled";
 			};
-- 
2.7.4

Re: [PATCH 04/10] rtc: at91rm9200: use of_platform_populate as return value

[Alexandre Belloni]

Hi,

On 18/12/2019 16:24:00+0000, Eugen.Hristev@microchip.com wrote:
> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> This allows the RTC node to have child nodes in DT.
> This allows subnodes to be probed.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  drivers/rtc/rtc-at91rm9200.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
> index 3b833e0..f1b5b3d 100644
> --- a/drivers/rtc/rtc-at91rm9200.c
> +++ b/drivers/rtc/rtc-at91rm9200.c
> @@ -421,7 +421,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
>  	at91_rtc_write_ier(AT91_RTC_SECEV);
>  
>  	dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n");
> -	return 0;
> +	return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>  

You can avoid the DT binding change and DT parsing by using
platform_add_device here. I don't think there is any point describing
the trigger as a child node (a watchdog functionality wouldn't be
described for example).

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 04/10] rtc: at91rm9200: use of_platform_populate as return value

[Eugen.Hristev]

On 18.12.2019 18:43, Alexandre Belloni wrote:

> Hi,
> 
> On 18/12/2019 16:24:00+0000, Eugen.Hristev@microchip.com wrote:
>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>
>> This allows the RTC node to have child nodes in DT.
>> This allows subnodes to be probed.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>>   drivers/rtc/rtc-at91rm9200.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
>> index 3b833e0..f1b5b3d 100644
>> --- a/drivers/rtc/rtc-at91rm9200.c
>> +++ b/drivers/rtc/rtc-at91rm9200.c
>> @@ -421,7 +421,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
>>        at91_rtc_write_ier(AT91_RTC_SECEV);
>>
>>        dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n");
>> -     return 0;
>> +     return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>>
> 
> You can avoid the DT binding change and DT parsing by using
> platform_add_device here. I don't think there is any point describing
> the trigger as a child node (a watchdog functionality wouldn't be
> described for example).
> 

Hi,

It's needed because the ADC needs a link to the trigger device. This is 
a hardware link inside the SoC, so I thought the best way is to describe 
this hardware is in the Device Tree.
Otherwise the ADC node is unaware of the RTC triggering possibility.
If we just assign the RTC trigger device to the ADC through the sysfs, 
the ADC cannot distinguish between the RTC trigger and other various 
triggers which can be attached.

> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

Re: [PATCH 04/10] rtc: at91rm9200: use of_platform_populate as return value

[Alexandre Belloni]

On 18/12/2019 16:52:21+0000, Eugen.Hristev@microchip.com wrote:
> 
> 
> On 18.12.2019 18:43, Alexandre Belloni wrote:
> 
> > Hi,
> > 
> > On 18/12/2019 16:24:00+0000, Eugen.Hristev@microchip.com wrote:
> >> From: Eugen Hristev <eugen.hristev@microchip.com>
> >>
> >> This allows the RTC node to have child nodes in DT.
> >> This allows subnodes to be probed.
> >>
> >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> >> ---
> >>   drivers/rtc/rtc-at91rm9200.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
> >> index 3b833e0..f1b5b3d 100644
> >> --- a/drivers/rtc/rtc-at91rm9200.c
> >> +++ b/drivers/rtc/rtc-at91rm9200.c
> >> @@ -421,7 +421,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
> >>        at91_rtc_write_ier(AT91_RTC_SECEV);
> >>
> >>        dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n");
> >> -     return 0;
> >> +     return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> >>
> > 
> > You can avoid the DT binding change and DT parsing by using
> > platform_add_device here. I don't think there is any point describing
> > the trigger as a child node (a watchdog functionality wouldn't be
> > described for example).
> > 
> 
> Hi,
> 
> It's needed because the ADC needs a link to the trigger device. This is 
> a hardware link inside the SoC, so I thought the best way is to describe 
> this hardware is in the Device Tree.
> Otherwise the ADC node is unaware of the RTC triggering possibility.
> If we just assign the RTC trigger device to the ADC through the sysfs, 
> the ADC cannot distinguish between the RTC trigger and other various 
> triggers which can be attached.
> 

I'm not sure this links is required but I will let Jonathan review. Even
if it is needed, you can still use the rtc node to describe that link.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 04/10] rtc: at91rm9200: use of_platform_populate as return value

[Eugen.Hristev]

On 18.12.2019 18:58, Alexandre Belloni wrote:
> On 18/12/2019 16:52:21+0000, Eugen.Hristev@microchip.com wrote:
>>
>>
>> On 18.12.2019 18:43, Alexandre Belloni wrote:
>>
>>> Hi,
>>>
>>> On 18/12/2019 16:24:00+0000, Eugen.Hristev@microchip.com wrote:
>>>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>>>
>>>> This allows the RTC node to have child nodes in DT.
>>>> This allows subnodes to be probed.
>>>>
>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>> ---
>>>>    drivers/rtc/rtc-at91rm9200.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
>>>> index 3b833e0..f1b5b3d 100644
>>>> --- a/drivers/rtc/rtc-at91rm9200.c
>>>> +++ b/drivers/rtc/rtc-at91rm9200.c
>>>> @@ -421,7 +421,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
>>>>         at91_rtc_write_ier(AT91_RTC_SECEV);
>>>>
>>>>         dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n");
>>>> -     return 0;
>>>> +     return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>>>>
>>>
>>> You can avoid the DT binding change and DT parsing by using
>>> platform_add_device here. I don't think there is any point describing
>>> the trigger as a child node (a watchdog functionality wouldn't be
>>> described for example).
>>>
>>
>> Hi,
>>
>> It's needed because the ADC needs a link to the trigger device. This is
>> a hardware link inside the SoC, so I thought the best way is to describe
>> this hardware is in the Device Tree.
>> Otherwise the ADC node is unaware of the RTC triggering possibility.
>> If we just assign the RTC trigger device to the ADC through the sysfs,
>> the ADC cannot distinguish between the RTC trigger and other various
>> triggers which can be attached.
>>
> 
> I'm not sure this links is required but I will let Jonathan review. Even
> if it is needed, you can still use the rtc node to describe that link.

Actually, the RTC node could potentially have two different ADC 
triggers. There is another OUT1 field that can do a second trigger for 
the ADC only for the last channel. Future development might add this 
trigger, so, with that in mind, I think it's best to link the exact 
trigger and not the RTC node.

> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

Re: [PATCH 04/10] rtc: at91rm9200: use of_platform_populate as return value

[Alexandre Belloni]

On 19/12/2019 09:15:02+0000, Eugen.Hristev@microchip.com wrote:
> 
> 
> On 18.12.2019 18:58, Alexandre Belloni wrote:
> > On 18/12/2019 16:52:21+0000, Eugen.Hristev@microchip.com wrote:
> >>
> >>
> >> On 18.12.2019 18:43, Alexandre Belloni wrote:
> >>
> >>> Hi,
> >>>
> >>> On 18/12/2019 16:24:00+0000, Eugen.Hristev@microchip.com wrote:
> >>>> From: Eugen Hristev <eugen.hristev@microchip.com>
> >>>>
> >>>> This allows the RTC node to have child nodes in DT.
> >>>> This allows subnodes to be probed.
> >>>>
> >>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> >>>> ---
> >>>>    drivers/rtc/rtc-at91rm9200.c | 2 +-
> >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
> >>>> index 3b833e0..f1b5b3d 100644
> >>>> --- a/drivers/rtc/rtc-at91rm9200.c
> >>>> +++ b/drivers/rtc/rtc-at91rm9200.c
> >>>> @@ -421,7 +421,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
> >>>>         at91_rtc_write_ier(AT91_RTC_SECEV);
> >>>>
> >>>>         dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n");
> >>>> -     return 0;
> >>>> +     return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> >>>>
> >>>
> >>> You can avoid the DT binding change and DT parsing by using
> >>> platform_add_device here. I don't think there is any point describing
> >>> the trigger as a child node (a watchdog functionality wouldn't be
> >>> described for example).
> >>>
> >>
> >> Hi,
> >>
> >> It's needed because the ADC needs a link to the trigger device. This is
> >> a hardware link inside the SoC, so I thought the best way is to describe
> >> this hardware is in the Device Tree.
> >> Otherwise the ADC node is unaware of the RTC triggering possibility.
> >> If we just assign the RTC trigger device to the ADC through the sysfs,
> >> the ADC cannot distinguish between the RTC trigger and other various
> >> triggers which can be attached.
> >>
> > 
> > I'm not sure this links is required but I will let Jonathan review. Even
> > if it is needed, you can still use the rtc node to describe that link.
> 
> Actually, the RTC node could potentially have two different ADC 
> triggers. There is another OUT1 field that can do a second trigger for 
> the ADC only for the last channel. Future development might add this 
> trigger, so, with that in mind, I think it's best to link the exact 
> trigger and not the RTC node.

Nothing prevents you from using an index with the phandle (and I would
add a type in that case then). Having subnodes in the DT is not really a
good idea. The IP is the RTC, it just happens to have some outputs.
See what has been done for the PMC.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 04/10] rtc: at91rm9200: use of_platform_populate as return value

[Jonathan Cameron]

On Thu, 19 Dec 2019 11:23:21 +0100
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> On 19/12/2019 09:15:02+0000, Eugen.Hristev@microchip.com wrote:
> > 
> > 
> > On 18.12.2019 18:58, Alexandre Belloni wrote:  
> > > On 18/12/2019 16:52:21+0000, Eugen.Hristev@microchip.com wrote:  
> > >>
> > >>
> > >> On 18.12.2019 18:43, Alexandre Belloni wrote:
> > >>  
> > >>> Hi,
> > >>>
> > >>> On 18/12/2019 16:24:00+0000, Eugen.Hristev@microchip.com wrote:  
> > >>>> From: Eugen Hristev <eugen.hristev@microchip.com>
> > >>>>
> > >>>> This allows the RTC node to have child nodes in DT.
> > >>>> This allows subnodes to be probed.
> > >>>>
> > >>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > >>>> ---
> > >>>>    drivers/rtc/rtc-at91rm9200.c | 2 +-
> > >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>>
> > >>>> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
> > >>>> index 3b833e0..f1b5b3d 100644
> > >>>> --- a/drivers/rtc/rtc-at91rm9200.c
> > >>>> +++ b/drivers/rtc/rtc-at91rm9200.c
> > >>>> @@ -421,7 +421,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
> > >>>>         at91_rtc_write_ier(AT91_RTC_SECEV);
> > >>>>
> > >>>>         dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n");
> > >>>> -     return 0;
> > >>>> +     return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> > >>>>  
> > >>>
> > >>> You can avoid the DT binding change and DT parsing by using
> > >>> platform_add_device here. I don't think there is any point describing
> > >>> the trigger as a child node (a watchdog functionality wouldn't be
> > >>> described for example).
> > >>>  
> > >>
> > >> Hi,
> > >>
> > >> It's needed because the ADC needs a link to the trigger device. This is
> > >> a hardware link inside the SoC, so I thought the best way is to describe
> > >> this hardware is in the Device Tree.
> > >> Otherwise the ADC node is unaware of the RTC triggering possibility.
> > >> If we just assign the RTC trigger device to the ADC through the sysfs,
> > >> the ADC cannot distinguish between the RTC trigger and other various
> > >> triggers which can be attached.
> > >>  
> > > 
> > > I'm not sure this links is required but I will let Jonathan review. Even
> > > if it is needed, you can still use the rtc node to describe that link.  
> > 
> > Actually, the RTC node could potentially have two different ADC 
> > triggers. There is another OUT1 field that can do a second trigger for 
> > the ADC only for the last channel. Future development might add this 
> > trigger, so, with that in mind, I think it's best to link the exact 
> > trigger and not the RTC node.  
> 
> Nothing prevents you from using an index with the phandle (and I would
> add a type in that case then). Having subnodes in the DT is not really a
> good idea. The IP is the RTC, it just happens to have some outputs.
> See what has been done for the PMC.
> 
> 

If it can be done either way, let's avoid adding to the rtc dt binding.

Jonathan

Re: [PATCH 01/10] iio: adc: at91-sama5d2_adc: update for other trigger usage

[Jonathan Cameron]

On Wed, 18 Dec 2019 16:23:58 +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)
> is replaced with specific at91 code, that will start the software conversion
> on the poll action(if it's not our trigger).

This one runs into a 'interesting' corner of IIO.  For the early software
triggers we eventually used irq_work magic to make a top half happen.  Later
we wondered what the point was given the complex dance needed to make a top half
interrupt happen safely.  Hence the iio_loop driver for example and a few others
call the interrupt thread functions directly and never call anything in irq
context.  It's a corner I've been meaning to look at cleaning up for a long
time, but for now it may give you some odd results.  The safest option
is to trigger the read from the thread rather than the irq context.  The disadvantage
is that if you use a trigger with a hardware irq involved, you will
start the capture later than ideal.

> 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.

I don't think this is quite what the code is doing.  You trigger the capture
in the pollfunc top half which is in irq context and can't sleep, but you
schedule the work queue from the associated interrupt thread, which can.
I think you can sleep there.   Note I'm also unsure how you know the workqueue
itself happens after the data is available.  It could in theory happen very
quickly if there is not much else going on in the system.

> 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.

I'm not sure you actually use that stashed timestamp in all the paths.
Approach is fine though!

A few more specific comments inline.

Also, please don't put parameter renames in the same patch as a function change.
Just makes it harder to review.  If you need them to make new code cleaner,
then do it as a precursor patch in the series.

Thanks,

Jonathan

> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  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)  { \

I'd rather see this as a function with clarity on parameters used etc.

> +	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)

Rename shouldn't be on a patch doing other stuff. Just adds noise.

>  {
> -	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))) {

I'm wondering if we are making this harder to follow than it needs to be.
Two cases:

1) Trigger is our hardware one and data should be ready now as we used the EOC
   interrupt to get here. - By all means check that but it's probably an error
   if that condition fails.
2) Trigger is not our hardware and we have only just issued the conversion request.
   Data might be ready or not.

So this particular function is running as an interrupt thread and we push the
handling off to a work queue that will happen later.   Why should the data
be ready by the time that workqueue happens?

Also as this is an interrupt thread (I think,it's been a while since much
of interest happened around these ;), we can sleep in here just fine.
We shouldn't be calling trigger_notify_done until we are sure a race can't
occur with a future trigger anyway (which it probably can if we don't wait
for the data).


> +		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)

I'd like a comment here on what 'ret' actually is telling us
(I assume that all handling wrt to the trigger is done and we can
have another one).

Actually with a slight tweak the code can become 'obvious'
and it'll make the flow clearer. (obvious to me anyway ;)

	if (st->dma_st.dma_chan) {
		at91_adc_trigger_handler_dma(indio_dev)
		iio_trigger_notify_done(indio_dev->trig);
	} else {
		ret = at91_adc_trigger_handler_nodma(indio_dev);
		if (ret != -EINPROGRESS)
			iio_trigger_notify_done(indio_dev->trig);
	}

> +		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)) {
Let me just check my understanding of these two conditions.

First one verifies we are doing triggered ADC capture, rather than touch screen
magic.

The second verifies that it's our own trigger rather than a different one...
> +		at91_adc_read_and_push_channels(indio_dev, st->timestamp);
> +		iio_trigger_notify_done(indio_dev->trig);
> +	} else {

So we can get here either because this is really touch screen stuff and
the triggering is effectively opaque to IIO, or because we have for example
a sysfs or high res timer type trigger.

If it's a sysfs type trigger why no timestamp?
I'm also surprised that path doesn't call iio_trigger_notify done.  We don't
want to allow that trigger to fire again until the capture is finished
and that includes pushing to the buffer.  Otherwise fun race conditions
can occur.


> +		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)

Re: [PATCH 02/10] dt-bindings: iio: adc: at91-sama5d2: add rtc-trigger optional property

[Jonathan Cameron]

On Wed, 18 Dec 2019 16:23:58 +0000
<Eugen.Hristev@microchip.com> wrote:

> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> Add property to connect RTC-type trigger to the ADC block.
> The ADC is connected internally with a line to the RTC block.
> The RTC can provide a trigger signal to the ADC to start conversions.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
As discussed, might want to just be the rtc handle rather than a new
trigger one.

> ---
>  Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> index 4a3c1d4..1980f0e 100644
> --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> @@ -23,6 +23,9 @@ Optional properties:
>    See ../../dma/dma.txt for details.
>    - #io-channel-cells: in case consumer drivers are attached, this must be 1.
>    See <Documentation/devicetree/bindings/iio/iio-bindings.txt> for details.
> +  - atmel,rtc-trigger: The ADC IP block can be triggered by the RTC block
> +inside the SoC. This property is a phandle to a node that provides a
> +trigger device, if the ADC block supports it.

Do we want to be more specific on devices that do support it?
Maybe it's enough of a complex mess that we don't.

>  
>  Properties for consumer drivers:
>    - Consumer drivers can be connected to this producer device, as specified
> @@ -44,6 +47,7 @@ adc: adc@fc030000 {
>  	vddana-supply = <&vdd_3v3_lp_reg>;
>  	vref-supply = <&vdd_3v3_lp_reg>;
>  	atmel,trigger-edge-type = <IRQ_TYPE_EDGE_BOTH>;
> +	atmel,rtc-trigger = <&rtc_adc_trigger>;
>  	dmas = <&dma0 (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) | AT91_XDMAC_DT_PERID(25))>;
>  	dma-names = "rx";
>  	#io-channel-cells = <1>;

Re: [PATCH 03/10] dt-bindings: iio: trigger: at91-rtc-trigger: add bindings

[Jonathan Cameron]

On Wed, 18 Dec 2019 16:23:59 +0000
<Eugen.Hristev@microchip.com> wrote:

> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> Add bindings for AT91 RTC ADC Trigger hardware node.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
If this does make sense (under discussion) I think it should be in
the rtc binding doc.  This doesn't make it explicit that this particular
block is a child node of that block as far as I can see...

Jonathan

> ---
>  .../bindings/iio/trigger/at91-rtc-trigger.yaml     | 44 ++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 040000 Documentation/devicetree/bindings/iio/trigger
>  create mode 100644 Documentation/devicetree/bindings/iio/trigger/at91-rtc-trigger.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/trigger/at91-rtc-trigger.yaml b/Documentation/devicetree/bindings/iio/trigger/at91-rtc-trigger.yaml
> new file mode 100644
> index 0000000..c8c5886
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/trigger/at91-rtc-trigger.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright 2019 Eugen Hristev <eugen.hristev@gmail.com>
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/bindings/iio/trigger/microchip,rtc-adc-trigger.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Microchip AT91 RTC ADC Trigger (Real Time Clock to Analog to Digital Converter)
> +
> +maintainers:
> +  - Eugen Hristev <eugen.hristev@microchip.com>
> +
> +description: |
> +  Bindings for the Microchip AT91 RTC ADC Trigger.
> +  The Real Time Clock block inside AT91 SoCs can be connected with a direct
> +  hardware line to the ADC. This line can be raised at a specific time
> +  interval in order to trigger the ADC to perform conversions.
> +  Datasheet can be found here: http://ww1.microchip.com/downloads/en/devicedoc/ds60001476b.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,rtc-adc-trigger
> +
> +  reg:
> +    description: |
> +      Register map address (start address, size).
> +    maxItems: 2
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    {
> +      #address_cells = <1>;
> +      #size-cells = <1>;
> +
> +      rtc_adc_trigger: rtc-adc-trigger {
> +        reg = <0x0 0x10>;
> +        compatible = "microchip,rtc-adc-trigger";
> +      };
> +    };

Re: [PATCH 05/10] iio: trigger: at91-rtc-trigger: introduce at91 rtc adc trigger driver

[Jonathan Cameron]

On Wed, 18 Dec 2019 16:24:01 +0000
<Eugen.Hristev@microchip.com> wrote:

> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> The AT91 RTC ADC trigger driver registers a trigger into the iio subsystem,
> which can be associated with at91_sama5d2-adc compatible drivers.
> The SAMA5D2 SoC contains a hardware link between the RTC block and the ADC
> block that will allow the RTC to trigger the ADC to perform conversions.
> The at91-rtc-trigger will allow the customisation of the trigger frequency
> from sysfs:
> trigger_frequency_hz_available will display possible frequencies in Hz
> trigger_frequency_hz will allow configuring this frequency.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

A few things inline.  Be careful in particular of mixing managed
and unmanaged calls.  Here you didn't actually need to, but they can cause
some really nasty and hard to debug races.

Simple rule of thumb. The first time you hit a non managed function stop
using any other managed functions after that point.  Here you sort
of did that with the devm_add_action_or_reset, but it needs to be step
wise to guarantee the ordering is all correct and you lumped two different
functions together and didn't take into account the _or_reset element
of the above.

Jonathan

> ---
>  drivers/iio/trigger/Kconfig            |  10 ++
>  drivers/iio/trigger/Makefile           |   1 +
>  drivers/iio/trigger/at91-rtc-trigger.c | 213 +++++++++++++++++++++++++++++++++
>  3 files changed, 224 insertions(+)
>  create mode 100644 drivers/iio/trigger/at91-rtc-trigger.c
> 
> diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
> index 8cef2f7..dc7e6f4 100644
> --- a/drivers/iio/trigger/Kconfig
> +++ b/drivers/iio/trigger/Kconfig
> @@ -68,4 +68,14 @@ config IIO_SYSFS_TRIGGER
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called iio-trig-sysfs.
>  
> +config IIO_AT91_RTC_TRIGGER
> +	tristate "AT91 RTC trigger"
> +	help
> +	  Provides support for using AT91 RTC IP block to generate trigger
> +	  events for ADC devices.
> +	  If unsure, say N (but it's safe to say "Y").
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called at91-rtc-trigger.
> +
>  endmenu
> diff --git a/drivers/iio/trigger/Makefile b/drivers/iio/trigger/Makefile
> index f3d11ac..6512436 100644
> --- a/drivers/iio/trigger/Makefile
> +++ b/drivers/iio/trigger/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_IIO_STM32_LPTIMER_TRIGGER) += stm32-lptimer-trigger.o
>  obj-$(CONFIG_IIO_STM32_TIMER_TRIGGER) += stm32-timer-trigger.o
>  obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
>  obj-$(CONFIG_IIO_TIGHTLOOP_TRIGGER) += iio-trig-loop.o
> +obj-$(CONFIG_IIO_AT91_RTC_TRIGGER) += at91-rtc-trigger.o
> diff --git a/drivers/iio/trigger/at91-rtc-trigger.c b/drivers/iio/trigger/at91-rtc-trigger.c
> new file mode 100644
> index 0000000..8cdcfeb
> --- /dev/null
> +++ b/drivers/iio/trigger/at91-rtc-trigger.c
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2019 Microchip Technology, Inc. and its subsidiaries
> + *
> + * Author: Eugen Hristev <eugen.hristev@microchip.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/irq_work.h>

Check your headers :)  irq_work is a somewhat obscure so it
stood out as rather an odd entry.

> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +
> +#define at91_adc_readl(t, reg)		readl_relaxed((t)->base + (reg))
> +#define at91_adc_writel(t, reg, val)	writel_relaxed(val, (t)->base + (reg))

Given these are only used in one place each, probably better to just
put the code inline.

> +
> +#define AT91_RTC_MR		0x4
> +#define AT91_RTC_OUT0_MASK	GENMASK(18, 16)
> +#define AT91_RTC_OUT0_NO_WAVE	(0x0 << 16)
> +#define AT91_RTC_OUT0_1HZ	(0x1 << 16)
> +#define AT91_RTC_OUT0_32HZ	(0x2 << 16)
> +#define AT91_RTC_OUT0_64HZ	(0x3 << 16)
> +#define AT91_RTC_OUT0_512HZ	(0x4 << 16)

This might be better done as a an enum and FIELD_PREP 
define

> +
> +/* attribute pack list */
> +#define AT91_RTC_1HZ		1
> +#define AT91_RTC_32HZ		32
> +#define AT91_RTC_64HZ		64
> +#define AT91_RTC_512HZ		512

I'm not keen on defines that match the thing in them.
Just use the values directly.

> +
> +struct at91_rtc_adc_trig {
> +	struct iio_trigger	*trig;
> +	void __iomem		*base;
> +	unsigned int		hz_config;
> +};
> +
> +static int at91_hz_config_sysfs_to_reg(unsigned int hz_config)
> +{
> +	switch (hz_config) {
> +	case AT91_RTC_1HZ:
> +		return AT91_RTC_OUT0_1HZ;
> +	case AT91_RTC_32HZ:
> +		return AT91_RTC_OUT0_32HZ;
> +	case AT91_RTC_64HZ:
> +		return AT91_RTC_OUT0_64HZ;
> +	case AT91_RTC_512HZ:
> +		return AT91_RTC_OUT0_512HZ;
> +	}
> +	return AT91_RTC_OUT0_1HZ;
> +}
> +
> +static int at91_configure_trigger(struct iio_trigger *trig, bool state)
> +{
> +	struct at91_rtc_adc_trig *t = iio_trigger_get_drvdata(trig);
> +	u32 mr = at91_adc_readl(t, AT91_RTC_MR);
> +
> +	mr &= ~AT91_RTC_OUT0_MASK;
> +
> +	if (state)
> +		mr |= at91_hz_config_sysfs_to_reg(t->hz_config);
> +
> +	at91_adc_writel(t, AT91_RTC_MR, mr);
> +
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops at91_rtc_adc_trigger_ops = {
> +	.set_trigger_state = &at91_configure_trigger,
> +};
> +
> +static ssize_t at91_rtc_trigger_frequency_get(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
> +{
> +	struct iio_trigger *trig = to_iio_trigger(dev);
> +	struct at91_rtc_adc_trig *t = iio_trigger_get_drvdata(trig);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%u\n", t->hz_config);
> +}
> +
> +static ssize_t at91_rtc_trigger_frequency_set(struct device *dev,
> +					      struct device_attribute *attr,
> +					      const char *buf, size_t count)
> +{
> +	struct iio_trigger *trig = to_iio_trigger(dev);
> +	struct at91_rtc_adc_trig *t = iio_trigger_get_drvdata(trig);
> +	int ret;
> +	unsigned int val;
> +
> +	ret = kstrtou32(buf, 10, &val);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (val != AT91_RTC_1HZ && val != AT91_RTC_32HZ &&
> +	    val != AT91_RTC_64HZ && val != AT91_RTC_512HZ)
> +		return -EINVAL;
> +
> +	t->hz_config = val;
> +
> +	return count;
> +}
> +
> +static IIO_DEVICE_ATTR(trigger_frequency_hz, 0644,
There is some convention for this we should keep to.
ht-timer-trigger uses

"sampling_frequency" which seems to make sense here.

> +		       at91_rtc_trigger_frequency_get,
> +		       at91_rtc_trigger_frequency_set, 0);
> +
> +static IIO_CONST_ATTR(trigger_frequency_hz_available,
> +		      __stringify(AT91_RTC_1HZ) " "
> +		      __stringify(AT91_RTC_32HZ) " "
> +		      __stringify(AT91_RTC_64HZ) " "
> +		      __stringify(AT91_RTC_512HZ));
> +
> +static struct attribute *at91_rtc_adc_trigger_attributes[] = {
> +	&iio_const_attr_trigger_frequency_hz_available.dev_attr.attr,
> +	&iio_dev_attr_trigger_frequency_hz.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group at91_rtc_adc_trigger_attribute_group = {
> +	.attrs = at91_rtc_adc_trigger_attributes,
> +};
> +
> +static const struct attribute_group *at91_rtc_adc_trigger_attr_groups[] = {
> +	&at91_rtc_adc_trigger_attribute_group,
> +	NULL
> +};
> +
> +static void at91_rtc_adc_trigger_remove(void *priv)
> +{
> +	struct at91_rtc_adc_trig *t = priv;
> +
> +	iio_trigger_unregister(t->trig);
> +	iio_trigger_free(t->trig);

> +}
> +
> +static int at91_rtc_adc_trigger_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource	*res;
> +	struct at91_rtc_adc_trig *t;
> +	int ret = 0;
> +
> +	t = devm_kzalloc(dev, sizeof(*t), GFP_KERNEL);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +
> +	t->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(t->base))
> +		return PTR_ERR(t->base);
> +
> +	t->trig = iio_trigger_alloc("%x.at91_rtc_adc", res->start);
> +	if (!t->trig) {
> +		ret = -ENOMEM;
> +		return ret;
> +	}
> +
> +	t->hz_config = AT91_RTC_1HZ;
> +
> +	t->trig->ops = &at91_rtc_adc_trigger_ops;
> +	t->trig->dev.parent = dev;
> +	t->trig->dev.groups = at91_rtc_adc_trigger_attr_groups;
> +
> +	iio_trigger_set_drvdata(t->trig, t);
> +
> +	ret = iio_trigger_register(t->trig);
> +	if (ret) {
> +		dev_err(dev, "failed to register trigger.\n");
> +		goto at91_rtc_adc_trigger_probe_fail_register;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, at91_rtc_adc_trigger_remove, t);
> +	if (ret) {
This rings alarm bells.  There should definitely never be anything 
to do in an error handler for a call to devm_add_action_or_reset.

The remove order should be in the inverse of probe.  Hence if I see a devm
function called after something that needs to be unwound by hand it
screams race condition of bug.  Now there may not actually be one, but
it fails the obviously correct test so I'm not going to spend time working
out if there is ;)

However, both the things that can unwound are undoing things I'm
fairly sure have managed forms.

devm_iio_trigger_register
devm_iio_trigger_alloc

Use them and it will all work fine I think.

Note that the trigger_remove function gets called on any error
in devm_add_action_or_reset so you'd have had a double free here
if you ever hit this path.

> +		dev_err(dev, "failed to add disable action.\n");
> +		goto at91_rtc_adc_trigger_probe_fail_add_action;
> +	}
> +
> +	return 0;
> +
> +at91_rtc_adc_trigger_probe_fail_add_action:
> +	iio_trigger_unregister(t->trig);
> +at91_rtc_adc_trigger_probe_fail_register:
> +	iio_trigger_free(t->trig);
> +	return ret;
> +}
> +
> +static const struct of_device_id at91_rtc_adc_trigger_of_match[] = {
> +	{
> +		.compatible = "microchip,rtc-adc-trigger",
> +	},
> +	{ /* end node */ },
> +};
> +MODULE_DEVICE_TABLE(of, at91_rtc_adc_trigger_of_match);
> +
> +static struct platform_driver at91_rtc_adc_trigger_driver = {
> +	.probe = at91_rtc_adc_trigger_probe,
> +	.driver = {
> +		.name = "at91-rtc-adc-trigger",
> +		.of_match_table = at91_rtc_adc_trigger_of_match,
> +	},
> +};
> +module_platform_driver(at91_rtc_adc_trigger_driver);
> +
> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");
> +MODULE_DESCRIPTION("AT91 RTC ADC trigger driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:iio-at91-rtc-trigger");

Re: [PATCH 06/10] iio: adc: at91-sama5d2_adc: handle unfinished conversions

[Jonathan Cameron]

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 {

Re: [PATCH 07/10] iio: adc: at91-sama5d2_adc: fix differential channels in triggered mode

[Jonathan Cameron]

On Wed, 18 Dec 2019 16:24:02 +0000
<Eugen.Hristev@microchip.com> wrote:

> 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.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
I'm not totally sure whether this predates the changes here, but if it does
please pull it to the front of the set and give it a fixes tag.

Otherwise, look at merging it in with where-ever it was introduced.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index a6b4dff..ccffa48 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -900,6 +900,7 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio)
>  
>  	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;
> @@ -908,6 +909,17 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio)
>  		    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 (use_irq) {
>  			at91_adc_writel(st, AT91_SAMA5D2_IER,

Re: [PATCH 08/10] iio: adc: at91-sama5d2_adc: implement RTC triggering

[Jonathan Cameron]

On Wed, 18 Dec 2019 16:24:02 +0000
<Eugen.Hristev@microchip.com> wrote:

> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> Implement the property atmel,rtc-trigger which provides a phandle
> to a RTC trigger.
> To make it work, one has to check at buffer_postenable if the trigger
> the device is using is the one we provide using the phandle link.
> The trigger mode must be selected accordingly in the trigger mode selection
> register.
> The RTC trigger will use our IRQ. Dedicated hardware line inside the SoC
> will actually trigger the ADC to make the conversion, and EOC irqs are fired
> when conversion is done.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Minor points inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 109 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 104 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index ccffa48..ac97f4a 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -58,6 +58,8 @@
>  #define	AT91_SAMA5D2_MR_TRGSEL_TRIG6	6
>  /* RTCOUT0 */
>  #define	AT91_SAMA5D2_MR_TRGSEL_TRIG7	7
> +/* TRGSEL mask */
> +#define AT91_SAMA5D2_MR_TRGSEL_MASK	GENMASK(3, 1)
>  /* Sleep Mode */
>  #define	AT91_SAMA5D2_MR_SLEEP		BIT(5)
>  /* Fast Wake Up */
> @@ -195,6 +197,8 @@
>  #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
>  /* Trigger Mode external trigger any edge */
>  #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
> +/* Trigger Mode RTC - must be any of the above 3 values */
> +#define AT91_SAMA5D2_TRGR_TRGMOD_RTC AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE
>  /* Trigger Mode internal periodic */
>  #define AT91_SAMA5D2_TRGR_TRGMOD_PERIODIC 5
>  /* Trigger Mode - trigger period mask */
> @@ -407,6 +411,8 @@ struct at91_adc_state {
>  	struct mutex			lock;
>  	struct work_struct		workq;
>  	s64				timestamp;
> +	struct device			*rtc_trig_dev;
> +	bool				rtc_triggered;
>  };
>  
>  static const struct at91_adc_trigger at91_adc_trigger_list[] = {
> @@ -737,6 +743,42 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>  	/* set/unset hw trigger */
>  	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
>  
> +	status = at91_adc_readl(st, AT91_SAMA5D2_MR);
> +
> +	status &= ~AT91_SAMA5D2_MR_TRGSEL_MASK;
> +
> +	/* set/unset TRGSEL to ADTRG */
> +	if (state)
> +		status |= AT91_SAMA5D2_MR_TRGSEL(AT91_SAMA5D2_MR_TRGSEL_TRIG0);
> +
> +	at91_adc_writel(st, AT91_SAMA5D2_MR, status);
> +
> +	return 0;
> +}
> +
> +static int at91_adc_rtc_configure_trigger(struct at91_adc_state *st, bool state)
> +{
> +	u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
> +
> +	/* clear TRGMOD */
> +	status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
> +
> +	if (state)
> +		status |= AT91_SAMA5D2_TRGR_TRGMOD_RTC;
> +
> +	/* set/unset hw trigger */
> +	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
> +
> +	status = at91_adc_readl(st, AT91_SAMA5D2_MR);
> +
> +	status &= ~AT91_SAMA5D2_MR_TRGSEL_MASK;
> +
> +	/* set/unset TRGSEL to RTCOUT0 */
> +	if (state)
> +		status |= AT91_SAMA5D2_MR_TRGSEL(AT91_SAMA5D2_MR_TRGSEL_TRIG7);
> +
> +	at91_adc_writel(st, AT91_SAMA5D2_MR, status);
> +
>  	return 0;
>  }
>  
> @@ -866,7 +908,8 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
>  	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)) \
> +	if (iio_trigger_validate_own_device(indio->trig, indio) && \

This increasingly feels like it should be a function with clearly
passed parameters rather than macro fun.

> +		!st->rtc_triggered) \
>  		use_irq = false; \
>  	}
>  
> @@ -884,6 +927,18 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio)
>  		/* touchscreen enabling */
>  		return at91_adc_configure_touch(st, true);
>  	}
> +
> +	/*
> +	 * If our rtc trigger link is identical to the current trigger,
> +	 * then we are rtc-triggered.
> +	 * Configure accordingly.
> +	 */
> +	if (!IS_ERR_OR_NULL(st->rtc_trig_dev) &&
> +	    st->rtc_trig_dev == indio->trig->dev.parent) {
> +		at91_adc_rtc_configure_trigger(st, true);
> +		st->rtc_triggered = true;
> +	}
> +
>  	/* if we are not in triggered mode, we cannot enable the buffer. */
>  	if (!(indio->currentmode & INDIO_ALL_TRIGGERED_MODES))
>  		return -EINVAL;
> @@ -947,6 +1002,17 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio)
>  	if (!(indio->currentmode & INDIO_ALL_TRIGGERED_MODES))
>  		return -EINVAL;
>  
> +	/*
> +	 * If our rtc trigger link is identical to the current trigger,
> +	 * then we are rtc-triggered.
> +	 * Unconfigure accordingly.
> +	 */
> +	if (!IS_ERR_OR_NULL(st->rtc_trig_dev) &&
> +	    st->rtc_trig_dev == indio->trig->dev.parent) {
> +		at91_adc_rtc_configure_trigger(st, false);
> +		st->rtc_triggered = false;
> +	}
> +
>  	AT91_ADC_BUFFER_CHECK_USE_IRQ(use_irq);
>  	/*
>  	 * For each enable channel we must disable it in hardware.
> @@ -1153,8 +1219,15 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>  	else
>  		ret = at91_adc_trigger_handler_nodma(indio_dev, pf);
>  
> -	if (!ret)
> +	if (!ret) {
>  		iio_trigger_notify_done(indio_dev->trig);
> +		/*
> +		 * RTC trigger does not know how to reenable our IRQ.
> +		 * So, we must do it.
> +		 */
> +		if (st->rtc_triggered)
> +			enable_irq(st->irq);

Hmm. This is a bit nasty but I guess we can't avoid it.

> +	}
>  
>  	return IRQ_HANDLED;
>  }
> @@ -1166,10 +1239,13 @@ irqreturn_t at91_adc_pollfunc(int irq, void *p)
>  	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.
> +	 * We need to start a software trigger if we are not using a trigger
> +	 * that uses our own IRQ.
> +	 * External trigger and RTC trigger do not not need software start

External trigger is a bit of a generic name - sounds like one coming from
'somewhere else'.  Perhaps "External trigger in the ADC .." or similar?

> +	 * However the other triggers do.
>  	 */
> -	if (iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
> +	if (iio_trigger_validate_own_device(indio_dev->trig, indio_dev) &&
> +	    !st->rtc_triggered)
>  		at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
>  
>  	return iio_pollfunc_store_time(irq, p);
> @@ -1307,6 +1383,12 @@ static void at91_adc_workq_handler(struct work_struct *workq)
>  
>  		at91_adc_read_and_push_channels(indio_dev, st->timestamp);
>  		iio_trigger_notify_done(indio_dev->trig);
> +		/*
> +		 * RTC trigger does not know how to reenable our IRQ.
> +		 * So, we must do it.
> +		 */
> +		if (st->rtc_triggered)
> +			enable_irq(st->irq);
>  	} else {
>  		iio_push_to_buffers(indio_dev, st->buffer);
>  	}
> @@ -1712,6 +1794,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	struct iio_dev *indio_dev;
>  	struct at91_adc_state *st;
>  	struct resource	*res;
> +	struct device_node *rtc_trig_np;
>  	int ret, i;
>  	u32 edge_type = IRQ_TYPE_NONE;
>  
> @@ -1737,6 +1820,8 @@ static int at91_adc_probe(struct platform_device *pdev)
>  
>  	st->oversampling_ratio = AT91_OSR_1SAMPLES;
>  
> +	st->rtc_trig_dev = ERR_PTR(-EINVAL);
> +
>  	ret = of_property_read_u32(pdev->dev.of_node,
>  				   "atmel,min-sample-rate-hz",
>  				   &st->soc_info.min_sample_rate);
> @@ -1784,6 +1869,20 @@ static int at91_adc_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	rtc_trig_np = of_parse_phandle(pdev->dev.of_node, "atmel,rtc-trigger",
> +				       0);
> +	if (rtc_trig_np) {
> +		struct platform_device *rtc_trig_plat_dev;
> +
> +		rtc_trig_plat_dev = of_find_device_by_node(rtc_trig_np);
> +		if (rtc_trig_plat_dev) {
> +			st->rtc_trig_dev = &rtc_trig_plat_dev->dev;
> +			dev_info(&pdev->dev,
> +				 "RTC trigger link set-up with %s\n",
> +				 dev_name(st->rtc_trig_dev));
> +		}
> +	}
> +
>  	init_waitqueue_head(&st->wq_data_available);
>  	mutex_init(&st->lock);
>  	INIT_WORK(&st->workq, at91_adc_workq_handler);

Re: [PATCH 04/10] rtc: at91rm9200: use of_platform_populate as return value

[Eugen.Hristev]

On 23.12.2019 13:16, Jonathan Cameron wrote:

> On Thu, 19 Dec 2019 11:23:21 +0100
> Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> 
>> On 19/12/2019 09:15:02+0000, Eugen.Hristev@microchip.com wrote:
>>>
>>>
>>> On 18.12.2019 18:58, Alexandre Belloni wrote:
>>>> On 18/12/2019 16:52:21+0000, Eugen.Hristev@microchip.com wrote:
>>>>>
>>>>>
>>>>> On 18.12.2019 18:43, Alexandre Belloni wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 18/12/2019 16:24:00+0000, Eugen.Hristev@microchip.com wrote:
>>>>>>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>>>>>>
>>>>>>> This allows the RTC node to have child nodes in DT.
>>>>>>> This allows subnodes to be probed.
>>>>>>>
>>>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>>>>> ---
>>>>>>>     drivers/rtc/rtc-at91rm9200.c | 2 +-
>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
>>>>>>> index 3b833e0..f1b5b3d 100644
>>>>>>> --- a/drivers/rtc/rtc-at91rm9200.c
>>>>>>> +++ b/drivers/rtc/rtc-at91rm9200.c
>>>>>>> @@ -421,7 +421,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
>>>>>>>          at91_rtc_write_ier(AT91_RTC_SECEV);
>>>>>>>
>>>>>>>          dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n");
>>>>>>> -     return 0;
>>>>>>> +     return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>>>>>>>
>>>>>>
>>>>>> You can avoid the DT binding change and DT parsing by using
>>>>>> platform_add_device here. I don't think there is any point describing
>>>>>> the trigger as a child node (a watchdog functionality wouldn't be
>>>>>> described for example).

Hi Alexandre,

I started to work on this, I am trying to add and probe the 
rtc_adc_trigger with platform_device_add.

However, some issues arise: this means that the rtc_adc_trigger will not 
be OF-compatible, so, how can I identify the driver to probe ?
Second, by adding a new platform device from the RTC driver, would mean 
that I would have to supply it's probe/remove functions, which I cannot 
have here. Those are in the rtc_adc_trigger iio driver.

In fact, the question is, which is the mechanism you suggested, to be 
able to probe the rtc_adc_trigger, from inside the rtc driver, without 
using a child node in DT, as you requested ?
The rtc_adc_trigger needs a MEM resource, and a parent, and it must 
reside inside the IIO subsystem.

Thanks,
Eugen


>>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> It's needed because the ADC needs a link to the trigger device. This is
>>>>> a hardware link inside the SoC, so I thought the best way is to describe
>>>>> this hardware is in the Device Tree.
>>>>> Otherwise the ADC node is unaware of the RTC triggering possibility.
>>>>> If we just assign the RTC trigger device to the ADC through the sysfs,
>>>>> the ADC cannot distinguish between the RTC trigger and other various
>>>>> triggers which can be attached.
>>>>>
>>>>
>>>> I'm not sure this links is required but I will let Jonathan review. Even
>>>> if it is needed, you can still use the rtc node to describe that link.
>>>
>>> Actually, the RTC node could potentially have two different ADC
>>> triggers. There is another OUT1 field that can do a second trigger for
>>> the ADC only for the last channel. Future development might add this
>>> trigger, so, with that in mind, I think it's best to link the exact
>>> trigger and not the RTC node.
>>
>> Nothing prevents you from using an index with the phandle (and I would
>> add a type in that case then). Having subnodes in the DT is not really a
>> good idea. The IP is the RTC, it just happens to have some outputs.
>> See what has been done for the PMC.
>>
>>
> 
> If it can be done either way, let's avoid adding to the rtc dt binding.
> 
> Jonathan
> 
> 

Re: [PATCH 04/10] rtc: at91rm9200: use of_platform_populate as return value

[Alexandre Belloni]

On 09/01/2020 11:19:45+0000, Eugen.Hristev@microchip.com wrote:
> I started to work on this, I am trying to add and probe the 
> rtc_adc_trigger with platform_device_add.
> 
> However, some issues arise: this means that the rtc_adc_trigger will not 
> be OF-compatible, so, how can I identify the driver to probe ?
> Second, by adding a new platform device from the RTC driver, would mean 
> that I would have to supply it's probe/remove functions, which I cannot 
> have here. Those are in the rtc_adc_trigger iio driver.
> 
> In fact, the question is, which is the mechanism you suggested, to be 
> able to probe the rtc_adc_trigger, from inside the rtc driver, without 
> using a child node in DT, as you requested ?
> The rtc_adc_trigger needs a MEM resource, and a parent, and it must 
> reside inside the IIO subsystem.
> 

As suggested earlier in the thread, you can use platform_add_device
which fits all your requirements.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com