[PATCH v2 0/5] iio: adc: at91-sama5d2_adc: add DMA support

[PATCH v2 0/5] iio: adc: at91-sama5d2_adc: add DMA support

[Eugen Hristev]

This patch implements the DMA support for the ADC in sama5d2 SoC.
After discussing on the mailing list, this approach is based on triggered
kfifo buffer, with DMA support added on top of it.
Thus, the trigger is enabled by the buffer. The ADC itself will not have
an IRQ enabled if using DMA. With DMA, the channels are enabled, and DMA
controller is configured to read from the data ready registers.
When DMA starts, the trigger will start the conversion (external trigger
configured), then the registers will have the conversion data ready, the ADC
will trigger the DMA controller to read from the registers, the DMA will
copy the data into the software buffer, and trigger the DMA IRQ.
In the bottom half, the trigger polled and the data from the DMA buffer is
pushed to buffer.

The DMA will use a cyclic buffer to write to one half, and the software can
read from the other half. The DMA operation doesn't stop until the buffer is
disabled.
The DMA coherent area is allocated when DMA is initially started, and
deallocated only if the watermark is changed to 1 (no more DMA usage). The
coherent area is large enough to cope with maximum fifo size for all possible
channels enabled.
The fifo size is set to 128 conversions by default in the driver.

The implementation uses the user watermark  to decide whether DMA will be
used or not. For watermark 1, DMA will not be used. If watermark is greater,
DMA will be used.
Sysfs attributes are created to indicate whether the DMA is used,
with hwfifo_enabled, and the current DMA watermark is readable
in hwfifo_watermark. Minimum and maximum values are in hwfifo_watermark_min
and hwfifo_watermark_max.

Devicetree binding added for dma as well.
Modified devicetree for sama5d2 SoC to add connected DMA channel.

A different commit modifies the driver to use the highest ADC clock
to demonstrate the capabilities of the DMA support at best sampling rate.

A different commit addresses the issue of not clearing the DRDY irq in direct
mode. This is required to avoid erroneous overrun warnings.

  Changes in v2:
 - No longer add last timestamp to all samples. Now, compute an interval
between samples w.r.t. start and end time of the transfer and number
of samples. Then distribute them each in the time interval.
 - Add warning for conversion overrun. This helps user identify cases
when the watermark needs adjustment : the software is too slow in reading
data from the ADC.
 - Protection around watermark is not needed, changing of the watermark
cannot be done while the buffer is enabled. When buffer is disabled, all
DMA resources are freed anyway.
 - Added validation on trigger to be used by own device
 - Best sample rate I could obtain using the low frequency clock was about
4k samples/second, with a watermark of 100. To get up to 50k samples/second
the ADC frequency must be increased to max.
 - Addressed other comments from mailing list review. Feedback is appreciated
 - Added [PATCH v2 4/5] iio: adc: at91-sama5d2_adc: ack DRDY irq in direct
 - Added [PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

Eugen Hristev (5):
  dt-bindings: iio: at91-sama5d2_adc: add optional dma property
  ARM: dts: at91: sama5d2: added dma property for ADC device
  iio: adc: at91-sama5d2_adc: add support for DMA
  iio: adc: at91-sama5d2_adc: ack DRDY irq in direct mode
  iio: adc: at91-sama5d2_adc: use max sample rate

 .../bindings/iio/adc/at91-sama5d2_adc.txt          |   7 +
 arch/arm/boot/dts/sama5d2.dtsi                     |   2 +
 drivers/iio/adc/Kconfig                            |   1 +
 drivers/iio/adc/at91-sama5d2_adc.c                 | 437 ++++++++++++++++++++-
 4 files changed, 428 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH v2 0/5] iio: adc: at91-sama5d2_adc: add DMA support

[Eugen Hristev]

This patch implements the DMA support for the ADC in sama5d2 SoC.
After discussing on the mailing list, this approach is based on triggered
kfifo buffer, with DMA support added on top of it.
Thus, the trigger is enabled by the buffer. The ADC itself will not have
an IRQ enabled if using DMA. With DMA, the channels are enabled, and DMA
controller is configured to read from the data ready registers.
When DMA starts, the trigger will start the conversion (external trigger
configured), then the registers will have the conversion data ready, the ADC
will trigger the DMA controller to read from the registers, the DMA will
copy the data into the software buffer, and trigger the DMA IRQ.
In the bottom half, the trigger polled and the data from the DMA buffer is
pushed to buffer.

The DMA will use a cyclic buffer to write to one half, and the software can
read from the other half. The DMA operation doesn't stop until the buffer is
disabled.
The DMA coherent area is allocated when DMA is initially started, and
deallocated only if the watermark is changed to 1 (no more DMA usage). The
coherent area is large enough to cope with maximum fifo size for all possible
channels enabled.
The fifo size is set to 128 conversions by default in the driver.

The implementation uses the user watermark  to decide whether DMA will be
used or not. For watermark 1, DMA will not be used. If watermark is greater,
DMA will be used.
Sysfs attributes are created to indicate whether the DMA is used,
with hwfifo_enabled, and the current DMA watermark is readable
in hwfifo_watermark. Minimum and maximum values are in hwfifo_watermark_min
and hwfifo_watermark_max.

Devicetree binding added for dma as well.
Modified devicetree for sama5d2 SoC to add connected DMA channel.

A different commit modifies the driver to use the highest ADC clock
to demonstrate the capabilities of the DMA support at best sampling rate.

A different commit addresses the issue of not clearing the DRDY irq in direct
mode. This is required to avoid erroneous overrun warnings.

  Changes in v2:
 - No longer add last timestamp to all samples. Now, compute an interval
between samples w.r.t. start and end time of the transfer and number
of samples. Then distribute them each in the time interval.
 - Add warning for conversion overrun. This helps user identify cases
when the watermark needs adjustment : the software is too slow in reading
data from the ADC.
 - Protection around watermark is not needed, changing of the watermark
cannot be done while the buffer is enabled. When buffer is disabled, all
DMA resources are freed anyway.
 - Added validation on trigger to be used by own device
 - Best sample rate I could obtain using the low frequency clock was about
4k samples/second, with a watermark of 100. To get up to 50k samples/second
the ADC frequency must be increased to max.
 - Addressed other comments from mailing list review. Feedback is appreciated
 - Added [PATCH v2 4/5] iio: adc: at91-sama5d2_adc: ack DRDY irq in direct
 - Added [PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

Eugen Hristev (5):
  dt-bindings: iio: at91-sama5d2_adc: add optional dma property
  ARM: dts: at91: sama5d2: added dma property for ADC device
  iio: adc: at91-sama5d2_adc: add support for DMA
  iio: adc: at91-sama5d2_adc: ack DRDY irq in direct mode
  iio: adc: at91-sama5d2_adc: use max sample rate

 .../bindings/iio/adc/at91-sama5d2_adc.txt          |   7 +
 arch/arm/boot/dts/sama5d2.dtsi                     |   2 +
 drivers/iio/adc/Kconfig                            |   1 +
 drivers/iio/adc/at91-sama5d2_adc.c                 | 437 ++++++++++++++++++++-
 4 files changed, 428 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH v2 0/5] iio: adc: at91-sama5d2_adc: add DMA support

[Eugen Hristev]

This patch implements the DMA support for the ADC in sama5d2 SoC.
After discussing on the mailing list, this approach is based on triggered
kfifo buffer, with DMA support added on top of it.
Thus, the trigger is enabled by the buffer. The ADC itself will not have
an IRQ enabled if using DMA. With DMA, the channels are enabled, and DMA
controller is configured to read from the data ready registers.
When DMA starts, the trigger will start the conversion (external trigger
configured), then the registers will have the conversion data ready, the ADC
will trigger the DMA controller to read from the registers, the DMA will
copy the data into the software buffer, and trigger the DMA IRQ.
In the bottom half, the trigger polled and the data from the DMA buffer is
pushed to buffer.

The DMA will use a cyclic buffer to write to one half, and the software can
read from the other half. The DMA operation doesn't stop until the buffer is
disabled.
The DMA coherent area is allocated when DMA is initially started, and
deallocated only if the watermark is changed to 1 (no more DMA usage). The
coherent area is large enough to cope with maximum fifo size for all possible
channels enabled.
The fifo size is set to 128 conversions by default in the driver.

The implementation uses the user watermark  to decide whether DMA will be
used or not. For watermark 1, DMA will not be used. If watermark is greater,
DMA will be used.
Sysfs attributes are created to indicate whether the DMA is used,
with hwfifo_enabled, and the current DMA watermark is readable
in hwfifo_watermark. Minimum and maximum values are in hwfifo_watermark_min
and hwfifo_watermark_max.

Devicetree binding added for dma as well.
Modified devicetree for sama5d2 SoC to add connected DMA channel.

A different commit modifies the driver to use the highest ADC clock
to demonstrate the capabilities of the DMA support at best sampling rate.

A different commit addresses the issue of not clearing the DRDY irq in direct
mode. This is required to avoid erroneous overrun warnings.

  Changes in v2:
 - No longer add last timestamp to all samples. Now, compute an interval
between samples w.r.t. start and end time of the transfer and number
of samples. Then distribute them each in the time interval.
 - Add warning for conversion overrun. This helps user identify cases
when the watermark needs adjustment : the software is too slow in reading
data from the ADC.
 - Protection around watermark is not needed, changing of the watermark
cannot be done while the buffer is enabled. When buffer is disabled, all
DMA resources are freed anyway.
 - Added validation on trigger to be used by own device
 - Best sample rate I could obtain using the low frequency clock was about
4k samples/second, with a watermark of 100. To get up to 50k samples/second
the ADC frequency must be increased to max.
 - Addressed other comments from mailing list review. Feedback is appreciated
 - Added [PATCH v2 4/5] iio: adc: at91-sama5d2_adc: ack DRDY irq in direct
 - Added [PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

Eugen Hristev (5):
  dt-bindings: iio: at91-sama5d2_adc: add optional dma property
  ARM: dts: at91: sama5d2: added dma property for ADC device
  iio: adc: at91-sama5d2_adc: add support for DMA
  iio: adc: at91-sama5d2_adc: ack DRDY irq in direct mode
  iio: adc: at91-sama5d2_adc: use max sample rate

 .../bindings/iio/adc/at91-sama5d2_adc.txt          |   7 +
 arch/arm/boot/dts/sama5d2.dtsi                     |   2 +
 drivers/iio/adc/Kconfig                            |   1 +
 drivers/iio/adc/at91-sama5d2_adc.c                 | 437 ++++++++++++++++++++-
 4 files changed, 428 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Eugen Hristev]

Added property for DMA configuration of the device.

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

diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
index 552e7a8..5f94d479 100644
--- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
@@ -17,6 +17,11 @@ Required properties:
   This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
   IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
 
+Optional properties:
+  - dmas: Phandle to dma channel for the ADC.
+  See ../../dma/dma.txt for details.
+  - dma-names: Must be "rx" when dmas property is being used.
+
 Example:
 
 adc: adc@fc030000 {
@@ -31,4 +36,6 @@ adc: adc@fc030000 {
 	vddana-supply = <&vdd_3v3_lp_reg>;
 	vref-supply = <&vdd_3v3_lp_reg>;
 	atmel,trigger-edge-type = <IRQ_TYPE_EDGE_BOTH>;
+	dmas = <&dma0 (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) | AT91_XDMAC_DT_PERID(25))>;
+	dma-names = "rx";
 }
-- 
2.7.4

[PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Eugen Hristev]

Added property for DMA configuration of the device.

Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
---
 Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
index 552e7a8..5f94d479 100644
--- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
@@ -17,6 +17,11 @@ Required properties:
   This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
   IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
 
+Optional properties:
+  - dmas: Phandle to dma channel for the ADC.
+  See ../../dma/dma.txt for details.
+  - dma-names: Must be "rx" when dmas property is being used.
+
 Example:
 
 adc: adc@fc030000 {
@@ -31,4 +36,6 @@ adc: adc@fc030000 {
 	vddana-supply = <&vdd_3v3_lp_reg>;
 	vref-supply = <&vdd_3v3_lp_reg>;
 	atmel,trigger-edge-type = <IRQ_TYPE_EDGE_BOTH>;
+	dmas = <&dma0 (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) | AT91_XDMAC_DT_PERID(25))>;
+	dma-names = "rx";
 }
-- 
2.7.4

[PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Eugen Hristev]

Added property for DMA configuration of the device.

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

diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
index 552e7a8..5f94d479 100644
--- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
@@ -17,6 +17,11 @@ Required properties:
   This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
   IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
 
+Optional properties:
+  - dmas: Phandle to dma channel for the ADC.
+  See ../../dma/dma.txt for details.
+  - dma-names: Must be "rx" when dmas property is being used.
+
 Example:
 
 adc: adc at fc030000 {
@@ -31,4 +36,6 @@ adc: adc at fc030000 {
 	vddana-supply = <&vdd_3v3_lp_reg>;
 	vref-supply = <&vdd_3v3_lp_reg>;
 	atmel,trigger-edge-type = <IRQ_TYPE_EDGE_BOTH>;
+	dmas = <&dma0 (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) | AT91_XDMAC_DT_PERID(25))>;
+	dma-names = "rx";
 }
-- 
2.7.4

[PATCH v2 2/5] ARM: dts: at91: sama5d2: added dma property for ADC device

[Eugen Hristev]

Added DMA property for ADC device

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

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index cc06da3..8b4c60c 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -1368,6 +1368,8 @@
 				atmel,min-sample-rate-hz = <200000>;
 				atmel,max-sample-rate-hz = <20000000>;
 				atmel,startup-time-ms = <4>;
+				dmas = <&dma0 (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) | AT91_XDMAC_DT_PERID(25))>;
+				dma-names = "rx";
 				status = "disabled";
 			};
 
-- 
2.7.4

[PATCH v2 2/5] ARM: dts: at91: sama5d2: added dma property for ADC device

[Eugen Hristev]

Added DMA property for ADC device

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

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index cc06da3..8b4c60c 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -1368,6 +1368,8 @@
 				atmel,min-sample-rate-hz = <200000>;
 				atmel,max-sample-rate-hz = <20000000>;
 				atmel,startup-time-ms = <4>;
+				dmas = <&dma0 (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) | AT91_XDMAC_DT_PERID(25))>;
+				dma-names = "rx";
 				status = "disabled";
 			};
 
-- 
2.7.4

[PATCH v2 2/5] ARM: dts: at91: sama5d2: added dma property for ADC device

[Eugen Hristev]

Added DMA property for ADC device

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

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index cc06da3..8b4c60c 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -1368,6 +1368,8 @@
 				atmel,min-sample-rate-hz = <200000>;
 				atmel,max-sample-rate-hz = <20000000>;
 				atmel,startup-time-ms = <4>;
+				dmas = <&dma0 (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) | AT91_XDMAC_DT_PERID(25))>;
+				dma-names = "rx";
 				status = "disabled";
 			};
 
-- 
2.7.4

[PATCH v2 3/5] iio: adc: at91-sama5d2_adc: add support for DMA

[Eugen Hristev]

Added support for DMA transfers. The implementation uses the user watermark
to decide whether DMA will be used or not. For watermark 1, DMA will not be
used. If watermark is bigger, DMA will be used.
Sysfs attributes are created to indicate whether the DMA is used,
with hwfifo_enabled, and the current DMA watermark is readable
in hwfifo_watermark. Minimum and maximum values are in hwfifo_watermark_min
and hwfifo_watermark_max.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
  Changes in v2:
 - No longer add last timestamp to all samples. Now, compute an interval
between samples w.r.t. start and end time of the transfer and number
of samples. Then distribute them each in the time interval.
 - Add warning for conversion overrun. This helps user identify cases
when the watermark needs adjustment : the software is too slow in reading
data from the ADC.
 - Protection around watermark is not needed, changing of the watermark
cannot be done while the buffer is enabled. When buffer is disabled, all
DMA resources are freed anyway.
 - Added validation on trigger to be used by own device
 - Best sample rate I could obtain using the low frequency clock was about
4k samples/second, with a watermark of 100. To get up to 50k samples/second
the ADC frequency must be increased to max.
 - Fixed computation of DMA buffer size
 - Addressed other comments from mailing list review. Feedback is appreciated

 drivers/iio/adc/Kconfig            |   1 +
 drivers/iio/adc/at91-sama5d2_adc.c | 432 +++++++++++++++++++++++++++++++++++--
 2 files changed, 415 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 5762565..9ef288f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -158,6 +158,7 @@ config AT91_SAMA5D2_ADC
 	tristate "Atmel AT91 SAMA5D2 ADC"
 	depends on ARCH_AT91 || COMPILE_TEST
 	depends on HAS_IOMEM
+	depends on HAS_DMA
 	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Atmel SAMA5D2 ADC which is
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index bc5b38e..7a9305a 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -16,6 +16,8 @@
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -100,6 +102,8 @@
 #define AT91_SAMA5D2_LCDR	0x20
 /* Interrupt Enable Register */
 #define AT91_SAMA5D2_IER	0x24
+/* Interrupt Enable Register - general overrun error */
+#define AT91_SAMA5D2_IER_GOVRE BIT(25)
 /* Interrupt Disable Register */
 #define AT91_SAMA5D2_IDR	0x28
 /* Interrupt Mask Register */
@@ -167,13 +171,16 @@
 
 /*
  * Maximum number of bytes to hold conversion from all channels
- * plus the timestamp
+ * without the timestamp.
  */
 #define AT91_BUFFER_MAX_BYTES ((AT91_SAMA5D2_SINGLE_CHAN_CNT +		\
-				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2 + 8)
+				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2)
 
 #define AT91_BUFFER_MAX_HWORDS (AT91_BUFFER_MAX_BYTES / 2)
 
