Re: [PATCH 2/7] iio: adc: stm32: Enable use of stm32 timer triggers

[Fabrice Gasnier]

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.gasn...@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;
 }