Re: [PATCH 03/10] iio: adc: stm32: add optional dma support

[Jonathan Cameron]

On 25/10/16 17:25, Fabrice Gasnier wrote:
> Add optional DMA support to STM32 ADC.
> Use dma cyclic mode with at least two periods.
> 
> Signed-off-by: Fabrice Gasnier 
Few little bits inline, but looks superficially good (my dma knowledge isn't
really up to reviewing this.) Lars can you take a look (perhaps at the next
version)?

Also, you 'could' potentially use the dma buffer stuff to give a lower overhead
route for the data to userspace if the device goes quick enough to warrant
it.

Jonathan
> ---
>  drivers/iio/adc/stm32/Kconfig   |   2 +
>  drivers/iio/adc/stm32/stm32-adc.c   | 222 
> 
>  drivers/iio/adc/stm32/stm32-adc.h   |  15 +++
>  drivers/iio/adc/stm32/stm32f4-adc.c |  20 +++-
>  4 files changed, 235 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32/Kconfig b/drivers/iio/adc/stm32/Kconfig
> index 245d037..5554ac8 100644
> --- a/drivers/iio/adc/stm32/Kconfig
> +++ b/drivers/iio/adc/stm32/Kconfig
> @@ -8,6 +8,7 @@ config STM32_ADC
>   select REGULATOR_FIXED_VOLTAGE
>   select IIO_BUFFER
>   select IIO_TRIGGERED_BUFFER
> + select IRQ_WORK
>   help
> Say yes here to build the driver for the STMicroelectronics
> STM32 analog-to-digital converter (ADC).
> @@ -18,6 +19,7 @@ config STM32_ADC
>  config STM32F4_ADC
>   tristate "STMicroelectronics STM32F4 adc"
>   depends on ARCH_STM32 || COMPILE_TEST
> + depends on HAS_DMA
>   depends on OF
>   select STM32_ADC
>   help
> diff --git a/drivers/iio/adc/stm32/stm32-adc.c 
> b/drivers/iio/adc/stm32/stm32-adc.c
> index 1e0850d..25d0307 100644
> --- a/drivers/iio/adc/stm32/stm32-adc.c
> +++ b/drivers/iio/adc/stm32/stm32-adc.c
> @@ -21,6 +21,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -371,6 +372,34 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
>   return ret;
>  }
>  
> +static int stm32_adc_residue(struct stm32_adc *adc)
> +{
> + struct dma_tx_state state;
> + enum dma_status status;
> +
> + if (!adc->rx_buf)
> + return 0;
> +
> + status = dmaengine_tx_status(adc->dma_chan,
> +  adc->dma_chan->cookie,
> +  );
> + if (status == DMA_IN_PROGRESS) {
> + /* Residue is size in bytes from end of buffer */
> + int i = adc->rx_buf_sz - state.residue;
> + int size;
> +
> + /* Return available bytes */
> + if (i >= adc->bufi)
> + size = i - adc->bufi;
> + else
> + size = adc->rx_buf_sz - adc->bufi + i;
> +
> + return size;
> + }
> +
> + return 0;
> +}
> +
>  /**
>   * stm32_adc_isr() - Treat interrupt for one ADC instance within ADC block
>   */
> @@ -394,8 +423,8 @@ static irqreturn_t stm32_adc_isr(struct stm32_adc *adc)
>   stm32_adc_writel(adc, reginfo->isr, status & ~clr_mask);
>   status &= mask;
>  
> - /* Regular data */
> - if (status & reginfo->eoc) {
> + /* Regular data (when dma isn't used) */
> + if ((status & reginfo->eoc) && (!adc->rx_buf)) {
>   adc->buffer[adc->bufi] = stm32_adc_readl(adc, reginfo->dr);
>   if (iio_buffer_enabled(indio_dev)) {
>   adc->bufi++;
> @@ -553,29 +582,154 @@ static int stm32_adc_validate_device(struct 
> iio_trigger *trig,
>   return indio != indio_dev ? -EINVAL : 0;
>  }
>  
> -static int stm32_adc_set_trigger_state(struct iio_trigger *trig,
> -bool state)
> +static void stm32_adc_dma_irq_work(struct irq_work *work)
>  {
> - struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct stm32_adc *adc = container_of(work, struct stm32_adc, work);
> + struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> +
> + /**
> +  * iio_trigger_poll calls generic_handle_irq(). So, it requires hard
> +  * irq context, and cannot be called directly from dma callback,
> +  * dma cb has to schedule this work instead.
> +  */
> + iio_trigger_poll(indio_dev->trig);
> +}
> +
> +static void stm32_adc_dma_buffer_done(void *data)
> +{
> + struct iio_dev *indio_dev = data;
>   struct stm32_adc *adc = iio_priv(indio_dev);
> - int ret;
>  
> - if (state) {
> - /* Reset adc buffer index */
> - adc->bufi = 0;
> + /* invoques iio_trigger_poll() from hard irq context */
invokes.
> + irq_work_queue(>work);
> +}
> +
> +static int stm32_adc_buffer_alloc_dma_start(struct iio_dev *indio_dev)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> + const struct stm32_adc_reginfo *reginfo =
> + adc->common->data->adc_reginfo;
> + struct dma_async_tx_descriptor *desc;
> + struct dma_slave_config config;
> + dma_cookie_t cookie;
> + int ret, size, watermark;
>  
> + /* Reset adc buffer index */
> + 

Re: [PATCH 03/10] iio: adc: stm32: add optional dma support

[Jonathan Cameron]

On 25/10/16 17:25, Fabrice Gasnier wrote:
> Add optional DMA support to STM32 ADC.
> Use dma cyclic mode with at least two periods.
> 
> Signed-off-by: Fabrice Gasnier 
Few little bits inline, but looks superficially good (my dma knowledge isn't
really up to reviewing this.) Lars can you take a look (perhaps at the next
version)?

Also, you 'could' potentially use the dma buffer stuff to give a lower overhead
route for the data to userspace if the device goes quick enough to warrant
it.

Jonathan
> ---
>  drivers/iio/adc/stm32/Kconfig   |   2 +
>  drivers/iio/adc/stm32/stm32-adc.c   | 222 
> 
>  drivers/iio/adc/stm32/stm32-adc.h   |  15 +++
>  drivers/iio/adc/stm32/stm32f4-adc.c |  20 +++-
>  4 files changed, 235 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32/Kconfig b/drivers/iio/adc/stm32/Kconfig
> index 245d037..5554ac8 100644
> --- a/drivers/iio/adc/stm32/Kconfig
> +++ b/drivers/iio/adc/stm32/Kconfig
> @@ -8,6 +8,7 @@ config STM32_ADC
>   select REGULATOR_FIXED_VOLTAGE
>   select IIO_BUFFER
>   select IIO_TRIGGERED_BUFFER
> + select IRQ_WORK
>   help
> Say yes here to build the driver for the STMicroelectronics
> STM32 analog-to-digital converter (ADC).
> @@ -18,6 +19,7 @@ config STM32_ADC
>  config STM32F4_ADC
>   tristate "STMicroelectronics STM32F4 adc"
>   depends on ARCH_STM32 || COMPILE_TEST
> + depends on HAS_DMA
>   depends on OF
>   select STM32_ADC
>   help
> diff --git a/drivers/iio/adc/stm32/stm32-adc.c 
> b/drivers/iio/adc/stm32/stm32-adc.c
> index 1e0850d..25d0307 100644
> --- a/drivers/iio/adc/stm32/stm32-adc.c
> +++ b/drivers/iio/adc/stm32/stm32-adc.c
> @@ -21,6 +21,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -371,6 +372,34 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
>   return ret;
>  }
>  
> +static int stm32_adc_residue(struct stm32_adc *adc)
> +{
> + struct dma_tx_state state;
> + enum dma_status status;
> +
> + if (!adc->rx_buf)
> + return 0;
> +
> + status = dmaengine_tx_status(adc->dma_chan,
> +  adc->dma_chan->cookie,
> +  );
> + if (status == DMA_IN_PROGRESS) {
> + /* Residue is size in bytes from end of buffer */
> + int i = adc->rx_buf_sz - state.residue;
> + int size;
> +
> + /* Return available bytes */
> + if (i >= adc->bufi)
> + size = i - adc->bufi;
> + else
> + size = adc->rx_buf_sz - adc->bufi + i;
> +
> + return size;
> + }
> +
> + return 0;
> +}
> +
>  /**
>   * stm32_adc_isr() - Treat interrupt for one ADC instance within ADC block
>   */
> @@ -394,8 +423,8 @@ static irqreturn_t stm32_adc_isr(struct stm32_adc *adc)
>   stm32_adc_writel(adc, reginfo->isr, status & ~clr_mask);
>   status &= mask;
>  
> - /* Regular data */
> - if (status & reginfo->eoc) {
> + /* Regular data (when dma isn't used) */
> + if ((status & reginfo->eoc) && (!adc->rx_buf)) {
>   adc->buffer[adc->bufi] = stm32_adc_readl(adc, reginfo->dr);
>   if (iio_buffer_enabled(indio_dev)) {
>   adc->bufi++;
> @@ -553,29 +582,154 @@ static int stm32_adc_validate_device(struct 
> iio_trigger *trig,
>   return indio != indio_dev ? -EINVAL : 0;
>  }
>  
> -static int stm32_adc_set_trigger_state(struct iio_trigger *trig,
> -bool state)
> +static void stm32_adc_dma_irq_work(struct irq_work *work)
>  {
> - struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct stm32_adc *adc = container_of(work, struct stm32_adc, work);
> + struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> +
> + /**
> +  * iio_trigger_poll calls generic_handle_irq(). So, it requires hard
> +  * irq context, and cannot be called directly from dma callback,
> +  * dma cb has to schedule this work instead.
> +  */
> + iio_trigger_poll(indio_dev->trig);
> +}
> +
> +static void stm32_adc_dma_buffer_done(void *data)
> +{
> + struct iio_dev *indio_dev = data;
>   struct stm32_adc *adc = iio_priv(indio_dev);
> - int ret;
>  
> - if (state) {
> - /* Reset adc buffer index */
> - adc->bufi = 0;
> + /* invoques iio_trigger_poll() from hard irq context */
invokes.
> + irq_work_queue(>work);
> +}
> +
> +static int stm32_adc_buffer_alloc_dma_start(struct iio_dev *indio_dev)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> + const struct stm32_adc_reginfo *reginfo =
> + adc->common->data->adc_reginfo;
> + struct dma_async_tx_descriptor *desc;
> + struct dma_slave_config config;
> + dma_cookie_t cookie;
> + int ret, size, watermark;
>  
> + /* Reset adc buffer index */
> + adc->bufi = 0;
> +

[PATCH 03/10] iio: adc: stm32: add optional dma support

[Fabrice Gasnier]

Add optional DMA support to STM32 ADC.
Use dma cyclic mode with at least two periods.

Signed-off-by: Fabrice Gasnier 
---
 drivers/iio/adc/stm32/Kconfig   |   2 +
 drivers/iio/adc/stm32/stm32-adc.c   | 222 
 drivers/iio/adc/stm32/stm32-adc.h   |  15 +++
 drivers/iio/adc/stm32/stm32f4-adc.c |  20 +++-
 4 files changed, 235 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/stm32/Kconfig b/drivers/iio/adc/stm32/Kconfig
index 245d037..5554ac8 100644
--- a/drivers/iio/adc/stm32/Kconfig
+++ b/drivers/iio/adc/stm32/Kconfig
@@ -8,6 +8,7 @@ config STM32_ADC
select REGULATOR_FIXED_VOLTAGE
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER
+   select IRQ_WORK
help
  Say yes here to build the driver for the STMicroelectronics
  STM32 analog-to-digital converter (ADC).
@@ -18,6 +19,7 @@ config STM32_ADC
 config STM32F4_ADC
tristate "STMicroelectronics STM32F4 adc"
depends on ARCH_STM32 || COMPILE_TEST
+   depends on HAS_DMA
depends on OF
select STM32_ADC
help
diff --git a/drivers/iio/adc/stm32/stm32-adc.c 
b/drivers/iio/adc/stm32/stm32-adc.c
index 1e0850d..25d0307 100644
--- a/drivers/iio/adc/stm32/stm32-adc.c
+++ b/drivers/iio/adc/stm32/stm32-adc.c
@@ -21,6 +21,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -371,6 +372,34 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
return ret;
 }
 
+static int stm32_adc_residue(struct stm32_adc *adc)
+{
+   struct dma_tx_state state;
+   enum dma_status status;
+
+   if (!adc->rx_buf)
+   return 0;
+
+   status = dmaengine_tx_status(adc->dma_chan,
+adc->dma_chan->cookie,
+);
+   if (status == DMA_IN_PROGRESS) {
+   /* Residue is size in bytes from end of buffer */
+   int i = adc->rx_buf_sz - state.residue;
+   int size;
+
+   /* Return available bytes */
+   if (i >= adc->bufi)
+   size = i - adc->bufi;
+   else
+   size = adc->rx_buf_sz - adc->bufi + i;
+
+   return size;
+   }
+
+   return 0;
+}
+
 /**
  * stm32_adc_isr() - Treat interrupt for one ADC instance within ADC block
  */
@@ -394,8 +423,8 @@ static irqreturn_t stm32_adc_isr(struct stm32_adc *adc)
stm32_adc_writel(adc, reginfo->isr, status & ~clr_mask);
status &= mask;
 
-   /* Regular data */
-   if (status & reginfo->eoc) {
+   /* Regular data (when dma isn't used) */
+   if ((status & reginfo->eoc) && (!adc->rx_buf)) {
adc->buffer[adc->bufi] = stm32_adc_readl(adc, reginfo->dr);
if (iio_buffer_enabled(indio_dev)) {
adc->bufi++;
@@ -553,29 +582,154 @@ static int stm32_adc_validate_device(struct iio_trigger 
*trig,
return indio != indio_dev ? -EINVAL : 0;
 }
 
-static int stm32_adc_set_trigger_state(struct iio_trigger *trig,
-  bool state)
+static void stm32_adc_dma_irq_work(struct irq_work *work)
 {
-   struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+   struct stm32_adc *adc = container_of(work, struct stm32_adc, work);
+   struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+
+   /**
+* iio_trigger_poll calls generic_handle_irq(). So, it requires hard
+* irq context, and cannot be called directly from dma callback,
+* dma cb has to schedule this work instead.
+*/
+   iio_trigger_poll(indio_dev->trig);
+}
+
+static void stm32_adc_dma_buffer_done(void *data)
+{
+   struct iio_dev *indio_dev = data;
struct stm32_adc *adc = iio_priv(indio_dev);
-   int ret;
 
-   if (state) {
-   /* Reset adc buffer index */
-   adc->bufi = 0;
+   /* invoques iio_trigger_poll() from hard irq context */
+   irq_work_queue(>work);
+}
+
+static int stm32_adc_buffer_alloc_dma_start(struct iio_dev *indio_dev)
+{
+   struct stm32_adc *adc = iio_priv(indio_dev);
+   const struct stm32_adc_reginfo *reginfo =
+   adc->common->data->adc_reginfo;
+   struct dma_async_tx_descriptor *desc;
+   struct dma_slave_config config;
+   dma_cookie_t cookie;
+   int ret, size, watermark;
 
+   /* Reset adc buffer index */
+   adc->bufi = 0;
+
+   if (!adc->dma_chan) {
/* Allocate adc buffer */
adc->buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
if (!adc->buffer)
return -ENOMEM;
 
+   return 0;
+   }
+
+   /*
+* Allocate at least twice the buffer size for dma cyclic transfers, so
+* we can work with at least two dma periods. There should be :
+* - always one buffer (period) dma is working on
+* - one 

[PATCH 03/10] iio: adc: stm32: add optional dma support

[Fabrice Gasnier]

Add optional DMA support to STM32 ADC.
Use dma cyclic mode with at least two periods.

Signed-off-by: Fabrice Gasnier 
---
 drivers/iio/adc/stm32/Kconfig   |   2 +
 drivers/iio/adc/stm32/stm32-adc.c   | 222 
 drivers/iio/adc/stm32/stm32-adc.h   |  15 +++
 drivers/iio/adc/stm32/stm32f4-adc.c |  20 +++-
 4 files changed, 235 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/stm32/Kconfig b/drivers/iio/adc/stm32/Kconfig
index 245d037..5554ac8 100644
--- a/drivers/iio/adc/stm32/Kconfig
+++ b/drivers/iio/adc/stm32/Kconfig
@@ -8,6 +8,7 @@ config STM32_ADC
select REGULATOR_FIXED_VOLTAGE
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER
+   select IRQ_WORK
help
  Say yes here to build the driver for the STMicroelectronics
  STM32 analog-to-digital converter (ADC).
@@ -18,6 +19,7 @@ config STM32_ADC
 config STM32F4_ADC
tristate "STMicroelectronics STM32F4 adc"
depends on ARCH_STM32 || COMPILE_TEST
+   depends on HAS_DMA
depends on OF
select STM32_ADC
help
diff --git a/drivers/iio/adc/stm32/stm32-adc.c 
b/drivers/iio/adc/stm32/stm32-adc.c
index 1e0850d..25d0307 100644
--- a/drivers/iio/adc/stm32/stm32-adc.c
+++ b/drivers/iio/adc/stm32/stm32-adc.c
@@ -21,6 +21,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -371,6 +372,34 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
return ret;
 }
 
+static int stm32_adc_residue(struct stm32_adc *adc)
+{
+   struct dma_tx_state state;
+   enum dma_status status;
+
+   if (!adc->rx_buf)
+   return 0;
+
+   status = dmaengine_tx_status(adc->dma_chan,
+adc->dma_chan->cookie,
+);
+   if (status == DMA_IN_PROGRESS) {
+   /* Residue is size in bytes from end of buffer */
+   int i = adc->rx_buf_sz - state.residue;
+   int size;
+
+   /* Return available bytes */
+   if (i >= adc->bufi)
+   size = i - adc->bufi;
+   else
+   size = adc->rx_buf_sz - adc->bufi + i;
+
+   return size;
+   }
+
+   return 0;
+}
+
 /**
  * stm32_adc_isr() - Treat interrupt for one ADC instance within ADC block
  */
@@ -394,8 +423,8 @@ static irqreturn_t stm32_adc_isr(struct stm32_adc *adc)
stm32_adc_writel(adc, reginfo->isr, status & ~clr_mask);
status &= mask;
 
-   /* Regular data */
-   if (status & reginfo->eoc) {
+   /* Regular data (when dma isn't used) */
+   if ((status & reginfo->eoc) && (!adc->rx_buf)) {
adc->buffer[adc->bufi] = stm32_adc_readl(adc, reginfo->dr);
if (iio_buffer_enabled(indio_dev)) {
adc->bufi++;
@@ -553,29 +582,154 @@ static int stm32_adc_validate_device(struct iio_trigger 
*trig,
return indio != indio_dev ? -EINVAL : 0;
 }
 
-static int stm32_adc_set_trigger_state(struct iio_trigger *trig,
-  bool state)
+static void stm32_adc_dma_irq_work(struct irq_work *work)
 {
-   struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+   struct stm32_adc *adc = container_of(work, struct stm32_adc, work);
+   struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+
+   /**
+* iio_trigger_poll calls generic_handle_irq(). So, it requires hard
+* irq context, and cannot be called directly from dma callback,
+* dma cb has to schedule this work instead.
+*/
+   iio_trigger_poll(indio_dev->trig);
+}
+
+static void stm32_adc_dma_buffer_done(void *data)
+{
+   struct iio_dev *indio_dev = data;
struct stm32_adc *adc = iio_priv(indio_dev);
-   int ret;
 
-   if (state) {
-   /* Reset adc buffer index */
-   adc->bufi = 0;
+   /* invoques iio_trigger_poll() from hard irq context */
+   irq_work_queue(>work);
+}
+
+static int stm32_adc_buffer_alloc_dma_start(struct iio_dev *indio_dev)
+{
+   struct stm32_adc *adc = iio_priv(indio_dev);
+   const struct stm32_adc_reginfo *reginfo =
+   adc->common->data->adc_reginfo;
+   struct dma_async_tx_descriptor *desc;
+   struct dma_slave_config config;
+   dma_cookie_t cookie;
+   int ret, size, watermark;
 
+   /* Reset adc buffer index */
+   adc->bufi = 0;
+
+   if (!adc->dma_chan) {
/* Allocate adc buffer */
adc->buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
if (!adc->buffer)
return -ENOMEM;
 
+   return 0;
+   }
+
+   /*
+* Allocate at least twice the buffer size for dma cyclic transfers, so
+* we can work with at least two dma periods. There should be :
+* - always one buffer (period) dma is working on
+* - one buffer (period) driver