+#define AT91_HWFIFO_MAX_SIZE_STR	"128"
+#define AT91_HWFIFO_MAX_SIZE		128
+
 #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
 	{								\
 		.type = IIO_VOLTAGE,					\
@@ -227,6 +234,28 @@ struct at91_adc_trigger {
 	unsigned int			edge_type;
 };
 
+/**
+ * at91_adc_dma - at91-sama5d2 dma information struct
+ * @dma_chan:		the dma channel acquired
+ * @rx_buf:		dma coherent allocated area
+ * @rx_dma_buf:		dma handler for the buffer
+ * @phys_addr:		physical address of the ADC base register
+ * @buf_idx:		index inside the dma buffer where reading was last done
+ * @rx_buf_sz:		size of buffer used by DMA operation
+ * @watermark:		number of conversions to copy before DMA triggers irq
+ * @dma_ts:		hold the start timestamp of dma operation
+ */
+struct at91_adc_dma {
+	struct dma_chan			*dma_chan;
+	u8				*rx_buf;
+	dma_addr_t			rx_dma_buf;
+	phys_addr_t			phys_addr;
+	int				buf_idx;
+	int				rx_buf_sz;
+	int				watermark;
+	s64				dma_ts;
+};
+
 struct at91_adc_state {
 	void __iomem			*base;
 	int				irq;
@@ -241,6 +270,7 @@ struct at91_adc_state {
 	u32				conversion_value;
 	struct at91_adc_soc_info	soc_info;
 	wait_queue_head_t		wq_data_available;
+	struct at91_adc_dma		dma_st;
 	u16				buffer[AT91_BUFFER_MAX_HWORDS];
 	/*
 	 * lock to prevent concurrent 'single conversion' requests through
@@ -312,11 +342,17 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
 		if (state) {
 			at91_adc_writel(st, AT91_SAMA5D2_CHER,
 					BIT(chan->channel));
-			at91_adc_writel(st, AT91_SAMA5D2_IER,
-					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 {
-			at91_adc_writel(st, AT91_SAMA5D2_IDR,
-					BIT(chan->channel));
+			/* 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));
 		}
@@ -330,6 +366,10 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
 	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
 	struct at91_adc_state *st = iio_priv(indio);
 
+	/* if we are using DMA, we must not reenable irq after each trigger */
+	if (st->dma_st.dma_chan)
+		return 0;
+
 	enable_irq(st->irq);
 
 	/* Needed to ACK the DRDY interruption */
@@ -341,6 +381,153 @@ static const struct iio_trigger_ops at91_adc_trigger_ops = {
 	.owner = THIS_MODULE,
 	.set_trigger_state = &at91_adc_configure_trigger,
 	.try_reenable = &at91_adc_reenable_trigger,
+	.validate_device = iio_trigger_validate_own_device,
+};
+
+static int at91_adc_dma_size_done(struct at91_adc_state *st)
+{
+	struct dma_tx_state state;
+	enum dma_status status;
+	int i, size;
+
+	status = dmaengine_tx_status(st->dma_st.dma_chan,
+				     st->dma_st.dma_chan->cookie,
+				     &state);
+	if (status != DMA_IN_PROGRESS)
+		return 0;
+
+	/* Transferred length is size in bytes from end of buffer */
+	i = st->dma_st.rx_buf_sz - state.residue;
+
+	/* Return available bytes */
+	if (i >= st->dma_st.buf_idx)
+		size = i - st->dma_st.buf_idx;
+	else
+		size = st->dma_st.rx_buf_sz + i - st->dma_st.buf_idx;
+	return size;
+}
+
+static void at91_dma_buffer_done(void *data)
+{
+	struct iio_dev *indio_dev = data;
+
+	iio_trigger_poll_chained(indio_dev->trig);
+}
+
+static int at91_adc_dma_start(struct iio_dev *indio_dev)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	struct dma_async_tx_descriptor *desc;
+	dma_cookie_t cookie;
+	int ret;
+	u8 bit;
+
+	if (!st->dma_st.dma_chan)
+		return 0;
+
+	/* we start a new DMA, so set buffer index to start */
+	st->dma_st.buf_idx = 0;
+
+	/*
+	 * compute buffer size w.r.t. watermark and enabled channels.
+	 * scan_bytes is aligned so we need an exact size for DMA
+	 */
+	st->dma_st.rx_buf_sz = 0;
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->num_channels) {
+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
+
+		st->dma_st.rx_buf_sz += chan->scan_type.storagebits / 8;
+	}
+	st->dma_st.rx_buf_sz *= st->dma_st.watermark;
+
+	/* Prepare a DMA cyclic transaction */
+	desc = dmaengine_prep_dma_cyclic(st->dma_st.dma_chan,
+					 st->dma_st.rx_dma_buf,
+					 st->dma_st.rx_buf_sz,
+					 st->dma_st.rx_buf_sz / 2,
+					 DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
+
+	if (!desc) {
+		dev_err(&indio_dev->dev, "cannot prepare DMA cyclic\n");
+		return -EBUSY;
+	}
+
+	desc->callback = at91_dma_buffer_done;
+	desc->callback_param = indio_dev;
+
+	cookie = dmaengine_submit(desc);
+	ret = dma_submit_error(cookie);
+	if (ret) {
+		dev_err(&indio_dev->dev, "cannot submit DMA cyclic\n");
+		dmaengine_terminate_async(st->dma_st.dma_chan);
+		return ret;
+	}
+
+	/* enable general overrun error signaling */
+	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_GOVRE);
+	/* Issue pending DMA requests */
+	dma_async_issue_pending(st->dma_st.dma_chan);
+
+	/* consider current time as DMA start time for timestamps */
+	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
+
+	dev_dbg(&indio_dev->dev, "DMA cyclic started\n");
+
+	return 0;
+}
+
+static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
+{
+	int ret;
+
+	ret = at91_adc_dma_start(indio_dev);
+	if (ret) {
+		dev_err(&indio_dev->dev, "buffer postenable failed\n");
+		return ret;
+	}
+
+	return iio_triggered_buffer_postenable(indio_dev);
+}
+
+static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	int ret;
+	u8 bit;
+
+	ret = iio_triggered_buffer_predisable(indio_dev);
+	if (ret < 0)
+		dev_err(&indio_dev->dev, "buffer predisable failed\n");
+
+	if (!st->dma_st.dma_chan)
+		return ret;
+
+	/* if we are using DMA we must clear registers and end DMA */
+	dmaengine_terminate_sync(st->dma_st.dma_chan);
+
+	/*
+	 * For each enabled channel we must read the last converted value
+	 * to clear EOC status and not get a possible interrupt later.
+	 * This value is being read by DMA from LCDR anyway
+	 */
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->num_channels) {
+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
+
+		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);
+	return ret;
+}
+
+static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
+	.postenable = &at91_adc_buffer_postenable,
+	.predisable = &at91_adc_buffer_predisable,
 };
 
 static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
@@ -379,24 +566,71 @@ static int at91_adc_trigger_init(struct iio_dev *indio)
 	return 0;
 }
 
-static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
+static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
+					   struct iio_poll_func *pf)
 {
-	struct iio_poll_func *pf = p;
-	struct iio_dev *indio = pf->indio_dev;
-	struct at91_adc_state *st = iio_priv(indio);
+	struct at91_adc_state *st = iio_priv(indio_dev);
 	int i = 0;
 	u8 bit;
 
-	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
-		struct iio_chan_spec const *chan = indio->channels + bit;
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->num_channels) {
+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
 
 		st->buffer[i] = at91_adc_readl(st, chan->address);
 		i++;
 	}
+	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer,
+					   pf->timestamp);
+}
+
+static void 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);
+	s64 ns = iio_get_time_ns(indio_dev);
+	s64 interval;
+	int sample_index = 0, sample_count, sample_size;
 
-	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
+	sample_size = div_s64(st->dma_st.rx_buf_sz, st->dma_st.watermark);
 
-	iio_trigger_notify_done(indio->trig);
+	sample_count = div_s64(transferred_len, sample_size);
+
+	/*
+	 * interval between samples is total time since last transfer handling
+	 * divided by the number of samples (total size divided by sample size)
+	 */
+	interval = div_s64((ns - st->dma_st.dma_ts), sample_count);
+
+	while (transferred_len >= sample_size) {
+		iio_push_to_buffers_with_timestamp(indio_dev,
+				(st->dma_st.rx_buf + st->dma_st.buf_idx),
+				(st->dma_st.dma_ts + interval * sample_index));
+		/* adjust remaining length */
+		transferred_len -= sample_size;
+		/* adjust buffer index */
+		st->dma_st.buf_idx += sample_size;
+		/* in case of reaching end of buffer, reset index */
+		if (st->dma_st.buf_idx >= st->dma_st.rx_buf_sz)
+			st->dma_st.buf_idx = 0;
+		sample_index++;
+	}
+	/* adjust saved time for next transfer handling */
+	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
+}
+
+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);
+
+	if (st->dma_st.dma_chan)
+		at91_adc_trigger_handler_dma(indio_dev);
+	else
+		at91_adc_trigger_handler_nodma(indio_dev, pf);
+
+	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
 }
@@ -405,7 +639,7 @@ static int at91_adc_buffer_init(struct iio_dev *indio)
 {
 	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
 			&iio_pollfunc_store_time,
-			&at91_adc_trigger_handler, NULL);
+			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
 }
 
 static unsigned at91_adc_startup_time(unsigned startup_time_min,
@@ -476,10 +710,17 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
 	if (!(status & imr))
 		return IRQ_NONE;
 
-	if (iio_buffer_enabled(indio)) {
+	/* if we reached this point, we cannot sample faster */
+	if (status & AT91_SAMA5D2_IER_GOVRE)
+		pr_alert_ratelimited("Conversion overrun detected\n");
+
+	if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
 		disable_irq_nosync(irq);
 		iio_trigger_poll(indio->trig);
-	} else {
+	} else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
+		disable_irq_nosync(irq);
+		WARN(true, "Unexpected irq occurred\n");
+	} else if (!iio_buffer_enabled(indio)) {
 		st->conversion_value = at91_adc_readl(st, st->chan->address);
 		st->conversion_done = true;
 		wake_up_interruptible(&st->wq_data_available);
@@ -571,9 +812,111 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 	return 0;
 }
 
+static void at91_adc_dma_init(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	struct dma_slave_config config = {0};
+	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
+					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
+
+	if (st->dma_st.dma_chan)
+		return;
+
+	st->dma_st.dma_chan = dma_request_slave_channel(&pdev->dev, "rx");
+
+	if (!st->dma_st.dma_chan)  {
+		dev_info(&pdev->dev, "can't get DMA channel\n");
+		goto dma_exit;
+	}
+
+	st->dma_st.rx_buf = dma_alloc_coherent(st->dma_st.dma_chan->device->dev,
+					       pages * PAGE_SIZE,
+					       &st->dma_st.rx_dma_buf,
+					       GFP_KERNEL);
+	if (!st->dma_st.rx_buf) {
+		dev_info(&pdev->dev, "can't allocate coherent DMA area\n");
+		goto dma_chan_disable;
+	}
+
+	/* Configure DMA channel to read data register */
+	config.direction = DMA_DEV_TO_MEM;
+	config.src_addr = (phys_addr_t)(st->dma_st.phys_addr
+			  + AT91_SAMA5D2_LCDR);
+	config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+	config.src_maxburst = 1;
+	config.dst_maxburst = 1;
+
+	if (dmaengine_slave_config(st->dma_st.dma_chan, &config)) {
+		dev_info(&pdev->dev, "can't configure DMA slave\n");
+		goto dma_free_area;
+	}
+
+	dev_info(&pdev->dev, "using %s for rx DMA transfers\n",
+		 dma_chan_name(st->dma_st.dma_chan));
+
+	return;
+
+dma_free_area:
+	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
+			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
+dma_chan_disable:
+	dma_release_channel(st->dma_st.dma_chan);
+	st->dma_st.dma_chan = 0;
+dma_exit:
+	dev_info(&pdev->dev, "continuing without DMA support\n");
+}
+
+static void at91_adc_dma_disable(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
+					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
+
+	/* if we are not using DMA, just return */
+	if (!st->dma_st.dma_chan)
+		return;
+
+	/* wait for all transactions to be terminated first*/
+	dmaengine_terminate_sync(st->dma_st.dma_chan);
+
+	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
+			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
+	dma_release_channel(st->dma_st.dma_chan);
+	st->dma_st.dma_chan = 0;
+
+	dev_info(&pdev->dev, "continuing without DMA support\n");
+}
+
+static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	if (val > AT91_HWFIFO_MAX_SIZE)
+		return -EINVAL;
+
+	dev_dbg(&indio_dev->dev, "new watermark is %u\n", val);
+	st->dma_st.watermark = val;
+
+	/*
+	 * The logic here is: if we have watermark 1, it means we do
+	 * each conversion with it's own IRQ, thus we don't need DMA.
+	 * If the watermark is higher, we do DMA to do all the transfers in bulk
+	 */
+
+	if (val == 1)
+		at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
+	else if (val > 1)
+		at91_adc_dma_init(to_platform_device(&indio_dev->dev));
+
+	return 0;
+}
+
 static const struct iio_info at91_adc_info = {
 	.read_raw = &at91_adc_read_raw,
 	.write_raw = &at91_adc_write_raw,
+	.hwfifo_set_watermark = &at91_adc_set_watermark,
 	.driver_module = THIS_MODULE,
 };
 
@@ -591,6 +934,42 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
 	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
 }
 
+static ssize_t at91_adc_get_fifo_state(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev =
+			platform_get_drvdata(to_platform_device(dev));
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", !!st->dma_st.dma_chan);
+}
+
+static ssize_t at91_adc_get_watermark(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev =
+			platform_get_drvdata(to_platform_device(dev));
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", st->dma_st.watermark);
+}
+
+static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
+		       at91_adc_get_fifo_state, NULL, 0);
+static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
+		       at91_adc_get_watermark, NULL, 0);
+
+static IIO_CONST_ATTR(hwfifo_watermark_min, "2");
+static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
+
+static const struct attribute *at91_adc_fifo_attributes[] = {
+	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
+	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
+	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
+	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	NULL,
+};
+
 static int at91_adc_probe(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev;
@@ -666,6 +1045,9 @@ static int at91_adc_probe(struct platform_device *pdev)
 	if (!res)
 		return -EINVAL;
 
+	/* if we plan to use DMA, we need the physical address of the regs */
+	st->dma_st.phys_addr = res->start;
+
 	st->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(st->base))
 		return PTR_ERR(st->base);
@@ -729,18 +1111,30 @@ static int at91_adc_probe(struct platform_device *pdev)
 		goto per_clk_disable_unprepare;
 	}
 
+	if (dma_coerce_mask_and_coherent(&indio_dev->dev, DMA_BIT_MASK(32)))
+		dev_info(&pdev->dev, "cannot set DMA mask to 32-bit\n");
+
+	/* the iio buffer has initially a length of 2 and a watermark of 1 */
+	st->dma_st.watermark = 1;
+
+	iio_buffer_set_attrs(indio_dev->buffer, at91_adc_fifo_attributes);
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
-		goto per_clk_disable_unprepare;
+		goto dma_disable;
 
 	dev_info(&pdev->dev, "setting up trigger as %s\n",
 		 st->selected_trig->name);
 
+	dev_info(&pdev->dev, "continuing without DMA support\n");
+
 	dev_info(&pdev->dev, "version: %x\n",
 		 readl_relaxed(st->base + AT91_SAMA5D2_VERSION));
 
 	return 0;
 
+dma_disable:
+	at91_adc_dma_disable(pdev);
 per_clk_disable_unprepare:
 	clk_disable_unprepare(st->per_clk);
 vref_disable:
@@ -757,6 +1151,8 @@ static int at91_adc_remove(struct platform_device *pdev)
 
 	iio_device_unregister(indio_dev);
 
+	at91_adc_dma_disable(pdev);
+
 	clk_disable_unprepare(st->per_clk);
 
 	regulator_disable(st->vref);
-- 
2.7.4

[PATCH v2 3/5] iio: adc: at91-sama5d2_adc: add support for DMA

[Eugen Hristev]

Added support for DMA transfers. The implementation uses the user watermark
to decide whether DMA will be used or not. For watermark 1, DMA will not be
used. If watermark is bigger, DMA will be used.
Sysfs attributes are created to indicate whether the DMA is used,
with hwfifo_enabled, and the current DMA watermark is readable
in hwfifo_watermark. Minimum and maximum values are in hwfifo_watermark_min
and hwfifo_watermark_max.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
  Changes in v2:
 - No longer add last timestamp to all samples. Now, compute an interval
between samples w.r.t. start and end time of the transfer and number
of samples. Then distribute them each in the time interval.
 - Add warning for conversion overrun. This helps user identify cases
when the watermark needs adjustment : the software is too slow in reading
data from the ADC.
 - Protection around watermark is not needed, changing of the watermark
cannot be done while the buffer is enabled. When buffer is disabled, all
DMA resources are freed anyway.
 - Added validation on trigger to be used by own device
 - Best sample rate I could obtain using the low frequency clock was about
4k samples/second, with a watermark of 100. To get up to 50k samples/second
the ADC frequency must be increased to max.
 - Fixed computation of DMA buffer size
 - Addressed other comments from mailing list review. Feedback is appreciated

 drivers/iio/adc/Kconfig            |   1 +
 drivers/iio/adc/at91-sama5d2_adc.c | 432 +++++++++++++++++++++++++++++++++++--
 2 files changed, 415 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 5762565..9ef288f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -158,6 +158,7 @@ config AT91_SAMA5D2_ADC
 	tristate "Atmel AT91 SAMA5D2 ADC"
 	depends on ARCH_AT91 || COMPILE_TEST
 	depends on HAS_IOMEM
+	depends on HAS_DMA
 	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Atmel SAMA5D2 ADC which is
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index bc5b38e..7a9305a 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -16,6 +16,8 @@
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -100,6 +102,8 @@
 #define AT91_SAMA5D2_LCDR	0x20
 /* Interrupt Enable Register */
 #define AT91_SAMA5D2_IER	0x24
+/* Interrupt Enable Register - general overrun error */
+#define AT91_SAMA5D2_IER_GOVRE BIT(25)
 /* Interrupt Disable Register */
 #define AT91_SAMA5D2_IDR	0x28
 /* Interrupt Mask Register */
@@ -167,13 +171,16 @@
 
 /*
  * Maximum number of bytes to hold conversion from all channels
- * plus the timestamp
+ * without the timestamp.
  */
 #define AT91_BUFFER_MAX_BYTES ((AT91_SAMA5D2_SINGLE_CHAN_CNT +		\
-				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2 + 8)
+				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2)
 
 #define AT91_BUFFER_MAX_HWORDS (AT91_BUFFER_MAX_BYTES / 2)
 
+#define AT91_HWFIFO_MAX_SIZE_STR	"128"
+#define AT91_HWFIFO_MAX_SIZE		128
+
 #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
 	{								\
 		.type = IIO_VOLTAGE,					\
@@ -227,6 +234,28 @@ struct at91_adc_trigger {
 	unsigned int			edge_type;
 };
 
+/**
+ * at91_adc_dma - at91-sama5d2 dma information struct
+ * @dma_chan:		the dma channel acquired
+ * @rx_buf:		dma coherent allocated area
+ * @rx_dma_buf:		dma handler for the buffer
+ * @phys_addr:		physical address of the ADC base register
+ * @buf_idx:		index inside the dma buffer where reading was last done
+ * @rx_buf_sz:		size of buffer used by DMA operation
+ * @watermark:		number of conversions to copy before DMA triggers irq
+ * @dma_ts:		hold the start timestamp of dma operation
+ */
+struct at91_adc_dma {
+	struct dma_chan			*dma_chan;
+	u8				*rx_buf;
+	dma_addr_t			rx_dma_buf;
+	phys_addr_t			phys_addr;
+	int				buf_idx;
+	int				rx_buf_sz;
+	int				watermark;
+	s64				dma_ts;
+};
+
 struct at91_adc_state {
 	void __iomem			*base;
 	int				irq;
@@ -241,6 +270,7 @@ struct at91_adc_state {
 	u32				conversion_value;
 	struct at91_adc_soc_info	soc_info;
 	wait_queue_head_t		wq_data_available;
+	struct at91_adc_dma		dma_st;
 	u16				buffer[AT91_BUFFER_MAX_HWORDS];
 	/*
 	 * lock to prevent concurrent 'single conversion' requests through
@@ -312,11 +342,17 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
 		if (state) {
 			at91_adc_writel(st, AT91_SAMA5D2_CHER,
 					BIT(chan->channel));
-			at91_adc_writel(st, AT91_SAMA5D2_IER,
-					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 {
-			at91_adc_writel(st, AT91_SAMA5D2_IDR,
-					BIT(chan->channel));
+			/* 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));
 		}
@@ -330,6 +366,10 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
 	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
 	struct at91_adc_state *st = iio_priv(indio);
 
+	/* if we are using DMA, we must not reenable irq after each trigger */
+	if (st->dma_st.dma_chan)
+		return 0;
+
 	enable_irq(st->irq);
 
 	/* Needed to ACK the DRDY interruption */
@@ -341,6 +381,153 @@ static const struct iio_trigger_ops at91_adc_trigger_ops = {
 	.owner = THIS_MODULE,
 	.set_trigger_state = &at91_adc_configure_trigger,
 	.try_reenable = &at91_adc_reenable_trigger,
+	.validate_device = iio_trigger_validate_own_device,
+};
+
+static int at91_adc_dma_size_done(struct at91_adc_state *st)
+{
+	struct dma_tx_state state;
+	enum dma_status status;
+	int i, size;
+
+	status = dmaengine_tx_status(st->dma_st.dma_chan,
+				     st->dma_st.dma_chan->cookie,
+				     &state);
+	if (status != DMA_IN_PROGRESS)
+		return 0;
+
+	/* Transferred length is size in bytes from end of buffer */
+	i = st->dma_st.rx_buf_sz - state.residue;
+
+	/* Return available bytes */
+	if (i >= st->dma_st.buf_idx)
+		size = i - st->dma_st.buf_idx;
+	else
+		size = st->dma_st.rx_buf_sz + i - st->dma_st.buf_idx;
+	return size;
+}
+
+static void at91_dma_buffer_done(void *data)
+{
+	struct iio_dev *indio_dev = data;
+
+	iio_trigger_poll_chained(indio_dev->trig);
+}
+
+static int at91_adc_dma_start(struct iio_dev *indio_dev)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	struct dma_async_tx_descriptor *desc;
+	dma_cookie_t cookie;
+	int ret;
+	u8 bit;
+
+	if (!st->dma_st.dma_chan)
+		return 0;
+
+	/* we start a new DMA, so set buffer index to start */
+	st->dma_st.buf_idx = 0;
+
+	/*
+	 * compute buffer size w.r.t. watermark and enabled channels.
+	 * scan_bytes is aligned so we need an exact size for DMA
+	 */
+	st->dma_st.rx_buf_sz = 0;
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->num_channels) {
+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
+
+		st->dma_st.rx_buf_sz += chan->scan_type.storagebits / 8;
+	}
+	st->dma_st.rx_buf_sz *= st->dma_st.watermark;
+
+	/* Prepare a DMA cyclic transaction */
+	desc = dmaengine_prep_dma_cyclic(st->dma_st.dma_chan,
+					 st->dma_st.rx_dma_buf,
+					 st->dma_st.rx_buf_sz,
+					 st->dma_st.rx_buf_sz / 2,
+					 DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
+
+	if (!desc) {
+		dev_err(&indio_dev->dev, "cannot prepare DMA cyclic\n");
+		return -EBUSY;
+	}
+
+	desc->callback = at91_dma_buffer_done;
+	desc->callback_param = indio_dev;
+
+	cookie = dmaengine_submit(desc);
+	ret = dma_submit_error(cookie);
+	if (ret) {
+		dev_err(&indio_dev->dev, "cannot submit DMA cyclic\n");
+		dmaengine_terminate_async(st->dma_st.dma_chan);
+		return ret;
+	}
+
+	/* enable general overrun error signaling */
+	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_GOVRE);
+	/* Issue pending DMA requests */
+	dma_async_issue_pending(st->dma_st.dma_chan);
+
+	/* consider current time as DMA start time for timestamps */
+	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
+
+	dev_dbg(&indio_dev->dev, "DMA cyclic started\n");
+
+	return 0;
+}
+
+static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
+{
+	int ret;
+
+	ret = at91_adc_dma_start(indio_dev);
+	if (ret) {
+		dev_err(&indio_dev->dev, "buffer postenable failed\n");
+		return ret;
+	}
+
+	return iio_triggered_buffer_postenable(indio_dev);
+}
+
+static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	int ret;
+	u8 bit;
+
+	ret = iio_triggered_buffer_predisable(indio_dev);
+	if (ret < 0)
+		dev_err(&indio_dev->dev, "buffer predisable failed\n");
+
+	if (!st->dma_st.dma_chan)
+		return ret;
+
+	/* if we are using DMA we must clear registers and end DMA */
+	dmaengine_terminate_sync(st->dma_st.dma_chan);
+
+	/*
+	 * For each enabled channel we must read the last converted value
+	 * to clear EOC status and not get a possible interrupt later.
+	 * This value is being read by DMA from LCDR anyway
+	 */
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->num_channels) {
+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
+
+		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);
+	return ret;
+}
+
+static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
+	.postenable = &at91_adc_buffer_postenable,
+	.predisable = &at91_adc_buffer_predisable,
 };
 
 static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
