* [PATCH 1/7] iio: adc: stm32: add support for triggered buffer mode
2017-01-19 13:34 [PATCH 0/7] Add support for triggered buffer mode to STM32 ADC Fabrice Gasnier
@ 2017-01-19 13:34 ` Fabrice Gasnier
2017-01-22 12:53 ` Jonathan Cameron
2017-01-19 13:34 ` [PATCH 2/7] iio: adc: stm32: Enable use of stm32 timer triggers Fabrice Gasnier
` (5 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Fabrice Gasnier @ 2017-01-19 13:34 UTC (permalink / raw)
To: jic23, linux, robh+dt, linux-arm-kernel, devicetree, linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, fabrice.gasnier, benjamin.gaignard,
benjamin.gaignard
STM32 ADC conversions can be launched using hardware triggers.
It can be used to start conversion sequences (group of channels).
Selected channels are select via sequence registers.
Trigger source is selected via 'extsel' (external trigger mux).
Trigger polarity is set to rising edge by default.
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
drivers/iio/adc/Kconfig | 2 +
drivers/iio/adc/stm32-adc.c | 349 +++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 348 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 9c8b558..33341f4 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -446,6 +446,8 @@ config STM32_ADC_CORE
depends on ARCH_STM32 || COMPILE_TEST
depends on OF
depends on REGULATOR
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
help
Select this option to enable the core driver for STMicroelectronics
STM32 analog-to-digital converter (ADC).
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 5715e79..8d0b74b 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -22,11 +22,16 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/of.h>
+#include <linux/slab.h>
#include "stm32-adc-core.h"
@@ -58,21 +63,66 @@
/* STM32F4_ADC_CR2 - bit fields */
#define STM32F4_SWSTART BIT(30)
+#define STM32F4_EXTEN_SHIFT 28
#define STM32F4_EXTEN_MASK GENMASK(29, 28)
+#define STM32F4_EXTSEL_SHIFT 24
+#define STM32F4_EXTSEL_MASK GENMASK(27, 24)
#define STM32F4_EOCS BIT(10)
#define STM32F4_ADON BIT(0)
/* STM32F4_ADC_SQR1 - bit fields */
#define STM32F4_L_SHIFT 20
#define STM32F4_L_MASK GENMASK(23, 20)
+#define STM32F4_SQ16_SHIFT 15
+#define STM32F4_SQ16_MASK GENMASK(19, 15)
+#define STM32F4_SQ15_SHIFT 10
+#define STM32F4_SQ15_MASK GENMASK(14, 10)
+#define STM32F4_SQ14_SHIFT 5
+#define STM32F4_SQ14_MASK GENMASK(9, 5)
+#define STM32F4_SQ13_SHIFT 0
+#define STM32F4_SQ13_MASK GENMASK(4, 0)
+
+/* STM32F4_ADC_SQR2 - bit fields */
+#define STM32F4_SQ12_SHIFT 25
+#define STM32F4_SQ12_MASK GENMASK(29, 25)
+#define STM32F4_SQ11_SHIFT 20
+#define STM32F4_SQ11_MASK GENMASK(24, 20)
+#define STM32F4_SQ10_SHIFT 15
+#define STM32F4_SQ10_MASK GENMASK(19, 15)
+#define STM32F4_SQ9_SHIFT 10
+#define STM32F4_SQ9_MASK GENMASK(14, 10)
+#define STM32F4_SQ8_SHIFT 5
+#define STM32F4_SQ8_MASK GENMASK(9, 5)
+#define STM32F4_SQ7_SHIFT 0
+#define STM32F4_SQ7_MASK GENMASK(4, 0)
/* STM32F4_ADC_SQR3 - bit fields */
+#define STM32F4_SQ6_SHIFT 25
+#define STM32F4_SQ6_MASK GENMASK(29, 25)
+#define STM32F4_SQ5_SHIFT 20
+#define STM32F4_SQ5_MASK GENMASK(24, 20)
+#define STM32F4_SQ4_SHIFT 15
+#define STM32F4_SQ4_MASK GENMASK(19, 15)
+#define STM32F4_SQ3_SHIFT 10
+#define STM32F4_SQ3_MASK GENMASK(14, 10)
+#define STM32F4_SQ2_SHIFT 5
+#define STM32F4_SQ2_MASK GENMASK(9, 5)
#define STM32F4_SQ1_SHIFT 0
#define STM32F4_SQ1_MASK GENMASK(4, 0)
+#define STM32_ADC_MAX_SQ 16 /* SQ1..SQ16 */
#define STM32_ADC_TIMEOUT_US 100000
#define STM32_ADC_TIMEOUT (msecs_to_jiffies(STM32_ADC_TIMEOUT_US / 1000))
+/* External trigger enable */
+enum stm32_adc_exten {
+ STM32_EXTEN_SWTRIG,
+ STM32_EXTEN_HWTRIG_RISING_EDGE,
+ STM32_EXTEN_HWTRIG_FALLING_EDGE,
+ STM32_EXTEN_HWTRIG_BOTH_EDGES,
+};
+
+
/**
* struct stm32_adc - private data of each ADC IIO instance
* @common: reference to ADC block common data
@@ -82,6 +132,8 @@
* @clk: clock for this adc instance
* @irq: interrupt for this adc instance
* @lock: spinlock
+ * @bufi: data buffer index
+ * @num_conv: expected number of scan conversions
*/
struct stm32_adc {
struct stm32_adc_common *common;
@@ -91,6 +143,8 @@ struct stm32_adc {
struct clk *clk;
int irq;
spinlock_t lock; /* interrupt lock */
+ int bufi;
+ int num_conv;
};
/**
@@ -105,6 +159,18 @@ struct stm32_adc_chan_spec {
const char *name;
};
+/**
+ * stm32_adc_regs - stm32 ADC misc registers & bitfield desc
+ * @reg: register offset
+ * @mask: bitfield mask
+ * @shift: left shift
+ */
+struct stm32_adc_regs {
+ int reg;
+ int mask;
+ int shift;
+};
+
/* Input definitions common for all STM32F4 instances */
static const struct stm32_adc_chan_spec stm32f4_adc123_channels[] = {
{ IIO_VOLTAGE, 0, "in0" },
@@ -126,6 +192,33 @@ struct stm32_adc_chan_spec {
};
/**
+ * stm32f4_sqr_regs - describe regular sequence registers
+ * - L: sequence len (register & bit field)
+ * - SQ1..SQ16: sequence entries (register & bit field)
+ */
+static const struct stm32_adc_regs stm32f4_sqr_regs[STM32_ADC_MAX_SQ + 1] = {
+ /* L: len bit field description to be kept as first element */
+ { STM32F4_ADC_SQR1, STM32F4_L_MASK, STM32F4_L_SHIFT },
+ /* SQ1..SQ16 registers & bit fields */
+ { STM32F4_ADC_SQR3, STM32F4_SQ1_MASK, STM32F4_SQ1_SHIFT },
+ { STM32F4_ADC_SQR3, STM32F4_SQ2_MASK, STM32F4_SQ2_SHIFT },
+ { STM32F4_ADC_SQR3, STM32F4_SQ3_MASK, STM32F4_SQ3_SHIFT },
+ { STM32F4_ADC_SQR3, STM32F4_SQ4_MASK, STM32F4_SQ4_SHIFT },
+ { STM32F4_ADC_SQR3, STM32F4_SQ5_MASK, STM32F4_SQ5_SHIFT },
+ { STM32F4_ADC_SQR3, STM32F4_SQ6_MASK, STM32F4_SQ6_SHIFT },
+ { STM32F4_ADC_SQR2, STM32F4_SQ7_MASK, STM32F4_SQ7_SHIFT },
+ { STM32F4_ADC_SQR2, STM32F4_SQ8_MASK, STM32F4_SQ8_SHIFT },
+ { STM32F4_ADC_SQR2, STM32F4_SQ9_MASK, STM32F4_SQ9_SHIFT },
+ { STM32F4_ADC_SQR2, STM32F4_SQ10_MASK, STM32F4_SQ10_SHIFT },
+ { STM32F4_ADC_SQR2, STM32F4_SQ11_MASK, STM32F4_SQ11_SHIFT },
+ { STM32F4_ADC_SQR2, STM32F4_SQ12_MASK, STM32F4_SQ12_SHIFT },
+ { STM32F4_ADC_SQR1, STM32F4_SQ13_MASK, STM32F4_SQ13_SHIFT },
+ { STM32F4_ADC_SQR1, STM32F4_SQ14_MASK, STM32F4_SQ14_SHIFT },
+ { STM32F4_ADC_SQR1, STM32F4_SQ15_MASK, STM32F4_SQ15_SHIFT },
+ { STM32F4_ADC_SQR1, STM32F4_SQ16_MASK, STM32F4_SQ16_SHIFT },
+};
+
+/**
* STM32 ADC registers access routines
* @adc: stm32 adc instance
* @reg: reg offset in adc instance
@@ -211,6 +304,106 @@ static void stm32_adc_stop_conv(struct stm32_adc *adc)
}
/**
+ * stm32_adc_conf_scan_seq() - Build regular channels scan sequence
+ * @indio_dev: IIO device
+ * @scan_mask: channels to be converted
+ *
+ * Conversion sequence :
+ * Configure ADC scan sequence based on selected channels in scan_mask.
+ * Add channels to SQR registers, from scan_mask LSB to MSB, then
+ * program sequence len.
+ */
+static int stm32_adc_conf_scan_seq(struct iio_dev *indio_dev,
+ const unsigned long *scan_mask)
+{
+ struct stm32_adc *adc = iio_priv(indio_dev);
+ const struct iio_chan_spec *chan;
+ u32 val, bit;
+ int i = 0;
+
+ for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
+ chan = indio_dev->channels + bit;
+ /*
+ * Assign one channel per SQ entry in regular
+ * sequence, starting with SQ1.
+ */
+ i++;
+ if (i > STM32_ADC_MAX_SQ)
+ return -EINVAL;
+
+ dev_dbg(&indio_dev->dev, "%s chan %d to SQ%d\n",
+ __func__, chan->channel, i);
+
+ val = stm32_adc_readl(adc, stm32f4_sqr_regs[i].reg);
+ val &= ~stm32f4_sqr_regs[i].mask;
+ val |= chan->channel << stm32f4_sqr_regs[i].shift;
+ stm32_adc_writel(adc, stm32f4_sqr_regs[i].reg, val);
+ }
+
+ if (!i)
+ return -EINVAL;
+
+ /* Sequence len */
+ val = stm32_adc_readl(adc, stm32f4_sqr_regs[0].reg);
+ val &= ~stm32f4_sqr_regs[0].mask;
+ val |= ((i - 1) << stm32f4_sqr_regs[0].shift);
+ stm32_adc_writel(adc, stm32f4_sqr_regs[0].reg, val);
+
+ return 0;
+}
+
+/**
+ * stm32_adc_get_trig_extsel() - Get external trigger selection
+ * @indio_dev: IIO device
+ * @trig: trigger
+ *
+ * Returns trigger extsel value, if trig matches, -EINVAL otherwise.
+ */
+static int stm32_adc_get_trig_extsel(struct iio_dev *indio_dev,
+ struct iio_trigger *trig)
+{
+ return -EINVAL;
+}
+
+/**
+ * stm32_adc_set_trig() - Set a regular trigger
+ * @indio_dev: IIO device
+ * @trig: IIO trigger
+ *
+ * Set trigger source/polarity (e.g. SW, or HW with polarity) :
+ * - if HW trigger disabled (e.g. trig == NULL, conversion launched by sw)
+ * - if HW trigger enabled, set source & polarity
+ */
+static int stm32_adc_set_trig(struct iio_dev *indio_dev,
+ struct iio_trigger *trig)
+{
+ struct stm32_adc *adc = iio_priv(indio_dev);
+ u32 val, extsel = 0, exten = STM32_EXTEN_SWTRIG;
+ unsigned long flags;
+ int ret;
+
+ if (trig) {
+ ret = stm32_adc_get_trig_extsel(indio_dev, trig);
+ if (ret < 0)
+ return ret;
+
+ /* set trigger source, default to rising edge */
+ extsel = ret;
+ exten = STM32_EXTEN_HWTRIG_RISING_EDGE;
+ }
+
+ spin_lock_irqsave(&adc->lock, flags);
+ val = stm32_adc_readl(adc, STM32F4_ADC_CR2);
+ val &= ~(STM32F4_EXTEN_MASK | STM32F4_EXTSEL_MASK);
+ val |= exten << STM32F4_EXTEN_SHIFT;
+ val |= extsel << STM32F4_EXTSEL_SHIFT;
+ stm32_adc_writel(adc, STM32F4_ADC_CR2, val);
+ spin_unlock_irqrestore(&adc->lock, flags);
+
+ return 0;
+}
+
+/**
* stm32_adc_single_conv() - Performs a single conversion
* @indio_dev: IIO device
* @chan: IIO channel
@@ -234,6 +427,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
reinit_completion(&adc->completion);
adc->buffer = &result;
+ adc->bufi = 0;
/* Program chan number in regular sequence */
val = stm32_adc_readl(adc, STM32F4_ADC_SQR3);
@@ -301,17 +495,58 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
static irqreturn_t stm32_adc_isr(int irq, void *data)
{
struct stm32_adc *adc = data;
+ struct iio_dev *indio_dev = iio_priv_to_dev(adc);
u32 status = stm32_adc_readl(adc, STM32F4_ADC_SR);
if (status & STM32F4_EOC) {
- *adc->buffer = stm32_adc_readw(adc, STM32F4_ADC_DR);
- complete(&adc->completion);
+ adc->buffer[adc->bufi] = stm32_adc_readw(adc, STM32F4_ADC_DR);
+ if (iio_buffer_enabled(indio_dev)) {
+ adc->bufi++;
+ if (adc->bufi >= adc->num_conv) {
+ stm32_adc_conv_irq_disable(adc);
+ iio_trigger_poll(indio_dev->trig);
+ }
+ } else {
+ complete(&adc->completion);
+ }
return IRQ_HANDLED;
}
return IRQ_NONE;
}
+/**
+ * stm32_adc_validate_trigger() - validate trigger for stm32 adc
+ * @indio_dev: IIO device
+ * @trig: new trigger
+ *
+ * Returns: 0 if trig matches one of the triggers registered by stm32 adc
+ * driver, -EINVAL otherwise.
+ */
+static int stm32_adc_validate_trigger(struct iio_dev *indio_dev,
+ struct iio_trigger *trig)
+{
+ return stm32_adc_get_trig_extsel(indio_dev, trig) < 0 ? -EINVAL : 0;
+}
+
+static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev,
+ const unsigned long *scan_mask)
+{
+ struct stm32_adc *adc = iio_priv(indio_dev);
+ int ret;
+ u32 bit;
+
+ adc->num_conv = 0;
+ for_each_set_bit(bit, scan_mask, indio_dev->masklength)
+ adc->num_conv++;
+
+ ret = stm32_adc_conf_scan_seq(indio_dev, scan_mask);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
static int stm32_adc_of_xlate(struct iio_dev *indio_dev,
const struct of_phandle_args *iiospec)
{
@@ -350,11 +585,106 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
static const struct iio_info stm32_adc_iio_info = {
.read_raw = stm32_adc_read_raw,
+ .validate_trigger = stm32_adc_validate_trigger,
+ .update_scan_mode = stm32_adc_update_scan_mode,
.debugfs_reg_access = stm32_adc_debugfs_reg_access,
.of_xlate = stm32_adc_of_xlate,
.driver_module = THIS_MODULE,
};
+static int stm32_adc_buffer_preenable(struct iio_dev *indio_dev)
+{
+ struct stm32_adc *adc = iio_priv(indio_dev);
+
+ /* Reset adc buffer index */
+ adc->bufi = 0;
+
+ /* Allocate adc buffer */
+ adc->buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
+ if (!adc->buffer)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
+{
+ struct stm32_adc *adc = iio_priv(indio_dev);
+ int ret;
+
+ ret = stm32_adc_set_trig(indio_dev, indio_dev->trig);
+ if (ret) {
+ dev_err(&indio_dev->dev, "Can't set trigger\n");
+ return ret;
+ }
+
+ ret = iio_triggered_buffer_postenable(indio_dev);
+ if (ret < 0)
+ return ret;
+
+ stm32_adc_conv_irq_enable(adc);
+ stm32_adc_start_conv(adc);
+
+ return 0;
+}
+
+static int stm32_adc_buffer_predisable(struct iio_dev *indio_dev)
+{
+ struct stm32_adc *adc = iio_priv(indio_dev);
+ int ret;
+
+ stm32_adc_stop_conv(adc);
+ stm32_adc_conv_irq_disable(adc);
+
+ ret = iio_triggered_buffer_predisable(indio_dev);
+ if (ret < 0)
+ return ret;
+
+ ret = stm32_adc_set_trig(indio_dev, NULL);
+ if (ret)
+ dev_err(&indio_dev->dev, "Can't clear trigger\n");
+
+ return ret;
+}
+
+static int stm32_adc_buffer_postdisable(struct iio_dev *indio_dev)
+{
+ struct stm32_adc *adc = iio_priv(indio_dev);
+
+ kfree(adc->buffer);
+ adc->buffer = NULL;
+
+ return 0;
+}
+
+static const struct iio_buffer_setup_ops stm32_adc_buffer_setup_ops = {
+ .preenable = &stm32_adc_buffer_preenable,
+ .postenable = &stm32_adc_buffer_postenable,
+ .predisable = &stm32_adc_buffer_predisable,
+ .postdisable = &stm32_adc_buffer_postdisable,
+};
+
+static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct stm32_adc *adc = iio_priv(indio_dev);
+
+ dev_dbg(&indio_dev->dev, "%s bufi=%d\n", __func__, adc->bufi);
+
+ /* reset buffer index */
+ adc->bufi = 0;
+ iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
+ pf->timestamp);
+
+ iio_trigger_notify_done(indio_dev->trig);
+
+ /* re-enable eoc irq */
+ stm32_adc_conv_irq_enable(adc);
+
+ return IRQ_HANDLED;
+}
+
static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
struct iio_chan_spec *chan,
const struct stm32_adc_chan_spec *channel,
@@ -471,14 +801,26 @@ static int stm32_adc_probe(struct platform_device *pdev)
if (ret < 0)
goto err_clk_disable;
+ ret = iio_triggered_buffer_setup(indio_dev,
+ &iio_pollfunc_store_time,
+ &stm32_adc_trigger_handler,
+ &stm32_adc_buffer_setup_ops);
+ if (ret) {
+ dev_err(&pdev->dev, "buffer setup failed\n");
+ goto err_clk_disable;
+ }
+
ret = iio_device_register(indio_dev);
if (ret) {
dev_err(&pdev->dev, "iio dev register failed\n");
- goto err_clk_disable;
+ goto err_buffer_cleanup;
}
return 0;
+err_buffer_cleanup:
+ iio_triggered_buffer_cleanup(indio_dev);
+
err_clk_disable:
clk_disable_unprepare(adc->clk);
@@ -491,6 +833,7 @@ static int stm32_adc_remove(struct platform_device *pdev)
struct iio_dev *indio_dev = iio_priv_to_dev(adc);
iio_device_unregister(indio_dev);
+ iio_triggered_buffer_cleanup(indio_dev);
clk_disable_unprepare(adc->clk);
return 0;
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 1/7] iio: adc: stm32: add support for triggered buffer mode
2017-01-19 13:34 ` [PATCH 1/7] iio: adc: stm32: add support for triggered buffer mode Fabrice Gasnier
@ 2017-01-22 12:53 ` Jonathan Cameron
[not found] ` <2c683b78-6c78-3224-0646-37793deba0e9@st.com>
0 siblings, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2017-01-22 12:53 UTC (permalink / raw)
To: Fabrice Gasnier, linux, robh+dt, linux-arm-kernel, devicetree,
linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, benjamin.gaignard, benjamin.gaignard
On 19/01/17 13:34, Fabrice Gasnier wrote:
> STM32 ADC conversions can be launched using hardware triggers.
> It can be used to start conversion sequences (group of channels).
> Selected channels are select via sequence registers.
> Trigger source is selected via 'extsel' (external trigger mux).
> Trigger polarity is set to rising edge by default.
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
A few small bits and pieces inline. The only significant element is about
making the data flow conform a little better to our poorly documented :(
typical data flow. All about what the trigger is responsible for rather than
the device.
Looks pretty good to me!
Jonathan
> ---
> drivers/iio/adc/Kconfig | 2 +
> drivers/iio/adc/stm32-adc.c | 349 +++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 348 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 9c8b558..33341f4 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -446,6 +446,8 @@ config STM32_ADC_CORE
> depends on ARCH_STM32 || COMPILE_TEST
> depends on OF
> depends on REGULATOR
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> help
> Select this option to enable the core driver for STMicroelectronics
> STM32 analog-to-digital converter (ADC).
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 5715e79..8d0b74b 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -22,11 +22,16 @@
> #include <linux/clk.h>
> #include <linux/delay.h>
> #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/of.h>
> +#include <linux/slab.h>
>
> #include "stm32-adc-core.h"
>
> @@ -58,21 +63,66 @@
>
> /* STM32F4_ADC_CR2 - bit fields */
> #define STM32F4_SWSTART BIT(30)
> +#define STM32F4_EXTEN_SHIFT 28
> #define STM32F4_EXTEN_MASK GENMASK(29, 28)
> +#define STM32F4_EXTSEL_SHIFT 24
> +#define STM32F4_EXTSEL_MASK GENMASK(27, 24)
> #define STM32F4_EOCS BIT(10)
> #define STM32F4_ADON BIT(0)
>
> /* STM32F4_ADC_SQR1 - bit fields */
> #define STM32F4_L_SHIFT 20
> #define STM32F4_L_MASK GENMASK(23, 20)
> +#define STM32F4_SQ16_SHIFT 15
> +#define STM32F4_SQ16_MASK GENMASK(19, 15)
> +#define STM32F4_SQ15_SHIFT 10
> +#define STM32F4_SQ15_MASK GENMASK(14, 10)
> +#define STM32F4_SQ14_SHIFT 5
> +#define STM32F4_SQ14_MASK GENMASK(9, 5)
> +#define STM32F4_SQ13_SHIFT 0
> +#define STM32F4_SQ13_MASK GENMASK(4, 0)
> +
> +/* STM32F4_ADC_SQR2 - bit fields */
> +#define STM32F4_SQ12_SHIFT 25
> +#define STM32F4_SQ12_MASK GENMASK(29, 25)
> +#define STM32F4_SQ11_SHIFT 20
> +#define STM32F4_SQ11_MASK GENMASK(24, 20)
> +#define STM32F4_SQ10_SHIFT 15
> +#define STM32F4_SQ10_MASK GENMASK(19, 15)
> +#define STM32F4_SQ9_SHIFT 10
> +#define STM32F4_SQ9_MASK GENMASK(14, 10)
> +#define STM32F4_SQ8_SHIFT 5
> +#define STM32F4_SQ8_MASK GENMASK(9, 5)
> +#define STM32F4_SQ7_SHIFT 0
> +#define STM32F4_SQ7_MASK GENMASK(4, 0)
>
> /* STM32F4_ADC_SQR3 - bit fields */
> +#define STM32F4_SQ6_SHIFT 25
> +#define STM32F4_SQ6_MASK GENMASK(29, 25)
> +#define STM32F4_SQ5_SHIFT 20
> +#define STM32F4_SQ5_MASK GENMASK(24, 20)
> +#define STM32F4_SQ4_SHIFT 15
> +#define STM32F4_SQ4_MASK GENMASK(19, 15)
> +#define STM32F4_SQ3_SHIFT 10
> +#define STM32F4_SQ3_MASK GENMASK(14, 10)
> +#define STM32F4_SQ2_SHIFT 5
> +#define STM32F4_SQ2_MASK GENMASK(9, 5)
> #define STM32F4_SQ1_SHIFT 0
> #define STM32F4_SQ1_MASK GENMASK(4, 0)
>
Given all the above are only used in a big table which basically says what
the are, I wonder in this particular case if there is any real benefit to
using the defines? Might actually be clearer just to have the numbers
inline. What do you think? I'm not fussed either way.
> +#define STM32_ADC_MAX_SQ 16 /* SQ1..SQ16 */
> #define STM32_ADC_TIMEOUT_US 100000
> #define STM32_ADC_TIMEOUT (msecs_to_jiffies(STM32_ADC_TIMEOUT_US / 1000))
>
> +/* External trigger enable */
> +enum stm32_adc_exten {
> + STM32_EXTEN_SWTRIG,
> + STM32_EXTEN_HWTRIG_RISING_EDGE,
> + STM32_EXTEN_HWTRIG_FALLING_EDGE,
> + STM32_EXTEN_HWTRIG_BOTH_EDGES,
> +};
> +
> +
> /**
> * struct stm32_adc - private data of each ADC IIO instance
> * @common: reference to ADC block common data
> @@ -82,6 +132,8 @@
> * @clk: clock for this adc instance
> * @irq: interrupt for this adc instance
> * @lock: spinlock
> + * @bufi: data buffer index
> + * @num_conv: expected number of scan conversions
> */
> struct stm32_adc {
> struct stm32_adc_common *common;
> @@ -91,6 +143,8 @@ struct stm32_adc {
> struct clk *clk;
> int irq;
> spinlock_t lock; /* interrupt lock */
> + int bufi;
> + int num_conv;
> };
>
> /**
> @@ -105,6 +159,18 @@ struct stm32_adc_chan_spec {
> const char *name;
> };
>
> +/**
> + * stm32_adc_regs - stm32 ADC misc registers & bitfield desc
> + * @reg: register offset
> + * @mask: bitfield mask
> + * @shift: left shift
> + */
> +struct stm32_adc_regs {
> + int reg;
> + int mask;
> + int shift;
You could use the FIELD_PREP macro to avoid having to store both the mask
and shift. Up to you though as definitely a matter for personal taste!
> +};
> +
> /* Input definitions common for all STM32F4 instances */
> static const struct stm32_adc_chan_spec stm32f4_adc123_channels[] = {
> { IIO_VOLTAGE, 0, "in0" },
> @@ -126,6 +192,33 @@ struct stm32_adc_chan_spec {
> };
>
> /**
> + * stm32f4_sqr_regs - describe regular sequence registers
> + * - L: sequence len (register & bit field)
> + * - SQ1..SQ16: sequence entries (register & bit field)
> + */
> +static const struct stm32_adc_regs stm32f4_sqr_regs[STM32_ADC_MAX_SQ + 1] = {
> + /* L: len bit field description to be kept as first element */
> + { STM32F4_ADC_SQR1, STM32F4_L_MASK, STM32F4_L_SHIFT },
> + /* SQ1..SQ16 registers & bit fields */
> + { STM32F4_ADC_SQR3, STM32F4_SQ1_MASK, STM32F4_SQ1_SHIFT },
> + { STM32F4_ADC_SQR3, STM32F4_SQ2_MASK, STM32F4_SQ2_SHIFT },
> + { STM32F4_ADC_SQR3, STM32F4_SQ3_MASK, STM32F4_SQ3_SHIFT },
> + { STM32F4_ADC_SQR3, STM32F4_SQ4_MASK, STM32F4_SQ4_SHIFT },
> + { STM32F4_ADC_SQR3, STM32F4_SQ5_MASK, STM32F4_SQ5_SHIFT },
> + { STM32F4_ADC_SQR3, STM32F4_SQ6_MASK, STM32F4_SQ6_SHIFT },
> + { STM32F4_ADC_SQR2, STM32F4_SQ7_MASK, STM32F4_SQ7_SHIFT },
> + { STM32F4_ADC_SQR2, STM32F4_SQ8_MASK, STM32F4_SQ8_SHIFT },
> + { STM32F4_ADC_SQR2, STM32F4_SQ9_MASK, STM32F4_SQ9_SHIFT },
> + { STM32F4_ADC_SQR2, STM32F4_SQ10_MASK, STM32F4_SQ10_SHIFT },
> + { STM32F4_ADC_SQR2, STM32F4_SQ11_MASK, STM32F4_SQ11_SHIFT },
> + { STM32F4_ADC_SQR2, STM32F4_SQ12_MASK, STM32F4_SQ12_SHIFT },
> + { STM32F4_ADC_SQR1, STM32F4_SQ13_MASK, STM32F4_SQ13_SHIFT },
> + { STM32F4_ADC_SQR1, STM32F4_SQ14_MASK, STM32F4_SQ14_SHIFT },
> + { STM32F4_ADC_SQR1, STM32F4_SQ15_MASK, STM32F4_SQ15_SHIFT },
> + { STM32F4_ADC_SQR1, STM32F4_SQ16_MASK, STM32F4_SQ16_SHIFT },
> +};
> +
> +/**
> * STM32 ADC registers access routines
> * @adc: stm32 adc instance
> * @reg: reg offset in adc instance
> @@ -211,6 +304,106 @@ static void stm32_adc_stop_conv(struct stm32_adc *adc)
> }
>
> /**
> + * stm32_adc_conf_scan_seq() - Build regular channels scan sequence
> + * @indio_dev: IIO device
> + * @scan_mask: channels to be converted
> + *
> + * Conversion sequence :
> + * Configure ADC scan sequence based on selected channels in scan_mask.
> + * Add channels to SQR registers, from scan_mask LSB to MSB, then
> + * program sequence len.
> + */
> +static int stm32_adc_conf_scan_seq(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> + const struct iio_chan_spec *chan;
> + u32 val, bit;
> + int i = 0;
> +
> + for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
> + chan = indio_dev->channels + bit;
> + /*
> + * Assign one channel per SQ entry in regular
> + * sequence, starting with SQ1.
> + */
> + i++;
> + if (i > STM32_ADC_MAX_SQ)
> + return -EINVAL;
> +
> + dev_dbg(&indio_dev->dev, "%s chan %d to SQ%d\n",
> + __func__, chan->channel, i);
> +
> + val = stm32_adc_readl(adc, stm32f4_sqr_regs[i].reg);
> + val &= ~stm32f4_sqr_regs[i].mask;
> + val |= chan->channel << stm32f4_sqr_regs[i].shift;
> + stm32_adc_writel(adc, stm32f4_sqr_regs[i].reg, val);
> + }
> +
> + if (!i)
> + return -EINVAL;
> +
> + /* Sequence len */
> + val = stm32_adc_readl(adc, stm32f4_sqr_regs[0].reg);
> + val &= ~stm32f4_sqr_regs[0].mask;
> + val |= ((i - 1) << stm32f4_sqr_regs[0].shift);
> + stm32_adc_writel(adc, stm32f4_sqr_regs[0].reg, val);
> +
> + return 0;
> +}
> +
> +/**
> + * stm32_adc_get_trig_extsel() - Get external trigger selection
> + * @indio_dev: IIO device
> + * @trig: trigger
> + *
> + * Returns trigger extsel value, if trig matches, -EINVAL otherwise.
> + */
> +static int stm32_adc_get_trig_extsel(struct iio_dev *indio_dev,
> + struct iio_trigger *trig)
> +{
> + return -EINVAL;
?????
> +}
> +
> +/**
> + * stm32_adc_set_trig() - Set a regular trigger
> + * @indio_dev: IIO device
> + * @trig: IIO trigger
> + *
> + * Set trigger source/polarity (e.g. SW, or HW with polarity) :
> + * - if HW trigger disabled (e.g. trig == NULL, conversion launched by sw)
> + * - if HW trigger enabled, set source & polarity
> + */
> +static int stm32_adc_set_trig(struct iio_dev *indio_dev,
> + struct iio_trigger *trig)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> + u32 val, extsel = 0, exten = STM32_EXTEN_SWTRIG;
> + unsigned long flags;
> + int ret;
> +
> + if (trig) {
> + ret = stm32_adc_get_trig_extsel(indio_dev, trig);
> + if (ret < 0)
> + return ret;
> +
> + /* set trigger source, default to rising edge */
Interesting. The complexity of this device kicks in again.
If we are talking true exposed hardware pin (which I think it can be?) then
this ought to come from DT. For the case of trigging on either edge of the
pwm style signals, we probably want to figure some way of allowing userspace
to control this. For now in either case this is fine though! Got to start
somewhere.
> + extsel = ret;
> + exten = STM32_EXTEN_HWTRIG_RISING_EDGE;
> + }
> +
> + spin_lock_irqsave(&adc->lock, flags);
> + val = stm32_adc_readl(adc, STM32F4_ADC_CR2);
> + val &= ~(STM32F4_EXTEN_MASK | STM32F4_EXTSEL_MASK);
> + val |= exten << STM32F4_EXTEN_SHIFT;
> + val |= extsel << STM32F4_EXTSEL_SHIFT;
> + stm32_adc_writel(adc, STM32F4_ADC_CR2, val);
> + spin_unlock_irqrestore(&adc->lock, flags);
> +
> + return 0;
> +}
> +
> +/**
> * stm32_adc_single_conv() - Performs a single conversion
> * @indio_dev: IIO device
> * @chan: IIO channel
> @@ -234,6 +427,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
> reinit_completion(&adc->completion);
>
> adc->buffer = &result;
> + adc->bufi = 0;
>
> /* Program chan number in regular sequence */
> val = stm32_adc_readl(adc, STM32F4_ADC_SQR3);
> @@ -301,17 +495,58 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
> static irqreturn_t stm32_adc_isr(int irq, void *data)
> {
> struct stm32_adc *adc = data;
> + struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> u32 status = stm32_adc_readl(adc, STM32F4_ADC_SR);
>
> if (status & STM32F4_EOC) {
> - *adc->buffer = stm32_adc_readw(adc, STM32F4_ADC_DR);
> - complete(&adc->completion);
Normally I'd have put the separation between trigger and device a bit
earlier and had the actual reads in the function called by iio_trigger_poll
(and here after the wait for completion for the sysfs reads).
I think it will make little practical difference, but it will conform better
to the data flow model (that kicks around in my head and has been in various
presentations others have done) of the trigger indicating either:
1. Data ready
2. Data should be ready - i.e. grab it now if on demand.
The device side is responsible for then actually doing the read.
Code wise I think this means moving a few lines but I may have missed some
details.
> + adc->buffer[adc->bufi] = stm32_adc_readw(adc, STM32F4_ADC_DR);
> + if (iio_buffer_enabled(indio_dev)) {
> + adc->bufi++;
> + if (adc->bufi >= adc->num_conv) {
> + stm32_adc_conv_irq_disable(adc);
> + iio_trigger_poll(indio_dev->trig);
> + }
> + } else {
> + complete(&adc->completion);
> + }
> return IRQ_HANDLED;
> }
>
> return IRQ_NONE;
> }
>
> +/**
> + * stm32_adc_validate_trigger() - validate trigger for stm32 adc
> + * @indio_dev: IIO device
> + * @trig: new trigger
> + *
> + * Returns: 0 if trig matches one of the triggers registered by stm32 adc
> + * driver, -EINVAL otherwise.
> + */
> +static int stm32_adc_validate_trigger(struct iio_dev *indio_dev,
> + struct iio_trigger *trig)
> +{
> + return stm32_adc_get_trig_extsel(indio_dev, trig) < 0 ? -EINVAL : 0;
> +}
> +
> +static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> + int ret;
> + u32 bit;
> +
> + adc->num_conv = 0;
> + for_each_set_bit(bit, scan_mask, indio_dev->masklength)
> + adc->num_conv++;
Isn't there a count bits function? bitmap_weight I think which using the
hamming weight.
> +
> + ret = stm32_adc_conf_scan_seq(indio_dev, scan_mask);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> static int stm32_adc_of_xlate(struct iio_dev *indio_dev,
> const struct of_phandle_args *iiospec)
> {
> @@ -350,11 +585,106 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
>
> static const struct iio_info stm32_adc_iio_info = {
> .read_raw = stm32_adc_read_raw,
> + .validate_trigger = stm32_adc_validate_trigger,
> + .update_scan_mode = stm32_adc_update_scan_mode,
> .debugfs_reg_access = stm32_adc_debugfs_reg_access,
> .of_xlate = stm32_adc_of_xlate,
> .driver_module = THIS_MODULE,
> };
>
> +static int stm32_adc_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> +
> + /* Reset adc buffer index */
> + adc->bufi = 0;
> +
> + /* Allocate adc buffer */
> + adc->buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
Is this big enough to justify a separate allocation? I'd be tempted to
just allocate the maximum possible size as part of the stm32_adc structure..
> + if (!adc->buffer)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> + int ret;
> +
> + ret = stm32_adc_set_trig(indio_dev, indio_dev->trig);
> + if (ret) {
> + dev_err(&indio_dev->dev, "Can't set trigger\n");
> + return ret;
> + }
> +
> + ret = iio_triggered_buffer_postenable(indio_dev);
> + if (ret < 0)
Handle unwinding of previous calls if this fails.
> + return ret;
> +
> + stm32_adc_conv_irq_enable(adc);
> + stm32_adc_start_conv(adc);
> +
> + return 0;
> +}
> +
> +static int stm32_adc_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> + int ret;
> +
> + stm32_adc_stop_conv(adc);
> + stm32_adc_conv_irq_disable(adc);
> +
> + ret = iio_triggered_buffer_predisable(indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = stm32_adc_set_trig(indio_dev, NULL);
> + if (ret)
> + dev_err(&indio_dev->dev, "Can't clear trigger\n");
> +
> + return ret;
> +}
> +
> +static int stm32_adc_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> +
> + kfree(adc->buffer);
> + adc->buffer = NULL;
> +
> + return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops stm32_adc_buffer_setup_ops = {
> + .preenable = &stm32_adc_buffer_preenable,
> + .postenable = &stm32_adc_buffer_postenable,
> + .predisable = &stm32_adc_buffer_predisable,
> + .postdisable = &stm32_adc_buffer_postdisable,
> +};
> +
> +static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct stm32_adc *adc = iio_priv(indio_dev);
> +
> + dev_dbg(&indio_dev->dev, "%s bufi=%d\n", __func__, adc->bufi);
> +
> + /* reset buffer index */
> + adc->bufi = 0;
> + iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
> + pf->timestamp);
> +
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + /* re-enable eoc irq */
> + stm32_adc_conv_irq_enable(adc);
> +
> + return IRQ_HANDLED;
> +}
> +
> static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
> struct iio_chan_spec *chan,
> const struct stm32_adc_chan_spec *channel,
> @@ -471,14 +801,26 @@ static int stm32_adc_probe(struct platform_device *pdev)
> if (ret < 0)
> goto err_clk_disable;
>
> + ret = iio_triggered_buffer_setup(indio_dev,
> + &iio_pollfunc_store_time,
> + &stm32_adc_trigger_handler,
> + &stm32_adc_buffer_setup_ops);
> + if (ret) {
> + dev_err(&pdev->dev, "buffer setup failed\n");
> + goto err_clk_disable;
> + }
> +
> ret = iio_device_register(indio_dev);
> if (ret) {
> dev_err(&pdev->dev, "iio dev register failed\n");
> - goto err_clk_disable;
> + goto err_buffer_cleanup;
> }
>
> return 0;
>
> +err_buffer_cleanup:
> + iio_triggered_buffer_cleanup(indio_dev);
> +
> err_clk_disable:
> clk_disable_unprepare(adc->clk);
>
> @@ -491,6 +833,7 @@ static int stm32_adc_remove(struct platform_device *pdev)
> struct iio_dev *indio_dev = iio_priv_to_dev(adc);
>
> iio_device_unregister(indio_dev);
> + iio_triggered_buffer_cleanup(indio_dev);
> clk_disable_unprepare(adc->clk);
>
> return 0;
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/7] iio: adc: stm32: Enable use of stm32 timer triggers
2017-01-19 13:34 [PATCH 0/7] Add support for triggered buffer mode to STM32 ADC Fabrice Gasnier
2017-01-19 13:34 ` [PATCH 1/7] iio: adc: stm32: add support for triggered buffer mode Fabrice Gasnier
@ 2017-01-19 13:34 ` Fabrice Gasnier
2017-01-19 23:31 ` kbuild test robot
2017-01-22 12:55 ` Jonathan Cameron
2017-01-19 13:34 ` [PATCH 3/7] iio: adc: stm32: add trigger polarity extended attribute Fabrice Gasnier
` (4 subsequent siblings)
6 siblings, 2 replies; 24+ messages in thread
From: Fabrice Gasnier @ 2017-01-19 13:34 UTC (permalink / raw)
To: jic23, linux, robh+dt, linux-arm-kernel, devicetree, linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, fabrice.gasnier, benjamin.gaignard,
benjamin.gaignard
STM32 ADC has external timer trigger sources. Use stm32 timer triggers
API (e.g. is_stm32_timer_trigger()) with local ADC lookup table to
validate a trigger can be used.
This also provides correct trigger selection value (e.g. extsel).
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
drivers/iio/adc/Kconfig | 2 ++
drivers/iio/adc/stm32-adc.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 33341f4..9a7b090 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -447,6 +447,8 @@ config STM32_ADC_CORE
depends on OF
depends on REGULATOR
select IIO_BUFFER
+ select MFD_STM32_TIMERS
+ select IIO_STM32_TIMER_TRIGGER
select IIO_TRIGGERED_BUFFER
help
Select this option to enable the core driver for STMicroelectronics
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 8d0b74b..30708bc 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -23,6 +23,7 @@
#include <linux/delay.h>
#include <linux/iio/iio.h>
#include <linux/iio/buffer.h>
+#include <linux/iio/timer/stm32-timer-trigger.h>
#include <linux/iio/trigger.h>
#include <linux/iio/trigger_consumer.h>
#include <linux/iio/triggered_buffer.h>
@@ -122,6 +123,35 @@ enum stm32_adc_exten {
STM32_EXTEN_HWTRIG_BOTH_EDGES,
};
+/* extsel - trigger mux selection value */
+enum stm32_adc_extsel {
+ STM32_EXT0,
+ STM32_EXT1,
+ STM32_EXT2,
+ STM32_EXT3,
+ STM32_EXT4,
+ STM32_EXT5,
+ STM32_EXT6,
+ STM32_EXT7,
+ STM32_EXT8,
+ STM32_EXT9,
+ STM32_EXT10,
+ STM32_EXT11,
+ STM32_EXT12,
+ STM32_EXT13,
+ STM32_EXT14,
+ STM32_EXT15,
+};
+
+/**
+ * struct stm32_adc_trig_info - ADC trigger info
+ * @name: name of the trigger, corresponding to its source
+ * @extsel: trigger selection
+ */
+struct stm32_adc_trig_info {
+ const char *name;
+ enum stm32_adc_extsel extsel;
+};
/**
* struct stm32_adc - private data of each ADC IIO instance
@@ -218,6 +248,26 @@ struct stm32_adc_regs {
{ STM32F4_ADC_SQR1, STM32F4_SQ16_MASK, STM32F4_SQ16_SHIFT },
};
+/* STM32F4 external trigger sources for all instances */
+static struct stm32_adc_trig_info stm32f4_adc_timer_trigs[] = {
+ { TIM1_CH1, STM32_EXT0 },
+ { TIM1_CH2, STM32_EXT1 },
+ { TIM1_CH3, STM32_EXT2 },
+ { TIM2_CH2, STM32_EXT3 },
+ { TIM2_CH3, STM32_EXT4 },
+ { TIM2_CH4, STM32_EXT5 },
+ { TIM2_TRGO, STM32_EXT6 },
+ { TIM3_CH1, STM32_EXT7 },
+ { TIM3_TRGO, STM32_EXT8 },
+ { TIM4_CH4, STM32_EXT9 },
+ { TIM5_CH1, STM32_EXT10 },
+ { TIM5_CH2, STM32_EXT11 },
+ { TIM5_CH3, STM32_EXT12 },
+ { TIM8_CH1, STM32_EXT13 },
+ { TIM8_TRGO, STM32_EXT14 },
+ {}, /* sentinel */
+};
+
/**
* STM32 ADC registers access routines
* @adc: stm32 adc instance
@@ -362,6 +412,16 @@ static int stm32_adc_conf_scan_seq(struct iio_dev *indio_dev,
static int stm32_adc_get_trig_extsel(struct iio_dev *indio_dev,
struct iio_trigger *trig)
{
+ int i;
+
+ /* lookup triggers registered by stm32 timer trigger driver */
+ for (i = 0; stm32f4_adc_timer_trigs[i].name; i++) {
+ if (is_stm32_timer_trigger(trig) &&
+ !strcmp(stm32f4_adc_timer_trigs[i].name, trig->name)) {
+ return stm32f4_adc_timer_trigs[i].extsel;
+ }
+ }
+
return -EINVAL;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 2/7] iio: adc: stm32: Enable use of stm32 timer triggers
2017-01-19 13:34 ` [PATCH 2/7] iio: adc: stm32: Enable use of stm32 timer triggers Fabrice Gasnier
@ 2017-01-19 23:31 ` kbuild test robot
2017-01-21 12:55 ` Jonathan Cameron
2017-01-22 12:55 ` Jonathan Cameron
1 sibling, 1 reply; 24+ messages in thread
From: kbuild test robot @ 2017-01-19 23:31 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: kbuild-all, jic23, linux, robh+dt, linux-arm-kernel, devicetree,
linux-kernel, linux-iio, mark.rutland, mcoquelin.stm32,
alexandre.torgue, lars, knaack.h, pmeerw, fabrice.gasnier,
benjamin.gaignard, benjamin.gaignard
[-- Attachment #1: Type: text/plain, Size: 1409 bytes --]
Hi Fabrice,
[auto build test ERROR on next-20170119]
[also build test ERROR on v4.10-rc4]
[cannot apply to iio/togreg robh/for-next v4.9-rc8 v4.9-rc7 v4.9-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Fabrice-Gasnier/Add-support-for-triggered-buffer-mode-to-STM32-ADC/20170120-062214
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
>> drivers/iio/adc/stm32-adc.c:26:49: fatal error: linux/iio/timer/stm32-timer-trigger.h: No such file or directory
#include <linux/iio/timer/stm32-timer-trigger.h>
^
compilation terminated.
vim +26 drivers/iio/adc/stm32-adc.c
20 */
21
22 #include <linux/clk.h>
23 #include <linux/delay.h>
24 #include <linux/iio/iio.h>
25 #include <linux/iio/buffer.h>
> 26 #include <linux/iio/timer/stm32-timer-trigger.h>
27 #include <linux/iio/trigger.h>
28 #include <linux/iio/trigger_consumer.h>
29 #include <linux/iio/triggered_buffer.h>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58121 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/7] iio: adc: stm32: Enable use of stm32 timer triggers
2017-01-19 23:31 ` kbuild test robot
@ 2017-01-21 12:55 ` Jonathan Cameron
0 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2017-01-21 12:55 UTC (permalink / raw)
To: kbuild test robot, Fabrice Gasnier
Cc: kbuild-all, linux, robh+dt, linux-arm-kernel, devicetree,
linux-kernel, linux-iio, mark.rutland, mcoquelin.stm32,
alexandre.torgue, lars, knaack.h, pmeerw, benjamin.gaignard,
benjamin.gaignard
On 19/01/17 23:31, kbuild test robot wrote:
> Hi Fabrice,
>
> [auto build test ERROR on next-20170119]
> [also build test ERROR on v4.10-rc4]
> [cannot apply to iio/togreg robh/for-next v4.9-rc8 v4.9-rc7 v4.9-rc6]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Fabrice-Gasnier/Add-support-for-triggered-buffer-mode-to-STM32-ADC/20170120-062214
> config: i386-allmodconfig (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386
There is a precursor patch set mentioned in the cover letter. Not sure there is
a sensible automated way to deal with that!
Jonathan
>
> All errors (new ones prefixed by >>):
>
>>> drivers/iio/adc/stm32-adc.c:26:49: fatal error: linux/iio/timer/stm32-timer-trigger.h: No such file or directory
> #include <linux/iio/timer/stm32-timer-trigger.h>
> ^
> compilation terminated.
>
> vim +26 drivers/iio/adc/stm32-adc.c
>
> 20 */
> 21
> 22 #include <linux/clk.h>
> 23 #include <linux/delay.h>
> 24 #include <linux/iio/iio.h>
> 25 #include <linux/iio/buffer.h>
> > 26 #include <linux/iio/timer/stm32-timer-trigger.h>
> 27 #include <linux/iio/trigger.h>
> 28 #include <linux/iio/trigger_consumer.h>
> 29 #include <linux/iio/triggered_buffer.h>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/7] iio: adc: stm32: Enable use of stm32 timer triggers
2017-01-19 13:34 ` [PATCH 2/7] iio: adc: stm32: Enable use of stm32 timer triggers Fabrice Gasnier
2017-01-19 23:31 ` kbuild test robot
@ 2017-01-22 12:55 ` Jonathan Cameron
2017-01-24 14:37 ` Fabrice Gasnier
1 sibling, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2017-01-22 12:55 UTC (permalink / raw)
To: Fabrice Gasnier, linux, robh+dt, linux-arm-kernel, devicetree,
linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, benjamin.gaignard, benjamin.gaignard
On 19/01/17 13:34, Fabrice Gasnier wrote:
> STM32 ADC has external timer trigger sources. Use stm32 timer triggers
> API (e.g. is_stm32_timer_trigger()) with local ADC lookup table to
> validate a trigger can be used.
> This also provides correct trigger selection value (e.g. extsel).
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Looks good. Observations inline.
> ---
> drivers/iio/adc/Kconfig | 2 ++
> drivers/iio/adc/stm32-adc.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 62 insertions(+)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 33341f4..9a7b090 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -447,6 +447,8 @@ config STM32_ADC_CORE
> depends on OF
> depends on REGULATOR
> select IIO_BUFFER
> + select MFD_STM32_TIMERS
> + select IIO_STM32_TIMER_TRIGGER
> select IIO_TRIGGERED_BUFFER
> help
> Select this option to enable the core driver for STMicroelectronics
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 8d0b74b..30708bc 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -23,6 +23,7 @@
> #include <linux/delay.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/buffer.h>
> +#include <linux/iio/timer/stm32-timer-trigger.h>
> #include <linux/iio/trigger.h>
> #include <linux/iio/trigger_consumer.h>
> #include <linux/iio/triggered_buffer.h>
> @@ -122,6 +123,35 @@ enum stm32_adc_exten {
> STM32_EXTEN_HWTRIG_BOTH_EDGES,
> };
>
> +/* extsel - trigger mux selection value */
> +enum stm32_adc_extsel {
> + STM32_EXT0,
> + STM32_EXT1,
> + STM32_EXT2,
> + STM32_EXT3,
> + STM32_EXT4,
> + STM32_EXT5,
> + STM32_EXT6,
> + STM32_EXT7,
> + STM32_EXT8,
> + STM32_EXT9,
> + STM32_EXT10,
> + STM32_EXT11,
> + STM32_EXT12,
> + STM32_EXT13,
> + STM32_EXT14,
> + STM32_EXT15,
> +};
> +
> +/**
> + * struct stm32_adc_trig_info - ADC trigger info
> + * @name: name of the trigger, corresponding to its source
> + * @extsel: trigger selection
> + */
> +struct stm32_adc_trig_info {
> + const char *name;
> + enum stm32_adc_extsel extsel;
> +};
>
> /**
> * struct stm32_adc - private data of each ADC IIO instance
> @@ -218,6 +248,26 @@ struct stm32_adc_regs {
> { STM32F4_ADC_SQR1, STM32F4_SQ16_MASK, STM32F4_SQ16_SHIFT },
> };
>
> +/* STM32F4 external trigger sources for all instances */
> +static struct stm32_adc_trig_info stm32f4_adc_timer_trigs[] = {
> + { TIM1_CH1, STM32_EXT0 },
> + { TIM1_CH2, STM32_EXT1 },
> + { TIM1_CH3, STM32_EXT2 },
> + { TIM2_CH2, STM32_EXT3 },
> + { TIM2_CH3, STM32_EXT4 },
> + { TIM2_CH4, STM32_EXT5 },
> + { TIM2_TRGO, STM32_EXT6 },
> + { TIM3_CH1, STM32_EXT7 },
> + { TIM3_TRGO, STM32_EXT8 },
> + { TIM4_CH4, STM32_EXT9 },
> + { TIM5_CH1, STM32_EXT10 },
> + { TIM5_CH2, STM32_EXT11 },
> + { TIM5_CH3, STM32_EXT12 },
> + { TIM8_CH1, STM32_EXT13 },
> + { TIM8_TRGO, STM32_EXT14 },
> + {}, /* sentinel */
> +};
> +
> /**
> * STM32 ADC registers access routines
> * @adc: stm32 adc instance
> @@ -362,6 +412,16 @@ static int stm32_adc_conf_scan_seq(struct iio_dev *indio_dev,
> static int stm32_adc_get_trig_extsel(struct iio_dev *indio_dev,
> struct iio_trigger *trig)
> {
> + int i;
Ah. This makes more sense than patch 1 on it's own did.
> +
> + /* lookup triggers registered by stm32 timer trigger driver */
> + for (i = 0; stm32f4_adc_timer_trigs[i].name; i++) {
> + if (is_stm32_timer_trigger(trig) &&
> + !strcmp(stm32f4_adc_timer_trigs[i].name, trig->name)) {
> + return stm32f4_adc_timer_trigs[i].extsel;
Good. The combination of the first check and the name match should make this safe
against those triggers that can be assigned arbitrary names.
> + }
> + }
> +
> return -EINVAL;
> }
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/7] iio: adc: stm32: Enable use of stm32 timer triggers
2017-01-22 12:55 ` Jonathan Cameron
@ 2017-01-24 14:37 ` Fabrice Gasnier
2017-01-28 12:46 ` Jonathan Cameron
0 siblings, 1 reply; 24+ messages in thread
From: Fabrice Gasnier @ 2017-01-24 14:37 UTC (permalink / raw)
To: Jonathan Cameron, linux, robh+dt, linux-arm-kernel, devicetree,
linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, benjamin.gaignard, benjamin.gaignard
On 01/22/2017 01:55 PM, Jonathan Cameron wrote:
> On 19/01/17 13:34, Fabrice Gasnier wrote:
>> STM32 ADC has external timer trigger sources. Use stm32 timer triggers
>> API (e.g. is_stm32_timer_trigger()) with local ADC lookup table to
>> validate a trigger can be used.
>> This also provides correct trigger selection value (e.g. extsel).
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Looks good. Observations inline.
>> ---
>> drivers/iio/adc/Kconfig | 2 ++
>> drivers/iio/adc/stm32-adc.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 62 insertions(+)
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 33341f4..9a7b090 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -447,6 +447,8 @@ config STM32_ADC_CORE
>> depends on OF
>> depends on REGULATOR
>> select IIO_BUFFER
>> + select MFD_STM32_TIMERS
>> + select IIO_STM32_TIMER_TRIGGER
>> select IIO_TRIGGERED_BUFFER
>> help
>> Select this option to enable the core driver for STMicroelectronics
>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
>> index 8d0b74b..30708bc 100644
>> --- a/drivers/iio/adc/stm32-adc.c
>> +++ b/drivers/iio/adc/stm32-adc.c
>> @@ -23,6 +23,7 @@
>> #include <linux/delay.h>
>> #include <linux/iio/iio.h>
>> #include <linux/iio/buffer.h>
>> +#include <linux/iio/timer/stm32-timer-trigger.h>
>> #include <linux/iio/trigger.h>
>> #include <linux/iio/trigger_consumer.h>
>> #include <linux/iio/triggered_buffer.h>
>> @@ -122,6 +123,35 @@ enum stm32_adc_exten {
>> STM32_EXTEN_HWTRIG_BOTH_EDGES,
>> };
>>
>> +/* extsel - trigger mux selection value */
>> +enum stm32_adc_extsel {
>> + STM32_EXT0,
>> + STM32_EXT1,
>> + STM32_EXT2,
>> + STM32_EXT3,
>> + STM32_EXT4,
>> + STM32_EXT5,
>> + STM32_EXT6,
>> + STM32_EXT7,
>> + STM32_EXT8,
>> + STM32_EXT9,
>> + STM32_EXT10,
>> + STM32_EXT11,
>> + STM32_EXT12,
>> + STM32_EXT13,
>> + STM32_EXT14,
>> + STM32_EXT15,
>> +};
>> +
>> +/**
>> + * struct stm32_adc_trig_info - ADC trigger info
>> + * @name: name of the trigger, corresponding to its source
>> + * @extsel: trigger selection
>> + */
>> +struct stm32_adc_trig_info {
>> + const char *name;
>> + enum stm32_adc_extsel extsel;
>> +};
>>
>> /**
>> * struct stm32_adc - private data of each ADC IIO instance
>> @@ -218,6 +248,26 @@ struct stm32_adc_regs {
>> { STM32F4_ADC_SQR1, STM32F4_SQ16_MASK, STM32F4_SQ16_SHIFT },
>> };
>>
>> +/* STM32F4 external trigger sources for all instances */
>> +static struct stm32_adc_trig_info stm32f4_adc_timer_trigs[] = {
>> + { TIM1_CH1, STM32_EXT0 },
>> + { TIM1_CH2, STM32_EXT1 },
>> + { TIM1_CH3, STM32_EXT2 },
>> + { TIM2_CH2, STM32_EXT3 },
>> + { TIM2_CH3, STM32_EXT4 },
>> + { TIM2_CH4, STM32_EXT5 },
>> + { TIM2_TRGO, STM32_EXT6 },
>> + { TIM3_CH1, STM32_EXT7 },
>> + { TIM3_TRGO, STM32_EXT8 },
>> + { TIM4_CH4, STM32_EXT9 },
>> + { TIM5_CH1, STM32_EXT10 },
>> + { TIM5_CH2, STM32_EXT11 },
>> + { TIM5_CH3, STM32_EXT12 },
>> + { TIM8_CH1, STM32_EXT13 },
>> + { TIM8_TRGO, STM32_EXT14 },
>> + {}, /* sentinel */
>> +};
>> +
>> /**
>> * STM32 ADC registers access routines
>> * @adc: stm32 adc instance
>> @@ -362,6 +412,16 @@ static int stm32_adc_conf_scan_seq(struct iio_dev *indio_dev,
>> static int stm32_adc_get_trig_extsel(struct iio_dev *indio_dev,
>> struct iio_trigger *trig)
>> {
>> + int i;
> Ah. This makes more sense than patch 1 on it's own did.
>> +
>> + /* lookup triggers registered by stm32 timer trigger driver */
>> + for (i = 0; stm32f4_adc_timer_trigs[i].name; i++) {
>> + if (is_stm32_timer_trigger(trig) &&
>> + !strcmp(stm32f4_adc_timer_trigs[i].name, trig->name)) {
>> + return stm32f4_adc_timer_trigs[i].extsel;
> Good. The combination of the first check and the name match should make this safe
> against those triggers that can be assigned arbitrary names.
Do you wish I add a comment about it ?
Best Regards,
Fabrice
>> + }
>> + }
>> +
>> return -EINVAL;
>> }
>>
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/7] iio: adc: stm32: Enable use of stm32 timer triggers
2017-01-24 14:37 ` Fabrice Gasnier
@ 2017-01-28 12:46 ` Jonathan Cameron
0 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2017-01-28 12:46 UTC (permalink / raw)
To: Fabrice Gasnier, linux, robh+dt, linux-arm-kernel, devicetree,
linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, benjamin.gaignard, benjamin.gaignard
On 24/01/17 14:37, Fabrice Gasnier wrote:
> On 01/22/2017 01:55 PM, Jonathan Cameron wrote:
>> On 19/01/17 13:34, Fabrice Gasnier wrote:
>>> STM32 ADC has external timer trigger sources. Use stm32 timer triggers
>>> API (e.g. is_stm32_timer_trigger()) with local ADC lookup table to
>>> validate a trigger can be used.
>>> This also provides correct trigger selection value (e.g. extsel).
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> Looks good. Observations inline.
>>> ---
>>> drivers/iio/adc/Kconfig | 2 ++
>>> drivers/iio/adc/stm32-adc.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 62 insertions(+)
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 33341f4..9a7b090 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -447,6 +447,8 @@ config STM32_ADC_CORE
>>> depends on OF
>>> depends on REGULATOR
>>> select IIO_BUFFER
>>> + select MFD_STM32_TIMERS
>>> + select IIO_STM32_TIMER_TRIGGER
>>> select IIO_TRIGGERED_BUFFER
>>> help
>>> Select this option to enable the core driver for STMicroelectronics
>>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
>>> index 8d0b74b..30708bc 100644
>>> --- a/drivers/iio/adc/stm32-adc.c
>>> +++ b/drivers/iio/adc/stm32-adc.c
>>> @@ -23,6 +23,7 @@
>>> #include <linux/delay.h>
>>> #include <linux/iio/iio.h>
>>> #include <linux/iio/buffer.h>
>>> +#include <linux/iio/timer/stm32-timer-trigger.h>
>>> #include <linux/iio/trigger.h>
>>> #include <linux/iio/trigger_consumer.h>
>>> #include <linux/iio/triggered_buffer.h>
>>> @@ -122,6 +123,35 @@ enum stm32_adc_exten {
>>> STM32_EXTEN_HWTRIG_BOTH_EDGES,
>>> };
>>>
>>> +/* extsel - trigger mux selection value */
>>> +enum stm32_adc_extsel {
>>> + STM32_EXT0,
>>> + STM32_EXT1,
>>> + STM32_EXT2,
>>> + STM32_EXT3,
>>> + STM32_EXT4,
>>> + STM32_EXT5,
>>> + STM32_EXT6,
>>> + STM32_EXT7,
>>> + STM32_EXT8,
>>> + STM32_EXT9,
>>> + STM32_EXT10,
>>> + STM32_EXT11,
>>> + STM32_EXT12,
>>> + STM32_EXT13,
>>> + STM32_EXT14,
>>> + STM32_EXT15,
>>> +};
>>> +
>>> +/**
>>> + * struct stm32_adc_trig_info - ADC trigger info
>>> + * @name: name of the trigger, corresponding to its source
>>> + * @extsel: trigger selection
>>> + */
>>> +struct stm32_adc_trig_info {
>>> + const char *name;
>>> + enum stm32_adc_extsel extsel;
>>> +};
>>>
>>> /**
>>> * struct stm32_adc - private data of each ADC IIO instance
>>> @@ -218,6 +248,26 @@ struct stm32_adc_regs {
>>> { STM32F4_ADC_SQR1, STM32F4_SQ16_MASK, STM32F4_SQ16_SHIFT },
>>> };
>>>
>>> +/* STM32F4 external trigger sources for all instances */
>>> +static struct stm32_adc_trig_info stm32f4_adc_timer_trigs[] = {
>>> + { TIM1_CH1, STM32_EXT0 },
>>> + { TIM1_CH2, STM32_EXT1 },
>>> + { TIM1_CH3, STM32_EXT2 },
>>> + { TIM2_CH2, STM32_EXT3 },
>>> + { TIM2_CH3, STM32_EXT4 },
>>> + { TIM2_CH4, STM32_EXT5 },
>>> + { TIM2_TRGO, STM32_EXT6 },
>>> + { TIM3_CH1, STM32_EXT7 },
>>> + { TIM3_TRGO, STM32_EXT8 },
>>> + { TIM4_CH4, STM32_EXT9 },
>>> + { TIM5_CH1, STM32_EXT10 },
>>> + { TIM5_CH2, STM32_EXT11 },
>>> + { TIM5_CH3, STM32_EXT12 },
>>> + { TIM8_CH1, STM32_EXT13 },
>>> + { TIM8_TRGO, STM32_EXT14 },
>>> + {}, /* sentinel */
>>> +};
>>> +
>>> /**
>>> * STM32 ADC registers access routines
>>> * @adc: stm32 adc instance
>>> @@ -362,6 +412,16 @@ static int stm32_adc_conf_scan_seq(struct iio_dev *indio_dev,
>>> static int stm32_adc_get_trig_extsel(struct iio_dev *indio_dev,
>>> struct iio_trigger *trig)
>>> {
>>> + int i;
>> Ah. This makes more sense than patch 1 on it's own did.
>>> +
>>> + /* lookup triggers registered by stm32 timer trigger driver */
>>> + for (i = 0; stm32f4_adc_timer_trigs[i].name; i++) {
>>> + if (is_stm32_timer_trigger(trig) &&
>>> + !strcmp(stm32f4_adc_timer_trigs[i].name, trig->name)) {
>>> + return stm32f4_adc_timer_trigs[i].extsel;
>> Good. The combination of the first check and the name match should make this safe
>> against those triggers that can be assigned arbitrary names.
>
> Do you wish I add a comment about it ?
Nope. I just thought it was nicely done ;)
>
> Best Regards,
> Fabrice
>
>>> + }
>>> + }
>>> +
>>> return -EINVAL;
>>> }
>>>
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/7] iio: adc: stm32: add trigger polarity extended attribute
2017-01-19 13:34 [PATCH 0/7] Add support for triggered buffer mode to STM32 ADC Fabrice Gasnier
2017-01-19 13:34 ` [PATCH 1/7] iio: adc: stm32: add support for triggered buffer mode Fabrice Gasnier
2017-01-19 13:34 ` [PATCH 2/7] iio: adc: stm32: Enable use of stm32 timer triggers Fabrice Gasnier
@ 2017-01-19 13:34 ` Fabrice Gasnier
2017-01-22 12:58 ` Jonathan Cameron
2017-01-19 13:34 ` [PATCH 4/7] Documentation: dt: iio: stm32-adc: optional dma support Fabrice Gasnier
` (3 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Fabrice Gasnier @ 2017-01-19 13:34 UTC (permalink / raw)
To: jic23, linux, robh+dt, linux-arm-kernel, devicetree, linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, fabrice.gasnier, benjamin.gaignard,
benjamin.gaignard
Define extended attribute so that user may choose rising, falling or both
edges for external trigger sources.
Default to rising edge in case it isn't set.
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
drivers/iio/adc/stm32-adc.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 30708bc..9753c39 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -164,6 +164,7 @@ struct stm32_adc_trig_info {
* @lock: spinlock
* @bufi: data buffer index
* @num_conv: expected number of scan conversions
+ * @exten: external trigger config (enable/polarity)
*/
struct stm32_adc {
struct stm32_adc_common *common;
@@ -175,6 +176,7 @@ struct stm32_adc {
spinlock_t lock; /* interrupt lock */
int bufi;
int num_conv;
+ enum stm32_adc_exten exten;
};
/**
@@ -449,7 +451,9 @@ static int stm32_adc_set_trig(struct iio_dev *indio_dev,
/* set trigger source, default to rising edge */
extsel = ret;
- exten = STM32_EXTEN_HWTRIG_RISING_EDGE;
+ if (adc->exten == STM32_EXTEN_SWTRIG)
+ adc->exten = STM32_EXTEN_HWTRIG_RISING_EDGE;
+ exten = adc->exten;
}
spin_lock_irqsave(&adc->lock, flags);
@@ -463,6 +467,39 @@ static int stm32_adc_set_trig(struct iio_dev *indio_dev,
return 0;
}
+static int stm32_adc_set_trig_pol(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int type)
+{
+ struct stm32_adc *adc = iio_priv(indio_dev);
+
+ adc->exten = type;
+
+ return 0;
+}
+
+static int stm32_adc_get_trig_pol(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct stm32_adc *adc = iio_priv(indio_dev);
+
+ return adc->exten;
+}
+
+static const char * const stm32_trig_pol_items[] = {
+ [STM32_EXTEN_SWTRIG] = "swtrig",
+ [STM32_EXTEN_HWTRIG_RISING_EDGE] = "rising-edge",
+ [STM32_EXTEN_HWTRIG_FALLING_EDGE] = "falling-edge",
+ [STM32_EXTEN_HWTRIG_BOTH_EDGES] = "both-edges",
+};
+
+const struct iio_enum stm32_adc_trig_pol = {
+ .items = stm32_trig_pol_items,
+ .num_items = ARRAY_SIZE(stm32_trig_pol_items),
+ .get = stm32_adc_get_trig_pol,
+ .set = stm32_adc_set_trig_pol,
+};
+
/**
* stm32_adc_single_conv() - Performs a single conversion
* @indio_dev: IIO device
@@ -745,6 +782,17 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
return IRQ_HANDLED;
}
+static const struct iio_chan_spec_ext_info stm32_adc_ext_info[] = {
+ IIO_ENUM("trigger_pol", IIO_SHARED_BY_ALL, &stm32_adc_trig_pol),
+ {
+ .name = "trigger_pol_available",
+ .shared = IIO_SHARED_BY_ALL,
+ .read = iio_enum_available_read,
+ .private = (uintptr_t)&stm32_adc_trig_pol,
+ },
+ {},
+};
+
static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
struct iio_chan_spec *chan,
const struct stm32_adc_chan_spec *channel,
@@ -760,6 +808,7 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
chan->scan_type.sign = 'u';
chan->scan_type.realbits = 12;
chan->scan_type.storagebits = 16;
+ chan->ext_info = stm32_adc_ext_info;
}
static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 3/7] iio: adc: stm32: add trigger polarity extended attribute
2017-01-19 13:34 ` [PATCH 3/7] iio: adc: stm32: add trigger polarity extended attribute Fabrice Gasnier
@ 2017-01-22 12:58 ` Jonathan Cameron
2017-01-24 14:41 ` Fabrice Gasnier
0 siblings, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2017-01-22 12:58 UTC (permalink / raw)
To: Fabrice Gasnier, linux, robh+dt, linux-arm-kernel, devicetree,
linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, benjamin.gaignard, benjamin.gaignard
On 19/01/17 13:34, Fabrice Gasnier wrote:
> Define extended attribute so that user may choose rising, falling or both
> edges for external trigger sources.
> Default to rising edge in case it isn't set.
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Creates a custom attibute. Documentation please!
/Documentation/ABI/testing/sysfs-bus-iio-stm32 or similar.
The approach seems reasonable, but easier to discuss when it's
formally described.
I'd also not care about characters saved and go with
trigger_polarity rather than the short form (unless this you have
a good reason for it!)
Jonathan
> ---
> drivers/iio/adc/stm32-adc.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 30708bc..9753c39 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -164,6 +164,7 @@ struct stm32_adc_trig_info {
> * @lock: spinlock
> * @bufi: data buffer index
> * @num_conv: expected number of scan conversions
> + * @exten: external trigger config (enable/polarity)
> */
> struct stm32_adc {
> struct stm32_adc_common *common;
> @@ -175,6 +176,7 @@ struct stm32_adc {
> spinlock_t lock; /* interrupt lock */
> int bufi;
> int num_conv;
> + enum stm32_adc_exten exten;
> };
>
> /**
> @@ -449,7 +451,9 @@ static int stm32_adc_set_trig(struct iio_dev *indio_dev,
>
> /* set trigger source, default to rising edge */
> extsel = ret;
> - exten = STM32_EXTEN_HWTRIG_RISING_EDGE;
> + if (adc->exten == STM32_EXTEN_SWTRIG)
> + adc->exten = STM32_EXTEN_HWTRIG_RISING_EDGE;
> + exten = adc->exten;
> }
>
> spin_lock_irqsave(&adc->lock, flags);
> @@ -463,6 +467,39 @@ static int stm32_adc_set_trig(struct iio_dev *indio_dev,
> return 0;
> }
>
> +static int stm32_adc_set_trig_pol(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + unsigned int type)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> +
> + adc->exten = type;
> +
> + return 0;
> +}
> +
> +static int stm32_adc_get_trig_pol(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> +
> + return adc->exten;
> +}
> +
> +static const char * const stm32_trig_pol_items[] = {
> + [STM32_EXTEN_SWTRIG] = "swtrig",
> + [STM32_EXTEN_HWTRIG_RISING_EDGE] = "rising-edge",
> + [STM32_EXTEN_HWTRIG_FALLING_EDGE] = "falling-edge",
> + [STM32_EXTEN_HWTRIG_BOTH_EDGES] = "both-edges",
> +};
> +
> +const struct iio_enum stm32_adc_trig_pol = {
> + .items = stm32_trig_pol_items,
> + .num_items = ARRAY_SIZE(stm32_trig_pol_items),
> + .get = stm32_adc_get_trig_pol,
> + .set = stm32_adc_set_trig_pol,
> +};
> +
> /**
> * stm32_adc_single_conv() - Performs a single conversion
> * @indio_dev: IIO device
> @@ -745,6 +782,17 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
> return IRQ_HANDLED;
> }
>
> +static const struct iio_chan_spec_ext_info stm32_adc_ext_info[] = {
> + IIO_ENUM("trigger_pol", IIO_SHARED_BY_ALL, &stm32_adc_trig_pol),
> + {
> + .name = "trigger_pol_available",
> + .shared = IIO_SHARED_BY_ALL,
> + .read = iio_enum_available_read,
> + .private = (uintptr_t)&stm32_adc_trig_pol,
> + },
> + {},
> +};
> +
> static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
> struct iio_chan_spec *chan,
> const struct stm32_adc_chan_spec *channel,
> @@ -760,6 +808,7 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
> chan->scan_type.sign = 'u';
> chan->scan_type.realbits = 12;
> chan->scan_type.storagebits = 16;
> + chan->ext_info = stm32_adc_ext_info;
> }
>
> static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 3/7] iio: adc: stm32: add trigger polarity extended attribute
2017-01-22 12:58 ` Jonathan Cameron
@ 2017-01-24 14:41 ` Fabrice Gasnier
0 siblings, 0 replies; 24+ messages in thread
From: Fabrice Gasnier @ 2017-01-24 14:41 UTC (permalink / raw)
To: Jonathan Cameron, linux, robh+dt, linux-arm-kernel, devicetree,
linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, benjamin.gaignard, benjamin.gaignard
On 01/22/2017 01:58 PM, Jonathan Cameron wrote:
> On 19/01/17 13:34, Fabrice Gasnier wrote:
>> Define extended attribute so that user may choose rising, falling or both
>> edges for external trigger sources.
>> Default to rising edge in case it isn't set.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Creates a custom attibute. Documentation please!
> /Documentation/ABI/testing/sysfs-bus-iio-stm32 or similar.
Ok, I'll add something like:
Documentation/ABI/testing/sysfs-bus-iio-adc-stm32:
+What: /sys/bus/iio/devices/triggerX/trigger_polarity
+KernelVersion: 4.11
+Contact: fabrice.gasnier@st.com
+Description:
+ The STM32 ADC can be configured to use external trigger sources
+ (e.g. timers, pwm or exti gpio). Then, it can be tuned to start
+ conversions on external trigger by either:
+ - "rising-edge"
+ - "falling-edge"
+ - "both-edges".
+ Reading returns current trigger polarity.
+ Writing value before enabling conversions sets trigger polarity.
Best Regards,
Thanks,
Fabrice
>
> The approach seems reasonable, but easier to discuss when it's
> formally described.
>
> I'd also not care about characters saved and go with
> trigger_polarity rather than the short form (unless this you have
> a good reason for it!)
>
> Jonathan
>> ---
>> drivers/iio/adc/stm32-adc.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
>> index 30708bc..9753c39 100644
>> --- a/drivers/iio/adc/stm32-adc.c
>> +++ b/drivers/iio/adc/stm32-adc.c
>> @@ -164,6 +164,7 @@ struct stm32_adc_trig_info {
>> * @lock: spinlock
>> * @bufi: data buffer index
>> * @num_conv: expected number of scan conversions
>> + * @exten: external trigger config (enable/polarity)
>> */
>> struct stm32_adc {
>> struct stm32_adc_common *common;
>> @@ -175,6 +176,7 @@ struct stm32_adc {
>> spinlock_t lock; /* interrupt lock */
>> int bufi;
>> int num_conv;
>> + enum stm32_adc_exten exten;
>> };
>>
>> /**
>> @@ -449,7 +451,9 @@ static int stm32_adc_set_trig(struct iio_dev *indio_dev,
>>
>> /* set trigger source, default to rising edge */
>> extsel = ret;
>> - exten = STM32_EXTEN_HWTRIG_RISING_EDGE;
>> + if (adc->exten == STM32_EXTEN_SWTRIG)
>> + adc->exten = STM32_EXTEN_HWTRIG_RISING_EDGE;
>> + exten = adc->exten;
>> }
>>
>> spin_lock_irqsave(&adc->lock, flags);
>> @@ -463,6 +467,39 @@ static int stm32_adc_set_trig(struct iio_dev *indio_dev,
>> return 0;
>> }
>>
>> +static int stm32_adc_set_trig_pol(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + unsigned int type)
>> +{
>> + struct stm32_adc *adc = iio_priv(indio_dev);
>> +
>> + adc->exten = type;
>> +
>> + return 0;
>> +}
>> +
>> +static int stm32_adc_get_trig_pol(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan)
>> +{
>> + struct stm32_adc *adc = iio_priv(indio_dev);
>> +
>> + return adc->exten;
>> +}
>> +
>> +static const char * const stm32_trig_pol_items[] = {
>> + [STM32_EXTEN_SWTRIG] = "swtrig",
>> + [STM32_EXTEN_HWTRIG_RISING_EDGE] = "rising-edge",
>> + [STM32_EXTEN_HWTRIG_FALLING_EDGE] = "falling-edge",
>> + [STM32_EXTEN_HWTRIG_BOTH_EDGES] = "both-edges",
>> +};
>> +
>> +const struct iio_enum stm32_adc_trig_pol = {
>> + .items = stm32_trig_pol_items,
>> + .num_items = ARRAY_SIZE(stm32_trig_pol_items),
>> + .get = stm32_adc_get_trig_pol,
>> + .set = stm32_adc_set_trig_pol,
>> +};
>> +
>> /**
>> * stm32_adc_single_conv() - Performs a single conversion
>> * @indio_dev: IIO device
>> @@ -745,6 +782,17 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
>> return IRQ_HANDLED;
>> }
>>
>> +static const struct iio_chan_spec_ext_info stm32_adc_ext_info[] = {
>> + IIO_ENUM("trigger_pol", IIO_SHARED_BY_ALL, &stm32_adc_trig_pol),
>> + {
>> + .name = "trigger_pol_available",
>> + .shared = IIO_SHARED_BY_ALL,
>> + .read = iio_enum_available_read,
>> + .private = (uintptr_t)&stm32_adc_trig_pol,
>> + },
>> + {},
>> +};
>> +
>> static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>> struct iio_chan_spec *chan,
>> const struct stm32_adc_chan_spec *channel,
>> @@ -760,6 +808,7 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>> chan->scan_type.sign = 'u';
>> chan->scan_type.realbits = 12;
>> chan->scan_type.storagebits = 16;
>> + chan->ext_info = stm32_adc_ext_info;
>> }
>>
>> static int stm32_adc_chan_of_init(struct iio_dev *indio_dev)
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/7] Documentation: dt: iio: stm32-adc: optional dma support
2017-01-19 13:34 [PATCH 0/7] Add support for triggered buffer mode to STM32 ADC Fabrice Gasnier
` (2 preceding siblings ...)
2017-01-19 13:34 ` [PATCH 3/7] iio: adc: stm32: add trigger polarity extended attribute Fabrice Gasnier
@ 2017-01-19 13:34 ` Fabrice Gasnier
2017-01-21 20:54 ` Rob Herring
2017-01-19 13:34 ` [PATCH 5/7] iio: adc: stm32: add " Fabrice Gasnier
` (2 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Fabrice Gasnier @ 2017-01-19 13:34 UTC (permalink / raw)
To: jic23, linux, robh+dt, linux-arm-kernel, devicetree, linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, fabrice.gasnier, benjamin.gaignard,
benjamin.gaignard
STM32 ADC can use dma. Add dt documentation for optional dma support.
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
index 49ed82e..5dfc88e 100644
--- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
@@ -53,6 +53,11 @@ Required properties:
- #io-channel-cells = <1>: See the IIO bindings section "IIO consumers" in
Documentation/devicetree/bindings/iio/iio-bindings.txt
+Optional properties:
+- dmas: Phandle to dma channel for this ADC instance.
+ See ../../dma/dma.txt for details.
+- dma-names: Must be "rx" when dmas property is being used.
+
Example:
adc: adc@40012000 {
compatible = "st,stm32f4-adc-core";
@@ -77,6 +82,8 @@ Example:
interrupt-parent = <&adc>;
interrupts = <0>;
st,adc-channels = <8>;
+ dmas = <&dma2 0 0 0x400 0x0>;
+ dma-names = "rx";
};
...
other adc child nodes follow...
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 4/7] Documentation: dt: iio: stm32-adc: optional dma support
2017-01-19 13:34 ` [PATCH 4/7] Documentation: dt: iio: stm32-adc: optional dma support Fabrice Gasnier
@ 2017-01-21 20:54 ` Rob Herring
0 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2017-01-21 20:54 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: jic23, linux, linux-arm-kernel, devicetree, linux-kernel,
linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, benjamin.gaignard, benjamin.gaignard
On Thu, Jan 19, 2017 at 02:34:11PM +0100, Fabrice Gasnier wrote:
> STM32 ADC can use dma. Add dt documentation for optional dma support.
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
> Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 5/7] iio: adc: stm32: add optional dma support
2017-01-19 13:34 [PATCH 0/7] Add support for triggered buffer mode to STM32 ADC Fabrice Gasnier
` (3 preceding siblings ...)
2017-01-19 13:34 ` [PATCH 4/7] Documentation: dt: iio: stm32-adc: optional dma support Fabrice Gasnier
@ 2017-01-19 13:34 ` Fabrice Gasnier
2017-01-22 13:14 ` Jonathan Cameron
2017-01-19 13:34 ` [PATCH 6/7] ARM: dts: stm32: Enable dma by default on stm32f4 adc Fabrice Gasnier
2017-01-19 13:34 ` [PATCH 7/7] ARM: dts: stm32: Enable pwm1 and pwm3 for stm32f469-eval Fabrice Gasnier
6 siblings, 1 reply; 24+ messages in thread
From: Fabrice Gasnier @ 2017-01-19 13:34 UTC (permalink / raw)
To: jic23, linux, robh+dt, linux-arm-kernel, devicetree, linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, fabrice.gasnier, benjamin.gaignard,
benjamin.gaignard
Add optional DMA support to STM32 ADC.
Use dma cyclic mode with at least two periods.
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
drivers/iio/adc/Kconfig | 2 +
drivers/iio/adc/stm32-adc-core.c | 1 +
drivers/iio/adc/stm32-adc-core.h | 2 +
drivers/iio/adc/stm32-adc.c | 209 ++++++++++++++++++++++++++++++++++++---
4 files changed, 202 insertions(+), 12 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 9a7b090..2a2ef78 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -444,12 +444,14 @@ config ROCKCHIP_SARADC
config STM32_ADC_CORE
tristate "STMicroelectronics STM32 adc core"
depends on ARCH_STM32 || COMPILE_TEST
+ depends on HAS_DMA
depends on OF
depends on REGULATOR
select IIO_BUFFER
select MFD_STM32_TIMERS
select IIO_STM32_TIMER_TRIGGER
select IIO_TRIGGERED_BUFFER
+ select IRQ_WORK
help
Select this option to enable the core driver for STMicroelectronics
STM32 analog-to-digital converter (ADC).
diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
index 4214b0c..22b7c93 100644
--- a/drivers/iio/adc/stm32-adc-core.c
+++ b/drivers/iio/adc/stm32-adc-core.c
@@ -201,6 +201,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
priv->common.base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(priv->common.base))
return PTR_ERR(priv->common.base);
+ priv->common.phys_base = res->start;
priv->vref = devm_regulator_get(&pdev->dev, "vref");
if (IS_ERR(priv->vref)) {
diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
index 081fa5f..2ec7abb 100644
--- a/drivers/iio/adc/stm32-adc-core.h
+++ b/drivers/iio/adc/stm32-adc-core.h
@@ -42,10 +42,12 @@
/**
* struct stm32_adc_common - stm32 ADC driver common data (for all instances)
* @base: control registers base cpu addr
+ * @phys_base: control registers base physical addr
* @vref_mv: vref voltage (mv)
*/
struct stm32_adc_common {
void __iomem *base;
+ phys_addr_t phys_base;
int vref_mv;
};
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 9753c39..3439f4c 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -21,6 +21,8 @@
#include <linux/clk.h>
#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
#include <linux/iio/iio.h>
#include <linux/iio/buffer.h>
#include <linux/iio/timer/stm32-timer-trigger.h>
@@ -29,6 +31,7 @@
#include <linux/iio/triggered_buffer.h>
#include <linux/interrupt.h>
#include <linux/io.h>
+#include <linux/irq_work.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/of.h>
@@ -69,6 +72,8 @@
#define STM32F4_EXTSEL_SHIFT 24
#define STM32F4_EXTSEL_MASK GENMASK(27, 24)
#define STM32F4_EOCS BIT(10)
+#define STM32F4_DDS BIT(9)
+#define STM32F4_DMA BIT(8)
#define STM32F4_ADON BIT(0)
/* STM32F4_ADC_SQR1 - bit fields */
@@ -165,6 +170,11 @@ struct stm32_adc_trig_info {
* @bufi: data buffer index
* @num_conv: expected number of scan conversions
* @exten: external trigger config (enable/polarity)
+ * @work: irq work used to call trigger poll routine
+ * @dma_chan: dma channel
+ * @rx_buf: dma rx buffer cpu address
+ * @rx_dma_buf: dma rx buffer bus address
+ * @rx_buf_sz: dma rx buffer size
*/
struct stm32_adc {
struct stm32_adc_common *common;
@@ -177,6 +187,11 @@ struct stm32_adc {
int bufi;
int num_conv;
enum stm32_adc_exten exten;
+ struct irq_work work;
+ struct dma_chan *dma_chan;
+ u8 *rx_buf;
+ dma_addr_t rx_dma_buf;
+ int rx_buf_sz;
};
/**
@@ -332,10 +347,20 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
/**
* stm32_adc_start_conv() - Start conversions for regular channels.
* @adc: stm32 adc instance
+ *
+ * Start conversions for regular channels.
+ * Also take care of normal or DMA mode. DMA is used in circular mode for
+ * regular conversions, in IIO buffer modes. Rely on rx_buf as raw
+ * read doesn't use dma, but direct DR read.
*/
static void stm32_adc_start_conv(struct stm32_adc *adc)
{
stm32_adc_set_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
+
+ if (adc->rx_buf)
+ stm32_adc_set_bits(adc, STM32F4_ADC_CR2,
+ STM32F4_DMA | STM32F4_DDS);
+
stm32_adc_set_bits(adc, STM32F4_ADC_CR2, STM32F4_EOCS | STM32F4_ADON);
/* Wait for Power-up time (tSTAB from datasheet) */
@@ -353,6 +378,10 @@ static void stm32_adc_stop_conv(struct stm32_adc *adc)
stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
stm32_adc_clr_bits(adc, STM32F4_ADC_CR2, STM32F4_ADON);
+
+ if (adc->rx_buf)
+ stm32_adc_clr_bits(adc, STM32F4_ADC_CR2,
+ STM32F4_DMA | STM32F4_DDS);
}
/**
@@ -689,19 +718,138 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
.driver_module = THIS_MODULE,
};
+static int stm32_adc_dma_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,
+ &state);
+ 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;
+}
+
+static void stm32_adc_dma_irq_work(struct irq_work *work)
+{
+ 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 stm32_adc *adc = data;
+
+ /* invoques iio_trigger_poll() from hard irq context */
+ irq_work_queue(&adc->work);
+}
+
static int stm32_adc_buffer_preenable(struct iio_dev *indio_dev)
{
struct stm32_adc *adc = iio_priv(indio_dev);
+ 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;
- /* Allocate adc buffer */
- adc->buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
- if (!adc->buffer)
+ 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 can push with iio_trigger_poll().
+ */
+ size = indio_dev->buffer->bytes_per_datum * indio_dev->buffer->length;
+ size = max(indio_dev->scan_bytes * 2, size);
+
+ adc->rx_buf = dma_alloc_coherent(adc->dma_chan->device->dev,
+ PAGE_ALIGN(size), &adc->rx_dma_buf,
+ GFP_KERNEL);
+ if (!adc->rx_buf)
return -ENOMEM;
+ adc->rx_buf_sz = size;
+ watermark = indio_dev->buffer->bytes_per_datum
+ * indio_dev->buffer->watermark;
+ watermark = max(indio_dev->scan_bytes, watermark);
+ watermark = rounddown(watermark, indio_dev->scan_bytes);
+
+ dev_dbg(&indio_dev->dev, "%s size=%d watermark=%d\n", __func__, size,
+ watermark);
+
+ /* Configure DMA channel to read data register */
+ memset(&config, 0, sizeof(config));
+ config.src_addr = (dma_addr_t)adc->common->phys_base;
+ config.src_addr += adc->offset + STM32F4_ADC_DR;
+ config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+
+ ret = dmaengine_slave_config(adc->dma_chan, &config);
+ if (ret)
+ goto config_err;
+
+ /* Prepare a DMA cyclic transaction */
+ desc = dmaengine_prep_dma_cyclic(adc->dma_chan,
+ adc->rx_dma_buf,
+ size, watermark,
+ DMA_DEV_TO_MEM,
+ DMA_PREP_INTERRUPT);
+ if (!desc) {
+ ret = -ENODEV;
+ goto config_err;
+ }
+
+ desc->callback = stm32_adc_dma_buffer_done;
+ desc->callback_param = adc;
+
+ cookie = dmaengine_submit(desc);
+ if (dma_submit_error(cookie)) {
+ ret = dma_submit_error(cookie);
+ goto config_err;
+ }
+
+ /* Issue pending DMA requests */
+ dma_async_issue_pending(adc->dma_chan);
return 0;
+
+config_err:
+ dma_free_coherent(adc->dma_chan->device->dev, PAGE_ALIGN(size),
+ adc->rx_buf, adc->rx_dma_buf);
+
+ return ret;
}
static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
@@ -719,7 +867,8 @@ static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
if (ret < 0)
return ret;
- stm32_adc_conv_irq_enable(adc);
+ if (!adc->dma_chan)
+ stm32_adc_conv_irq_enable(adc);
stm32_adc_start_conv(adc);
return 0;
@@ -731,7 +880,8 @@ static int stm32_adc_buffer_predisable(struct iio_dev *indio_dev)
int ret;
stm32_adc_stop_conv(adc);
- stm32_adc_conv_irq_disable(adc);
+ if (!adc->dma_chan)
+ stm32_adc_conv_irq_disable(adc);
ret = iio_triggered_buffer_predisable(indio_dev);
if (ret < 0)
@@ -748,7 +898,16 @@ static int stm32_adc_buffer_postdisable(struct iio_dev *indio_dev)
{
struct stm32_adc *adc = iio_priv(indio_dev);
- kfree(adc->buffer);
+ if (!adc->dma_chan) {
+ kfree(adc->buffer);
+ } else {
+ dmaengine_terminate_all(adc->dma_chan);
+ irq_work_sync(&adc->work);
+ dma_free_coherent(adc->dma_chan->device->dev,
+ PAGE_ALIGN(adc->rx_buf_sz),
+ adc->rx_buf, adc->rx_dma_buf);
+ adc->rx_buf = NULL;
+ }
adc->buffer = NULL;
return 0;
@@ -769,15 +928,31 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
dev_dbg(&indio_dev->dev, "%s bufi=%d\n", __func__, adc->bufi);
- /* reset buffer index */
- adc->bufi = 0;
- iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
- pf->timestamp);
+ if (!adc->dma_chan) {
+ /* reset buffer index */
+ adc->bufi = 0;
+ iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
+ pf->timestamp);
+ } else {
+ int residue = stm32_adc_dma_residue(adc);
+
+ while (residue >= indio_dev->scan_bytes) {
+ adc->buffer = (u16 *)&adc->rx_buf[adc->bufi];
+ iio_push_to_buffers_with_timestamp(indio_dev,
+ adc->buffer,
+ pf->timestamp);
+ residue -= indio_dev->scan_bytes;
+ adc->bufi += indio_dev->scan_bytes;
+ if (adc->bufi >= adc->rx_buf_sz)
+ adc->bufi = 0;
+ }
+ }
iio_trigger_notify_done(indio_dev->trig);
/* re-enable eoc irq */
- stm32_adc_conv_irq_enable(adc);
+ if (!adc->dma_chan)
+ stm32_adc_conv_irq_enable(adc);
return IRQ_HANDLED;
}
@@ -910,13 +1085,17 @@ static int stm32_adc_probe(struct platform_device *pdev)
if (ret < 0)
goto err_clk_disable;
+ adc->dma_chan = dma_request_slave_channel(&indio_dev->dev, "rx");
+ if (adc->dma_chan)
+ init_irq_work(&adc->work, stm32_adc_dma_irq_work);
+
ret = iio_triggered_buffer_setup(indio_dev,
&iio_pollfunc_store_time,
&stm32_adc_trigger_handler,
&stm32_adc_buffer_setup_ops);
if (ret) {
dev_err(&pdev->dev, "buffer setup failed\n");
- goto err_clk_disable;
+ goto err_dma_disable;
}
ret = iio_device_register(indio_dev);
@@ -930,6 +1109,10 @@ static int stm32_adc_probe(struct platform_device *pdev)
err_buffer_cleanup:
iio_triggered_buffer_cleanup(indio_dev);
+err_dma_disable:
+ if (adc->dma_chan)
+ dma_release_channel(adc->dma_chan);
+
err_clk_disable:
clk_disable_unprepare(adc->clk);
@@ -943,6 +1126,8 @@ static int stm32_adc_remove(struct platform_device *pdev)
iio_device_unregister(indio_dev);
iio_triggered_buffer_cleanup(indio_dev);
+ if (adc->dma_chan)
+ dma_release_channel(adc->dma_chan);
clk_disable_unprepare(adc->clk);
return 0;
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 5/7] iio: adc: stm32: add optional dma support
2017-01-19 13:34 ` [PATCH 5/7] iio: adc: stm32: add " Fabrice Gasnier
@ 2017-01-22 13:14 ` Jonathan Cameron
2017-01-24 14:43 ` Fabrice Gasnier
0 siblings, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2017-01-22 13:14 UTC (permalink / raw)
To: Fabrice Gasnier, linux, robh+dt, linux-arm-kernel, devicetree,
linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, benjamin.gaignard, benjamin.gaignard
On 19/01/17 13:34, Fabrice Gasnier wrote:
> Add optional DMA support to STM32 ADC.
> Use dma cyclic mode with at least two periods.
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
What is the point going forward in supporting non dma buffered reads at all?
Is there hardware that doesn't have DMA support?
Just strikes me that the driver would be slight simpler if we dropped that
support.
Various comments inline. Mostly around crossing the boundary to fetch
from the IIO specific buffer (indio_dev->buffer). That should never be
used directly as we can have multiple consumers of the datastream so
the numbers you get from that may represent only part of what is going on.
> ---
> drivers/iio/adc/Kconfig | 2 +
> drivers/iio/adc/stm32-adc-core.c | 1 +
> drivers/iio/adc/stm32-adc-core.h | 2 +
> drivers/iio/adc/stm32-adc.c | 209 ++++++++++++++++++++++++++++++++++++---
> 4 files changed, 202 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 9a7b090..2a2ef78 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -444,12 +444,14 @@ config ROCKCHIP_SARADC
> config STM32_ADC_CORE
> tristate "STMicroelectronics STM32 adc core"
> depends on ARCH_STM32 || COMPILE_TEST
> + depends on HAS_DMA
> depends on OF
> depends on REGULATOR
> select IIO_BUFFER
> select MFD_STM32_TIMERS
> select IIO_STM32_TIMER_TRIGGER
> select IIO_TRIGGERED_BUFFER
> + select IRQ_WORK
> help
> Select this option to enable the core driver for STMicroelectronics
> STM32 analog-to-digital converter (ADC).
> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
> index 4214b0c..22b7c93 100644
> --- a/drivers/iio/adc/stm32-adc-core.c
> +++ b/drivers/iio/adc/stm32-adc-core.c
> @@ -201,6 +201,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
> priv->common.base = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(priv->common.base))
> return PTR_ERR(priv->common.base);
> + priv->common.phys_base = res->start;
>
> priv->vref = devm_regulator_get(&pdev->dev, "vref");
> if (IS_ERR(priv->vref)) {
> diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
> index 081fa5f..2ec7abb 100644
> --- a/drivers/iio/adc/stm32-adc-core.h
> +++ b/drivers/iio/adc/stm32-adc-core.h
> @@ -42,10 +42,12 @@
> /**
> * struct stm32_adc_common - stm32 ADC driver common data (for all instances)
> * @base: control registers base cpu addr
> + * @phys_base: control registers base physical addr
> * @vref_mv: vref voltage (mv)
> */
> struct stm32_adc_common {
> void __iomem *base;
> + phys_addr_t phys_base;
> int vref_mv;
> };
>
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 9753c39..3439f4c 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -21,6 +21,8 @@
>
> #include <linux/clk.h>
> #include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/buffer.h>
> #include <linux/iio/timer/stm32-timer-trigger.h>
> @@ -29,6 +31,7 @@
> #include <linux/iio/triggered_buffer.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> +#include <linux/irq_work.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/of.h>
> @@ -69,6 +72,8 @@
> #define STM32F4_EXTSEL_SHIFT 24
> #define STM32F4_EXTSEL_MASK GENMASK(27, 24)
> #define STM32F4_EOCS BIT(10)
> +#define STM32F4_DDS BIT(9)
> +#define STM32F4_DMA BIT(8)
> #define STM32F4_ADON BIT(0)
>
> /* STM32F4_ADC_SQR1 - bit fields */
> @@ -165,6 +170,11 @@ struct stm32_adc_trig_info {
> * @bufi: data buffer index
> * @num_conv: expected number of scan conversions
> * @exten: external trigger config (enable/polarity)
> + * @work: irq work used to call trigger poll routine
> + * @dma_chan: dma channel
> + * @rx_buf: dma rx buffer cpu address
> + * @rx_dma_buf: dma rx buffer bus address
> + * @rx_buf_sz: dma rx buffer size
> */
> struct stm32_adc {
> struct stm32_adc_common *common;
> @@ -177,6 +187,11 @@ struct stm32_adc {
> int bufi;
> int num_conv;
> enum stm32_adc_exten exten;
> + struct irq_work work;
> + struct dma_chan *dma_chan;
> + u8 *rx_buf;
> + dma_addr_t rx_dma_buf;
> + int rx_buf_sz;
> };
>
> /**
> @@ -332,10 +347,20 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
> /**
> * stm32_adc_start_conv() - Start conversions for regular channels.
> * @adc: stm32 adc instance
> + *
> + * Start conversions for regular channels.
> + * Also take care of normal or DMA mode. DMA is used in circular mode for
> + * regular conversions, in IIO buffer modes. Rely on rx_buf as raw
> + * read doesn't use dma, but direct DR read.
> */
> static void stm32_adc_start_conv(struct stm32_adc *adc)
> {
> stm32_adc_set_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
> +
> + if (adc->rx_buf)
> + stm32_adc_set_bits(adc, STM32F4_ADC_CR2,
> + STM32F4_DMA | STM32F4_DDS);
> +
> stm32_adc_set_bits(adc, STM32F4_ADC_CR2, STM32F4_EOCS | STM32F4_ADON);
>
> /* Wait for Power-up time (tSTAB from datasheet) */
> @@ -353,6 +378,10 @@ static void stm32_adc_stop_conv(struct stm32_adc *adc)
>
> stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
> stm32_adc_clr_bits(adc, STM32F4_ADC_CR2, STM32F4_ADON);
> +
> + if (adc->rx_buf)
> + stm32_adc_clr_bits(adc, STM32F4_ADC_CR2,
> + STM32F4_DMA | STM32F4_DDS);
> }
>
> /**
> @@ -689,19 +718,138 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
> .driver_module = THIS_MODULE,
> };
>
> +static int stm32_adc_dma_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,
> + &state);
> + 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;
> +}
> +
> +static void stm32_adc_dma_irq_work(struct irq_work *work)
> +{
> + 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);
This is nasty ;) iio_trigger_poll_chained is your friend. It is missnamed.
If you only need to call the threaded part of the pollfunc (and I think you
can construct it so you do) then it will call it without needing to bounce
back into interrupt context.
> +}
> +
> +static void stm32_adc_dma_buffer_done(void *data)
> +{
> + struct stm32_adc *adc = data;
> +
> + /* invoques iio_trigger_poll() from hard irq context */
> + irq_work_queue(&adc->work);
> +}
> +
> static int stm32_adc_buffer_preenable(struct iio_dev *indio_dev)
> {
> struct stm32_adc *adc = iio_priv(indio_dev);
> + 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;
>
> - /* Allocate adc buffer */
> - adc->buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
> - if (!adc->buffer)
> + 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 can push with iio_trigger_poll().
> + */
> + size = indio_dev->buffer->bytes_per_datum * indio_dev->buffer->length;
> + size = max(indio_dev->scan_bytes * 2, size);
Hmm. There is a bit of a weird mix going on here. Firstly, you may have more
than one consumer buffer, the one you are checking is only the one directly
associated with the IIO userspace interface.
So scan_bytes is the right value to use for both of these statements.
The buffer length is typically not knowable either or relevant here.
If you are ultimately going to deal with watermarks there is an interface
to guide read sizes based on that but this isn't it.
So basically device should never know anything at all about the software
buffer. All info should pass through the core which knows about all the
consumers hanging off this interface (and the demux that is going on to
feed them all).
Some drivers provide additional info to allow the modifying of the
precise hardware watermark being used. That's an acceptable form of
'tweak'.
> +
> + adc->rx_buf = dma_alloc_coherent(adc->dma_chan->device->dev,
> + PAGE_ALIGN(size), &adc->rx_dma_buf,
> + GFP_KERNEL);
> + if (!adc->rx_buf)
> return -ENOMEM;
> + adc->rx_buf_sz = size;
> + watermark = indio_dev->buffer->bytes_per_datum
> + * indio_dev->buffer->watermark;
> + watermark = max(indio_dev->scan_bytes, watermark);
> + watermark = rounddown(watermark, indio_dev->scan_bytes);
> +
> + dev_dbg(&indio_dev->dev, "%s size=%d watermark=%d\n", __func__, size,
> + watermark);
> +
> + /* Configure DMA channel to read data register */
> + memset(&config, 0, sizeof(config));
> + config.src_addr = (dma_addr_t)adc->common->phys_base;
> + config.src_addr += adc->offset + STM32F4_ADC_DR;
> + config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> +
> + ret = dmaengine_slave_config(adc->dma_chan, &config);
> + if (ret)
> + goto config_err;
> +
> + /* Prepare a DMA cyclic transaction */
> + desc = dmaengine_prep_dma_cyclic(adc->dma_chan,
> + adc->rx_dma_buf,
> + size, watermark,
> + DMA_DEV_TO_MEM,
> + DMA_PREP_INTERRUPT);
> + if (!desc) {
> + ret = -ENODEV;
> + goto config_err;
> + }
> +
> + desc->callback = stm32_adc_dma_buffer_done;
> + desc->callback_param = adc;
> +
> + cookie = dmaengine_submit(desc);
> + if (dma_submit_error(cookie)) {
> + ret = dma_submit_error(cookie);
> + goto config_err;
> + }
> +
> + /* Issue pending DMA requests */
> + dma_async_issue_pending(adc->dma_chan);
>
> return 0;
> +
> +config_err:
> + dma_free_coherent(adc->dma_chan->device->dev, PAGE_ALIGN(size),
> + adc->rx_buf, adc->rx_dma_buf);
> +
> + return ret;
> }
>
> static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
> @@ -719,7 +867,8 @@ static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
> if (ret < 0)
> return ret;
>
> - stm32_adc_conv_irq_enable(adc);
> + if (!adc->dma_chan)
> + stm32_adc_conv_irq_enable(adc);
> stm32_adc_start_conv(adc);
>
> return 0;
> @@ -731,7 +880,8 @@ static int stm32_adc_buffer_predisable(struct iio_dev *indio_dev)
> int ret;
>
> stm32_adc_stop_conv(adc);
> - stm32_adc_conv_irq_disable(adc);
> + if (!adc->dma_chan)
> + stm32_adc_conv_irq_disable(adc);
>
> ret = iio_triggered_buffer_predisable(indio_dev);
> if (ret < 0)
> @@ -748,7 +898,16 @@ static int stm32_adc_buffer_postdisable(struct iio_dev *indio_dev)
> {
> struct stm32_adc *adc = iio_priv(indio_dev);
>
> - kfree(adc->buffer);
> + if (!adc->dma_chan) {
> + kfree(adc->buffer);
> + } else {
> + dmaengine_terminate_all(adc->dma_chan);
> + irq_work_sync(&adc->work);
> + dma_free_coherent(adc->dma_chan->device->dev,
> + PAGE_ALIGN(adc->rx_buf_sz),
> + adc->rx_buf, adc->rx_dma_buf);
> + adc->rx_buf = NULL;
> + }
> adc->buffer = NULL;
>
> return 0;
> @@ -769,15 +928,31 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
>
> dev_dbg(&indio_dev->dev, "%s bufi=%d\n", __func__, adc->bufi);
>
> - /* reset buffer index */
> - adc->bufi = 0;
> - iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
> - pf->timestamp);
> + if (!adc->dma_chan) {
> + /* reset buffer index */
> + adc->bufi = 0;
> + iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
> + pf->timestamp);
> + } else {
> + int residue = stm32_adc_dma_residue(adc);
> +
> + while (residue >= indio_dev->scan_bytes) {
> + adc->buffer = (u16 *)&adc->rx_buf[adc->bufi];
> + iio_push_to_buffers_with_timestamp(indio_dev,
> + adc->buffer,
> + pf->timestamp);
> + residue -= indio_dev->scan_bytes;
> + adc->bufi += indio_dev->scan_bytes;
> + if (adc->bufi >= adc->rx_buf_sz)
> + adc->bufi = 0;
> + }
> + }
>
> iio_trigger_notify_done(indio_dev->trig);
>
> /* re-enable eoc irq */
> - stm32_adc_conv_irq_enable(adc);
> + if (!adc->dma_chan)
> + stm32_adc_conv_irq_enable(adc);
>
> return IRQ_HANDLED;
> }
> @@ -910,13 +1085,17 @@ static int stm32_adc_probe(struct platform_device *pdev)
> if (ret < 0)
> goto err_clk_disable;
>
> + adc->dma_chan = dma_request_slave_channel(&indio_dev->dev, "rx");
> + if (adc->dma_chan)
> + init_irq_work(&adc->work, stm32_adc_dma_irq_work);
> +
> ret = iio_triggered_buffer_setup(indio_dev,
> &iio_pollfunc_store_time,
> &stm32_adc_trigger_handler,
> &stm32_adc_buffer_setup_ops);
> if (ret) {
> dev_err(&pdev->dev, "buffer setup failed\n");
> - goto err_clk_disable;
> + goto err_dma_disable;
> }
>
> ret = iio_device_register(indio_dev);
> @@ -930,6 +1109,10 @@ static int stm32_adc_probe(struct platform_device *pdev)
> err_buffer_cleanup:
> iio_triggered_buffer_cleanup(indio_dev);
>
> +err_dma_disable:
> + if (adc->dma_chan)
> + dma_release_channel(adc->dma_chan);
> +
> err_clk_disable:
> clk_disable_unprepare(adc->clk);
>
> @@ -943,6 +1126,8 @@ static int stm32_adc_remove(struct platform_device *pdev)
>
> iio_device_unregister(indio_dev);
> iio_triggered_buffer_cleanup(indio_dev);
> + if (adc->dma_chan)
> + dma_release_channel(adc->dma_chan);
> clk_disable_unprepare(adc->clk);
>
> return 0;
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 5/7] iio: adc: stm32: add optional dma support
2017-01-22 13:14 ` Jonathan Cameron
@ 2017-01-24 14:43 ` Fabrice Gasnier
2017-01-24 18:25 ` Jonathan Cameron
0 siblings, 1 reply; 24+ messages in thread
From: Fabrice Gasnier @ 2017-01-24 14:43 UTC (permalink / raw)
To: Jonathan Cameron, linux, robh+dt, linux-arm-kernel, devicetree,
linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, benjamin.gaignard, benjamin.gaignard
On 01/22/2017 02:14 PM, Jonathan Cameron wrote:
> On 19/01/17 13:34, Fabrice Gasnier wrote:
>> Add optional DMA support to STM32 ADC.
>> Use dma cyclic mode with at least two periods.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> What is the point going forward in supporting non dma buffered reads at all?
> Is there hardware that doesn't have DMA support?
Hi Jonathan,
Basically no, but there is a limited number of DMA request lines.
DMA request lines mapping is documented in STM32F4 reference manual
(DMA1/2 request mapping).
There may be a situation where user (in dt) assign all DMA channels to
other IPs. Then only choice would be to fall back using interrupt mode
for ADC.
This is why I'd like to keep support for both interrupt and DMA modes.
> Just strikes me that the driver would be slight simpler if we dropped that
> support.
>
> Various comments inline. Mostly around crossing the boundary to fetch
> from the IIO specific buffer (indio_dev->buffer). That should never be
> used directly as we can have multiple consumers of the datastream so
> the numbers you get from that may represent only part of what is going on.
>
>
>> ---
>> drivers/iio/adc/Kconfig | 2 +
>> drivers/iio/adc/stm32-adc-core.c | 1 +
>> drivers/iio/adc/stm32-adc-core.h | 2 +
>> drivers/iio/adc/stm32-adc.c | 209 ++++++++++++++++++++++++++++++++++++---
>> 4 files changed, 202 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 9a7b090..2a2ef78 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -444,12 +444,14 @@ config ROCKCHIP_SARADC
>> config STM32_ADC_CORE
>> tristate "STMicroelectronics STM32 adc core"
>> depends on ARCH_STM32 || COMPILE_TEST
>> + depends on HAS_DMA
>> depends on OF
>> depends on REGULATOR
>> select IIO_BUFFER
>> select MFD_STM32_TIMERS
>> select IIO_STM32_TIMER_TRIGGER
>> select IIO_TRIGGERED_BUFFER
>> + select IRQ_WORK
>> help
>> Select this option to enable the core driver for STMicroelectronics
>> STM32 analog-to-digital converter (ADC).
>> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
>> index 4214b0c..22b7c93 100644
>> --- a/drivers/iio/adc/stm32-adc-core.c
>> +++ b/drivers/iio/adc/stm32-adc-core.c
>> @@ -201,6 +201,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
>> priv->common.base = devm_ioremap_resource(&pdev->dev, res);
>> if (IS_ERR(priv->common.base))
>> return PTR_ERR(priv->common.base);
>> + priv->common.phys_base = res->start;
>>
>> priv->vref = devm_regulator_get(&pdev->dev, "vref");
>> if (IS_ERR(priv->vref)) {
>> diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
>> index 081fa5f..2ec7abb 100644
>> --- a/drivers/iio/adc/stm32-adc-core.h
>> +++ b/drivers/iio/adc/stm32-adc-core.h
>> @@ -42,10 +42,12 @@
>> /**
>> * struct stm32_adc_common - stm32 ADC driver common data (for all instances)
>> * @base: control registers base cpu addr
>> + * @phys_base: control registers base physical addr
>> * @vref_mv: vref voltage (mv)
>> */
>> struct stm32_adc_common {
>> void __iomem *base;
>> + phys_addr_t phys_base;
>> int vref_mv;
>> };
>>
>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
>> index 9753c39..3439f4c 100644
>> --- a/drivers/iio/adc/stm32-adc.c
>> +++ b/drivers/iio/adc/stm32-adc.c
>> @@ -21,6 +21,8 @@
>>
>> #include <linux/clk.h>
>> #include <linux/delay.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dmaengine.h>
>> #include <linux/iio/iio.h>
>> #include <linux/iio/buffer.h>
>> #include <linux/iio/timer/stm32-timer-trigger.h>
>> @@ -29,6 +31,7 @@
>> #include <linux/iio/triggered_buffer.h>
>> #include <linux/interrupt.h>
>> #include <linux/io.h>
>> +#include <linux/irq_work.h>
>> #include <linux/module.h>
>> #include <linux/platform_device.h>
>> #include <linux/of.h>
>> @@ -69,6 +72,8 @@
>> #define STM32F4_EXTSEL_SHIFT 24
>> #define STM32F4_EXTSEL_MASK GENMASK(27, 24)
>> #define STM32F4_EOCS BIT(10)
>> +#define STM32F4_DDS BIT(9)
>> +#define STM32F4_DMA BIT(8)
>> #define STM32F4_ADON BIT(0)
>>
>> /* STM32F4_ADC_SQR1 - bit fields */
>> @@ -165,6 +170,11 @@ struct stm32_adc_trig_info {
>> * @bufi: data buffer index
>> * @num_conv: expected number of scan conversions
>> * @exten: external trigger config (enable/polarity)
>> + * @work: irq work used to call trigger poll routine
>> + * @dma_chan: dma channel
>> + * @rx_buf: dma rx buffer cpu address
>> + * @rx_dma_buf: dma rx buffer bus address
>> + * @rx_buf_sz: dma rx buffer size
>> */
>> struct stm32_adc {
>> struct stm32_adc_common *common;
>> @@ -177,6 +187,11 @@ struct stm32_adc {
>> int bufi;
>> int num_conv;
>> enum stm32_adc_exten exten;
>> + struct irq_work work;
>> + struct dma_chan *dma_chan;
>> + u8 *rx_buf;
>> + dma_addr_t rx_dma_buf;
>> + int rx_buf_sz;
>> };
>>
>> /**
>> @@ -332,10 +347,20 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
>> /**
>> * stm32_adc_start_conv() - Start conversions for regular channels.
>> * @adc: stm32 adc instance
>> + *
>> + * Start conversions for regular channels.
>> + * Also take care of normal or DMA mode. DMA is used in circular mode for
>> + * regular conversions, in IIO buffer modes. Rely on rx_buf as raw
>> + * read doesn't use dma, but direct DR read.
>> */
>> static void stm32_adc_start_conv(struct stm32_adc *adc)
>> {
>> stm32_adc_set_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
>> +
>> + if (adc->rx_buf)
>> + stm32_adc_set_bits(adc, STM32F4_ADC_CR2,
>> + STM32F4_DMA | STM32F4_DDS);
>> +
>> stm32_adc_set_bits(adc, STM32F4_ADC_CR2, STM32F4_EOCS | STM32F4_ADON);
>>
>> /* Wait for Power-up time (tSTAB from datasheet) */
>> @@ -353,6 +378,10 @@ static void stm32_adc_stop_conv(struct stm32_adc *adc)
>>
>> stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
>> stm32_adc_clr_bits(adc, STM32F4_ADC_CR2, STM32F4_ADON);
>> +
>> + if (adc->rx_buf)
>> + stm32_adc_clr_bits(adc, STM32F4_ADC_CR2,
>> + STM32F4_DMA | STM32F4_DDS);
>> }
>>
>> /**
>> @@ -689,19 +718,138 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
>> .driver_module = THIS_MODULE,
>> };
>>
>> +static int stm32_adc_dma_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,
>> + &state);
>> + 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;
>> +}
>> +
>> +static void stm32_adc_dma_irq_work(struct irq_work *work)
>> +{
>> + 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);
> This is nasty ;) iio_trigger_poll_chained is your friend. It is missnamed.
> If you only need to call the threaded part of the pollfunc (and I think you
> can construct it so you do) then it will call it without needing to bounce
> back into interrupt context.
I'll look into it :-)
>
>> +}
>> +
>> +static void stm32_adc_dma_buffer_done(void *data)
>> +{
>> + struct stm32_adc *adc = data;
>> +
>> + /* invoques iio_trigger_poll() from hard irq context */
>> + irq_work_queue(&adc->work);
>> +}
>> +
>> static int stm32_adc_buffer_preenable(struct iio_dev *indio_dev)
>> {
>> struct stm32_adc *adc = iio_priv(indio_dev);
>> + 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;
>>
>> - /* Allocate adc buffer */
>> - adc->buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
>> - if (!adc->buffer)
>> + 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 can push with iio_trigger_poll().
>> + */
>> + size = indio_dev->buffer->bytes_per_datum * indio_dev->buffer->length;
>> + size = max(indio_dev->scan_bytes * 2, size);
> Hmm. There is a bit of a weird mix going on here. Firstly, you may have more
> than one consumer buffer, the one you are checking is only the one directly
> associated with the IIO userspace interface.
>
> So scan_bytes is the right value to use for both of these statements.
> The buffer length is typically not knowable either or relevant here.
> If you are ultimately going to deal with watermarks there is an interface
> to guide read sizes based on that but this isn't it.
>
> So basically device should never know anything at all about the software
> buffer. All info should pass through the core which knows about all the
> consumers hanging off this interface (and the demux that is going on to
> feed them all).
>
> Some drivers provide additional info to allow the modifying of the
> precise hardware watermark being used. That's an acceptable form of
> 'tweak'.
I'll follow your guidelines and rework this part. I also had some doubts
on this. This is more clear!
Thanks,
Regards,
Fabrice
>
>> +
>> + adc->rx_buf = dma_alloc_coherent(adc->dma_chan->device->dev,
>> + PAGE_ALIGN(size), &adc->rx_dma_buf,
>> + GFP_KERNEL);
>> + if (!adc->rx_buf)
>> return -ENOMEM;
>> + adc->rx_buf_sz = size;
>> + watermark = indio_dev->buffer->bytes_per_datum
>> + * indio_dev->buffer->watermark;
>> + watermark = max(indio_dev->scan_bytes, watermark);
>> + watermark = rounddown(watermark, indio_dev->scan_bytes);
>> +
>> + dev_dbg(&indio_dev->dev, "%s size=%d watermark=%d\n", __func__, size,
>> + watermark);
>> +
>> + /* Configure DMA channel to read data register */
>> + memset(&config, 0, sizeof(config));
>> + config.src_addr = (dma_addr_t)adc->common->phys_base;
>> + config.src_addr += adc->offset + STM32F4_ADC_DR;
>> + config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>> +
>> + ret = dmaengine_slave_config(adc->dma_chan, &config);
>> + if (ret)
>> + goto config_err;
>> +
>> + /* Prepare a DMA cyclic transaction */
>> + desc = dmaengine_prep_dma_cyclic(adc->dma_chan,
>> + adc->rx_dma_buf,
>> + size, watermark,
>> + DMA_DEV_TO_MEM,
>> + DMA_PREP_INTERRUPT);
>> + if (!desc) {
>> + ret = -ENODEV;
>> + goto config_err;
>> + }
>> +
>> + desc->callback = stm32_adc_dma_buffer_done;
>> + desc->callback_param = adc;
>> +
>> + cookie = dmaengine_submit(desc);
>> + if (dma_submit_error(cookie)) {
>> + ret = dma_submit_error(cookie);
>> + goto config_err;
>> + }
>> +
>> + /* Issue pending DMA requests */
>> + dma_async_issue_pending(adc->dma_chan);
>>
>> return 0;
>> +
>> +config_err:
>> + dma_free_coherent(adc->dma_chan->device->dev, PAGE_ALIGN(size),
>> + adc->rx_buf, adc->rx_dma_buf);
>> +
>> + return ret;
>> }
>>
>> static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
>> @@ -719,7 +867,8 @@ static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
>> if (ret < 0)
>> return ret;
>>
>> - stm32_adc_conv_irq_enable(adc);
>> + if (!adc->dma_chan)
>> + stm32_adc_conv_irq_enable(adc);
>> stm32_adc_start_conv(adc);
>>
>> return 0;
>> @@ -731,7 +880,8 @@ static int stm32_adc_buffer_predisable(struct iio_dev *indio_dev)
>> int ret;
>>
>> stm32_adc_stop_conv(adc);
>> - stm32_adc_conv_irq_disable(adc);
>> + if (!adc->dma_chan)
>> + stm32_adc_conv_irq_disable(adc);
>>
>> ret = iio_triggered_buffer_predisable(indio_dev);
>> if (ret < 0)
>> @@ -748,7 +898,16 @@ static int stm32_adc_buffer_postdisable(struct iio_dev *indio_dev)
>> {
>> struct stm32_adc *adc = iio_priv(indio_dev);
>>
>> - kfree(adc->buffer);
>> + if (!adc->dma_chan) {
>> + kfree(adc->buffer);
>> + } else {
>> + dmaengine_terminate_all(adc->dma_chan);
>> + irq_work_sync(&adc->work);
>> + dma_free_coherent(adc->dma_chan->device->dev,
>> + PAGE_ALIGN(adc->rx_buf_sz),
>> + adc->rx_buf, adc->rx_dma_buf);
>> + adc->rx_buf = NULL;
>> + }
>> adc->buffer = NULL;
>>
>> return 0;
>> @@ -769,15 +928,31 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
>>
>> dev_dbg(&indio_dev->dev, "%s bufi=%d\n", __func__, adc->bufi);
>>
>> - /* reset buffer index */
>> - adc->bufi = 0;
>> - iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
>> - pf->timestamp);
>> + if (!adc->dma_chan) {
>> + /* reset buffer index */
>> + adc->bufi = 0;
>> + iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
>> + pf->timestamp);
>> + } else {
>> + int residue = stm32_adc_dma_residue(adc);
>> +
>> + while (residue >= indio_dev->scan_bytes) {
>> + adc->buffer = (u16 *)&adc->rx_buf[adc->bufi];
>> + iio_push_to_buffers_with_timestamp(indio_dev,
>> + adc->buffer,
>> + pf->timestamp);
>> + residue -= indio_dev->scan_bytes;
>> + adc->bufi += indio_dev->scan_bytes;
>> + if (adc->bufi >= adc->rx_buf_sz)
>> + adc->bufi = 0;
>> + }
>> + }
>>
>> iio_trigger_notify_done(indio_dev->trig);
>>
>> /* re-enable eoc irq */
>> - stm32_adc_conv_irq_enable(adc);
>> + if (!adc->dma_chan)
>> + stm32_adc_conv_irq_enable(adc);
>>
>> return IRQ_HANDLED;
>> }
>> @@ -910,13 +1085,17 @@ static int stm32_adc_probe(struct platform_device *pdev)
>> if (ret < 0)
>> goto err_clk_disable;
>>
>> + adc->dma_chan = dma_request_slave_channel(&indio_dev->dev, "rx");
>> + if (adc->dma_chan)
>> + init_irq_work(&adc->work, stm32_adc_dma_irq_work);
>> +
>> ret = iio_triggered_buffer_setup(indio_dev,
>> &iio_pollfunc_store_time,
>> &stm32_adc_trigger_handler,
>> &stm32_adc_buffer_setup_ops);
>> if (ret) {
>> dev_err(&pdev->dev, "buffer setup failed\n");
>> - goto err_clk_disable;
>> + goto err_dma_disable;
>> }
>>
>> ret = iio_device_register(indio_dev);
>> @@ -930,6 +1109,10 @@ static int stm32_adc_probe(struct platform_device *pdev)
>> err_buffer_cleanup:
>> iio_triggered_buffer_cleanup(indio_dev);
>>
>> +err_dma_disable:
>> + if (adc->dma_chan)
>> + dma_release_channel(adc->dma_chan);
>> +
>> err_clk_disable:
>> clk_disable_unprepare(adc->clk);
>>
>> @@ -943,6 +1126,8 @@ static int stm32_adc_remove(struct platform_device *pdev)
>>
>> iio_device_unregister(indio_dev);
>> iio_triggered_buffer_cleanup(indio_dev);
>> + if (adc->dma_chan)
>> + dma_release_channel(adc->dma_chan);
>> clk_disable_unprepare(adc->clk);
>>
>> return 0;
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 5/7] iio: adc: stm32: add optional dma support
2017-01-24 14:43 ` Fabrice Gasnier
@ 2017-01-24 18:25 ` Jonathan Cameron
0 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2017-01-24 18:25 UTC (permalink / raw)
To: Fabrice Gasnier, Jonathan Cameron, linux, robh+dt,
linux-arm-kernel, devicetree, linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, benjamin.gaignard, benjamin.gaignard
On 24 January 2017 14:43:56 GMT+00:00, Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
>On 01/22/2017 02:14 PM, Jonathan Cameron wrote:
>> On 19/01/17 13:34, Fabrice Gasnier wrote:
>>> Add optional DMA support to STM32 ADC.
>>> Use dma cyclic mode with at least two periods.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> What is the point going forward in supporting non dma buffered reads
>at all?
>> Is there hardware that doesn't have DMA support?
>
>Hi Jonathan,
>
>Basically no, but there is a limited number of DMA request lines.
>DMA request lines mapping is documented in STM32F4 reference manual
>(DMA1/2 request mapping).
>There may be a situation where user (in dt) assign all DMA channels to
>other IPs. Then only choice would be to fall back using interrupt mode
>for ADC.
>This is why I'd like to keep support for both interrupt and DMA modes.
Ah. Fair enough. Thanks for the explanation. Perhaps a note in the patch description
would give us something to point at in future or even something in the bonding docs?
>
>> Just strikes me that the driver would be slight simpler if we dropped
>that
>> support.
>>
>> Various comments inline. Mostly around crossing the boundary to
>fetch
>> from the IIO specific buffer (indio_dev->buffer). That should never
>be
>> used directly as we can have multiple consumers of the datastream so
>> the numbers you get from that may represent only part of what is
>going on.
>>
>>
>>> ---
>>> drivers/iio/adc/Kconfig | 2 +
>>> drivers/iio/adc/stm32-adc-core.c | 1 +
>>> drivers/iio/adc/stm32-adc-core.h | 2 +
>>> drivers/iio/adc/stm32-adc.c | 209
>++++++++++++++++++++++++++++++++++++---
>>> 4 files changed, 202 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 9a7b090..2a2ef78 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -444,12 +444,14 @@ config ROCKCHIP_SARADC
>>> config STM32_ADC_CORE
>>> tristate "STMicroelectronics STM32 adc core"
>>> depends on ARCH_STM32 || COMPILE_TEST
>>> + depends on HAS_DMA
>>> depends on OF
>>> depends on REGULATOR
>>> select IIO_BUFFER
>>> select MFD_STM32_TIMERS
>>> select IIO_STM32_TIMER_TRIGGER
>>> select IIO_TRIGGERED_BUFFER
>>> + select IRQ_WORK
>>> help
>>> Select this option to enable the core driver for
>STMicroelectronics
>>> STM32 analog-to-digital converter (ADC).
>>> diff --git a/drivers/iio/adc/stm32-adc-core.c
>b/drivers/iio/adc/stm32-adc-core.c
>>> index 4214b0c..22b7c93 100644
>>> --- a/drivers/iio/adc/stm32-adc-core.c
>>> +++ b/drivers/iio/adc/stm32-adc-core.c
>>> @@ -201,6 +201,7 @@ static int stm32_adc_probe(struct
>platform_device *pdev)
>>> priv->common.base = devm_ioremap_resource(&pdev->dev, res);
>>> if (IS_ERR(priv->common.base))
>>> return PTR_ERR(priv->common.base);
>>> + priv->common.phys_base = res->start;
>>>
>>> priv->vref = devm_regulator_get(&pdev->dev, "vref");
>>> if (IS_ERR(priv->vref)) {
>>> diff --git a/drivers/iio/adc/stm32-adc-core.h
>b/drivers/iio/adc/stm32-adc-core.h
>>> index 081fa5f..2ec7abb 100644
>>> --- a/drivers/iio/adc/stm32-adc-core.h
>>> +++ b/drivers/iio/adc/stm32-adc-core.h
>>> @@ -42,10 +42,12 @@
>>> /**
>>> * struct stm32_adc_common - stm32 ADC driver common data (for all
>instances)
>>> * @base: control registers base cpu addr
>>> + * @phys_base: control registers base physical addr
>>> * @vref_mv: vref voltage (mv)
>>> */
>>> struct stm32_adc_common {
>>> void __iomem *base;
>>> + phys_addr_t phys_base;
>>> int vref_mv;
>>> };
>>>
>>> diff --git a/drivers/iio/adc/stm32-adc.c
>b/drivers/iio/adc/stm32-adc.c
>>> index 9753c39..3439f4c 100644
>>> --- a/drivers/iio/adc/stm32-adc.c
>>> +++ b/drivers/iio/adc/stm32-adc.c
>>> @@ -21,6 +21,8 @@
>>>
>>> #include <linux/clk.h>
>>> #include <linux/delay.h>
>>> +#include <linux/dma-mapping.h>
>>> +#include <linux/dmaengine.h>
>>> #include <linux/iio/iio.h>
>>> #include <linux/iio/buffer.h>
>>> #include <linux/iio/timer/stm32-timer-trigger.h>
>>> @@ -29,6 +31,7 @@
>>> #include <linux/iio/triggered_buffer.h>
>>> #include <linux/interrupt.h>
>>> #include <linux/io.h>
>>> +#include <linux/irq_work.h>
>>> #include <linux/module.h>
>>> #include <linux/platform_device.h>
>>> #include <linux/of.h>
>>> @@ -69,6 +72,8 @@
>>> #define STM32F4_EXTSEL_SHIFT 24
>>> #define STM32F4_EXTSEL_MASK GENMASK(27, 24)
>>> #define STM32F4_EOCS BIT(10)
>>> +#define STM32F4_DDS BIT(9)
>>> +#define STM32F4_DMA BIT(8)
>>> #define STM32F4_ADON BIT(0)
>>>
>>> /* STM32F4_ADC_SQR1 - bit fields */
>>> @@ -165,6 +170,11 @@ struct stm32_adc_trig_info {
>>> * @bufi: data buffer index
>>> * @num_conv: expected number of scan conversions
>>> * @exten: external trigger config (enable/polarity)
>>> + * @work: irq work used to call trigger poll routine
>>> + * @dma_chan: dma channel
>>> + * @rx_buf: dma rx buffer cpu address
>>> + * @rx_dma_buf: dma rx buffer bus address
>>> + * @rx_buf_sz: dma rx buffer size
>>> */
>>> struct stm32_adc {
>>> struct stm32_adc_common *common;
>>> @@ -177,6 +187,11 @@ struct stm32_adc {
>>> int bufi;
>>> int num_conv;
>>> enum stm32_adc_exten exten;
>>> + struct irq_work work;
>>> + struct dma_chan *dma_chan;
>>> + u8 *rx_buf;
>>> + dma_addr_t rx_dma_buf;
>>> + int rx_buf_sz;
>>> };
>>>
>>> /**
>>> @@ -332,10 +347,20 @@ static void stm32_adc_conv_irq_disable(struct
>stm32_adc *adc)
>>> /**
>>> * stm32_adc_start_conv() - Start conversions for regular channels.
>>> * @adc: stm32 adc instance
>>> + *
>>> + * Start conversions for regular channels.
>>> + * Also take care of normal or DMA mode. DMA is used in circular
>mode for
>>> + * regular conversions, in IIO buffer modes. Rely on rx_buf as raw
>>> + * read doesn't use dma, but direct DR read.
>>> */
>>> static void stm32_adc_start_conv(struct stm32_adc *adc)
>>> {
>>> stm32_adc_set_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
>>> +
>>> + if (adc->rx_buf)
>>> + stm32_adc_set_bits(adc, STM32F4_ADC_CR2,
>>> + STM32F4_DMA | STM32F4_DDS);
>>> +
>>> stm32_adc_set_bits(adc, STM32F4_ADC_CR2, STM32F4_EOCS |
>STM32F4_ADON);
>>>
>>> /* Wait for Power-up time (tSTAB from datasheet) */
>>> @@ -353,6 +378,10 @@ static void stm32_adc_stop_conv(struct
>stm32_adc *adc)
>>>
>>> stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
>>> stm32_adc_clr_bits(adc, STM32F4_ADC_CR2, STM32F4_ADON);
>>> +
>>> + if (adc->rx_buf)
>>> + stm32_adc_clr_bits(adc, STM32F4_ADC_CR2,
>>> + STM32F4_DMA | STM32F4_DDS);
>>> }
>>>
>>> /**
>>> @@ -689,19 +718,138 @@ static int
>stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
>>> .driver_module = THIS_MODULE,
>>> };
>>>
>>> +static int stm32_adc_dma_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,
>>> + &state);
>>> + 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;
>>> +}
>>> +
>>> +static void stm32_adc_dma_irq_work(struct irq_work *work)
>>> +{
>>> + 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);
>> This is nasty ;) iio_trigger_poll_chained is your friend. It is
>missnamed.
>> If you only need to call the threaded part of the pollfunc (and I
>think you
>> can construct it so you do) then it will call it without needing to
>bounce
>> back into interrupt context.
>
>I'll look into it :-)
>
>>
>>> +}
>>> +
>>> +static void stm32_adc_dma_buffer_done(void *data)
>>> +{
>>> + struct stm32_adc *adc = data;
>>> +
>>> + /* invoques iio_trigger_poll() from hard irq context */
>>> + irq_work_queue(&adc->work);
>>> +}
>>> +
>>> static int stm32_adc_buffer_preenable(struct iio_dev *indio_dev)
>>> {
>>> struct stm32_adc *adc = iio_priv(indio_dev);
>>> + 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;
>>>
>>> - /* Allocate adc buffer */
>>> - adc->buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
>>> - if (!adc->buffer)
>>> + 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 can push with iio_trigger_poll().
>>> + */
>>> + size = indio_dev->buffer->bytes_per_datum *
>indio_dev->buffer->length;
>>> + size = max(indio_dev->scan_bytes * 2, size);
>> Hmm. There is a bit of a weird mix going on here. Firstly, you may
>have more
>> than one consumer buffer, the one you are checking is only the one
>directly
>> associated with the IIO userspace interface.
>>
>> So scan_bytes is the right value to use for both of these statements.
>> The buffer length is typically not knowable either or relevant here.
>> If you are ultimately going to deal with watermarks there is an
>interface
>> to guide read sizes based on that but this isn't it.
>>
>> So basically device should never know anything at all about the
>software
>> buffer. All info should pass through the core which knows about all
>the
>> consumers hanging off this interface (and the demux that is going on
>to
>> feed them all).
>>
>> Some drivers provide additional info to allow the modifying of the
>> precise hardware watermark being used. That's an acceptable form of
>> 'tweak'.
>
>I'll follow your guidelines and rework this part. I also had some
>doubts
>on this. This is more clear!
>
>Thanks,
>Regards,
>Fabrice
>
>>
>>> +
>>> + adc->rx_buf = dma_alloc_coherent(adc->dma_chan->device->dev,
>>> + PAGE_ALIGN(size), &adc->rx_dma_buf,
>>> + GFP_KERNEL);
>>> + if (!adc->rx_buf)
>>> return -ENOMEM;
>>> + adc->rx_buf_sz = size;
>>> + watermark = indio_dev->buffer->bytes_per_datum
>>> + * indio_dev->buffer->watermark;
>>> + watermark = max(indio_dev->scan_bytes, watermark);
>>> + watermark = rounddown(watermark, indio_dev->scan_bytes);
>>> +
>>> + dev_dbg(&indio_dev->dev, "%s size=%d watermark=%d\n", __func__,
>size,
>>> + watermark);
>>> +
>>> + /* Configure DMA channel to read data register */
>>> + memset(&config, 0, sizeof(config));
>>> + config.src_addr = (dma_addr_t)adc->common->phys_base;
>>> + config.src_addr += adc->offset + STM32F4_ADC_DR;
>>> + config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>>> +
>>> + ret = dmaengine_slave_config(adc->dma_chan, &config);
>>> + if (ret)
>>> + goto config_err;
>>> +
>>> + /* Prepare a DMA cyclic transaction */
>>> + desc = dmaengine_prep_dma_cyclic(adc->dma_chan,
>>> + adc->rx_dma_buf,
>>> + size, watermark,
>>> + DMA_DEV_TO_MEM,
>>> + DMA_PREP_INTERRUPT);
>>> + if (!desc) {
>>> + ret = -ENODEV;
>>> + goto config_err;
>>> + }
>>> +
>>> + desc->callback = stm32_adc_dma_buffer_done;
>>> + desc->callback_param = adc;
>>> +
>>> + cookie = dmaengine_submit(desc);
>>> + if (dma_submit_error(cookie)) {
>>> + ret = dma_submit_error(cookie);
>>> + goto config_err;
>>> + }
>>> +
>>> + /* Issue pending DMA requests */
>>> + dma_async_issue_pending(adc->dma_chan);
>>>
>>> return 0;
>>> +
>>> +config_err:
>>> + dma_free_coherent(adc->dma_chan->device->dev, PAGE_ALIGN(size),
>>> + adc->rx_buf, adc->rx_dma_buf);
>>> +
>>> + return ret;
>>> }
>>>
>>> static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
>>> @@ -719,7 +867,8 @@ static int stm32_adc_buffer_postenable(struct
>iio_dev *indio_dev)
>>> if (ret < 0)
>>> return ret;
>>>
>>> - stm32_adc_conv_irq_enable(adc);
>>> + if (!adc->dma_chan)
>>> + stm32_adc_conv_irq_enable(adc);
>>> stm32_adc_start_conv(adc);
>>>
>>> return 0;
>>> @@ -731,7 +880,8 @@ static int stm32_adc_buffer_predisable(struct
>iio_dev *indio_dev)
>>> int ret;
>>>
>>> stm32_adc_stop_conv(adc);
>>> - stm32_adc_conv_irq_disable(adc);
>>> + if (!adc->dma_chan)
>>> + stm32_adc_conv_irq_disable(adc);
>>>
>>> ret = iio_triggered_buffer_predisable(indio_dev);
>>> if (ret < 0)
>>> @@ -748,7 +898,16 @@ static int stm32_adc_buffer_postdisable(struct
>iio_dev *indio_dev)
>>> {
>>> struct stm32_adc *adc = iio_priv(indio_dev);
>>>
>>> - kfree(adc->buffer);
>>> + if (!adc->dma_chan) {
>>> + kfree(adc->buffer);
>>> + } else {
>>> + dmaengine_terminate_all(adc->dma_chan);
>>> + irq_work_sync(&adc->work);
>>> + dma_free_coherent(adc->dma_chan->device->dev,
>>> + PAGE_ALIGN(adc->rx_buf_sz),
>>> + adc->rx_buf, adc->rx_dma_buf);
>>> + adc->rx_buf = NULL;
>>> + }
>>> adc->buffer = NULL;
>>>
>>> return 0;
>>> @@ -769,15 +928,31 @@ static irqreturn_t
>stm32_adc_trigger_handler(int irq, void *p)
>>>
>>> dev_dbg(&indio_dev->dev, "%s bufi=%d\n", __func__, adc->bufi);
>>>
>>> - /* reset buffer index */
>>> - adc->bufi = 0;
>>> - iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
>>> - pf->timestamp);
>>> + if (!adc->dma_chan) {
>>> + /* reset buffer index */
>>> + adc->bufi = 0;
>>> + iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
>>> + pf->timestamp);
>>> + } else {
>>> + int residue = stm32_adc_dma_residue(adc);
>>> +
>>> + while (residue >= indio_dev->scan_bytes) {
>>> + adc->buffer = (u16 *)&adc->rx_buf[adc->bufi];
>>> + iio_push_to_buffers_with_timestamp(indio_dev,
>>> + adc->buffer,
>>> + pf->timestamp);
>>> + residue -= indio_dev->scan_bytes;
>>> + adc->bufi += indio_dev->scan_bytes;
>>> + if (adc->bufi >= adc->rx_buf_sz)
>>> + adc->bufi = 0;
>>> + }
>>> + }
>>>
>>> iio_trigger_notify_done(indio_dev->trig);
>>>
>>> /* re-enable eoc irq */
>>> - stm32_adc_conv_irq_enable(adc);
>>> + if (!adc->dma_chan)
>>> + stm32_adc_conv_irq_enable(adc);
>>>
>>> return IRQ_HANDLED;
>>> }
>>> @@ -910,13 +1085,17 @@ static int stm32_adc_probe(struct
>platform_device *pdev)
>>> if (ret < 0)
>>> goto err_clk_disable;
>>>
>>> + adc->dma_chan = dma_request_slave_channel(&indio_dev->dev, "rx");
>>> + if (adc->dma_chan)
>>> + init_irq_work(&adc->work, stm32_adc_dma_irq_work);
>>> +
>>> ret = iio_triggered_buffer_setup(indio_dev,
>>> &iio_pollfunc_store_time,
>>> &stm32_adc_trigger_handler,
>>> &stm32_adc_buffer_setup_ops);
>>> if (ret) {
>>> dev_err(&pdev->dev, "buffer setup failed\n");
>>> - goto err_clk_disable;
>>> + goto err_dma_disable;
>>> }
>>>
>>> ret = iio_device_register(indio_dev);
>>> @@ -930,6 +1109,10 @@ static int stm32_adc_probe(struct
>platform_device *pdev)
>>> err_buffer_cleanup:
>>> iio_triggered_buffer_cleanup(indio_dev);
>>>
>>> +err_dma_disable:
>>> + if (adc->dma_chan)
>>> + dma_release_channel(adc->dma_chan);
>>> +
>>> err_clk_disable:
>>> clk_disable_unprepare(adc->clk);
>>>
>>> @@ -943,6 +1126,8 @@ static int stm32_adc_remove(struct
>platform_device *pdev)
>>>
>>> iio_device_unregister(indio_dev);
>>> iio_triggered_buffer_cleanup(indio_dev);
>>> + if (adc->dma_chan)
>>> + dma_release_channel(adc->dma_chan);
>>> clk_disable_unprepare(adc->clk);
>>>
>>> return 0;
>>>
>>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 6/7] ARM: dts: stm32: Enable dma by default on stm32f4 adc
2017-01-19 13:34 [PATCH 0/7] Add support for triggered buffer mode to STM32 ADC Fabrice Gasnier
` (4 preceding siblings ...)
2017-01-19 13:34 ` [PATCH 5/7] iio: adc: stm32: add " Fabrice Gasnier
@ 2017-01-19 13:34 ` Fabrice Gasnier
2017-01-19 13:34 ` [PATCH 7/7] ARM: dts: stm32: Enable pwm1 and pwm3 for stm32f469-eval Fabrice Gasnier
6 siblings, 0 replies; 24+ messages in thread
From: Fabrice Gasnier @ 2017-01-19 13:34 UTC (permalink / raw)
To: jic23, linux, robh+dt, linux-arm-kernel, devicetree, linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, fabrice.gasnier, benjamin.gaignard,
benjamin.gaignard
Configure STM32F4 ADC to use dma by default.
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
arch/arm/boot/dts/stm32f429.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index 11d2715..e85db07 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -207,6 +207,8 @@
clocks = <&rcc 0 168>;
interrupt-parent = <&adc>;
interrupts = <0>;
+ dmas = <&dma2 0 0 0x400 0x0>;
+ dma-names = "rx";
status = "disabled";
};
@@ -217,6 +219,8 @@
clocks = <&rcc 0 169>;
interrupt-parent = <&adc>;
interrupts = <1>;
+ dmas = <&dma2 3 1 0x400 0x0>;
+ dma-names = "rx";
status = "disabled";
};
@@ -227,6 +231,8 @@
clocks = <&rcc 0 170>;
interrupt-parent = <&adc>;
interrupts = <2>;
+ dmas = <&dma2 1 2 0x400 0x0>;
+ dma-names = "rx";
status = "disabled";
};
};
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 7/7] ARM: dts: stm32: Enable pwm1 and pwm3 for stm32f469-eval
2017-01-19 13:34 [PATCH 0/7] Add support for triggered buffer mode to STM32 ADC Fabrice Gasnier
` (5 preceding siblings ...)
2017-01-19 13:34 ` [PATCH 6/7] ARM: dts: stm32: Enable dma by default on stm32f4 adc Fabrice Gasnier
@ 2017-01-19 13:34 ` Fabrice Gasnier
2017-01-20 0:09 ` kbuild test robot
2017-01-20 10:19 ` Alexandre Torgue
6 siblings, 2 replies; 24+ messages in thread
From: Fabrice Gasnier @ 2017-01-19 13:34 UTC (permalink / raw)
To: jic23, linux, robh+dt, linux-arm-kernel, devicetree, linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, alexandre.torgue, lars,
knaack.h, pmeerw, fabrice.gasnier, benjamin.gaignard,
benjamin.gaignard
Define and enable pwm1 and pwm3, timers1 & 3 trigger outputs on
stm32f469-eval board.
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
arch/arm/boot/dts/stm32429i-eval.dts | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/arch/arm/boot/dts/stm32429i-eval.dts b/arch/arm/boot/dts/stm32429i-eval.dts
index 2181220..892100f 100644
--- a/arch/arm/boot/dts/stm32429i-eval.dts
+++ b/arch/arm/boot/dts/stm32429i-eval.dts
@@ -171,3 +171,31 @@
pinctrl-names = "default";
status = "okay";
};
+
+&timers1 {
+ status = "okay";
+
+ pwm {
+ pinctrl-0 = <&pwm1_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+ };
+
+ timer@0 {
+ status = "okay";
+ };
+};
+
+&timers3 {
+ status = "okay";
+
+ pwm {
+ pinctrl-0 = <&pwm3_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+ };
+
+ timer@2 {
+ status = "okay";
+ };
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 7/7] ARM: dts: stm32: Enable pwm1 and pwm3 for stm32f469-eval
2017-01-19 13:34 ` [PATCH 7/7] ARM: dts: stm32: Enable pwm1 and pwm3 for stm32f469-eval Fabrice Gasnier
@ 2017-01-20 0:09 ` kbuild test robot
2017-01-20 10:19 ` Alexandre Torgue
1 sibling, 0 replies; 24+ messages in thread
From: kbuild test robot @ 2017-01-20 0:09 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: kbuild-all, jic23, linux, robh+dt, linux-arm-kernel, devicetree,
linux-kernel, linux-iio, mark.rutland, mcoquelin.stm32,
alexandre.torgue, lars, knaack.h, pmeerw, fabrice.gasnier,
benjamin.gaignard, benjamin.gaignard
[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]
Hi Fabrice,
[auto build test ERROR on next-20170119]
[cannot apply to iio/togreg robh/for-next v4.9-rc8 v4.9-rc7 v4.9-rc6 v4.10-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Fabrice-Gasnier/Add-support-for-triggered-buffer-mode-to-STM32-ADC/20170120-062214
config: arm-at91_dt_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
>> Error: arch/arm/boot/dts/stm32429i-eval.dts:179.1-9 Label or path timers1 not found
>> Error: arch/arm/boot/dts/stm32429i-eval.dts:193.1-9 Label or path timers3 not found
FATAL ERROR: Syntax error parsing input tree
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 22227 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 7/7] ARM: dts: stm32: Enable pwm1 and pwm3 for stm32f469-eval
2017-01-19 13:34 ` [PATCH 7/7] ARM: dts: stm32: Enable pwm1 and pwm3 for stm32f469-eval Fabrice Gasnier
2017-01-20 0:09 ` kbuild test robot
@ 2017-01-20 10:19 ` Alexandre Torgue
2017-01-20 10:36 ` Fabrice Gasnier
1 sibling, 1 reply; 24+ messages in thread
From: Alexandre Torgue @ 2017-01-20 10:19 UTC (permalink / raw)
To: Fabrice Gasnier, jic23, linux, robh+dt, linux-arm-kernel,
devicetree, linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, lars, knaack.h, pmeerw,
benjamin.gaignard, benjamin.gaignard
Hi Fabrice
On 01/19/2017 02:34 PM, Fabrice Gasnier wrote:
> Define and enable pwm1 and pwm3, timers1 & 3 trigger outputs on
> stm32f469-eval board.
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
Typo issue in commit header (stm32f469 --> stm32f429)
> arch/arm/boot/dts/stm32429i-eval.dts | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stm32429i-eval.dts b/arch/arm/boot/dts/stm32429i-eval.dts
> index 2181220..892100f 100644
> --- a/arch/arm/boot/dts/stm32429i-eval.dts
> +++ b/arch/arm/boot/dts/stm32429i-eval.dts
> @@ -171,3 +171,31 @@
> pinctrl-names = "default";
> status = "okay";
> };
> +
> +&timers1 {
> + status = "okay";
> +
> + pwm {
> + pinctrl-0 = <&pwm1_pins>;
> + pinctrl-names = "default";
> + status = "okay";
> + };
> +
> + timer@0 {
> + status = "okay";
> + };
> +};
> +
> +&timers3 {
> + status = "okay";
> +
> + pwm {
> + pinctrl-0 = <&pwm3_pins>;
> + pinctrl-names = "default";
> + status = "okay";
> + };
> +
> + timer@2 {
> + status = "okay";
> + };
> +};
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 7/7] ARM: dts: stm32: Enable pwm1 and pwm3 for stm32f469-eval
2017-01-20 10:19 ` Alexandre Torgue
@ 2017-01-20 10:36 ` Fabrice Gasnier
0 siblings, 0 replies; 24+ messages in thread
From: Fabrice Gasnier @ 2017-01-20 10:36 UTC (permalink / raw)
To: Alexandre Torgue, jic23, linux, robh+dt, linux-arm-kernel,
devicetree, linux-kernel
Cc: linux-iio, mark.rutland, mcoquelin.stm32, lars, knaack.h, pmeerw,
benjamin.gaignard, benjamin.gaignard
On 01/20/2017 11:19 AM, Alexandre Torgue wrote:
> Hi Fabrice
>
>
> On 01/19/2017 02:34 PM, Fabrice Gasnier wrote:
>> Define and enable pwm1 and pwm3, timers1 & 3 trigger outputs on
>> stm32f469-eval board.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> ---
>
> Typo issue in commit header (stm32f469 --> stm32f429)
Hi Alexandre,
I'll fix this in next revision.
Tanks for reviewing.
BR,
Fabrice
>
>
>> arch/arm/boot/dts/stm32429i-eval.dts | 28 ++++++++++++++++++++++++++++
>> 1 file changed, 28 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/stm32429i-eval.dts
>> b/arch/arm/boot/dts/stm32429i-eval.dts
>> index 2181220..892100f 100644
>> --- a/arch/arm/boot/dts/stm32429i-eval.dts
>> +++ b/arch/arm/boot/dts/stm32429i-eval.dts
>> @@ -171,3 +171,31 @@
>> pinctrl-names = "default";
>> status = "okay";
>> };
>> +
>> +&timers1 {
>> + status = "okay";
>> +
>> + pwm {
>> + pinctrl-0 = <&pwm1_pins>;
>> + pinctrl-names = "default";
>> + status = "okay";
>> + };
>> +
>> + timer@0 {
>> + status = "okay";
>> + };
>> +};
>> +
>> +&timers3 {
>> + status = "okay";
>> +
>> + pwm {
>> + pinctrl-0 = <&pwm3_pins>;
>> + pinctrl-names = "default";
>> + status = "okay";
>> + };
>> +
>> + timer@2 {
>> + status = "okay";
>> + };
>> +};
>>
^ permalink raw reply [flat|nested] 24+ messages in thread