@@ -379,24 +566,71 @@ static int at91_adc_trigger_init(struct iio_dev *indio)
 	return 0;
 }
 
-static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
+static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
+					   struct iio_poll_func *pf)
 {
-	struct iio_poll_func *pf = p;
-	struct iio_dev *indio = pf->indio_dev;
-	struct at91_adc_state *st = iio_priv(indio);
+	struct at91_adc_state *st = iio_priv(indio_dev);
 	int i = 0;
 	u8 bit;
 
-	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
-		struct iio_chan_spec const *chan = indio->channels + bit;
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->num_channels) {
+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
 
 		st->buffer[i] = at91_adc_readl(st, chan->address);
 		i++;
 	}
+	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer,
+					   pf->timestamp);
+}
+
+static void 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);
+	s64 ns = iio_get_time_ns(indio_dev);
+	s64 interval;
+	int sample_index = 0, sample_count, sample_size;
 
-	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
+	sample_size = div_s64(st->dma_st.rx_buf_sz, st->dma_st.watermark);
 
-	iio_trigger_notify_done(indio->trig);
+	sample_count = div_s64(transferred_len, sample_size);
+
+	/*
+	 * interval between samples is total time since last transfer handling
+	 * divided by the number of samples (total size divided by sample size)
+	 */
+	interval = div_s64((ns - st->dma_st.dma_ts), sample_count);
+
+	while (transferred_len >= sample_size) {
+		iio_push_to_buffers_with_timestamp(indio_dev,
+				(st->dma_st.rx_buf + st->dma_st.buf_idx),
+				(st->dma_st.dma_ts + interval * sample_index));
+		/* adjust remaining length */
+		transferred_len -= sample_size;
+		/* adjust buffer index */
+		st->dma_st.buf_idx += sample_size;
+		/* in case of reaching end of buffer, reset index */
+		if (st->dma_st.buf_idx >= st->dma_st.rx_buf_sz)
+			st->dma_st.buf_idx = 0;
+		sample_index++;
+	}
+	/* adjust saved time for next transfer handling */
+	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
+}
+
+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);
+
+	if (st->dma_st.dma_chan)
+		at91_adc_trigger_handler_dma(indio_dev);
+	else
+		at91_adc_trigger_handler_nodma(indio_dev, pf);
+
+	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
 }
@@ -405,7 +639,7 @@ static int at91_adc_buffer_init(struct iio_dev *indio)
 {
 	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
 			&iio_pollfunc_store_time,
-			&at91_adc_trigger_handler, NULL);
+			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
 }
 
 static unsigned at91_adc_startup_time(unsigned startup_time_min,
@@ -476,10 +710,17 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
 	if (!(status & imr))
 		return IRQ_NONE;
 
-	if (iio_buffer_enabled(indio)) {
+	/* if we reached this point, we cannot sample faster */
+	if (status & AT91_SAMA5D2_IER_GOVRE)
+		pr_alert_ratelimited("Conversion overrun detected\n");
+
+	if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
 		disable_irq_nosync(irq);
 		iio_trigger_poll(indio->trig);
-	} else {
+	} else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
+		disable_irq_nosync(irq);
+		WARN(true, "Unexpected irq occurred\n");
+	} else if (!iio_buffer_enabled(indio)) {
 		st->conversion_value = at91_adc_readl(st, st->chan->address);
 		st->conversion_done = true;
 		wake_up_interruptible(&st->wq_data_available);
@@ -571,9 +812,111 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 	return 0;
 }
 
+static void at91_adc_dma_init(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	struct dma_slave_config config = {0};
+	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
+					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
+
+	if (st->dma_st.dma_chan)
+		return;
+
+	st->dma_st.dma_chan = dma_request_slave_channel(&pdev->dev, "rx");
+
+	if (!st->dma_st.dma_chan)  {
+		dev_info(&pdev->dev, "can't get DMA channel\n");
+		goto dma_exit;
+	}
+
+	st->dma_st.rx_buf = dma_alloc_coherent(st->dma_st.dma_chan->device->dev,
+					       pages * PAGE_SIZE,
+					       &st->dma_st.rx_dma_buf,
+					       GFP_KERNEL);
+	if (!st->dma_st.rx_buf) {
+		dev_info(&pdev->dev, "can't allocate coherent DMA area\n");
+		goto dma_chan_disable;
+	}
+
+	/* Configure DMA channel to read data register */
+	config.direction = DMA_DEV_TO_MEM;
+	config.src_addr = (phys_addr_t)(st->dma_st.phys_addr
+			  + AT91_SAMA5D2_LCDR);
+	config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+	config.src_maxburst = 1;
+	config.dst_maxburst = 1;
+
+	if (dmaengine_slave_config(st->dma_st.dma_chan, &config)) {
+		dev_info(&pdev->dev, "can't configure DMA slave\n");
+		goto dma_free_area;
+	}
+
+	dev_info(&pdev->dev, "using %s for rx DMA transfers\n",
+		 dma_chan_name(st->dma_st.dma_chan));
+
+	return;
+
+dma_free_area:
+	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
+			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
+dma_chan_disable:
+	dma_release_channel(st->dma_st.dma_chan);
+	st->dma_st.dma_chan = 0;
+dma_exit:
+	dev_info(&pdev->dev, "continuing without DMA support\n");
+}
+
+static void at91_adc_dma_disable(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
+					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
+
+	/* if we are not using DMA, just return */
+	if (!st->dma_st.dma_chan)
+		return;
+
+	/* wait for all transactions to be terminated first*/
+	dmaengine_terminate_sync(st->dma_st.dma_chan);
+
+	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
+			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
+	dma_release_channel(st->dma_st.dma_chan);
+	st->dma_st.dma_chan = 0;
+
+	dev_info(&pdev->dev, "continuing without DMA support\n");
+}
+
+static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	if (val > AT91_HWFIFO_MAX_SIZE)
+		return -EINVAL;
+
+	dev_dbg(&indio_dev->dev, "new watermark is %u\n", val);
+	st->dma_st.watermark = val;
+
+	/*
+	 * The logic here is: if we have watermark 1, it means we do
+	 * each conversion with it's own IRQ, thus we don't need DMA.
+	 * If the watermark is higher, we do DMA to do all the transfers in bulk
+	 */
+
+	if (val == 1)
+		at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
+	else if (val > 1)
+		at91_adc_dma_init(to_platform_device(&indio_dev->dev));
+
+	return 0;
+}
+
 static const struct iio_info at91_adc_info = {
 	.read_raw = &at91_adc_read_raw,
 	.write_raw = &at91_adc_write_raw,
+	.hwfifo_set_watermark = &at91_adc_set_watermark,
 	.driver_module = THIS_MODULE,
 };
 
@@ -591,6 +934,42 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
 	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
 }
 
+static ssize_t at91_adc_get_fifo_state(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev =
+			platform_get_drvdata(to_platform_device(dev));
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", !!st->dma_st.dma_chan);
+}
+
+static ssize_t at91_adc_get_watermark(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev =
+			platform_get_drvdata(to_platform_device(dev));
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", st->dma_st.watermark);
+}
+
+static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
+		       at91_adc_get_fifo_state, NULL, 0);
+static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
+		       at91_adc_get_watermark, NULL, 0);
+
+static IIO_CONST_ATTR(hwfifo_watermark_min, "2");
+static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
+
+static const struct attribute *at91_adc_fifo_attributes[] = {
+	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
+	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
+	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
+	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	NULL,
+};
+
 static int at91_adc_probe(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev;
@@ -666,6 +1045,9 @@ static int at91_adc_probe(struct platform_device *pdev)
 	if (!res)
 		return -EINVAL;
 
+	/* if we plan to use DMA, we need the physical address of the regs */
+	st->dma_st.phys_addr = res->start;
+
 	st->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(st->base))
 		return PTR_ERR(st->base);
@@ -729,18 +1111,30 @@ static int at91_adc_probe(struct platform_device *pdev)
 		goto per_clk_disable_unprepare;
 	}
 
+	if (dma_coerce_mask_and_coherent(&indio_dev->dev, DMA_BIT_MASK(32)))
+		dev_info(&pdev->dev, "cannot set DMA mask to 32-bit\n");
+
+	/* the iio buffer has initially a length of 2 and a watermark of 1 */
+	st->dma_st.watermark = 1;
+
+	iio_buffer_set_attrs(indio_dev->buffer, at91_adc_fifo_attributes);
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
-		goto per_clk_disable_unprepare;
+		goto dma_disable;
 
 	dev_info(&pdev->dev, "setting up trigger as %s\n",
 		 st->selected_trig->name);
 
+	dev_info(&pdev->dev, "continuing without DMA support\n");
+
 	dev_info(&pdev->dev, "version: %x\n",
 		 readl_relaxed(st->base + AT91_SAMA5D2_VERSION));
 
 	return 0;
 
+dma_disable:
+	at91_adc_dma_disable(pdev);
 per_clk_disable_unprepare:
 	clk_disable_unprepare(st->per_clk);
 vref_disable:
@@ -757,6 +1151,8 @@ static int at91_adc_remove(struct platform_device *pdev)
 
 	iio_device_unregister(indio_dev);
 
+	at91_adc_dma_disable(pdev);
+
 	clk_disable_unprepare(st->per_clk);
 
 	regulator_disable(st->vref);
-- 
2.7.4

[PATCH v2 3/5] iio: adc: at91-sama5d2_adc: add support for DMA

[Eugen Hristev]

Added support for DMA transfers. The implementation uses the user watermark
to decide whether DMA will be used or not. For watermark 1, DMA will not be
used. If watermark is bigger, DMA will be used.
Sysfs attributes are created to indicate whether the DMA is used,
with hwfifo_enabled, and the current DMA watermark is readable
in hwfifo_watermark. Minimum and maximum values are in hwfifo_watermark_min
and hwfifo_watermark_max.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
  Changes in v2:
 - No longer add last timestamp to all samples. Now, compute an interval
between samples w.r.t. start and end time of the transfer and number
of samples. Then distribute them each in the time interval.
 - Add warning for conversion overrun. This helps user identify cases
when the watermark needs adjustment : the software is too slow in reading
data from the ADC.
 - Protection around watermark is not needed, changing of the watermark
cannot be done while the buffer is enabled. When buffer is disabled, all
DMA resources are freed anyway.
 - Added validation on trigger to be used by own device
 - Best sample rate I could obtain using the low frequency clock was about
4k samples/second, with a watermark of 100. To get up to 50k samples/second
the ADC frequency must be increased to max.
 - Fixed computation of DMA buffer size
 - Addressed other comments from mailing list review. Feedback is appreciated

 drivers/iio/adc/Kconfig            |   1 +
 drivers/iio/adc/at91-sama5d2_adc.c | 432 +++++++++++++++++++++++++++++++++++--
 2 files changed, 415 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 5762565..9ef288f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -158,6 +158,7 @@ config AT91_SAMA5D2_ADC
 	tristate "Atmel AT91 SAMA5D2 ADC"
 	depends on ARCH_AT91 || COMPILE_TEST
 	depends on HAS_IOMEM
+	depends on HAS_DMA
 	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Atmel SAMA5D2 ADC which is
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index bc5b38e..7a9305a 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -16,6 +16,8 @@
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -100,6 +102,8 @@
 #define AT91_SAMA5D2_LCDR	0x20
 /* Interrupt Enable Register */
 #define AT91_SAMA5D2_IER	0x24
+/* Interrupt Enable Register - general overrun error */
+#define AT91_SAMA5D2_IER_GOVRE BIT(25)
 /* Interrupt Disable Register */
 #define AT91_SAMA5D2_IDR	0x28
 /* Interrupt Mask Register */
@@ -167,13 +171,16 @@
 
 /*
  * Maximum number of bytes to hold conversion from all channels
- * plus the timestamp
+ * without the timestamp.
  */
 #define AT91_BUFFER_MAX_BYTES ((AT91_SAMA5D2_SINGLE_CHAN_CNT +		\
-				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2 + 8)
+				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2)
 
 #define AT91_BUFFER_MAX_HWORDS (AT91_BUFFER_MAX_BYTES / 2)
 
+#define AT91_HWFIFO_MAX_SIZE_STR	"128"
+#define AT91_HWFIFO_MAX_SIZE		128
+
 #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
 	{								\
 		.type = IIO_VOLTAGE,					\
@@ -227,6 +234,28 @@ struct at91_adc_trigger {
 	unsigned int			edge_type;
 };
 
+/**
+ * at91_adc_dma - at91-sama5d2 dma information struct
+ * @dma_chan:		the dma channel acquired
+ * @rx_buf:		dma coherent allocated area
+ * @rx_dma_buf:		dma handler for the buffer
+ * @phys_addr:		physical address of the ADC base register
+ * @buf_idx:		index inside the dma buffer where reading was last done
+ * @rx_buf_sz:		size of buffer used by DMA operation
+ * @watermark:		number of conversions to copy before DMA triggers irq
+ * @dma_ts:		hold the start timestamp of dma operation
+ */
+struct at91_adc_dma {
+	struct dma_chan			*dma_chan;
+	u8				*rx_buf;
+	dma_addr_t			rx_dma_buf;
+	phys_addr_t			phys_addr;
+	int				buf_idx;
+	int				rx_buf_sz;
+	int				watermark;
+	s64				dma_ts;
+};
+
 struct at91_adc_state {
 	void __iomem			*base;
 	int				irq;
@@ -241,6 +270,7 @@ struct at91_adc_state {
 	u32				conversion_value;
 	struct at91_adc_soc_info	soc_info;
 	wait_queue_head_t		wq_data_available;
+	struct at91_adc_dma		dma_st;
 	u16				buffer[AT91_BUFFER_MAX_HWORDS];
 	/*
 	 * lock to prevent concurrent 'single conversion' requests through
@@ -312,11 +342,17 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
 		if (state) {
 			at91_adc_writel(st, AT91_SAMA5D2_CHER,
 					BIT(chan->channel));
-			at91_adc_writel(st, AT91_SAMA5D2_IER,
-					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 {
-			at91_adc_writel(st, AT91_SAMA5D2_IDR,
-					BIT(chan->channel));
+			/* 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));
 		}
@@ -330,6 +366,10 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
 	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
 	struct at91_adc_state *st = iio_priv(indio);
 
+	/* if we are using DMA, we must not reenable irq after each trigger */
+	if (st->dma_st.dma_chan)
+		return 0;
+
 	enable_irq(st->irq);
 
 	/* Needed to ACK the DRDY interruption */
@@ -341,6 +381,153 @@ static const struct iio_trigger_ops at91_adc_trigger_ops = {
 	.owner = THIS_MODULE,
 	.set_trigger_state = &at91_adc_configure_trigger,
 	.try_reenable = &at91_adc_reenable_trigger,
+	.validate_device = iio_trigger_validate_own_device,
+};
+
+static int at91_adc_dma_size_done(struct at91_adc_state *st)
+{
+	struct dma_tx_state state;
+	enum dma_status status;
+	int i, size;
+
+	status = dmaengine_tx_status(st->dma_st.dma_chan,
+				     st->dma_st.dma_chan->cookie,
+				     &state);
+	if (status != DMA_IN_PROGRESS)
+		return 0;
+
+	/* Transferred length is size in bytes from end of buffer */
+	i = st->dma_st.rx_buf_sz - state.residue;
+
+	/* Return available bytes */
+	if (i >= st->dma_st.buf_idx)
+		size = i - st->dma_st.buf_idx;
+	else
+		size = st->dma_st.rx_buf_sz + i - st->dma_st.buf_idx;
+	return size;
+}
+
+static void at91_dma_buffer_done(void *data)
+{
+	struct iio_dev *indio_dev = data;
+
+	iio_trigger_poll_chained(indio_dev->trig);
+}
+
+static int at91_adc_dma_start(struct iio_dev *indio_dev)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	struct dma_async_tx_descriptor *desc;
+	dma_cookie_t cookie;
+	int ret;
+	u8 bit;
+
+	if (!st->dma_st.dma_chan)
+		return 0;
+
+	/* we start a new DMA, so set buffer index to start */
+	st->dma_st.buf_idx = 0;
+
+	/*
+	 * compute buffer size w.r.t. watermark and enabled channels.
+	 * scan_bytes is aligned so we need an exact size for DMA
+	 */
+	st->dma_st.rx_buf_sz = 0;
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->num_channels) {
+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
+
+		st->dma_st.rx_buf_sz += chan->scan_type.storagebits / 8;
+	}
+	st->dma_st.rx_buf_sz *= st->dma_st.watermark;
+
+	/* Prepare a DMA cyclic transaction */
+	desc = dmaengine_prep_dma_cyclic(st->dma_st.dma_chan,
+					 st->dma_st.rx_dma_buf,
+					 st->dma_st.rx_buf_sz,
+					 st->dma_st.rx_buf_sz / 2,
+					 DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
+
+	if (!desc) {
+		dev_err(&indio_dev->dev, "cannot prepare DMA cyclic\n");
+		return -EBUSY;
+	}
+
+	desc->callback = at91_dma_buffer_done;
+	desc->callback_param = indio_dev;
+
+	cookie = dmaengine_submit(desc);
+	ret = dma_submit_error(cookie);
+	if (ret) {
+		dev_err(&indio_dev->dev, "cannot submit DMA cyclic\n");
+		dmaengine_terminate_async(st->dma_st.dma_chan);
+		return ret;
+	}
+
+	/* enable general overrun error signaling */
+	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_GOVRE);
+	/* Issue pending DMA requests */
+	dma_async_issue_pending(st->dma_st.dma_chan);
+
+	/* consider current time as DMA start time for timestamps */
+	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
+
+	dev_dbg(&indio_dev->dev, "DMA cyclic started\n");
+
+	return 0;
+}
+
+static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
+{
+	int ret;
+
+	ret = at91_adc_dma_start(indio_dev);
+	if (ret) {
+		dev_err(&indio_dev->dev, "buffer postenable failed\n");
+		return ret;
+	}
+
+	return iio_triggered_buffer_postenable(indio_dev);
+}
+
+static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	int ret;
+	u8 bit;
+
+	ret = iio_triggered_buffer_predisable(indio_dev);
+	if (ret < 0)
+		dev_err(&indio_dev->dev, "buffer predisable failed\n");
+
+	if (!st->dma_st.dma_chan)
+		return ret;
+
+	/* if we are using DMA we must clear registers and end DMA */
+	dmaengine_terminate_sync(st->dma_st.dma_chan);
+
+	/*
+	 * For each enabled channel we must read the last converted value
+	 * to clear EOC status and not get a possible interrupt later.
+	 * This value is being read by DMA from LCDR anyway
+	 */
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->num_channels) {
+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
+
+		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);
+	return ret;
+}
+
+static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
+	.postenable = &at91_adc_buffer_postenable,
+	.predisable = &at91_adc_buffer_predisable,
 };
 
 static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
@@ -379,24 +566,71 @@ static int at91_adc_trigger_init(struct iio_dev *indio)
 	return 0;
 }
 
-static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
+static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
+					   struct iio_poll_func *pf)
 {
-	struct iio_poll_func *pf = p;
-	struct iio_dev *indio = pf->indio_dev;
-	struct at91_adc_state *st = iio_priv(indio);
+	struct at91_adc_state *st = iio_priv(indio_dev);
 	int i = 0;
 	u8 bit;
 
-	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
-		struct iio_chan_spec const *chan = indio->channels + bit;
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->num_channels) {
+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
 
 		st->buffer[i] = at91_adc_readl(st, chan->address);
 		i++;
 	}
+	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer,
+					   pf->timestamp);
+}
+
+static void 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);
+	s64 ns = iio_get_time_ns(indio_dev);
+	s64 interval;
+	int sample_index = 0, sample_count, sample_size;
 
-	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
+	sample_size = div_s64(st->dma_st.rx_buf_sz, st->dma_st.watermark);
 
-	iio_trigger_notify_done(indio->trig);
+	sample_count = div_s64(transferred_len, sample_size);
+
+	/*
+	 * interval between samples is total time since last transfer handling
+	 * divided by the number of samples (total size divided by sample size)
+	 */
+	interval = div_s64((ns - st->dma_st.dma_ts), sample_count);
+
+	while (transferred_len >= sample_size) {
+		iio_push_to_buffers_with_timestamp(indio_dev,
+				(st->dma_st.rx_buf + st->dma_st.buf_idx),
+				(st->dma_st.dma_ts + interval * sample_index));
+		/* adjust remaining length */
+		transferred_len -= sample_size;
+		/* adjust buffer index */
+		st->dma_st.buf_idx += sample_size;
+		/* in case of reaching end of buffer, reset index */
+		if (st->dma_st.buf_idx >= st->dma_st.rx_buf_sz)
+			st->dma_st.buf_idx = 0;
+		sample_index++;
+	}
+	/* adjust saved time for next transfer handling */
+	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
+}
+
+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);
+
+	if (st->dma_st.dma_chan)
+		at91_adc_trigger_handler_dma(indio_dev);
+	else
+		at91_adc_trigger_handler_nodma(indio_dev, pf);
+
+	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
 }
@@ -405,7 +639,7 @@ static int at91_adc_buffer_init(struct iio_dev *indio)
 {
 	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
 			&iio_pollfunc_store_time,
-			&at91_adc_trigger_handler, NULL);
+			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
 }
 
 static unsigned at91_adc_startup_time(unsigned startup_time_min,
@@ -476,10 +710,17 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
 	if (!(status & imr))
 		return IRQ_NONE;
 
-	if (iio_buffer_enabled(indio)) {
+	/* if we reached this point, we cannot sample faster */
+	if (status & AT91_SAMA5D2_IER_GOVRE)
+		pr_alert_ratelimited("Conversion overrun detected\n");
+
+	if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
 		disable_irq_nosync(irq);
 		iio_trigger_poll(indio->trig);
-	} else {
+	} else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
+		disable_irq_nosync(irq);
+		WARN(true, "Unexpected irq occurred\n");
+	} else if (!iio_buffer_enabled(indio)) {
 		st->conversion_value = at91_adc_readl(st, st->chan->address);
 		st->conversion_done = true;
 		wake_up_interruptible(&st->wq_data_available);
@@ -571,9 +812,111 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
 	return 0;
 }
 
+static void at91_adc_dma_init(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	struct dma_slave_config config = {0};
+	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
+					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
+
+	if (st->dma_st.dma_chan)
+		return;
+
+	st->dma_st.dma_chan = dma_request_slave_channel(&pdev->dev, "rx");
+
+	if (!st->dma_st.dma_chan)  {
+		dev_info(&pdev->dev, "can't get DMA channel\n");
+		goto dma_exit;
+	}
+
+	st->dma_st.rx_buf = dma_alloc_coherent(st->dma_st.dma_chan->device->dev,
+					       pages * PAGE_SIZE,
+					       &st->dma_st.rx_dma_buf,
+					       GFP_KERNEL);
+	if (!st->dma_st.rx_buf) {
+		dev_info(&pdev->dev, "can't allocate coherent DMA area\n");
+		goto dma_chan_disable;
+	}
+
+	/* Configure DMA channel to read data register */
+	config.direction = DMA_DEV_TO_MEM;
+	config.src_addr = (phys_addr_t)(st->dma_st.phys_addr
+			  + AT91_SAMA5D2_LCDR);
+	config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+	config.src_maxburst = 1;
+	config.dst_maxburst = 1;
+
+	if (dmaengine_slave_config(st->dma_st.dma_chan, &config)) {
+		dev_info(&pdev->dev, "can't configure DMA slave\n");
+		goto dma_free_area;
+	}
+
+	dev_info(&pdev->dev, "using %s for rx DMA transfers\n",
+		 dma_chan_name(st->dma_st.dma_chan));
+
+	return;
+
+dma_free_area:
+	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
+			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
+dma_chan_disable:
+	dma_release_channel(st->dma_st.dma_chan);
+	st->dma_st.dma_chan = 0;
+dma_exit:
+	dev_info(&pdev->dev, "continuing without DMA support\n");
+}
+
+static void at91_adc_dma_disable(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
+					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
+
+	/* if we are not using DMA, just return */
+	if (!st->dma_st.dma_chan)
+		return;
+
+	/* wait for all transactions to be terminated first*/
+	dmaengine_terminate_sync(st->dma_st.dma_chan);
+
+	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
+			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
+	dma_release_channel(st->dma_st.dma_chan);
+	st->dma_st.dma_chan = 0;
+
+	dev_info(&pdev->dev, "continuing without DMA support\n");
+}
+
+static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	if (val > AT91_HWFIFO_MAX_SIZE)
+		return -EINVAL;
+
+	dev_dbg(&indio_dev->dev, "new watermark is %u\n", val);
+	st->dma_st.watermark = val;
+
+	/*
+	 * The logic here is: if we have watermark 1, it means we do
+	 * each conversion with it's own IRQ, thus we don't need DMA.
+	 * If the watermark is higher, we do DMA to do all the transfers in bulk
+	 */
+
+	if (val == 1)
+		at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
+	else if (val > 1)
+		at91_adc_dma_init(to_platform_device(&indio_dev->dev));
+
+	return 0;
+}
+
 static const struct iio_info at91_adc_info = {
 	.read_raw = &at91_adc_read_raw,
 	.write_raw = &at91_adc_write_raw,
+	.hwfifo_set_watermark = &at91_adc_set_watermark,
 	.driver_module = THIS_MODULE,
 };
 
@@ -591,6 +934,42 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
 	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
 }
 
+static ssize_t at91_adc_get_fifo_state(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev =
+			platform_get_drvdata(to_platform_device(dev));
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", !!st->dma_st.dma_chan);
+}
+
+static ssize_t at91_adc_get_watermark(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev =
+			platform_get_drvdata(to_platform_device(dev));
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", st->dma_st.watermark);
+}
+
+static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
+		       at91_adc_get_fifo_state, NULL, 0);
+static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
+		       at91_adc_get_watermark, NULL, 0);
+
+static IIO_CONST_ATTR(hwfifo_watermark_min, "2");
+static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
+
+static const struct attribute *at91_adc_fifo_attributes[] = {
+	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
+	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
+	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
+	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	NULL,
+};
+
 static int at91_adc_probe(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev;
@@ -666,6 +1045,9 @@ static int at91_adc_probe(struct platform_device *pdev)
 	if (!res)
 		return -EINVAL;
 
+	/* if we plan to use DMA, we need the physical address of the regs */
+	st->dma_st.phys_addr = res->start;
+
 	st->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(st->base))
 		return PTR_ERR(st->base);
@@ -729,18 +1111,30 @@ static int at91_adc_probe(struct platform_device *pdev)
 		goto per_clk_disable_unprepare;
 	}
 
+	if (dma_coerce_mask_and_coherent(&indio_dev->dev, DMA_BIT_MASK(32)))
+		dev_info(&pdev->dev, "cannot set DMA mask to 32-bit\n");
+
+	/* the iio buffer has initially a length of 2 and a watermark of 1 */
+	st->dma_st.watermark = 1;
+
+	iio_buffer_set_attrs(indio_dev->buffer, at91_adc_fifo_attributes);
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
-		goto per_clk_disable_unprepare;
+		goto dma_disable;
 
 	dev_info(&pdev->dev, "setting up trigger as %s\n",
 		 st->selected_trig->name);
 
+	dev_info(&pdev->dev, "continuing without DMA support\n");
+
 	dev_info(&pdev->dev, "version: %x\n",
 		 readl_relaxed(st->base + AT91_SAMA5D2_VERSION));
 
 	return 0;
 
+dma_disable:
+	at91_adc_dma_disable(pdev);
 per_clk_disable_unprepare:
 	clk_disable_unprepare(st->per_clk);
 vref_disable:
@@ -757,6 +1151,8 @@ static int at91_adc_remove(struct platform_device *pdev)
 
 	iio_device_unregister(indio_dev);
 
+	at91_adc_dma_disable(pdev);
+
 	clk_disable_unprepare(st->per_clk);
 
 	regulator_disable(st->vref);
-- 
2.7.4

[PATCH v2 4/5] iio: adc: at91-sama5d2_adc: ack DRDY irq in direct mode

[Eugen Hristev]

Need to acknowledge DRDY irq in direct mode/ software
triggered mode. Otherwise, on the next conversion, overrun
flag will be raised, which is not a correct state.
This doesn't affect the functionality, but will generate
possible incorrect overrun reports.

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

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 7a9305a..7e51ecb 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -773,6 +773,9 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
 		at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
 		at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
 
+		/* Needed to ACK the DRDY interruption */
+		at91_adc_readl(st, AT91_SAMA5D2_LCDR);
+
 		mutex_unlock(&st->lock);
 
 		iio_device_release_direct_mode(indio_dev);
-- 
2.7.4

[PATCH v2 4/5] iio: adc: at91-sama5d2_adc: ack DRDY irq in direct mode

[Eugen Hristev]

Need to acknowledge DRDY irq in direct mode/ software
triggered mode. Otherwise, on the next conversion, overrun
flag will be raised, which is not a correct state.
This doesn't affect the functionality, but will generate
possible incorrect overrun reports.

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

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 7a9305a..7e51ecb 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -773,6 +773,9 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
 		at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
 		at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
 
+		/* Needed to ACK the DRDY interruption */
+		at91_adc_readl(st, AT91_SAMA5D2_LCDR);
+
 		mutex_unlock(&st->lock);
 
 		iio_device_release_direct_mode(indio_dev);
-- 
2.7.4

[PATCH v2 4/5] iio: adc: at91-sama5d2_adc: ack DRDY irq in direct mode

[Eugen Hristev]

Need to acknowledge DRDY irq in direct mode/ software
triggered mode. Otherwise, on the next conversion, overrun
flag will be raised, which is not a correct state.
This doesn't affect the functionality, but will generate
possible incorrect overrun reports.

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

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 7a9305a..7e51ecb 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -773,6 +773,9 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
 		at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
 		at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
 
+		/* Needed to ACK the DRDY interruption */
+		at91_adc_readl(st, AT91_SAMA5D2_LCDR);
+
 		mutex_unlock(&st->lock);
 
 		iio_device_release_direct_mode(indio_dev);
-- 
2.7.4

[PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

[Eugen Hristev]

Change driver settings to use maximum sample rate clock.
This is useful to achieve best possible sampling rate
if we use DMA.

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

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 7e51ecb..b76f88f 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -934,7 +934,7 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
 	at91_adc_writel(st, AT91_SAMA5D2_MR,
 			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
 
-	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
+	at91_adc_setup_samp_freq(st, st->soc_info.max_sample_rate);
 }
 
 static ssize_t at91_adc_get_fifo_state(struct device *dev,
-- 
2.7.4

[PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

[Eugen Hristev]

Change driver settings to use maximum sample rate clock.
This is useful to achieve best possible sampling rate
if we use DMA.

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

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 7e51ecb..b76f88f 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -934,7 +934,7 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
 	at91_adc_writel(st, AT91_SAMA5D2_MR,
 			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
 
-	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
+	at91_adc_setup_samp_freq(st, st->soc_info.max_sample_rate);
 }
 
 static ssize_t at91_adc_get_fifo_state(struct device *dev,
-- 
2.7.4

[PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

[Eugen Hristev]

Change driver settings to use maximum sample rate clock.
This is useful to achieve best possible sampling rate
if we use DMA.

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

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 7e51ecb..b76f88f 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -934,7 +934,7 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
 	at91_adc_writel(st, AT91_SAMA5D2_MR,
 			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
 
-	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
+	at91_adc_setup_samp_freq(st, st->soc_info.max_sample_rate);
 }
 
 static ssize_t at91_adc_get_fifo_state(struct device *dev,
-- 
2.7.4

Re: [PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Rob Herring]

On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
> Added property for DMA configuration of the device.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> index 552e7a8..5f94d479 100644
> --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> @@ -17,6 +17,11 @@ Required properties:
>    This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
>    IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
>  
> +Optional properties:
> +  - dmas: Phandle to dma channel for the ADC.
> +  See ../../dma/dma.txt for details.
> +  - dma-names: Must be "rx" when dmas property is being used.

-names is pointless when there is only one.

Otherwise,

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Rob Herring]

On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
> Added property for DMA configuration of the device.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> index 552e7a8..5f94d479 100644
> --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> @@ -17,6 +17,11 @@ Required properties:
>    This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
>    IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
>  
> +Optional properties:
> +  - dmas: Phandle to dma channel for the ADC.
> +  See ../../dma/dma.txt for details.
> +  - dma-names: Must be "rx" when dmas property is being used.

-names is pointless when there is only one.

Otherwise,

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 3/5] iio: adc: at91-sama5d2_adc: add support for DMA

[Jonathan Cameron]

On Wed, 11 Oct 2017 09:35:30 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> Added support for DMA transfers. The implementation uses the user watermark
> to decide whether DMA will be used or not. For watermark 1, DMA will not be
> used. If watermark is bigger, DMA will be used.
> Sysfs attributes are created to indicate whether the DMA is used,
> with hwfifo_enabled, and the current DMA watermark is readable
> in hwfifo_watermark. Minimum and maximum values are in hwfifo_watermark_min
> and hwfifo_watermark_max.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

You have shrunk the size of the buffer passed to
iio_push_to_buffers_with_timestamp in the non DMA case such that it is
no longer big enough to take the timestamp (the data is used in place
in that call so we need the extra space).

Other than that (and what I think is a missleading dev_info) I think
this is looking pretty good.

Lars if you have time to look at this one that would be great as 
your knowledge of the dmaengine stuff is better than mine.

Jonathan

> ---
>   Changes in v2:
>  - No longer add last timestamp to all samples. Now, compute an interval
> between samples w.r.t. start and end time of the transfer and number
> of samples. Then distribute them each in the time interval.
>  - Add warning for conversion overrun. This helps user identify cases
> when the watermark needs adjustment : the software is too slow in reading
> data from the ADC.
>  - Protection around watermark is not needed, changing of the watermark
> cannot be done while the buffer is enabled. When buffer is disabled, all
> DMA resources are freed anyway.
>  - Added validation on trigger to be used by own device
>  - Best sample rate I could obtain using the low frequency clock was about
> 4k samples/second, with a watermark of 100. To get up to 50k samples/second
> the ADC frequency must be increased to max.
>  - Fixed computation of DMA buffer size
>  - Addressed other comments from mailing list review. Feedback is appreciated
> 
>  drivers/iio/adc/Kconfig            |   1 +
>  drivers/iio/adc/at91-sama5d2_adc.c | 432 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 415 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 5762565..9ef288f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -158,6 +158,7 @@ config AT91_SAMA5D2_ADC
>  	tristate "Atmel AT91 SAMA5D2 ADC"
>  	depends on ARCH_AT91 || COMPILE_TEST
>  	depends on HAS_IOMEM
> +	depends on HAS_DMA
>  	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Atmel SAMA5D2 ADC which is
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index bc5b38e..7a9305a 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -16,6 +16,8 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -100,6 +102,8 @@
>  #define AT91_SAMA5D2_LCDR	0x20
>  /* Interrupt Enable Register */
>  #define AT91_SAMA5D2_IER	0x24
> +/* Interrupt Enable Register - general overrun error */
> +#define AT91_SAMA5D2_IER_GOVRE BIT(25)
>  /* Interrupt Disable Register */
>  #define AT91_SAMA5D2_IDR	0x28
>  /* Interrupt Mask Register */
> @@ -167,13 +171,16 @@
>  
>  /*
>   * Maximum number of bytes to hold conversion from all channels
> - * plus the timestamp
> + * without the timestamp.
>   */
>  #define AT91_BUFFER_MAX_BYTES ((AT91_SAMA5D2_SINGLE_CHAN_CNT +		\
> -				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2 + 8)
> +				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2)
>  
>  #define AT91_BUFFER_MAX_HWORDS (AT91_BUFFER_MAX_BYTES / 2)
>  
> +#define AT91_HWFIFO_MAX_SIZE_STR	"128"
> +#define AT91_HWFIFO_MAX_SIZE		128
> +
>  #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
>  	{								\
>  		.type = IIO_VOLTAGE,					\
> @@ -227,6 +234,28 @@ struct at91_adc_trigger {
>  	unsigned int			edge_type;
>  };
>  
> +/**
> + * at91_adc_dma - at91-sama5d2 dma information struct
> + * @dma_chan:		the dma channel acquired
> + * @rx_buf:		dma coherent allocated area
> + * @rx_dma_buf:		dma handler for the buffer
> + * @phys_addr:		physical address of the ADC base register
> + * @buf_idx:		index inside the dma buffer where reading was last done
> + * @rx_buf_sz:		size of buffer used by DMA operation
> + * @watermark:		number of conversions to copy before DMA triggers irq
> + * @dma_ts:		hold the start timestamp of dma operation
> + */
> +struct at91_adc_dma {
> +	struct dma_chan			*dma_chan;
> +	u8				*rx_buf;
> +	dma_addr_t			rx_dma_buf;
> +	phys_addr_t			phys_addr;
> +	int				buf_idx;
> +	int				rx_buf_sz;
> +	int				watermark;
> +	s64				dma_ts;
> +};
> +
>  struct at91_adc_state {
>  	void __iomem			*base;
>  	int				irq;
> @@ -241,6 +270,7 @@ struct at91_adc_state {
>  	u32				conversion_value;
>  	struct at91_adc_soc_info	soc_info;
>  	wait_queue_head_t		wq_data_available;
> +	struct at91_adc_dma		dma_st;
>  	u16				buffer[AT91_BUFFER_MAX_HWORDS];

I think this is no longer big enough...  For the normal triggered case
you still need enough room for the timestamp to be inserted in
iio_push_to_buffers_with_timestamp.

>  	/*
>  	 * lock to prevent concurrent 'single conversion' requests through
> @@ -312,11 +342,17 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>  		if (state) {
>  			at91_adc_writel(st, AT91_SAMA5D2_CHER,
>  					BIT(chan->channel));
> -			at91_adc_writel(st, AT91_SAMA5D2_IER,
> -					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 {
> -			at91_adc_writel(st, AT91_SAMA5D2_IDR,
> -					BIT(chan->channel));
> +			/* 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));
>  		}
> @@ -330,6 +366,10 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
>  	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>  	struct at91_adc_state *st = iio_priv(indio);
>  
> +	/* if we are using DMA, we must not reenable irq after each trigger */
> +	if (st->dma_st.dma_chan)
> +		return 0;
> +
>  	enable_irq(st->irq);
>  
>  	/* Needed to ACK the DRDY interruption */
> @@ -341,6 +381,153 @@ static const struct iio_trigger_ops at91_adc_trigger_ops = {
>  	.owner = THIS_MODULE,
>  	.set_trigger_state = &at91_adc_configure_trigger,
>  	.try_reenable = &at91_adc_reenable_trigger,
> +	.validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static int at91_adc_dma_size_done(struct at91_adc_state *st)
> +{
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +	int i, size;
> +
> +	status = dmaengine_tx_status(st->dma_st.dma_chan,
> +				     st->dma_st.dma_chan->cookie,
> +				     &state);
> +	if (status != DMA_IN_PROGRESS)
> +		return 0;
> +
> +	/* Transferred length is size in bytes from end of buffer */
> +	i = st->dma_st.rx_buf_sz - state.residue;
> +
> +	/* Return available bytes */
> +	if (i >= st->dma_st.buf_idx)
> +		size = i - st->dma_st.buf_idx;
> +	else
> +		size = st->dma_st.rx_buf_sz + i - st->dma_st.buf_idx;
> +	return size;
> +}
> +
> +static void at91_dma_buffer_done(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +
> +	iio_trigger_poll_chained(indio_dev->trig);
> +}
> +
> +static int at91_adc_dma_start(struct iio_dev *indio_dev)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	struct dma_async_tx_descriptor *desc;
> +	dma_cookie_t cookie;
> +	int ret;
> +	u8 bit;
> +
> +	if (!st->dma_st.dma_chan)
> +		return 0;
> +
> +	/* we start a new DMA, so set buffer index to start */
> +	st->dma_st.buf_idx = 0;
> +
> +	/*
> +	 * compute buffer size w.r.t. watermark and enabled channels.
> +	 * scan_bytes is aligned so we need an exact size for DMA
> +	 */
> +	st->dma_st.rx_buf_sz = 0;
> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->num_channels) {
> +		struct iio_chan_spec const *chan = indio_dev->channels + bit;
> +
> +		st->dma_st.rx_buf_sz += chan->scan_type.storagebits / 8;
> +	}
> +	st->dma_st.rx_buf_sz *= st->dma_st.watermark;
> +
> +	/* Prepare a DMA cyclic transaction */
> +	desc = dmaengine_prep_dma_cyclic(st->dma_st.dma_chan,
> +					 st->dma_st.rx_dma_buf,
> +					 st->dma_st.rx_buf_sz,
> +					 st->dma_st.rx_buf_sz / 2,
> +					 DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
> +
> +	if (!desc) {
> +		dev_err(&indio_dev->dev, "cannot prepare DMA cyclic\n");
> +		return -EBUSY;
> +	}
> +
> +	desc->callback = at91_dma_buffer_done;
> +	desc->callback_param = indio_dev;
> +
> +	cookie = dmaengine_submit(desc);
> +	ret = dma_submit_error(cookie);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "cannot submit DMA cyclic\n");
> +		dmaengine_terminate_async(st->dma_st.dma_chan);
> +		return ret;
> +	}
> +
> +	/* enable general overrun error signaling */
> +	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_GOVRE);
> +	/* Issue pending DMA requests */
> +	dma_async_issue_pending(st->dma_st.dma_chan);
> +
> +	/* consider current time as DMA start time for timestamps */
> +	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
> +
> +	dev_dbg(&indio_dev->dev, "DMA cyclic started\n");
> +
> +	return 0;
> +}
> +
> +static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	int ret;
> +
> +	ret = at91_adc_dma_start(indio_dev);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "buffer postenable failed\n");
> +		return ret;
> +	}
> +
> +	return iio_triggered_buffer_postenable(indio_dev);
> +}
> +
> +static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret;
> +	u8 bit;
> +
> +	ret = iio_triggered_buffer_predisable(indio_dev);
> +	if (ret < 0)
> +		dev_err(&indio_dev->dev, "buffer predisable failed\n");
> +
> +	if (!st->dma_st.dma_chan)
> +		return ret;
> +
> +	/* if we are using DMA we must clear registers and end DMA */
> +	dmaengine_terminate_sync(st->dma_st.dma_chan);
> +
> +	/*
> +	 * For each enabled channel we must read the last converted value
> +	 * to clear EOC status and not get a possible interrupt later.
> +	 * This value is being read by DMA from LCDR anyway
> +	 */
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->num_channels) {
> +		struct iio_chan_spec const *chan = indio_dev->channels + bit;
> +
> +		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);
> +	return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
> +	.postenable = &at91_adc_buffer_postenable,
> +	.predisable = &at91_adc_buffer_predisable,
>  };
>  
>  static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
> @@ -379,24 +566,71 @@ static int at91_adc_trigger_init(struct iio_dev *indio)
>  	return 0;
>  }
>  
> -static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> +static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
> +					   struct iio_poll_func *pf)
>  {
> -	struct iio_poll_func *pf = p;
> -	struct iio_dev *indio = pf->indio_dev;
> -	struct at91_adc_state *st = iio_priv(indio);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
>  	int i = 0;
>  	u8 bit;
>  
> -	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> -		struct iio_chan_spec const *chan = indio->channels + bit;
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->num_channels) {
> +		struct iio_chan_spec const *chan = indio_dev->channels + bit;
>  
>  		st->buffer[i] = at91_adc_readl(st, chan->address);
>  		i++;
>  	}
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer,
> +					   pf->timestamp);
> +}
> +
> +static void 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);
> +	s64 ns = iio_get_time_ns(indio_dev);
> +	s64 interval;
> +	int sample_index = 0, sample_count, sample_size;
>  
> -	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
> +	sample_size = div_s64(st->dma_st.rx_buf_sz, st->dma_st.watermark);
>  
> -	iio_trigger_notify_done(indio->trig);
> +	sample_count = div_s64(transferred_len, sample_size);
> +
> +	/*
> +	 * interval between samples is total time since last transfer handling
> +	 * divided by the number of samples (total size divided by sample size)
> +	 */
> +	interval = div_s64((ns - st->dma_st.dma_ts), sample_count);
> +
> +	while (transferred_len >= sample_size) {
> +		iio_push_to_buffers_with_timestamp(indio_dev,
> +				(st->dma_st.rx_buf + st->dma_st.buf_idx),
> +				(st->dma_st.dma_ts + interval * sample_index));
> +		/* adjust remaining length */
> +		transferred_len -= sample_size;
> +		/* adjust buffer index */
> +		st->dma_st.buf_idx += sample_size;
> +		/* in case of reaching end of buffer, reset index */
> +		if (st->dma_st.buf_idx >= st->dma_st.rx_buf_sz)
> +			st->dma_st.buf_idx = 0;
> +		sample_index++;
> +	}
> +	/* adjust saved time for next transfer handling */
> +	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
> +}
> +
> +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);
> +
> +	if (st->dma_st.dma_chan)
> +		at91_adc_trigger_handler_dma(indio_dev);
> +	else
> +		at91_adc_trigger_handler_nodma(indio_dev, pf);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -405,7 +639,7 @@ static int at91_adc_buffer_init(struct iio_dev *indio)
>  {
>  	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
>  			&iio_pollfunc_store_time,
> -			&at91_adc_trigger_handler, NULL);
> +			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
>  }
>  
>  static unsigned at91_adc_startup_time(unsigned startup_time_min,
> @@ -476,10 +710,17 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>  	if (!(status & imr))
>  		return IRQ_NONE;
>  
> -	if (iio_buffer_enabled(indio)) {
> +	/* if we reached this point, we cannot sample faster */
> +	if (status & AT91_SAMA5D2_IER_GOVRE)
> +		pr_alert_ratelimited("Conversion overrun detected\n");
> +
> +	if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
>  		disable_irq_nosync(irq);
>  		iio_trigger_poll(indio->trig);
> -	} else {
> +	} else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
> +		disable_irq_nosync(irq);
> +		WARN(true, "Unexpected irq occurred\n");
> +	} else if (!iio_buffer_enabled(indio)) {
>  		st->conversion_value = at91_adc_readl(st, st->chan->address);
>  		st->conversion_done = true;
>  		wake_up_interruptible(&st->wq_data_available);
> @@ -571,9 +812,111 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> +static void at91_adc_dma_init(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	struct dma_slave_config config = {0};
> +	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
> +					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
> +
> +	if (st->dma_st.dma_chan)
> +		return;
> +
> +	st->dma_st.dma_chan = dma_request_slave_channel(&pdev->dev, "rx");
> +
> +	if (!st->dma_st.dma_chan)  {
> +		dev_info(&pdev->dev, "can't get DMA channel\n");
> +		goto dma_exit;
> +	}
> +
> +	st->dma_st.rx_buf = dma_alloc_coherent(st->dma_st.dma_chan->device->dev,
> +					       pages * PAGE_SIZE,
> +					       &st->dma_st.rx_dma_buf,
> +					       GFP_KERNEL);
> +	if (!st->dma_st.rx_buf) {
> +		dev_info(&pdev->dev, "can't allocate coherent DMA area\n");
> +		goto dma_chan_disable;
> +	}
> +
> +	/* Configure DMA channel to read data register */
> +	config.direction = DMA_DEV_TO_MEM;
> +	config.src_addr = (phys_addr_t)(st->dma_st.phys_addr
> +			  + AT91_SAMA5D2_LCDR);
> +	config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> +	config.src_maxburst = 1;
> +	config.dst_maxburst = 1;
> +
> +	if (dmaengine_slave_config(st->dma_st.dma_chan, &config)) {
> +		dev_info(&pdev->dev, "can't configure DMA slave\n");
> +		goto dma_free_area;
> +	}
> +
> +	dev_info(&pdev->dev, "using %s for rx DMA transfers\n",
> +		 dma_chan_name(st->dma_st.dma_chan));
> +
> +	return;
> +
> +dma_free_area:
> +	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
> +			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
> +dma_chan_disable:
> +	dma_release_channel(st->dma_st.dma_chan);
> +	st->dma_st.dma_chan = 0;
> +dma_exit:
> +	dev_info(&pdev->dev, "continuing without DMA support\n");
> +}
> +
> +static void at91_adc_dma_disable(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
> +					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
> +
> +	/* if we are not using DMA, just return */
> +	if (!st->dma_st.dma_chan)
> +		return;
> +
> +	/* wait for all transactions to be terminated first*/
> +	dmaengine_terminate_sync(st->dma_st.dma_chan);
> +
> +	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
> +			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
> +	dma_release_channel(st->dma_st.dma_chan);
> +	st->dma_st.dma_chan = 0;
> +
> +	dev_info(&pdev->dev, "continuing without DMA support\n");
> +}
> +
> +static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	if (val > AT91_HWFIFO_MAX_SIZE)
> +		return -EINVAL;
> +
> +	dev_dbg(&indio_dev->dev, "new watermark is %u\n", val);
> +	st->dma_st.watermark = val;
> +
> +	/*
> +	 * The logic here is: if we have watermark 1, it means we do
> +	 * each conversion with it's own IRQ, thus we don't need DMA.
> +	 * If the watermark is higher, we do DMA to do all the transfers in bulk
> +	 */
> +

> +	if (val == 1)
> +		at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
> +	else if (val > 1)
> +		at91_adc_dma_init(to_platform_device(&indio_dev->dev));
> +
> +	return 0;
> +}
> +
>  static const struct iio_info at91_adc_info = {
>  	.read_raw = &at91_adc_read_raw,
>  	.write_raw = &at91_adc_write_raw,
> +	.hwfifo_set_watermark = &at91_adc_set_watermark,
>  	.driver_module = THIS_MODULE,
>  };
>  
> @@ -591,6 +934,42 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
>  	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
>  }
>  
> +static ssize_t at91_adc_get_fifo_state(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev =
> +			platform_get_drvdata(to_platform_device(dev));
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", !!st->dma_st.dma_chan);
> +}
> +
> +static ssize_t at91_adc_get_watermark(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev =
> +			platform_get_drvdata(to_platform_device(dev));
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", st->dma_st.watermark);
> +}
> +
> +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
> +		       at91_adc_get_fifo_state, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
> +		       at91_adc_get_watermark, NULL, 0);
> +
> +static IIO_CONST_ATTR(hwfifo_watermark_min, "2");
> +static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
> +
> +static const struct attribute *at91_adc_fifo_attributes[] = {
> +	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
> +	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> +	NULL,
> +};
> +
>  static int at91_adc_probe(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev;
> @@ -666,6 +1045,9 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	if (!res)
>  		return -EINVAL;
>  
> +	/* if we plan to use DMA, we need the physical address of the regs */
> +	st->dma_st.phys_addr = res->start;
> +
>  	st->base = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(st->base))
>  		return PTR_ERR(st->base);
> @@ -729,18 +1111,30 @@ static int at91_adc_probe(struct platform_device *pdev)
>  		goto per_clk_disable_unprepare;
>  	}
>  
> +	if (dma_coerce_mask_and_coherent(&indio_dev->dev, DMA_BIT_MASK(32)))
> +		dev_info(&pdev->dev, "cannot set DMA mask to 32-bit\n");
> +
> +	/* the iio buffer has initially a length of 2 and a watermark of 1 */
> +	st->dma_st.watermark = 1;
> +
> +	iio_buffer_set_attrs(indio_dev->buffer, at91_adc_fifo_attributes);
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
> -		goto per_clk_disable_unprepare;
> +		goto dma_disable;
>  
>  	dev_info(&pdev->dev, "setting up trigger as %s\n",
>  		 st->selected_trig->name);
>  
> +	dev_info(&pdev->dev, "continuing without DMA support\n");
> +
This feels a little misleading.  I think the point here is that dma
is initially disabled because fo the default watermark.

If it makes sense to put any info statement here at all it should
be more specific rather than implying we are running entirely
without DMA support.

>  	dev_info(&pdev->dev, "version: %x\n",
>  		 readl_relaxed(st->base + AT91_SAMA5D2_VERSION));
>  
>  	return 0;
>  
> +dma_disable:
> +	at91_adc_dma_disable(pdev);
>  per_clk_disable_unprepare:
>  	clk_disable_unprepare(st->per_clk);
>  vref_disable:
> @@ -757,6 +1151,8 @@ static int at91_adc_remove(struct platform_device *pdev)
>  
>  	iio_device_unregister(indio_dev);
>  
> +	at91_adc_dma_disable(pdev);
> +
>  	clk_disable_unprepare(st->per_clk);
>  
>  	regulator_disable(st->vref);

Re: [PATCH v2 3/5] iio: adc: at91-sama5d2_adc: add support for DMA

[Jonathan Cameron]

On Wed, 11 Oct 2017 09:35:30 +0300
Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:

> Added support for DMA transfers. The implementation uses the user watermark
> to decide whether DMA will be used or not. For watermark 1, DMA will not be
> used. If watermark is bigger, DMA will be used.
> Sysfs attributes are created to indicate whether the DMA is used,
> with hwfifo_enabled, and the current DMA watermark is readable
> in hwfifo_watermark. Minimum and maximum values are in hwfifo_watermark_min
> and hwfifo_watermark_max.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>

You have shrunk the size of the buffer passed to
iio_push_to_buffers_with_timestamp in the non DMA case such that it is
no longer big enough to take the timestamp (the data is used in place
in that call so we need the extra space).

Other than that (and what I think is a missleading dev_info) I think
this is looking pretty good.

Lars if you have time to look at this one that would be great as 
your knowledge of the dmaengine stuff is better than mine.

Jonathan

> ---
>   Changes in v2:
>  - No longer add last timestamp to all samples. Now, compute an interval
> between samples w.r.t. start and end time of the transfer and number
> of samples. Then distribute them each in the time interval.
>  - Add warning for conversion overrun. This helps user identify cases
> when the watermark needs adjustment : the software is too slow in reading
> data from the ADC.
>  - Protection around watermark is not needed, changing of the watermark
> cannot be done while the buffer is enabled. When buffer is disabled, all
> DMA resources are freed anyway.
>  - Added validation on trigger to be used by own device
>  - Best sample rate I could obtain using the low frequency clock was about
> 4k samples/second, with a watermark of 100. To get up to 50k samples/second
> the ADC frequency must be increased to max.
>  - Fixed computation of DMA buffer size
>  - Addressed other comments from mailing list review. Feedback is appreciated
> 
>  drivers/iio/adc/Kconfig            |   1 +
>  drivers/iio/adc/at91-sama5d2_adc.c | 432 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 415 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 5762565..9ef288f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -158,6 +158,7 @@ config AT91_SAMA5D2_ADC
>  	tristate "Atmel AT91 SAMA5D2 ADC"
>  	depends on ARCH_AT91 || COMPILE_TEST
>  	depends on HAS_IOMEM
> +	depends on HAS_DMA
>  	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Atmel SAMA5D2 ADC which is
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index bc5b38e..7a9305a 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -16,6 +16,8 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -100,6 +102,8 @@
>  #define AT91_SAMA5D2_LCDR	0x20
>  /* Interrupt Enable Register */
>  #define AT91_SAMA5D2_IER	0x24
> +/* Interrupt Enable Register - general overrun error */
> +#define AT91_SAMA5D2_IER_GOVRE BIT(25)
>  /* Interrupt Disable Register */
>  #define AT91_SAMA5D2_IDR	0x28
>  /* Interrupt Mask Register */
> @@ -167,13 +171,16 @@
>  
>  /*
>   * Maximum number of bytes to hold conversion from all channels
> - * plus the timestamp
> + * without the timestamp.
>   */
>  #define AT91_BUFFER_MAX_BYTES ((AT91_SAMA5D2_SINGLE_CHAN_CNT +		\
> -				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2 + 8)
> +				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2)
>  
>  #define AT91_BUFFER_MAX_HWORDS (AT91_BUFFER_MAX_BYTES / 2)
>  
> +#define AT91_HWFIFO_MAX_SIZE_STR	"128"
> +#define AT91_HWFIFO_MAX_SIZE		128
> +
>  #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
>  	{								\
>  		.type = IIO_VOLTAGE,					\
> @@ -227,6 +234,28 @@ struct at91_adc_trigger {
>  	unsigned int			edge_type;
>  };
>  
> +/**
> + * at91_adc_dma - at91-sama5d2 dma information struct
> + * @dma_chan:		the dma channel acquired
> + * @rx_buf:		dma coherent allocated area
> + * @rx_dma_buf:		dma handler for the buffer
> + * @phys_addr:		physical address of the ADC base register
> + * @buf_idx:		index inside the dma buffer where reading was last done
> + * @rx_buf_sz:		size of buffer used by DMA operation
> + * @watermark:		number of conversions to copy before DMA triggers irq
> + * @dma_ts:		hold the start timestamp of dma operation
> + */
> +struct at91_adc_dma {
> +	struct dma_chan			*dma_chan;
> +	u8				*rx_buf;
> +	dma_addr_t			rx_dma_buf;
> +	phys_addr_t			phys_addr;
> +	int				buf_idx;
> +	int				rx_buf_sz;
> +	int				watermark;
> +	s64				dma_ts;
> +};
> +
>  struct at91_adc_state {
>  	void __iomem			*base;
>  	int				irq;
> @@ -241,6 +270,7 @@ struct at91_adc_state {
>  	u32				conversion_value;
>  	struct at91_adc_soc_info	soc_info;
>  	wait_queue_head_t		wq_data_available;
> +	struct at91_adc_dma		dma_st;
>  	u16				buffer[AT91_BUFFER_MAX_HWORDS];

I think this is no longer big enough...  For the normal triggered case
you still need enough room for the timestamp to be inserted in
iio_push_to_buffers_with_timestamp.

>  	/*
>  	 * lock to prevent concurrent 'single conversion' requests through
> @@ -312,11 +342,17 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>  		if (state) {
>  			at91_adc_writel(st, AT91_SAMA5D2_CHER,
>  					BIT(chan->channel));
> -			at91_adc_writel(st, AT91_SAMA5D2_IER,
> -					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 {
> -			at91_adc_writel(st, AT91_SAMA5D2_IDR,
> -					BIT(chan->channel));
> +			/* 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));
>  		}
> @@ -330,6 +366,10 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
>  	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>  	struct at91_adc_state *st = iio_priv(indio);
>  
> +	/* if we are using DMA, we must not reenable irq after each trigger */
> +	if (st->dma_st.dma_chan)
> +		return 0;
> +
>  	enable_irq(st->irq);
>  
>  	/* Needed to ACK the DRDY interruption */
> @@ -341,6 +381,153 @@ static const struct iio_trigger_ops at91_adc_trigger_ops = {
>  	.owner = THIS_MODULE,
>  	.set_trigger_state = &at91_adc_configure_trigger,
>  	.try_reenable = &at91_adc_reenable_trigger,
> +	.validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static int at91_adc_dma_size_done(struct at91_adc_state *st)
> +{
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +	int i, size;
> +
> +	status = dmaengine_tx_status(st->dma_st.dma_chan,
> +				     st->dma_st.dma_chan->cookie,
> +				     &state);
> +	if (status != DMA_IN_PROGRESS)
> +		return 0;
> +
> +	/* Transferred length is size in bytes from end of buffer */
> +	i = st->dma_st.rx_buf_sz - state.residue;
> +
> +	/* Return available bytes */
> +	if (i >= st->dma_st.buf_idx)
> +		size = i - st->dma_st.buf_idx;
> +	else
> +		size = st->dma_st.rx_buf_sz + i - st->dma_st.buf_idx;
> +	return size;
> +}
> +
> +static void at91_dma_buffer_done(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +
> +	iio_trigger_poll_chained(indio_dev->trig);
> +}
> +
> +static int at91_adc_dma_start(struct iio_dev *indio_dev)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	struct dma_async_tx_descriptor *desc;
> +	dma_cookie_t cookie;
> +	int ret;
> +	u8 bit;
> +
> +	if (!st->dma_st.dma_chan)
> +		return 0;
> +
> +	/* we start a new DMA, so set buffer index to start */
> +	st->dma_st.buf_idx = 0;
> +
> +	/*
> +	 * compute buffer size w.r.t. watermark and enabled channels.
> +	 * scan_bytes is aligned so we need an exact size for DMA
> +	 */
> +	st->dma_st.rx_buf_sz = 0;
> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->num_channels) {
> +		struct iio_chan_spec const *chan = indio_dev->channels + bit;
> +
> +		st->dma_st.rx_buf_sz += chan->scan_type.storagebits / 8;
> +	}
> +	st->dma_st.rx_buf_sz *= st->dma_st.watermark;
> +
> +	/* Prepare a DMA cyclic transaction */
> +	desc = dmaengine_prep_dma_cyclic(st->dma_st.dma_chan,
> +					 st->dma_st.rx_dma_buf,
> +					 st->dma_st.rx_buf_sz,
> +					 st->dma_st.rx_buf_sz / 2,
> +					 DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
> +
> +	if (!desc) {
> +		dev_err(&indio_dev->dev, "cannot prepare DMA cyclic\n");
> +		return -EBUSY;
> +	}
> +
> +	desc->callback = at91_dma_buffer_done;
> +	desc->callback_param = indio_dev;
> +
> +	cookie = dmaengine_submit(desc);
> +	ret = dma_submit_error(cookie);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "cannot submit DMA cyclic\n");
> +		dmaengine_terminate_async(st->dma_st.dma_chan);
> +		return ret;
> +	}
> +
> +	/* enable general overrun error signaling */
> +	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_GOVRE);
> +	/* Issue pending DMA requests */
> +	dma_async_issue_pending(st->dma_st.dma_chan);
> +
> +	/* consider current time as DMA start time for timestamps */
> +	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
> +
> +	dev_dbg(&indio_dev->dev, "DMA cyclic started\n");
> +
> +	return 0;
> +}
> +
> +static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	int ret;
> +
> +	ret = at91_adc_dma_start(indio_dev);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "buffer postenable failed\n");
> +		return ret;
> +	}
> +
> +	return iio_triggered_buffer_postenable(indio_dev);
> +}
> +
> +static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret;
> +	u8 bit;
> +
> +	ret = iio_triggered_buffer_predisable(indio_dev);
> +	if (ret < 0)
> +		dev_err(&indio_dev->dev, "buffer predisable failed\n");
> +
> +	if (!st->dma_st.dma_chan)
> +		return ret;
> +
> +	/* if we are using DMA we must clear registers and end DMA */
> +	dmaengine_terminate_sync(st->dma_st.dma_chan);
> +
> +	/*
> +	 * For each enabled channel we must read the last converted value
> +	 * to clear EOC status and not get a possible interrupt later.
> +	 * This value is being read by DMA from LCDR anyway
> +	 */
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->num_channels) {
> +		struct iio_chan_spec const *chan = indio_dev->channels + bit;
> +
> +		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);
> +	return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
> +	.postenable = &at91_adc_buffer_postenable,
> +	.predisable = &at91_adc_buffer_predisable,
>  };
>  
>  static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
> @@ -379,24 +566,71 @@ static int at91_adc_trigger_init(struct iio_dev *indio)
>  	return 0;
>  }
>  
> -static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> +static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
> +					   struct iio_poll_func *pf)
>  {
> -	struct iio_poll_func *pf = p;
> -	struct iio_dev *indio = pf->indio_dev;
> -	struct at91_adc_state *st = iio_priv(indio);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
>  	int i = 0;
>  	u8 bit;
>  
> -	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> -		struct iio_chan_spec const *chan = indio->channels + bit;
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->num_channels) {
> +		struct iio_chan_spec const *chan = indio_dev->channels + bit;
>  
>  		st->buffer[i] = at91_adc_readl(st, chan->address);
>  		i++;
>  	}
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer,
> +					   pf->timestamp);
> +}
> +
> +static void 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);
> +	s64 ns = iio_get_time_ns(indio_dev);
> +	s64 interval;
> +	int sample_index = 0, sample_count, sample_size;
>  
> -	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
> +	sample_size = div_s64(st->dma_st.rx_buf_sz, st->dma_st.watermark);
>  
> -	iio_trigger_notify_done(indio->trig);
> +	sample_count = div_s64(transferred_len, sample_size);
> +
> +	/*
> +	 * interval between samples is total time since last transfer handling
> +	 * divided by the number of samples (total size divided by sample size)
> +	 */
> +	interval = div_s64((ns - st->dma_st.dma_ts), sample_count);
> +
> +	while (transferred_len >= sample_size) {
> +		iio_push_to_buffers_with_timestamp(indio_dev,
> +				(st->dma_st.rx_buf + st->dma_st.buf_idx),
> +				(st->dma_st.dma_ts + interval * sample_index));
> +		/* adjust remaining length */
> +		transferred_len -= sample_size;
> +		/* adjust buffer index */
> +		st->dma_st.buf_idx += sample_size;
> +		/* in case of reaching end of buffer, reset index */
> +		if (st->dma_st.buf_idx >= st->dma_st.rx_buf_sz)
> +			st->dma_st.buf_idx = 0;
> +		sample_index++;
> +	}
> +	/* adjust saved time for next transfer handling */
> +	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
> +}
> +
> +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);
> +
> +	if (st->dma_st.dma_chan)
> +		at91_adc_trigger_handler_dma(indio_dev);
> +	else
> +		at91_adc_trigger_handler_nodma(indio_dev, pf);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -405,7 +639,7 @@ static int at91_adc_buffer_init(struct iio_dev *indio)
>  {
>  	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
>  			&iio_pollfunc_store_time,
> -			&at91_adc_trigger_handler, NULL);
> +			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
>  }
>  
>  static unsigned at91_adc_startup_time(unsigned startup_time_min,
> @@ -476,10 +710,17 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>  	if (!(status & imr))
>  		return IRQ_NONE;
>  
> -	if (iio_buffer_enabled(indio)) {
> +	/* if we reached this point, we cannot sample faster */
> +	if (status & AT91_SAMA5D2_IER_GOVRE)
> +		pr_alert_ratelimited("Conversion overrun detected\n");
> +
> +	if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
>  		disable_irq_nosync(irq);
>  		iio_trigger_poll(indio->trig);
> -	} else {
> +	} else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
> +		disable_irq_nosync(irq);
> +		WARN(true, "Unexpected irq occurred\n");
> +	} else if (!iio_buffer_enabled(indio)) {
>  		st->conversion_value = at91_adc_readl(st, st->chan->address);
>  		st->conversion_done = true;
>  		wake_up_interruptible(&st->wq_data_available);
> @@ -571,9 +812,111 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> +static void at91_adc_dma_init(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	struct dma_slave_config config = {0};
> +	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
> +					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
> +
> +	if (st->dma_st.dma_chan)
> +		return;
> +
> +	st->dma_st.dma_chan = dma_request_slave_channel(&pdev->dev, "rx");
> +
> +	if (!st->dma_st.dma_chan)  {
> +		dev_info(&pdev->dev, "can't get DMA channel\n");
> +		goto dma_exit;
> +	}
> +
> +	st->dma_st.rx_buf = dma_alloc_coherent(st->dma_st.dma_chan->device->dev,
> +					       pages * PAGE_SIZE,
> +					       &st->dma_st.rx_dma_buf,
> +					       GFP_KERNEL);
> +	if (!st->dma_st.rx_buf) {
> +		dev_info(&pdev->dev, "can't allocate coherent DMA area\n");
> +		goto dma_chan_disable;
> +	}
> +
> +	/* Configure DMA channel to read data register */
> +	config.direction = DMA_DEV_TO_MEM;
> +	config.src_addr = (phys_addr_t)(st->dma_st.phys_addr
> +			  + AT91_SAMA5D2_LCDR);
> +	config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> +	config.src_maxburst = 1;
> +	config.dst_maxburst = 1;
> +
> +	if (dmaengine_slave_config(st->dma_st.dma_chan, &config)) {
> +		dev_info(&pdev->dev, "can't configure DMA slave\n");
> +		goto dma_free_area;
> +	}
> +
> +	dev_info(&pdev->dev, "using %s for rx DMA transfers\n",
> +		 dma_chan_name(st->dma_st.dma_chan));
> +
> +	return;
> +
> +dma_free_area:
> +	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
> +			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
> +dma_chan_disable:
> +	dma_release_channel(st->dma_st.dma_chan);
> +	st->dma_st.dma_chan = 0;
> +dma_exit:
> +	dev_info(&pdev->dev, "continuing without DMA support\n");
> +}
> +
> +static void at91_adc_dma_disable(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
> +					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
> +
> +	/* if we are not using DMA, just return */
> +	if (!st->dma_st.dma_chan)
> +		return;
> +
> +	/* wait for all transactions to be terminated first*/
> +	dmaengine_terminate_sync(st->dma_st.dma_chan);
> +
> +	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
> +			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
> +	dma_release_channel(st->dma_st.dma_chan);
> +	st->dma_st.dma_chan = 0;
> +
> +	dev_info(&pdev->dev, "continuing without DMA support\n");
> +}
> +
> +static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	if (val > AT91_HWFIFO_MAX_SIZE)
> +		return -EINVAL;
> +
> +	dev_dbg(&indio_dev->dev, "new watermark is %u\n", val);
> +	st->dma_st.watermark = val;
> +
> +	/*
> +	 * The logic here is: if we have watermark 1, it means we do
> +	 * each conversion with it's own IRQ, thus we don't need DMA.
> +	 * If the watermark is higher, we do DMA to do all the transfers in bulk
> +	 */
> +

> +	if (val == 1)
> +		at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
> +	else if (val > 1)
> +		at91_adc_dma_init(to_platform_device(&indio_dev->dev));
> +
> +	return 0;
> +}
> +
>  static const struct iio_info at91_adc_info = {
>  	.read_raw = &at91_adc_read_raw,
>  	.write_raw = &at91_adc_write_raw,
> +	.hwfifo_set_watermark = &at91_adc_set_watermark,
>  	.driver_module = THIS_MODULE,
>  };
>  
> @@ -591,6 +934,42 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
>  	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
>  }
>  
> +static ssize_t at91_adc_get_fifo_state(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev =
> +			platform_get_drvdata(to_platform_device(dev));
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", !!st->dma_st.dma_chan);
> +}
> +
> +static ssize_t at91_adc_get_watermark(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev =
> +			platform_get_drvdata(to_platform_device(dev));
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", st->dma_st.watermark);
> +}
> +
> +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
> +		       at91_adc_get_fifo_state, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
> +		       at91_adc_get_watermark, NULL, 0);
> +
> +static IIO_CONST_ATTR(hwfifo_watermark_min, "2");
> +static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
> +
> +static const struct attribute *at91_adc_fifo_attributes[] = {
> +	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
> +	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> +	NULL,
> +};
> +
>  static int at91_adc_probe(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev;
> @@ -666,6 +1045,9 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	if (!res)
>  		return -EINVAL;
>  
> +	/* if we plan to use DMA, we need the physical address of the regs */
> +	st->dma_st.phys_addr = res->start;
> +
>  	st->base = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(st->base))
>  		return PTR_ERR(st->base);
> @@ -729,18 +1111,30 @@ static int at91_adc_probe(struct platform_device *pdev)
>  		goto per_clk_disable_unprepare;
>  	}
>  
> +	if (dma_coerce_mask_and_coherent(&indio_dev->dev, DMA_BIT_MASK(32)))
> +		dev_info(&pdev->dev, "cannot set DMA mask to 32-bit\n");
> +
> +	/* the iio buffer has initially a length of 2 and a watermark of 1 */
> +	st->dma_st.watermark = 1;
> +
> +	iio_buffer_set_attrs(indio_dev->buffer, at91_adc_fifo_attributes);
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
> -		goto per_clk_disable_unprepare;
> +		goto dma_disable;
>  
>  	dev_info(&pdev->dev, "setting up trigger as %s\n",
>  		 st->selected_trig->name);
>  
> +	dev_info(&pdev->dev, "continuing without DMA support\n");
> +
This feels a little misleading.  I think the point here is that dma
is initially disabled because fo the default watermark.

If it makes sense to put any info statement here at all it should
be more specific rather than implying we are running entirely
without DMA support.

>  	dev_info(&pdev->dev, "version: %x\n",
>  		 readl_relaxed(st->base + AT91_SAMA5D2_VERSION));
>  
>  	return 0;
>  
> +dma_disable:
> +	at91_adc_dma_disable(pdev);
>  per_clk_disable_unprepare:
>  	clk_disable_unprepare(st->per_clk);
>  vref_disable:
> @@ -757,6 +1151,8 @@ static int at91_adc_remove(struct platform_device *pdev)
>  
>  	iio_device_unregister(indio_dev);
>  
> +	at91_adc_dma_disable(pdev);
> +
>  	clk_disable_unprepare(st->per_clk);
>  
>  	regulator_disable(st->vref);

[PATCH v2 3/5] iio: adc: at91-sama5d2_adc: add support for DMA

[Jonathan Cameron]

On Wed, 11 Oct 2017 09:35:30 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> Added support for DMA transfers. The implementation uses the user watermark
> to decide whether DMA will be used or not. For watermark 1, DMA will not be
> used. If watermark is bigger, DMA will be used.
> Sysfs attributes are created to indicate whether the DMA is used,
> with hwfifo_enabled, and the current DMA watermark is readable
> in hwfifo_watermark. Minimum and maximum values are in hwfifo_watermark_min
> and hwfifo_watermark_max.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

You have shrunk the size of the buffer passed to
iio_push_to_buffers_with_timestamp in the non DMA case such that it is
no longer big enough to take the timestamp (the data is used in place
in that call so we need the extra space).

Other than that (and what I think is a missleading dev_info) I think
this is looking pretty good.

Lars if you have time to look at this one that would be great as 
your knowledge of the dmaengine stuff is better than mine.

Jonathan

> ---
>   Changes in v2:
>  - No longer add last timestamp to all samples. Now, compute an interval
> between samples w.r.t. start and end time of the transfer and number
> of samples. Then distribute them each in the time interval.
>  - Add warning for conversion overrun. This helps user identify cases
> when the watermark needs adjustment : the software is too slow in reading
> data from the ADC.
>  - Protection around watermark is not needed, changing of the watermark
> cannot be done while the buffer is enabled. When buffer is disabled, all
> DMA resources are freed anyway.
>  - Added validation on trigger to be used by own device
>  - Best sample rate I could obtain using the low frequency clock was about
> 4k samples/second, with a watermark of 100. To get up to 50k samples/second
> the ADC frequency must be increased to max.
>  - Fixed computation of DMA buffer size
>  - Addressed other comments from mailing list review. Feedback is appreciated
> 
>  drivers/iio/adc/Kconfig            |   1 +
>  drivers/iio/adc/at91-sama5d2_adc.c | 432 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 415 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 5762565..9ef288f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -158,6 +158,7 @@ config AT91_SAMA5D2_ADC
>  	tristate "Atmel AT91 SAMA5D2 ADC"
>  	depends on ARCH_AT91 || COMPILE_TEST
>  	depends on HAS_IOMEM
> +	depends on HAS_DMA
>  	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Atmel SAMA5D2 ADC which is
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index bc5b38e..7a9305a 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -16,6 +16,8 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -100,6 +102,8 @@
>  #define AT91_SAMA5D2_LCDR	0x20
>  /* Interrupt Enable Register */
>  #define AT91_SAMA5D2_IER	0x24
> +/* Interrupt Enable Register - general overrun error */
> +#define AT91_SAMA5D2_IER_GOVRE BIT(25)
>  /* Interrupt Disable Register */
>  #define AT91_SAMA5D2_IDR	0x28
>  /* Interrupt Mask Register */
> @@ -167,13 +171,16 @@
>  
>  /*
>   * Maximum number of bytes to hold conversion from all channels
> - * plus the timestamp
> + * without the timestamp.
>   */
>  #define AT91_BUFFER_MAX_BYTES ((AT91_SAMA5D2_SINGLE_CHAN_CNT +		\
> -				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2 + 8)
> +				AT91_SAMA5D2_DIFF_CHAN_CNT) * 2)
>  
>  #define AT91_BUFFER_MAX_HWORDS (AT91_BUFFER_MAX_BYTES / 2)
>  
> +#define AT91_HWFIFO_MAX_SIZE_STR	"128"
> +#define AT91_HWFIFO_MAX_SIZE		128
> +
>  #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
>  	{								\
>  		.type = IIO_VOLTAGE,					\
> @@ -227,6 +234,28 @@ struct at91_adc_trigger {
>  	unsigned int			edge_type;
>  };
>  
> +/**
> + * at91_adc_dma - at91-sama5d2 dma information struct
> + * @dma_chan:		the dma channel acquired
> + * @rx_buf:		dma coherent allocated area
> + * @rx_dma_buf:		dma handler for the buffer
> + * @phys_addr:		physical address of the ADC base register
> + * @buf_idx:		index inside the dma buffer where reading was last done
> + * @rx_buf_sz:		size of buffer used by DMA operation
> + * @watermark:		number of conversions to copy before DMA triggers irq
> + * @dma_ts:		hold the start timestamp of dma operation
> + */
> +struct at91_adc_dma {
> +	struct dma_chan			*dma_chan;
> +	u8				*rx_buf;
> +	dma_addr_t			rx_dma_buf;
> +	phys_addr_t			phys_addr;
> +	int				buf_idx;
> +	int				rx_buf_sz;
> +	int				watermark;
> +	s64				dma_ts;
> +};
> +
>  struct at91_adc_state {
>  	void __iomem			*base;
>  	int				irq;
> @@ -241,6 +270,7 @@ struct at91_adc_state {
>  	u32				conversion_value;
>  	struct at91_adc_soc_info	soc_info;
>  	wait_queue_head_t		wq_data_available;
> +	struct at91_adc_dma		dma_st;
>  	u16				buffer[AT91_BUFFER_MAX_HWORDS];

I think this is no longer big enough...  For the normal triggered case
you still need enough room for the timestamp to be inserted in
iio_push_to_buffers_with_timestamp.

>  	/*
>  	 * lock to prevent concurrent 'single conversion' requests through
> @@ -312,11 +342,17 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>  		if (state) {
>  			at91_adc_writel(st, AT91_SAMA5D2_CHER,
>  					BIT(chan->channel));
> -			at91_adc_writel(st, AT91_SAMA5D2_IER,
> -					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 {
> -			at91_adc_writel(st, AT91_SAMA5D2_IDR,
> -					BIT(chan->channel));
> +			/* 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));
>  		}
> @@ -330,6 +366,10 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
>  	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
>  	struct at91_adc_state *st = iio_priv(indio);
>  
> +	/* if we are using DMA, we must not reenable irq after each trigger */
> +	if (st->dma_st.dma_chan)
> +		return 0;
> +
>  	enable_irq(st->irq);
>  
>  	/* Needed to ACK the DRDY interruption */
> @@ -341,6 +381,153 @@ static const struct iio_trigger_ops at91_adc_trigger_ops = {
>  	.owner = THIS_MODULE,
>  	.set_trigger_state = &at91_adc_configure_trigger,
>  	.try_reenable = &at91_adc_reenable_trigger,
> +	.validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static int at91_adc_dma_size_done(struct at91_adc_state *st)
> +{
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +	int i, size;
> +
> +	status = dmaengine_tx_status(st->dma_st.dma_chan,
> +				     st->dma_st.dma_chan->cookie,
> +				     &state);
> +	if (status != DMA_IN_PROGRESS)
> +		return 0;
> +
> +	/* Transferred length is size in bytes from end of buffer */
> +	i = st->dma_st.rx_buf_sz - state.residue;
> +
> +	/* Return available bytes */
> +	if (i >= st->dma_st.buf_idx)
> +		size = i - st->dma_st.buf_idx;
> +	else
> +		size = st->dma_st.rx_buf_sz + i - st->dma_st.buf_idx;
> +	return size;
> +}
> +
> +static void at91_dma_buffer_done(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +
> +	iio_trigger_poll_chained(indio_dev->trig);
> +}
> +
> +static int at91_adc_dma_start(struct iio_dev *indio_dev)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	struct dma_async_tx_descriptor *desc;
> +	dma_cookie_t cookie;
> +	int ret;
> +	u8 bit;
> +
> +	if (!st->dma_st.dma_chan)
> +		return 0;
> +
> +	/* we start a new DMA, so set buffer index to start */
> +	st->dma_st.buf_idx = 0;
> +
> +	/*
> +	 * compute buffer size w.r.t. watermark and enabled channels.
> +	 * scan_bytes is aligned so we need an exact size for DMA
> +	 */
> +	st->dma_st.rx_buf_sz = 0;
> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->num_channels) {
> +		struct iio_chan_spec const *chan = indio_dev->channels + bit;
> +
> +		st->dma_st.rx_buf_sz += chan->scan_type.storagebits / 8;
> +	}
> +	st->dma_st.rx_buf_sz *= st->dma_st.watermark;
> +
> +	/* Prepare a DMA cyclic transaction */
> +	desc = dmaengine_prep_dma_cyclic(st->dma_st.dma_chan,
> +					 st->dma_st.rx_dma_buf,
> +					 st->dma_st.rx_buf_sz,
> +					 st->dma_st.rx_buf_sz / 2,
> +					 DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
> +
> +	if (!desc) {
> +		dev_err(&indio_dev->dev, "cannot prepare DMA cyclic\n");
> +		return -EBUSY;
> +	}
> +
> +	desc->callback = at91_dma_buffer_done;
> +	desc->callback_param = indio_dev;
> +
> +	cookie = dmaengine_submit(desc);
> +	ret = dma_submit_error(cookie);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "cannot submit DMA cyclic\n");
> +		dmaengine_terminate_async(st->dma_st.dma_chan);
> +		return ret;
> +	}
> +
> +	/* enable general overrun error signaling */
> +	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_GOVRE);
> +	/* Issue pending DMA requests */
> +	dma_async_issue_pending(st->dma_st.dma_chan);
> +
> +	/* consider current time as DMA start time for timestamps */
> +	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
> +
> +	dev_dbg(&indio_dev->dev, "DMA cyclic started\n");
> +
> +	return 0;
> +}
> +
> +static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	int ret;
> +
> +	ret = at91_adc_dma_start(indio_dev);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "buffer postenable failed\n");
> +		return ret;
> +	}
> +
> +	return iio_triggered_buffer_postenable(indio_dev);
> +}
> +
> +static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret;
> +	u8 bit;
> +
> +	ret = iio_triggered_buffer_predisable(indio_dev);
> +	if (ret < 0)
> +		dev_err(&indio_dev->dev, "buffer predisable failed\n");
> +
> +	if (!st->dma_st.dma_chan)
> +		return ret;
> +
> +	/* if we are using DMA we must clear registers and end DMA */
> +	dmaengine_terminate_sync(st->dma_st.dma_chan);
> +
> +	/*
> +	 * For each enabled channel we must read the last converted value
> +	 * to clear EOC status and not get a possible interrupt later.
> +	 * This value is being read by DMA from LCDR anyway
> +	 */
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->num_channels) {
> +		struct iio_chan_spec const *chan = indio_dev->channels + bit;
> +
> +		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);
> +	return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
> +	.postenable = &at91_adc_buffer_postenable,
> +	.predisable = &at91_adc_buffer_predisable,
>  };
>  
>  static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
> @@ -379,24 +566,71 @@ static int at91_adc_trigger_init(struct iio_dev *indio)
>  	return 0;
>  }
>  
> -static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> +static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
> +					   struct iio_poll_func *pf)
>  {
> -	struct iio_poll_func *pf = p;
> -	struct iio_dev *indio = pf->indio_dev;
> -	struct at91_adc_state *st = iio_priv(indio);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
>  	int i = 0;
>  	u8 bit;
>  
> -	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> -		struct iio_chan_spec const *chan = indio->channels + bit;
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->num_channels) {
> +		struct iio_chan_spec const *chan = indio_dev->channels + bit;
>  
>  		st->buffer[i] = at91_adc_readl(st, chan->address);
>  		i++;
>  	}
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer,
> +					   pf->timestamp);
> +}
> +
> +static void 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);
> +	s64 ns = iio_get_time_ns(indio_dev);
> +	s64 interval;
> +	int sample_index = 0, sample_count, sample_size;
>  
> -	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
> +	sample_size = div_s64(st->dma_st.rx_buf_sz, st->dma_st.watermark);
>  
> -	iio_trigger_notify_done(indio->trig);
> +	sample_count = div_s64(transferred_len, sample_size);
> +
> +	/*
> +	 * interval between samples is total time since last transfer handling
> +	 * divided by the number of samples (total size divided by sample size)
> +	 */
> +	interval = div_s64((ns - st->dma_st.dma_ts), sample_count);
> +
> +	while (transferred_len >= sample_size) {
> +		iio_push_to_buffers_with_timestamp(indio_dev,
> +				(st->dma_st.rx_buf + st->dma_st.buf_idx),
> +				(st->dma_st.dma_ts + interval * sample_index));
> +		/* adjust remaining length */
> +		transferred_len -= sample_size;
> +		/* adjust buffer index */
> +		st->dma_st.buf_idx += sample_size;
> +		/* in case of reaching end of buffer, reset index */
> +		if (st->dma_st.buf_idx >= st->dma_st.rx_buf_sz)
> +			st->dma_st.buf_idx = 0;
> +		sample_index++;
> +	}
> +	/* adjust saved time for next transfer handling */
> +	st->dma_st.dma_ts = iio_get_time_ns(indio_dev);
> +}
> +
> +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);
> +
> +	if (st->dma_st.dma_chan)
> +		at91_adc_trigger_handler_dma(indio_dev);
> +	else
> +		at91_adc_trigger_handler_nodma(indio_dev, pf);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -405,7 +639,7 @@ static int at91_adc_buffer_init(struct iio_dev *indio)
>  {
>  	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
>  			&iio_pollfunc_store_time,
> -			&at91_adc_trigger_handler, NULL);
> +			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
>  }
>  
>  static unsigned at91_adc_startup_time(unsigned startup_time_min,
> @@ -476,10 +710,17 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>  	if (!(status & imr))
>  		return IRQ_NONE;
>  
> -	if (iio_buffer_enabled(indio)) {
> +	/* if we reached this point, we cannot sample faster */
> +	if (status & AT91_SAMA5D2_IER_GOVRE)
> +		pr_alert_ratelimited("Conversion overrun detected\n");
> +
> +	if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
>  		disable_irq_nosync(irq);
>  		iio_trigger_poll(indio->trig);
> -	} else {
> +	} else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
> +		disable_irq_nosync(irq);
> +		WARN(true, "Unexpected irq occurred\n");
> +	} else if (!iio_buffer_enabled(indio)) {
>  		st->conversion_value = at91_adc_readl(st, st->chan->address);
>  		st->conversion_done = true;
>  		wake_up_interruptible(&st->wq_data_available);
> @@ -571,9 +812,111 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> +static void at91_adc_dma_init(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	struct dma_slave_config config = {0};
> +	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
> +					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
> +
> +	if (st->dma_st.dma_chan)
> +		return;
> +
> +	st->dma_st.dma_chan = dma_request_slave_channel(&pdev->dev, "rx");
> +
> +	if (!st->dma_st.dma_chan)  {
> +		dev_info(&pdev->dev, "can't get DMA channel\n");
> +		goto dma_exit;
> +	}
> +
> +	st->dma_st.rx_buf = dma_alloc_coherent(st->dma_st.dma_chan->device->dev,
> +					       pages * PAGE_SIZE,
> +					       &st->dma_st.rx_dma_buf,
> +					       GFP_KERNEL);
> +	if (!st->dma_st.rx_buf) {
> +		dev_info(&pdev->dev, "can't allocate coherent DMA area\n");
> +		goto dma_chan_disable;
> +	}
> +
> +	/* Configure DMA channel to read data register */
> +	config.direction = DMA_DEV_TO_MEM;
> +	config.src_addr = (phys_addr_t)(st->dma_st.phys_addr
> +			  + AT91_SAMA5D2_LCDR);
> +	config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> +	config.src_maxburst = 1;
> +	config.dst_maxburst = 1;
> +
> +	if (dmaengine_slave_config(st->dma_st.dma_chan, &config)) {
> +		dev_info(&pdev->dev, "can't configure DMA slave\n");
> +		goto dma_free_area;
> +	}
> +
> +	dev_info(&pdev->dev, "using %s for rx DMA transfers\n",
> +		 dma_chan_name(st->dma_st.dma_chan));
> +
> +	return;
> +
> +dma_free_area:
> +	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
> +			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
> +dma_chan_disable:
> +	dma_release_channel(st->dma_st.dma_chan);
> +	st->dma_st.dma_chan = 0;
> +dma_exit:
> +	dev_info(&pdev->dev, "continuing without DMA support\n");
> +}
> +
> +static void at91_adc_dma_disable(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
> +					  AT91_BUFFER_MAX_BYTES * 2, PAGE_SIZE);
> +
> +	/* if we are not using DMA, just return */
> +	if (!st->dma_st.dma_chan)
> +		return;
> +
> +	/* wait for all transactions to be terminated first*/
> +	dmaengine_terminate_sync(st->dma_st.dma_chan);
> +
> +	dma_free_coherent(st->dma_st.dma_chan->device->dev, pages * PAGE_SIZE,
> +			  st->dma_st.rx_buf, st->dma_st.rx_dma_buf);
> +	dma_release_channel(st->dma_st.dma_chan);
> +	st->dma_st.dma_chan = 0;
> +
> +	dev_info(&pdev->dev, "continuing without DMA support\n");
> +}
> +
> +static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	if (val > AT91_HWFIFO_MAX_SIZE)
> +		return -EINVAL;
> +
> +	dev_dbg(&indio_dev->dev, "new watermark is %u\n", val);
> +	st->dma_st.watermark = val;
> +
> +	/*
> +	 * The logic here is: if we have watermark 1, it means we do
> +	 * each conversion with it's own IRQ, thus we don't need DMA.
> +	 * If the watermark is higher, we do DMA to do all the transfers in bulk
> +	 */
> +

> +	if (val == 1)
> +		at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
> +	else if (val > 1)
> +		at91_adc_dma_init(to_platform_device(&indio_dev->dev));
> +
> +	return 0;
> +}
> +
>  static const struct iio_info at91_adc_info = {
>  	.read_raw = &at91_adc_read_raw,
>  	.write_raw = &at91_adc_write_raw,
> +	.hwfifo_set_watermark = &at91_adc_set_watermark,
>  	.driver_module = THIS_MODULE,
>  };
>  
> @@ -591,6 +934,42 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
>  	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
>  }
>  
> +static ssize_t at91_adc_get_fifo_state(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev =
> +			platform_get_drvdata(to_platform_device(dev));
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", !!st->dma_st.dma_chan);
> +}
> +
> +static ssize_t at91_adc_get_watermark(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev =
> +			platform_get_drvdata(to_platform_device(dev));
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", st->dma_st.watermark);
> +}
> +
> +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
> +		       at91_adc_get_fifo_state, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
> +		       at91_adc_get_watermark, NULL, 0);
> +
> +static IIO_CONST_ATTR(hwfifo_watermark_min, "2");
> +static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
> +
> +static const struct attribute *at91_adc_fifo_attributes[] = {
> +	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
> +	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> +	NULL,
> +};
> +
>  static int at91_adc_probe(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev;
> @@ -666,6 +1045,9 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	if (!res)
>  		return -EINVAL;
>  
> +	/* if we plan to use DMA, we need the physical address of the regs */
> +	st->dma_st.phys_addr = res->start;
> +
>  	st->base = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(st->base))
>  		return PTR_ERR(st->base);
> @@ -729,18 +1111,30 @@ static int at91_adc_probe(struct platform_device *pdev)
>  		goto per_clk_disable_unprepare;
>  	}
>  
> +	if (dma_coerce_mask_and_coherent(&indio_dev->dev, DMA_BIT_MASK(32)))
> +		dev_info(&pdev->dev, "cannot set DMA mask to 32-bit\n");
> +
> +	/* the iio buffer has initially a length of 2 and a watermark of 1 */
> +	st->dma_st.watermark = 1;
> +
> +	iio_buffer_set_attrs(indio_dev->buffer, at91_adc_fifo_attributes);
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
> -		goto per_clk_disable_unprepare;
> +		goto dma_disable;
>  
>  	dev_info(&pdev->dev, "setting up trigger as %s\n",
>  		 st->selected_trig->name);
>  
> +	dev_info(&pdev->dev, "continuing without DMA support\n");
> +
This feels a little misleading.  I think the point here is that dma
is initially disabled because fo the default watermark.

If it makes sense to put any info statement here at all it should
be more specific rather than implying we are running entirely
without DMA support.

>  	dev_info(&pdev->dev, "version: %x\n",
>  		 readl_relaxed(st->base + AT91_SAMA5D2_VERSION));
>  
>  	return 0;
>  
> +dma_disable:
> +	at91_adc_dma_disable(pdev);
>  per_clk_disable_unprepare:
>  	clk_disable_unprepare(st->per_clk);
>  vref_disable:
> @@ -757,6 +1151,8 @@ static int at91_adc_remove(struct platform_device *pdev)
>  
>  	iio_device_unregister(indio_dev);
>  
> +	at91_adc_dma_disable(pdev);
> +
>  	clk_disable_unprepare(st->per_clk);
>  
>  	regulator_disable(st->vref);

Re: [PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

[Jonathan Cameron]

On Wed, 11 Oct 2017 09:35:32 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> Change driver settings to use maximum sample rate clock.
> This is useful to achieve best possible sampling rate
> if we use DMA.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

I would normally expect this to have some bearing on the 
noise levels on the measured signals.  All I can find
is a reference to the track time being 15*a single
clock cycle.  We could change this so that it is appropriate
for the currently samping frequency rather than leaving it
as a fixed value.

Jonathan

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 7e51ecb..b76f88f 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -934,7 +934,7 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
>  	at91_adc_writel(st, AT91_SAMA5D2_MR,
>  			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>  
> -	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
> +	at91_adc_setup_samp_freq(st, st->soc_info.max_sample_rate);
>  }
>  
>  static ssize_t at91_adc_get_fifo_state(struct device *dev,

Re: [PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

[Jonathan Cameron]

On Wed, 11 Oct 2017 09:35:32 +0300
Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:

> Change driver settings to use maximum sample rate clock.
> This is useful to achieve best possible sampling rate
> if we use DMA.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>

I would normally expect this to have some bearing on the 
noise levels on the measured signals.  All I can find
is a reference to the track time being 15*a single
clock cycle.  We could change this so that it is appropriate
for the currently samping frequency rather than leaving it
as a fixed value.

Jonathan

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 7e51ecb..b76f88f 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -934,7 +934,7 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
>  	at91_adc_writel(st, AT91_SAMA5D2_MR,
>  			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>  
> -	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
> +	at91_adc_setup_samp_freq(st, st->soc_info.max_sample_rate);
>  }
>  
>  static ssize_t at91_adc_get_fifo_state(struct device *dev,

[PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

[Jonathan Cameron]

On Wed, 11 Oct 2017 09:35:32 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> Change driver settings to use maximum sample rate clock.
> This is useful to achieve best possible sampling rate
> if we use DMA.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

I would normally expect this to have some bearing on the 
noise levels on the measured signals.  All I can find
is a reference to the track time being 15*a single
clock cycle.  We could change this so that it is appropriate
for the currently samping frequency rather than leaving it
as a fixed value.

Jonathan

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 7e51ecb..b76f88f 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -934,7 +934,7 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
>  	at91_adc_writel(st, AT91_SAMA5D2_MR,
>  			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>  
> -	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
> +	at91_adc_setup_samp_freq(st, st->soc_info.max_sample_rate);
>  }
>  
>  static ssize_t at91_adc_get_fifo_state(struct device *dev,

Re: [PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

[Eugen Hristev]

On 15.10.2017 13:28, Jonathan Cameron wrote:
> On Wed, 11 Oct 2017 09:35:32 +0300
> Eugen Hristev <eugen.hristev@microchip.com> wrote:
> 
>> Change driver settings to use maximum sample rate clock.
>> This is useful to achieve best possible sampling rate
>> if we use DMA.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> 
> I would normally expect this to have some bearing on the
> noise levels on the measured signals.  All I can find
> is a reference to the track time being 15*a single
> clock cycle.  We could change this so that it is appropriate
> for the currently samping frequency rather than leaving it
> as a fixed value.

Actually this is the startup (default) value. The sampling frequency
can be changed from sysfs.
I will drop this change to let the ADC work with minimum frequency
at startup and user can change it from sysfs if greater speed is
required (as with the watermark).

Thanks !
Eugen
> 
> Jonathan
> 
>> ---
>>   drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 7e51ecb..b76f88f 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -934,7 +934,7 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
>>   	at91_adc_writel(st, AT91_SAMA5D2_MR,
>>   			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>>   
>> -	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
>> +	at91_adc_setup_samp_freq(st, st->soc_info.max_sample_rate);
>>   }
>>   
>>   static ssize_t at91_adc_get_fifo_state(struct device *dev,
> 

Re: [PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

[Eugen Hristev]

On 15.10.2017 13:28, Jonathan Cameron wrote:
> On Wed, 11 Oct 2017 09:35:32 +0300
> Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:
> 
>> Change driver settings to use maximum sample rate clock.
>> This is useful to achieve best possible sampling rate
>> if we use DMA.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> 
> I would normally expect this to have some bearing on the
> noise levels on the measured signals.  All I can find
> is a reference to the track time being 15*a single
> clock cycle.  We could change this so that it is appropriate
> for the currently samping frequency rather than leaving it
> as a fixed value.

Actually this is the startup (default) value. The sampling frequency
can be changed from sysfs.
I will drop this change to let the ADC work with minimum frequency
at startup and user can change it from sysfs if greater speed is
required (as with the watermark).

Thanks !
Eugen
> 
> Jonathan
> 
>> ---
>>   drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 7e51ecb..b76f88f 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -934,7 +934,7 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
>>   	at91_adc_writel(st, AT91_SAMA5D2_MR,
>>   			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>>   
>> -	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
>> +	at91_adc_setup_samp_freq(st, st->soc_info.max_sample_rate);
>>   }
>>   
>>   static ssize_t at91_adc_get_fifo_state(struct device *dev,
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/5] iio: adc: at91-sama5d2_adc: use max sample rate

[Eugen Hristev]

On 15.10.2017 13:28, Jonathan Cameron wrote:
> On Wed, 11 Oct 2017 09:35:32 +0300
> Eugen Hristev <eugen.hristev@microchip.com> wrote:
> 
>> Change driver settings to use maximum sample rate clock.
>> This is useful to achieve best possible sampling rate
>> if we use DMA.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> 
> I would normally expect this to have some bearing on the
> noise levels on the measured signals.  All I can find
> is a reference to the track time being 15*a single
> clock cycle.  We could change this so that it is appropriate
> for the currently samping frequency rather than leaving it
> as a fixed value.

Actually this is the startup (default) value. The sampling frequency
can be changed from sysfs.
I will drop this change to let the ADC work with minimum frequency
at startup and user can change it from sysfs if greater speed is
required (as with the watermark).

Thanks !
Eugen
> 
> Jonathan
> 
>> ---
>>   drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 7e51ecb..b76f88f 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -934,7 +934,7 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
>>   	at91_adc_writel(st, AT91_SAMA5D2_MR,
>>   			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>>   
>> -	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
>> +	at91_adc_setup_samp_freq(st, st->soc_info.max_sample_rate);
>>   }
>>   
>>   static ssize_t at91_adc_get_fifo_state(struct device *dev,
> 

Re: [PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Alexandre Belloni]

On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
> On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
> > Added property for DMA configuration of the device.
> > 
> > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > ---
> >  Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > index 552e7a8..5f94d479 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > @@ -17,6 +17,11 @@ Required properties:
> >    This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
> >    IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
> >  
> > +Optional properties:
> > +  - dmas: Phandle to dma channel for the ADC.
> > +  See ../../dma/dma.txt for details.
> > +  - dma-names: Must be "rx" when dmas property is being used.
> 
> -names is pointless when there is only one.
> 

You didn't reply to the question I had previously about that: What if at
some point, we have multiple dmas in the same binding?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Alexandre Belloni]

On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
> On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
> > Added property for DMA configuration of the device.
> > 
> > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > ---
> >  Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > index 552e7a8..5f94d479 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > @@ -17,6 +17,11 @@ Required properties:
> >    This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
> >    IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
> >  
> > +Optional properties:
> > +  - dmas: Phandle to dma channel for the ADC.
> > +  See ../../dma/dma.txt for details.
> > +  - dma-names: Must be "rx" when dmas property is being used.
> 
> -names is pointless when there is only one.
> 

You didn't reply to the question I had previously about that: What if at
some point, we have multiple dmas in the same binding?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Rob Herring]

On Thu, Oct 19, 2017 at 6:14 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
>> On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
>> > Added property for DMA configuration of the device.
>> >
>> > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> > ---
>> >  Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > index 552e7a8..5f94d479 100644
>> > --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > @@ -17,6 +17,11 @@ Required properties:
>> >    This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
>> >    IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
>> >
>> > +Optional properties:
>> > +  - dmas: Phandle to dma channel for the ADC.
>> > +  See ../../dma/dma.txt for details.
>> > +  - dma-names: Must be "rx" when dmas property is being used.
>>
>> -names is pointless when there is only one.
>>
>
> You didn't reply to the question I had previously about that: What if at
> some point, we have multiple dmas in the same binding?

Then add dma-names at that point and rx has to be first. If you know
there's other channels, then add them now. Don't evolve the bindings
needlessly based on what a driver supports.

Would another channel make sense here? Maybe multi-channel rx in which
case your naming wouldn't be setup for that. But "tx" on an ADC?

Rob

Re: [PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Rob Herring]

On Thu, Oct 19, 2017 at 6:14 AM, Alexandre Belloni
<alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
>> On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
>> > Added property for DMA configuration of the device.
>> >
>> > Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
>> > ---
>> >  Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > index 552e7a8..5f94d479 100644
>> > --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > @@ -17,6 +17,11 @@ Required properties:
>> >    This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
>> >    IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
>> >
>> > +Optional properties:
>> > +  - dmas: Phandle to dma channel for the ADC.
>> > +  See ../../dma/dma.txt for details.
>> > +  - dma-names: Must be "rx" when dmas property is being used.
>>
>> -names is pointless when there is only one.
>>
>
> You didn't reply to the question I had previously about that: What if at
> some point, we have multiple dmas in the same binding?

Then add dma-names at that point and rx has to be first. If you know
there's other channels, then add them now. Don't evolve the bindings
needlessly based on what a driver supports.

Would another channel make sense here? Maybe multi-channel rx in which
case your naming wouldn't be setup for that. But "tx" on an ADC?

Rob

Re: [PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Rob Herring]

On Thu, Oct 19, 2017 at 6:14 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
>> On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
>> > Added property for DMA configuration of the device.
>> >
>> > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> > ---
>> >  Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > index 552e7a8..5f94d479 100644
>> > --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > @@ -17,6 +17,11 @@ Required properties:
>> >    This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
>> >    IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
>> >
>> > +Optional properties:
>> > +  - dmas: Phandle to dma channel for the ADC.
>> > +  See ../../dma/dma.txt for details.
>> > +  - dma-names: Must be "rx" when dmas property is being used.
>>
>> -names is pointless when there is only one.
>>
>
> You didn't reply to the question I had previously about that: What if at
> some point, we have multiple dmas in the same binding?

Then add dma-names at that point and rx has to be first. If you know
there's other channels, then add them now. Don't evolve the bindings
needlessly based on what a driver supports.

Would another channel make sense here? Maybe multi-channel rx in which
case your naming wouldn't be setup for that. But "tx" on an ADC?

Rob

[PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Rob Herring]

On Thu, Oct 19, 2017 at 6:14 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
>> On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
>> > Added property for DMA configuration of the device.
>> >
>> > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> > ---
>> >  Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > index 552e7a8..5f94d479 100644
>> > --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>> > @@ -17,6 +17,11 @@ Required properties:
>> >    This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
>> >    IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
>> >
>> > +Optional properties:
>> > +  - dmas: Phandle to dma channel for the ADC.
>> > +  See ../../dma/dma.txt for details.
>> > +  - dma-names: Must be "rx" when dmas property is being used.
>>
>> -names is pointless when there is only one.
>>
>
> You didn't reply to the question I had previously about that: What if at
> some point, we have multiple dmas in the same binding?

Then add dma-names at that point and rx has to be first. If you know
there's other channels, then add them now. Don't evolve the bindings
needlessly based on what a driver supports.

Would another channel make sense here? Maybe multi-channel rx in which
case your naming wouldn't be setup for that. But "tx" on an ADC?

Rob

Re: [PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Eugen Hristev]

On 20.10.2017 00:58, Rob Herring wrote:
> On Thu, Oct 19, 2017 at 6:14 AM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
>> On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
>>> On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
>>>> Added property for DMA configuration of the device.
>>>>
>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>> ---
>>>>   Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> index 552e7a8..5f94d479 100644
>>>> --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> @@ -17,6 +17,11 @@ Required properties:
>>>>     This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
>>>>     IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
>>>>
>>>> +Optional properties:
>>>> +  - dmas: Phandle to dma channel for the ADC.
>>>> +  See ../../dma/dma.txt for details.
>>>> +  - dma-names: Must be "rx" when dmas property is being used.
>>>
>>> -names is pointless when there is only one.
>>>
>>
>> You didn't reply to the question I had previously about that: What if at
>> some point, we have multiple dmas in the same binding?
> 
> Then add dma-names at that point and rx has to be first. If you know
> there's other channels, then add them now. Don't evolve the bindings
> needlessly based on what a driver supports.
> 
> Would another channel make sense here? Maybe multi-channel rx in which
> case your naming wouldn't be setup for that. But "tx" on an ADC?
> 
> Rob

Hello Rob,

I can keep only "dmas" and remove "dma-names", but then, the API used by
the drivers that request channels requires a name parameter
(dma_request_slave_channel), and it will look always inside the
"dma-names" property to match the name and find the proper channel, and
it will fail in this case.

Looking in the general dma binding (dma.txt) I can see that both
properties are marked as required for a client driver, so that's why
I added both.

I can either keep dma-names; or try to find the channel properties by
looking directly into the OF node and create the dma_chan struct inside
my driver (maybe call some xlate function for the DMA controller), but
still need to get the DT property. A complicated option would be to
actually create an API inside the dmaengine to find a channel without
name.

Which approach you think it's best to pursue ?

Thanks,
Eugen



> 

Re: [PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Eugen Hristev]

On 20.10.2017 00:58, Rob Herring wrote:
> On Thu, Oct 19, 2017 at 6:14 AM, Alexandre Belloni
> <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
>>> On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
>>>> Added property for DMA configuration of the device.
>>>>
>>>> Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
>>>> ---
>>>>   Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> index 552e7a8..5f94d479 100644
>>>> --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> @@ -17,6 +17,11 @@ Required properties:
>>>>     This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
>>>>     IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
>>>>
>>>> +Optional properties:
>>>> +  - dmas: Phandle to dma channel for the ADC.
>>>> +  See ../../dma/dma.txt for details.
>>>> +  - dma-names: Must be "rx" when dmas property is being used.
>>>
>>> -names is pointless when there is only one.
>>>
>>
>> You didn't reply to the question I had previously about that: What if at
>> some point, we have multiple dmas in the same binding?
> 
> Then add dma-names at that point and rx has to be first. If you know
> there's other channels, then add them now. Don't evolve the bindings
> needlessly based on what a driver supports.
> 
> Would another channel make sense here? Maybe multi-channel rx in which
> case your naming wouldn't be setup for that. But "tx" on an ADC?
> 
> Rob

Hello Rob,

I can keep only "dmas" and remove "dma-names", but then, the API used by
the drivers that request channels requires a name parameter
(dma_request_slave_channel), and it will look always inside the
"dma-names" property to match the name and find the proper channel, and
it will fail in this case.

Looking in the general dma binding (dma.txt) I can see that both
properties are marked as required for a client driver, so that's why
I added both.

I can either keep dma-names; or try to find the channel properties by
looking directly into the OF node and create the dma_chan struct inside
my driver (maybe call some xlate function for the DMA controller), but
still need to get the DT property. A complicated option would be to
actually create an API inside the dmaengine to find a channel without
name.

Which approach you think it's best to pursue ?

Thanks,
Eugen



> 

Re: [PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Eugen Hristev]

On 20.10.2017 00:58, Rob Herring wrote:
> On Thu, Oct 19, 2017 at 6:14 AM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
>> On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
>>> On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
>>>> Added property for DMA configuration of the device.
>>>>
>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>> ---
>>>>   Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> index 552e7a8..5f94d479 100644
>>>> --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> @@ -17,6 +17,11 @@ Required properties:
>>>>     This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
>>>>     IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
>>>>
>>>> +Optional properties:
>>>> +  - dmas: Phandle to dma channel for the ADC.
>>>> +  See ../../dma/dma.txt for details.
>>>> +  - dma-names: Must be "rx" when dmas property is being used.
>>>
>>> -names is pointless when there is only one.
>>>
>>
>> You didn't reply to the question I had previously about that: What if at
>> some point, we have multiple dmas in the same binding?
> 
> Then add dma-names at that point and rx has to be first. If you know
> there's other channels, then add them now. Don't evolve the bindings
> needlessly based on what a driver supports.
> 
> Would another channel make sense here? Maybe multi-channel rx in which
> case your naming wouldn't be setup for that. But "tx" on an ADC?
> 
> Rob

Hello Rob,

I can keep only "dmas" and remove "dma-names", but then, the API used by
the drivers that request channels requires a name parameter
(dma_request_slave_channel), and it will look always inside the
"dma-names" property to match the name and find the proper channel, and
it will fail in this case.

Looking in the general dma binding (dma.txt) I can see that both
properties are marked as required for a client driver, so that's why
I added both.

I can either keep dma-names; or try to find the channel properties by
looking directly into the OF node and create the dma_chan struct inside
my driver (maybe call some xlate function for the DMA controller), but
still need to get the DT property. A complicated option would be to
actually create an API inside the dmaengine to find a channel without
name.

Which approach you think it's best to pursue ?

Thanks,
Eugen



> 

[PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Eugen Hristev]

On 20.10.2017 00:58, Rob Herring wrote:
> On Thu, Oct 19, 2017 at 6:14 AM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
>> On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
>>> On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
>>>> Added property for DMA configuration of the device.
>>>>
>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>> ---
>>>>   Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> index 552e7a8..5f94d479 100644
>>>> --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
>>>> @@ -17,6 +17,11 @@ Required properties:
>>>>     This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
>>>>     IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
>>>>
>>>> +Optional properties:
>>>> +  - dmas: Phandle to dma channel for the ADC.
>>>> +  See ../../dma/dma.txt for details.
>>>> +  - dma-names: Must be "rx" when dmas property is being used.
>>>
>>> -names is pointless when there is only one.
>>>
>>
>> You didn't reply to the question I had previously about that: What if at
>> some point, we have multiple dmas in the same binding?
> 
> Then add dma-names at that point and rx has to be first. If you know
> there's other channels, then add them now. Don't evolve the bindings
> needlessly based on what a driver supports.
> 
> Would another channel make sense here? Maybe multi-channel rx in which
> case your naming wouldn't be setup for that. But "tx" on an ADC?
> 
> Rob

Hello Rob,

I can keep only "dmas" and remove "dma-names", but then, the API used by
the drivers that request channels requires a name parameter
(dma_request_slave_channel), and it will look always inside the
"dma-names" property to match the name and find the proper channel, and
it will fail in this case.

Looking in the general dma binding (dma.txt) I can see that both
properties are marked as required for a client driver, so that's why
I added both.

I can either keep dma-names; or try to find the channel properties by
looking directly into the OF node and create the dma_chan struct inside
my driver (maybe call some xlate function for the DMA controller), but
still need to get the DT property. A complicated option would be to
actually create an API inside the dmaengine to find a channel without
name.

Which approach you think it's best to pursue ?

Thanks,
Eugen



> 

Re: [PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Ludovic Desroches]

On Tue, Nov 07, 2017 at 10:49:41AM +0200, Eugen Hristev wrote:
> 
> 
> On 20.10.2017 00:58, Rob Herring wrote:
> > On Thu, Oct 19, 2017 at 6:14 AM, Alexandre Belloni
> > <alexandre.belloni@free-electrons.com> wrote:
> > > On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
> > > > On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
> > > > > Added property for DMA configuration of the device.
> > > > > 
> > > > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > > > > ---
> > > > >   Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
> > > > >   1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > > > > index 552e7a8..5f94d479 100644
> > > > > --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > > > > +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > > > > @@ -17,6 +17,11 @@ Required properties:
> > > > >     This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
> > > > >     IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
> > > > > 
> > > > > +Optional properties:
> > > > > +  - dmas: Phandle to dma channel for the ADC.
> > > > > +  See ../../dma/dma.txt for details.
> > > > > +  - dma-names: Must be "rx" when dmas property is being used.
> > > > 
> > > > -names is pointless when there is only one.
> > > > 
> > > 
> > > You didn't reply to the question I had previously about that: What if at
> > > some point, we have multiple dmas in the same binding?
> > 
> > Then add dma-names at that point and rx has to be first. If you know
> > there's other channels, then add them now. Don't evolve the bindings
> > needlessly based on what a driver supports.
> > 
> > Would another channel make sense here? Maybe multi-channel rx in which
> > case your naming wouldn't be setup for that. But "tx" on an ADC?
> > 
> > Rob
> 
> Hello Rob,
> 
> I can keep only "dmas" and remove "dma-names", but then, the API used by
> the drivers that request channels requires a name parameter
> (dma_request_slave_channel), and it will look always inside the
> "dma-names" property to match the name and find the proper channel, and
> it will fail in this case.
> 
> Looking in the general dma binding (dma.txt) I can see that both
> properties are marked as required for a client driver, so that's why
> I added both.
> 

I don't understand why dma-names should be removed. For sure, if we have
only one channel, it becomes useless but the helpers to get a dma channel
rely on dma-names property.

Ludovic

> I can either keep dma-names; or try to find the channel properties by
> looking directly into the OF node and create the dma_chan struct inside
> my driver (maybe call some xlate function for the DMA controller), but
> still need to get the DT property. A complicated option would be to
> actually create an API inside the dmaengine to find a channel without
> name.
> 
> Which approach you think it's best to pursue ?
> 
> Thanks,
> Eugen
> 
> 
> 
> > 

Re: [PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Ludovic Desroches]

On Tue, Nov 07, 2017 at 10:49:41AM +0200, Eugen Hristev wrote:
> 
> 
> On 20.10.2017 00:58, Rob Herring wrote:
> > On Thu, Oct 19, 2017 at 6:14 AM, Alexandre Belloni
> > <alexandre.belloni@free-electrons.com> wrote:
> > > On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
> > > > On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
> > > > > Added property for DMA configuration of the device.
> > > > > 
> > > > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > > > > ---
> > > > >   Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
> > > > >   1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > > > > index 552e7a8..5f94d479 100644
> > > > > --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > > > > +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > > > > @@ -17,6 +17,11 @@ Required properties:
> > > > >     This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
> > > > >     IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
> > > > > 
> > > > > +Optional properties:
> > > > > +  - dmas: Phandle to dma channel for the ADC.
> > > > > +  See ../../dma/dma.txt for details.
> > > > > +  - dma-names: Must be "rx" when dmas property is being used.
> > > > 
> > > > -names is pointless when there is only one.
> > > > 
> > > 
> > > You didn't reply to the question I had previously about that: What if at
> > > some point, we have multiple dmas in the same binding?
> > 
> > Then add dma-names at that point and rx has to be first. If you know
> > there's other channels, then add them now. Don't evolve the bindings
> > needlessly based on what a driver supports.
> > 
> > Would another channel make sense here? Maybe multi-channel rx in which
> > case your naming wouldn't be setup for that. But "tx" on an ADC?
> > 
> > Rob
> 
> Hello Rob,
> 
> I can keep only "dmas" and remove "dma-names", but then, the API used by
> the drivers that request channels requires a name parameter
> (dma_request_slave_channel), and it will look always inside the
> "dma-names" property to match the name and find the proper channel, and
> it will fail in this case.
> 
> Looking in the general dma binding (dma.txt) I can see that both
> properties are marked as required for a client driver, so that's why
> I added both.
> 

I don't understand why dma-names should be removed. For sure, if we have
only one channel, it becomes useless but the helpers to get a dma channel
rely on dma-names property.

Ludovic

> I can either keep dma-names; or try to find the channel properties by
> looking directly into the OF node and create the dma_chan struct inside
> my driver (maybe call some xlate function for the DMA controller), but
> still need to get the DT property. A complicated option would be to
> actually create an API inside the dmaengine to find a channel without
> name.
> 
> Which approach you think it's best to pursue ?
> 
> Thanks,
> Eugen
> 
> 
> 
> > 

[PATCH v2 1/5] dt-bindings: iio: at91-sama5d2_adc: add optional dma property

[Ludovic Desroches]

On Tue, Nov 07, 2017 at 10:49:41AM +0200, Eugen Hristev wrote:
> 
> 
> On 20.10.2017 00:58, Rob Herring wrote:
> > On Thu, Oct 19, 2017 at 6:14 AM, Alexandre Belloni
> > <alexandre.belloni@free-electrons.com> wrote:
> > > On 13/10/2017 at 16:51:42 -0500, Rob Herring wrote:
> > > > On Wed, Oct 11, 2017 at 09:35:28AM +0300, Eugen Hristev wrote:
> > > > > Added property for DMA configuration of the device.
> > > > > 
> > > > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > > > > ---
> > > > >   Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt | 7 +++++++
> > > > >   1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > > > > index 552e7a8..5f94d479 100644
> > > > > --- a/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > > > > +++ b/Documentation/devicetree/bindings/iio/adc/at91-sama5d2_adc.txt
> > > > > @@ -17,6 +17,11 @@ Required properties:
> > > > >     This property uses the IRQ edge types values: IRQ_TYPE_EDGE_RISING ,
> > > > >     IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_EDGE_BOTH
> > > > > 
> > > > > +Optional properties:
> > > > > +  - dmas: Phandle to dma channel for the ADC.
> > > > > +  See ../../dma/dma.txt for details.
> > > > > +  - dma-names: Must be "rx" when dmas property is being used.
> > > > 
> > > > -names is pointless when there is only one.
> > > > 
> > > 
> > > You didn't reply to the question I had previously about that: What if at
> > > some point, we have multiple dmas in the same binding?
> > 
> > Then add dma-names at that point and rx has to be first. If you know
> > there's other channels, then add them now. Don't evolve the bindings
> > needlessly based on what a driver supports.
> > 
> > Would another channel make sense here? Maybe multi-channel rx in which
> > case your naming wouldn't be setup for that. But "tx" on an ADC?
> > 
> > Rob
> 
> Hello Rob,
> 
> I can keep only "dmas" and remove "dma-names", but then, the API used by
> the drivers that request channels requires a name parameter
> (dma_request_slave_channel), and it will look always inside the
> "dma-names" property to match the name and find the proper channel, and
> it will fail in this case.
> 
> Looking in the general dma binding (dma.txt) I can see that both
> properties are marked as required for a client driver, so that's why
> I added both.
> 

I don't understand why dma-names should be removed. For sure, if we have
only one channel, it becomes useless but the helpers to get a dma channel
rely on dma-names property.

Ludovic

> I can either keep dma-names; or try to find the channel properties by
> looking directly into the OF node and create the dma_chan struct inside
> my driver (maybe call some xlate function for the DMA controller), but
> still need to get the DT property. A complicated option would be to
> actually create an API inside the dmaengine to find a channel without
> name.
> 
> Which approach you think it's best to pursue ?
> 
> Thanks,
> Eugen
> 
> 
> 
> >