[PATCH v2 0/7] Add pwm and IIO timer drivers for stm32

[PATCH v2 0/7] Add pwm and IIO timer drivers for stm32

[Benjamin Gaignard]

version 2:
- keep only one compatible per driver
- use DT parameters to describe hardware block configuration:
  - pwm channels, complementary output, counter size, break input
  - triggers accepted and create by IIO timers
- change DT to limite use of reference to the node
- interrupt is now in IIO timer driver
- rename stm32-mfd-timer to stm32-gptimer (for general purpose timer)

The following patches enable pwm and IIO Timer features for stm32 platforms.

Those two features are mixed into the registers of the same hardware block
(named general purpose timer) which lead to introduce a multifunctions driver 
on the top of them to be able to share the registers.

In stm32 14 instances of timer hardware block exist, even if they all have
the same register mapping they could have a different number of pwm channels
and/or different triggers capabilities. We use various parameters in DT to 
describe the differences between hardware blocks

The MFD (stm32-gptimer.c) takes care of clock and register mapping
by using regmap. stm32_gptimer_dev structure is provided to its sub-node to
share those information.

PWM driver is implemented into pwm-stm32.c. Depending of the instance we may
have up to 4 channels, sometime with complementary outputs or 32 bits counter
instead of 16 bits. Some hardware blocks may also have a break input function
which allows to stop pwm depending of a level, defined in devicetree, on an
external pin.

IIO timer driver (stm32-iio-timer.c and stm32-iio-timers.h) define a list of 
hardware triggers usable by hardware blocks like ADC, DAC or other timers. 

The matrix of possible connections between blocks is quite complex so we use 
trigger names and is_stm32_iio_timer_trigger() function to be sure that
triggers are valid and configure the IPs.
Possible triggers ar listed in include/dt-bindings/iio/timer/st,stm32-iio-timer.h

At run time IIO timer hardware blocks can configure (through "master_mode" 
IIO device attribute) which internal signal (counter enable, reset,
comparison block, etc...) is used to generate the trigger.

By using "slave_mode" IIO device attribute timer can also configure on which
event (level, rising edge) of the block is enabled.

Since we can use trigger from one hardware to control an other block, we can
use a pwm to control an other one. The following example shows how to configure
pwm1 and pwm3 to make pwm3 generate pulse only when pwm1 pulse level is high.

/sys/bus/iio/devices # ls
iio:device0  iio:device1  trigger0     trigger1

configure timer1 to use pwm1 channel 0 as output trigger
/sys/bus/iio/devices # echo 4 > iio\:device0/master_mode
configure timer3 to enable only when input is high
/sys/bus/iio/devices # echo 5 > iio\:device1/slave_mode
/sys/bus/iio/devices # cat trigger0/name
tim1_trgo
configure timer2 to use timer1 trigger is input
/sys/bus/iio/devices # echo "tim1_trgo" > iio\:device1/trigger/current_trigger

configure pwm3 channel 0 to generate a signal with a period of 100ms and a
duty cycle of 50%
/sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 0 > export
/sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 100000000 > pwm0/period
/sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 50000000 > pwm0/duty_cycle
/sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4# echo 1 > pwm0/enable
here pwm3 channel 0, as expected, doesn't start because has to be triggered by
pwm1 channel 0

configure pwm1 channel 0 to generate a signal with a period of 1s and a
duty cycle of 50%
/sys/devices/platform/soc/40010000.gptimer1/40010000.gptimer1:pwm1@0/pwm/pwmchip0 # echo 0 > export
/sys/devices/platform/soc/40010000.gptimer1/40010000.gptimer1:pwm1@0/pwm/pwmchip0 # echo 1000000000 > pwm0/period
/sys/devices/platform/soc/40010000.gptimer1/40010000.gptimer1:pwm1@0/pwm/pwmchip0 # echo 500000000 > pwm0/duty_cycle
/sys/devices/platform/soc/40010000.gptimer1/40010000.gptimer1:pwm1@0/pwm/pwmchip0 # echo 1 > pwm0/enable 
finally pwm1 starts and pwm3 only generates pulse when pwm1 signal is high

An other example to use a timer as source of clock for another device.
Here timer1 is used a source clock for pwm3:

/sys/bus/iio/devices # echo 100000 > trigger0/sampling_frequency 
/sys/bus/iio/devices # echo tim1_trgo > iio\:device1/trigger/current_trigger 
/sys/bus/iio/devices # echo 7 > iio\:device1/slave_mode
/sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 0 > export 
/sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 1000000 > pwm0/period 
/sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 500000 > pwm0/duty_cycle 
/sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 1 > pwm0/enable 

Benjamin Gaignard (7):
  MFD: add bindings for stm32 general purpose timer driver
  MFD: add stm32 general purpose timer driver
  PWM: add pwm-stm32 DT bindings
  PWM: add pwm driver for stm32 plaftorm
  IIO: add bindings for stm32 IIO timer driver
  IIO: add STM32 IIO timer driver
  ARM: dts: stm32: add stm32 general purpose timer driver in DT

 .../bindings/iio/timer/stm32-iio-timer.txt         |  41 ++
 .../bindings/mfd/stm32-general-purpose-timer.txt   |  43 ++
 .../devicetree/bindings/pwm/pwm-stm32.txt          |  37 ++
 arch/arm/boot/dts/stm32f429.dtsi                   | 305 +++++++++++++-
 arch/arm/boot/dts/stm32f469-disco.dts              |  28 ++
 drivers/iio/Kconfig                                |   2 +-
 drivers/iio/Makefile                               |   1 +
 drivers/iio/timer/Kconfig                          |  15 +
 drivers/iio/timer/Makefile                         |   1 +
 drivers/iio/timer/stm32-iio-timer.c                | 448 +++++++++++++++++++++
 drivers/iio/trigger/Kconfig                        |   1 -
 drivers/mfd/Kconfig                                |  10 +
 drivers/mfd/Makefile                               |   2 +
 drivers/mfd/stm32-gptimer.c                        |  73 ++++
 drivers/pwm/Kconfig                                |   8 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-stm32.c                            | 285 +++++++++++++
 include/dt-bindings/iio/timer/st,stm32-iio-timer.h |  23 ++
 include/linux/iio/timer/stm32-iio-timers.h         |  16 +
 include/linux/mfd/stm32-gptimer.h                  |  62 +++
 20 files changed, 1399 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
 create mode 100644 drivers/iio/timer/Kconfig
 create mode 100644 drivers/iio/timer/Makefile
 create mode 100644 drivers/iio/timer/stm32-iio-timer.c
 create mode 100644 drivers/mfd/stm32-gptimer.c
 create mode 100644 drivers/pwm/pwm-stm32.c
 create mode 100644 include/dt-bindings/iio/timer/st,stm32-iio-timer.h
 create mode 100644 include/linux/iio/timer/stm32-iio-timers.h
 create mode 100644 include/linux/mfd/stm32-gptimer.h

-- 
1.9.1

[PATCH v2 1/7] MFD: add bindings for stm32 general purpose timer driver

[Benjamin Gaignard]

Add bindings information for stm32 general purpose timer

version 2:
- rename stm32-mfd-timer to stm32-gptimer
- only keep one compatible string

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 .../bindings/mfd/stm32-general-purpose-timer.txt   | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt

diff --git a/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
new file mode 100644
index 0000000..2f10e67
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
@@ -0,0 +1,43 @@
+STM32 general purpose timer driver
+
+Required parameters:
+- compatible: must be "st,stm32-gptimer"
+
+- reg:			Physical base address and length of the controller's
+			registers.
+- clock-names: 		Set to "clk_int".
+- clocks: 		Phandle to the clock used by the timer module.
+			For Clk properties, please refer to ../clock/clock-bindings.txt
+
+Optional parameters:
+- resets:		Phandle to the parent reset controller.
+			See ..reset/st,stm32-rcc.txt
+
+Optional subnodes:
+- pwm:			See ../pwm/pwm-stm32.txt
+- iiotimer:		See ../iio/timer/stm32-iio-timer.txt
+
+Example:
+	gptimer1: gptimer1@40010000 {
+		compatible = "st,stm32-gptimer";
+		reg = <0x40010000 0x400>;
+		clocks = <&rcc 0 160>;
+		clock-names = "clk_int";
+
+		pwm1@0 {
+			compatible = "st,stm32-pwm";
+			st,pwm-num-chan = <4>;
+			st,breakinput;
+			st,complementary;
+		};
+
+		iiotimer1@0 {
+			compatible = "st,stm32-iio-timer";
+			interrupts = <27>;
+			st,input-triggers-names = TIM5_TRGO,
+						  TIM2_TRGO,
+						  TIM4_TRGO,
+						  TIM3_TRGO;
+			st,output-triggers-names = TIM1_TRGO;
+		};
+	};
-- 
1.9.1

[PATCH v2 2/7] MFD: add stm32 general purpose timer driver

[Benjamin Gaignard]

This hardware block could at used at same time for PWM generation
and IIO timer for other IPs like DAC, ADC or other timers.
PWM and IIO timer configuration are mixed in the same registers
so we need a multi fonction driver to be able to share those registers.

version 2:
- rename driver "stm32-gptimer" to be align with SoC documentation
- only keep one compatible
- use of_platform_populate() instead of devm_mfd_add_devices()

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 drivers/mfd/Kconfig               | 10 ++++++
 drivers/mfd/Makefile              |  2 ++
 drivers/mfd/stm32-gptimer.c       | 73 +++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/stm32-gptimer.h | 62 +++++++++++++++++++++++++++++++++
 4 files changed, 147 insertions(+)
 create mode 100644 drivers/mfd/stm32-gptimer.c
 create mode 100644 include/linux/mfd/stm32-gptimer.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index c6df644..e75abcb 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1607,6 +1607,15 @@ config MFD_STW481X
 	  in various ST Microelectronics and ST-Ericsson embedded
 	  Nomadik series.
 
+config MFD_STM32_GP_TIMER
+	tristate "Support for STM32 General Purpose Timer"
+	select MFD_CORE
+	select REGMAP
+	depends on ARCH_STM32
+	depends on OF
+	help
+	  Select this option to enable stm32 general purpose timer
+
 menu "Multimedia Capabilities Port drivers"
 	depends on ARCH_SA1100
 
@@ -1644,4 +1653,5 @@ config MFD_VEXPRESS_SYSREG
 	  on the ARM Ltd. Versatile Express board.
 
 endmenu
+
 endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 9834e66..86353b9 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -211,3 +211,5 @@ obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
 obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
 
 obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
+
+obj-$(CONFIG_MFD_STM32_GP_TIMER) 	+= stm32-gptimer.o
diff --git a/drivers/mfd/stm32-gptimer.c b/drivers/mfd/stm32-gptimer.c
new file mode 100644
index 0000000..54fb95c
--- /dev/null
+++ b/drivers/mfd/stm32-gptimer.c
@@ -0,0 +1,73 @@
+/*
+ * stm32-gptimer.c
+ *
+ * Copyright (C) STMicroelectronics 2016
+ * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/reset.h>
+
+#include <linux/mfd/stm32-gptimer.h>
+
+static const struct regmap_config stm32_gptimer_regmap_cfg = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = sizeof(u32),
+	.max_register = 0x400,
+	.fast_io = true,
+};
+
+static int stm32_gptimer_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct stm32_gptimer_dev *mfd;
+	struct resource *res;
+	void __iomem *mmio;
+
+	mfd = devm_kzalloc(dev, sizeof(*mfd), GFP_KERNEL);
+	if (!mfd)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENOMEM;
+
+	mmio = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mmio))
+		return PTR_ERR(mmio);
+
+	mfd->regmap = devm_regmap_init_mmio_clk(dev, "clk_int", mmio,
+						&stm32_gptimer_regmap_cfg);
+	if (IS_ERR(mfd->regmap))
+		return PTR_ERR(mfd->regmap);
+
+	mfd->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(mfd->clk))
+		return PTR_ERR(mfd->clk);
+
+	platform_set_drvdata(pdev, mfd);
+
+	return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+}
+
+static const struct of_device_id stm32_gptimer_of_match[] = {
+	{
+		.compatible = "st,stm32-gptimer",
+	},
+};
+MODULE_DEVICE_TABLE(of, stm32_gptimer_of_match);
+
+static struct platform_driver stm32_gptimer_driver = {
+	.probe		= stm32_gptimer_probe,
+	.driver	= {
+		.name	= "stm32-gptimer",
+		.of_match_table = stm32_gptimer_of_match,
+	},
+};
+module_platform_driver(stm32_gptimer_driver);
+
+MODULE_DESCRIPTION("STMicroelectronics STM32 general purpose timer");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/stm32-gptimer.h b/include/linux/mfd/stm32-gptimer.h
new file mode 100644
index 0000000..f8c92de
--- /dev/null
+++ b/include/linux/mfd/stm32-gptimer.h
@@ -0,0 +1,62 @@
+/*
+ * stm32-gptimer.h
+ *
+ * Copyright (C) STMicroelectronics 2016
+ * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#ifndef _LINUX_STM32_GPTIMER_H_
+#define _LINUX_STM32_GPTIMER_H_
+
+#include <linux/clk.h>
+#include <linux/regmap.h>
+
+#define TIM_CR1		0x00	/* Control Register 1      */
+#define TIM_CR2		0x04	/* Control Register 2      */
+#define TIM_SMCR	0x08	/* Slave mode control reg  */
+#define TIM_DIER	0x0C	/* DMA/interrupt register  */
+#define TIM_SR		0x10	/* Status register	   */
+#define TIM_EGR		0x14	/* Event Generation Reg    */
+#define TIM_CCMR1	0x18	/* Capt/Comp 1 Mode Reg    */
+#define TIM_CCMR2	0x1C	/* Capt/Comp 2 Mode Reg    */
+#define TIM_CCER	0x20	/* Capt/Comp Enable Reg    */
+#define TIM_PSC		0x28	/* Prescaler               */
+#define TIM_ARR		0x2c	/* Auto-Reload Register    */
+#define TIM_CCR1	0x34	/* Capt/Comp Register 1    */
+#define TIM_CCR2	0x38	/* Capt/Comp Register 2    */
+#define TIM_CCR3	0x3C	/* Capt/Comp Register 3    */
+#define TIM_CCR4	0x40	/* Capt/Comp Register 4    */
+#define TIM_BDTR	0x44	/* Break and Dead-Time Reg */
+
+#define TIM_CR1_CEN	BIT(0)	/* Counter Enable	   */
+#define TIM_CR1_ARPE	BIT(7)	/* Auto-reload Preload Ena */
+#define TIM_CR2_MMS	(BIT(4) | BIT(5) | BIT(6)) /* Master mode selection */
+#define TIM_SMCR_SMS	(BIT(0) | BIT(1) | BIT(2)) /* Slave mode selection */
+#define TIM_SMCR_TS	(BIT(4) | BIT(5) | BIT(6)) /* Trigger selection */
+#define TIM_DIER_UIE	BIT(0)	/* Update interrupt	   */
+#define TIM_SR_UIF	BIT(0)	/* Update interrupt flag   */
+#define TIM_EGR_UG	BIT(0)	/* Update Generation       */
+#define TIM_CCMR_PE	BIT(3)	/* Channel Preload Enable  */
+#define TIM_CCMR_M1	(BIT(6) | BIT(5))  /* Channel PWM Mode 1 */
+#define TIM_CCER_CC1E	BIT(0)	/* Capt/Comp 1  out Ena    */
+#define TIM_CCER_CC1P	BIT(1)	/* Capt/Comp 1  Polarity   */
+#define TIM_CCER_CC1NE	BIT(2)	/* Capt/Comp 1N out Ena    */
+#define TIM_CCER_CC1NP	BIT(3)	/* Capt/Comp 1N Polarity   */
+#define TIM_CCER_CCXE	(BIT(0) | BIT(4) | BIT(8) | BIT(12))
+#define TIM_BDTR_BKE	BIT(12) /* Break input enable	   */
+#define TIM_BDTR_BKP	BIT(13) /* Break input polarity	   */
+#define TIM_BDTR_AOE	BIT(14)	/* Automatic Output Enable */
+#define TIM_BDTR_MOE	BIT(15)	/* Main Output Enable      */
+
+#define MAX_TIM_PSC		0xFFFF
+
+struct stm32_gptimer_dev {
+	/* Device data */
+	struct clk *clk;
+
+	/* Registers mapping */
+	struct regmap *regmap;
+};
+
+#endif
-- 
1.9.1

[PATCH v2 3/7] PWM: add pwm-stm32 DT bindings

[Benjamin Gaignard]

Define bindings for pwm-stm32

version 2:
- use parameters instead of compatible of handle the hardware configuration

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 .../devicetree/bindings/pwm/pwm-stm32.txt          | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
new file mode 100644
index 0000000..36263f0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
@@ -0,0 +1,37 @@
+STMicroelectronics PWM driver bindings for STM32
+
+Must be a sub-node of STM32 general purpose timer driver
+
+Required parameters:
+- compatible:		Must be "st,stm32-pwm"
+- pinctrl-names: 	Set to "default".
+- pinctrl-0: 		List of phandles pointing to pin configuration nodes
+			for PWM module.
+			For Pinctrl properties, please refer to [1].
+
+Optional parameters:
+- st,breakinput:	Set if the hardware have break input capabilities
+- st,breakinput-polarity: Set break input polarity. Default is 0
+			 The value define the active polarity:
+			  - 0 (active LOW)
+			  - 1 (active HIGH)
+- st,pwm-num-chan:	Number of available PWM channels.  Default is 0.
+- st,32bits-counter:	Set if the hardware have a 32 bits counter
+- st,complementary:	Set if the hardware have complementary output channels
+
+[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+
+Example:
+	gptimer1: gptimer1@40010000 {
+		compatible = "st,stm32-gptimer";
+		reg = <0x40010000 0x400>;
+		clocks = <&rcc 0 160>;
+		clock-names = "clk_int";
+
+		pwm1@0 {
+			compatible = "st,stm32-pwm";
+			st,pwm-num-chan = <4>;
+			st,breakinput;
+			st,complementary;
+		};
+	};
-- 
1.9.1

[PATCH v2 4/7] PWM: add pwm driver for stm32 plaftorm

[Benjamin Gaignard]

This driver add support for pwm driver on stm32 platform.
The SoC have multiple instances of the hardware IP and each
of them could have small differences: number of channels,
complementary output, counter register size...
Use DT parameters to handle those differentes configuration

version 2:
- only keep one comptatible
- use DT paramaters to discover hardware block configuration

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 drivers/pwm/Kconfig     |   8 ++
 drivers/pwm/Makefile    |   1 +
 drivers/pwm/pwm-stm32.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 294 insertions(+)
 create mode 100644 drivers/pwm/pwm-stm32.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index bf01288..a89fdba 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -388,6 +388,14 @@ config PWM_STI
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sti.
 
+config PWM_STM32
+	bool "STMicroelectronics STM32 PWM"
+	depends on ARCH_STM32
+	depends on OF
+	select MFD_STM32_GP_TIMER
+	help
+	  Generic PWM framework driver for STM32 SoCs.
+
 config PWM_STMPE
 	bool "STMPE expander PWM export"
 	depends on MFD_STMPE
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 1194c54..5aa9308 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
+obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
 obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
 obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
new file mode 100644
index 0000000..a362f63
--- /dev/null
+++ b/drivers/pwm/pwm-stm32.c
@@ -0,0 +1,285 @@
+/*
+ * Copyright (C) STMicroelectronics 2016
+ * Author:  Gerald Baeza <gerald.baeza@st.com>
+ * License terms:  GNU General Public License (GPL), version 2
+ *
+ * Inspired by timer-stm32.c from Maxime Coquelin
+ *             pwm-atmel.c from Bo Shen
+ */
+
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+#include <linux/mfd/stm32-gptimer.h>
+
+#define DRIVER_NAME "stm32-pwm"
+
+#define CAP_COMPLEMENTARY	BIT(0)
+#define CAP_32BITS_COUNTER	BIT(1)
+#define CAP_BREAKINPUT		BIT(2)
+#define CAP_BREAKINPUT_POLARITY BIT(3)
+
+struct stm32_pwm_dev {
+	struct device *dev;
+	struct clk *clk;
+	struct regmap *regmap;
+	struct pwm_chip chip;
+	int caps;
+	int npwm;
+	u32 polarity;
+};
+
+#define to_stm32_pwm_dev(x) container_of(chip, struct stm32_pwm_dev, chip)
+
+static u32 __active_channels(struct stm32_pwm_dev *pwm_dev)
+{
+	u32 ccer;
+
+	regmap_read(pwm_dev->regmap, TIM_CCER, &ccer);
+
+	return ccer & TIM_CCER_CCXE;
+}
+
+static int write_ccrx(struct stm32_pwm_dev *dev, struct pwm_device *pwm,
+		      u32 ccr)
+{
+	switch (pwm->hwpwm) {
+	case 0:
+		return regmap_write(dev->regmap, TIM_CCR1, ccr);
+	case 1:
+		return regmap_write(dev->regmap, TIM_CCR2, ccr);
+	case 2:
+		return regmap_write(dev->regmap, TIM_CCR3, ccr);
+	case 3:
+		return regmap_write(dev->regmap, TIM_CCR4, ccr);
+	}
+	return -EINVAL;
+}
+
+static int stm32_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			    int duty_ns, int period_ns)
+{
+	struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
+	unsigned long long prd, div, dty;
+	int prescaler = 0;
+	u32 max_arr = 0xFFFF, ccmr, mask, shift, bdtr;
+
+	if (dev->caps & CAP_32BITS_COUNTER)
+		max_arr = 0xFFFFFFFF;
+
+	/* Period and prescaler values depends of clock rate */
+	div = (unsigned long long)clk_get_rate(dev->clk) * period_ns;
+
+	do_div(div, NSEC_PER_SEC);
+	prd = div;
+
+	while (div > max_arr) {
+		prescaler++;
+		div = prd;
+		do_div(div, (prescaler + 1));
+	}
+	prd = div;
+
+	if (prescaler > MAX_TIM_PSC) {
+		dev_err(chip->dev, "prescaler exceeds the maximum value\n");
+		return -EINVAL;
+	}
+
+	/* All channels share the same prescaler and counter so
+	 * when two channels are active at the same we can't change them
+	 */
+	if (__active_channels(dev) & ~(1 << pwm->hwpwm * 4)) {
+		u32 psc, arr;
+
+		regmap_read(dev->regmap, TIM_PSC, &psc);
+		regmap_read(dev->regmap, TIM_ARR, &arr);
+
+		if ((psc != prescaler) || (arr != prd - 1))
+			return -EINVAL;
+	}
+
+	regmap_write(dev->regmap, TIM_PSC, prescaler);
+	regmap_write(dev->regmap, TIM_ARR, prd - 1);
+	regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
+
+	/* Calculate the duty cycles */
+	dty = prd * duty_ns;
+	do_div(dty, period_ns);
+
+	write_ccrx(dev, pwm, dty);
+
+	/* Configure output mode */
+	shift = (pwm->hwpwm & 0x1) * 8;
+	ccmr = (TIM_CCMR_PE | TIM_CCMR_M1) << shift;
+	mask = 0xFF << shift;
+
+	if (pwm->hwpwm & 0x2)
+		regmap_update_bits(dev->regmap, TIM_CCMR2, mask, ccmr);
+	else
+		regmap_update_bits(dev->regmap, TIM_CCMR1, mask, ccmr);
+
+	if (!(dev->caps & CAP_BREAKINPUT))
+		return 0;
+
+	bdtr = TIM_BDTR_MOE | TIM_BDTR_AOE;
+
+	if (dev->caps & CAP_BREAKINPUT_POLARITY)
+		bdtr |= TIM_BDTR_BKE;
+
+	if (dev->polarity)
+		bdtr |= TIM_BDTR_BKP;
+
+	regmap_update_bits(dev->regmap, TIM_BDTR,
+			   TIM_BDTR_MOE | TIM_BDTR_AOE |
+			   TIM_BDTR_BKP | TIM_BDTR_BKE,
+			   bdtr);
+
+	return 0;
+}
+
+static int stm32_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+				  enum pwm_polarity polarity)
+{
+	u32 mask;
+	struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
+
+	mask = TIM_CCER_CC1P << (pwm->hwpwm * 4);
+	if (dev->caps & CAP_COMPLEMENTARY)
+		mask |= TIM_CCER_CC1NP << (pwm->hwpwm * 4);
+
+	regmap_update_bits(dev->regmap, TIM_CCER, mask,
+			   polarity == PWM_POLARITY_NORMAL ? 0 : mask);
+
+	return 0;
+}
+
+static int stm32_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	u32 mask;
+	struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
+
+	clk_enable(dev->clk);
+
+	/* Enable channel */
+	mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
+	if (dev->caps & CAP_COMPLEMENTARY)
+		mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
+
+	regmap_update_bits(dev->regmap, TIM_CCER, mask, mask);
+
+	/* Make sure that registers are updated */
+	regmap_update_bits(dev->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
+
+	/* Enable controller */
+	regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
+
+	return 0;
+}
+
+static void stm32_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	u32 mask;
+	struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
+
+	/* Disable channel */
+	mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
+	if (dev->caps & CAP_COMPLEMENTARY)
+		mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
+
+	regmap_update_bits(dev->regmap, TIM_CCER, mask, 0);
+
+	/* When all channels are disabled, we can disable the controller */
+	if (!__active_channels(dev))
+		regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_CEN, 0);
+
+	clk_disable(dev->clk);
+}
+
+static const struct pwm_ops stm32pwm_ops = {
+	.config = stm32_pwm_config,
+	.set_polarity = stm32_pwm_set_polarity,
+	.enable = stm32_pwm_enable,
+	.disable = stm32_pwm_disable,
+};
+
+static int stm32_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct stm32_gptimer_dev *mfd = dev_get_drvdata(pdev->dev.parent);
+	struct stm32_pwm_dev *pwm;
+	int ret;
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->regmap = mfd->regmap;
+	pwm->clk = mfd->clk;
+
+	if (!pwm->regmap || !pwm->clk)
+		return -EINVAL;
+
+	if (of_property_read_bool(np, "st,complementary"))
+		pwm->caps |= CAP_COMPLEMENTARY;
+
+	if (of_property_read_bool(np, "st,32bits-counter"))
+		pwm->caps |= CAP_32BITS_COUNTER;
+
+	if (of_property_read_bool(np, "st,breakinput"))
+		pwm->caps |= CAP_BREAKINPUT;
+
+	if (!of_property_read_u32(np, "st,breakinput-polarity", &pwm->polarity))
+		pwm->caps |= CAP_BREAKINPUT_POLARITY;
+
+	of_property_read_u32(np, "st,pwm-num-chan", &pwm->npwm);
+
+	pwm->chip.base = -1;
+	pwm->chip.dev = dev;
+	pwm->chip.ops = &stm32pwm_ops;
+	pwm->chip.npwm = pwm->npwm;
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, pwm);
+
+	return 0;
+}
+
+static int stm32_pwm_remove(struct platform_device *pdev)
+{
+	struct stm32_pwm_dev *pwm = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < pwm->npwm; i++)
+		pwm_disable(&pwm->chip.pwms[i]);
+
+	pwmchip_remove(&pwm->chip);
+
+	return 0;
+}
+
+static const struct of_device_id stm32_pwm_of_match[] = {
+	{
+		.compatible = "st,stm32-pwm",
+	},
+};
+MODULE_DEVICE_TABLE(of, stm32_pwm_of_match);
+
+static struct platform_driver stm32_pwm_driver = {
+	.probe		= stm32_pwm_probe,
+	.remove		= stm32_pwm_remove,
+	.driver	= {
+		.name	= DRIVER_NAME,
+		.of_match_table = stm32_pwm_of_match,
+	},
+};
+module_platform_driver(stm32_pwm_driver);
+
+MODULE_ALIAS("platform:" DRIVER_NAME);
+MODULE_DESCRIPTION("STMicroelectronics STM32 PWM driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH v2 5/7] IIO: add bindings for stm32 IIO timer driver

[Benjamin Gaignard]

Define bindings for stm32 IIO timer

version 2:
- only keep one compatible
- add DT parameters to set lists of the triggers:
  one list describe the triggers created by the device
  another one give the triggers accepted by the device

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 .../bindings/iio/timer/stm32-iio-timer.txt         | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt

diff --git a/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
new file mode 100644
index 0000000..840b417
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
@@ -0,0 +1,41 @@
+timer IIO trigger bindings for STM32
+
+Must be a sub-node of STM32 general purpose timer driver
+
+Required parameters:
+- compatible:		must be "st,stm32-iio-timer"
+- interrupts:		Interrupt for this device
+			See ../interrupt-controller/st,stm32-exti.txt
+
+Optional parameters:
+- st,input-triggers-names:	List of the possible input triggers for
+				the device
+- st,output-triggers-names:	List of the possible output triggers for
+				the device
+
+Possible triggers are defined in include/dt-bindings/iio/timer/st,stm32-iio-timer.h
+
+Example:
+	gptimer1: gptimer1@40010000 {
+		compatible = "st,stm32-gptimer";
+		reg = <0x40010000 0x400>;
+		clocks = <&rcc 0 160>;
+		clock-names = "clk_int";
+
+		pwm1@0 {
+			compatible = "st,stm32-pwm";
+			st,pwm-num-chan = <4>;
+			st,breakinput;
+			st,complementary;
+		};
+
+		iiotimer1@0 {
+			compatible = "st,stm32-iio-timer";
+			interrupts = <27>;
+			st,input-triggers-names = TIM5_TRGO,
+						  TIM2_TRGO,
+						  TIM4_TRGO,
+						  TIM3_TRGO;
+			st,output-triggers-names = TIM1_TRGO;
+		};
+	};
-- 
1.9.1

[PATCH v2 6/7] IIO: add STM32 IIO timer driver

[Benjamin Gaignard]

Timers IPs can be used to generate triggers for other IPs like
DAC, ADC or other timers.
Each trigger may result of timer internals signals like counter enable,
reset or edge, this configuration could be done through "master_mode"
device attribute.

A timer device could be triggered by other timers, we use the trigger
name and is_stm32_iio_timer_trigger() function to distinguish them
and configure IP input switch.

Timer may also decide on which event (edge, level) they could
be activated by a trigger, this configuration is done by writing in
"slave_mode" device attribute.

Since triggers could also be used by DAC or ADC their names are defined
in include/dt-bindings/iio/timer/st,stm32-iio-timer.h so those IPs will be able
to configure themselves in valid_trigger function

Trigger have a "sampling_frequency" attribute which allow to configure
timer sampling frequency without using pwm interface

version 2:
- keep only one compatible
- use st,input-triggers-names and st,output-triggers-names
  to know which triggers are accepted and/or create by the device

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 drivers/iio/Kconfig                                |   2 +-
 drivers/iio/Makefile                               |   1 +
 drivers/iio/timer/Kconfig                          |  15 +
 drivers/iio/timer/Makefile                         |   1 +
 drivers/iio/timer/stm32-iio-timer.c                | 448 +++++++++++++++++++++
 drivers/iio/trigger/Kconfig                        |   1 -
 include/dt-bindings/iio/timer/st,stm32-iio-timer.h |  23 ++
 include/linux/iio/timer/stm32-iio-timers.h         |  16 +
 8 files changed, 505 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iio/timer/Kconfig
 create mode 100644 drivers/iio/timer/Makefile
 create mode 100644 drivers/iio/timer/stm32-iio-timer.c
 create mode 100644 include/dt-bindings/iio/timer/st,stm32-iio-timer.h
 create mode 100644 include/linux/iio/timer/stm32-iio-timers.h

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 6743b18..2de2a80 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -90,5 +90,5 @@ source "drivers/iio/potentiometer/Kconfig"
 source "drivers/iio/pressure/Kconfig"
 source "drivers/iio/proximity/Kconfig"
 source "drivers/iio/temperature/Kconfig"
-
+source "drivers/iio/timer/Kconfig"
 endif # IIO
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 87e4c43..b797c08 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -32,4 +32,5 @@ obj-y += potentiometer/
 obj-y += pressure/
 obj-y += proximity/
 obj-y += temperature/
+obj-y += timer/
 obj-y += trigger/
diff --git a/drivers/iio/timer/Kconfig b/drivers/iio/timer/Kconfig
new file mode 100644
index 0000000..7a73bc6
--- /dev/null
+++ b/drivers/iio/timer/Kconfig
@@ -0,0 +1,15 @@
+#
+# Timers drivers
+
+menu "Timers"
+
+config IIO_STM32_TIMER
+	tristate "stm32 iio timer"
+	depends on ARCH_STM32
+	depends on OF
+	select IIO_TRIGGERED_EVENT
+	select MFD_STM32_GP_TIMER
+	help
+	  Select this option to enable stm32 timers hardware IPs
+
+endmenu
diff --git a/drivers/iio/timer/Makefile b/drivers/iio/timer/Makefile
new file mode 100644
index 0000000..a360c9f
--- /dev/null
+++ b/drivers/iio/timer/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_IIO_STM32_TIMER) += stm32-iio-timer.o
diff --git a/drivers/iio/timer/stm32-iio-timer.c b/drivers/iio/timer/stm32-iio-timer.c
new file mode 100644
index 0000000..35f2687
--- /dev/null
+++ b/drivers/iio/timer/stm32-iio-timer.c
@@ -0,0 +1,448 @@
+/*
+ * stm32-iio-timer.c
+ *
+ * Copyright (C) STMicroelectronics 2016
+ * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/timer/stm32-iio-timers.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_event.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/stm32-gptimer.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define DRIVER_NAME "stm32-iio-timer"
+
+struct stm32_iio_timer_dev {
+	struct device *dev;
+	struct regmap *regmap;
+	struct clk *clk;
+	int irq;
+	bool own_timer;
+	unsigned int sampling_frequency;
+	struct iio_trigger *active_trigger;
+};
+
+static ssize_t _store_frequency(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t len)
+{
+	struct iio_trigger *trig = to_iio_trigger(dev);
+	struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
+	unsigned int freq;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &freq);
+	if (ret)
+		return ret;
+
+	stm32->sampling_frequency = freq;
+
+	return len;
+}
+
+static ssize_t _read_frequency(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct iio_trigger *trig = to_iio_trigger(dev);
+	struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
+	unsigned long long freq = stm32->sampling_frequency;
+	u32 psc, arr, cr1;
+
+	regmap_read(stm32->regmap, TIM_CR1, &cr1);
+	regmap_read(stm32->regmap, TIM_PSC, &psc);
+	regmap_read(stm32->regmap, TIM_ARR, &arr);
+
+	if (psc && arr && (cr1 & TIM_CR1_CEN)) {
+		freq = (unsigned long long)clk_get_rate(stm32->clk);
+		do_div(freq, psc);
+		do_div(freq, arr);
+	}
+
+	return sprintf(buf, "%d\n", (unsigned int)freq);
+}
+
+static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
+			      _read_frequency,
+			      _store_frequency);
+
+static struct attribute *stm32_trigger_attrs[] = {
+	&iio_dev_attr_sampling_frequency.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group stm32_trigger_attr_group = {
+	.attrs = stm32_trigger_attrs,
+};
+
+static const struct attribute_group *stm32_trigger_attr_groups[] = {
+	&stm32_trigger_attr_group,
+	NULL,
+};
+
+static
+ssize_t _show_master_mode(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
+	u32 cr2;
+
+	regmap_read(stm32->regmap, TIM_CR2, &cr2);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", (cr2 >> 4) & 0x7);
+}
+
+static
+ssize_t _store_master_mode(struct device *dev,
+			   struct device_attribute *attr,
+			   const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
+	u8 mode;
+	int ret;
+
+	ret = kstrtou8(buf, 10, &mode);
+	if (ret)
+		return ret;
+
+	if (mode > 0x7)
+		return -EINVAL;
+
+	regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, mode << 4);
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(master_mode, S_IRUGO | S_IWUSR,
+		       _show_master_mode,
+		       _store_master_mode,
+		       0);
+
+static
+ssize_t _show_slave_mode(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
+	u32 smcr;
+
+	regmap_read(stm32->regmap, TIM_SMCR, &smcr);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", smcr & 0x3);
+}
+
+static
+ssize_t _store_slave_mode(struct device *dev,
+			  struct device_attribute *attr,
+			  const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
+	u8 mode;
+	int ret;
+
+	ret = kstrtou8(buf, 10, &mode);
+	if (ret)
+		return ret;
+
+	if (mode > 0x7)
+		return -EINVAL;
+
+	regmap_update_bits(stm32->regmap, TIM_SMCR, TIM_SMCR_SMS, mode);
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(slave_mode, S_IRUGO | S_IWUSR,
+		       _show_slave_mode,
+		       _store_slave_mode,
+		       0);
+
+static struct attribute *stm32_timer_attrs[] = {
+	&iio_dev_attr_master_mode.dev_attr.attr,
+	&iio_dev_attr_slave_mode.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group stm32_timer_attr_group = {
+	.attrs = stm32_timer_attrs,
+};
+
+static int stm32_timer_start(struct stm32_iio_timer_dev *stm32)
+{
+	unsigned long long prd, div;
+	int prescaler = 0;
+	u32 max_arr = 0xFFFF, cr1;
+
+	if (stm32->sampling_frequency == 0)
+		return 0;
+
+	/* Period and prescaler values depends of clock rate */
+	div = (unsigned long long)clk_get_rate(stm32->clk);
+
+	do_div(div, stm32->sampling_frequency);
+
+	prd = div;
+
+	while (div > max_arr) {
+		prescaler++;
+		div = prd;
+		do_div(div, (prescaler + 1));
+	}
+	prd = div;
+
+	if (prescaler > MAX_TIM_PSC) {
+		dev_err(stm32->dev, "prescaler exceeds the maximum value\n");
+		return -EINVAL;
+	}
+
+	/* Check that we own the timer */
+	regmap_read(stm32->regmap, TIM_CR1, &cr1);
+	if ((cr1 & TIM_CR1_CEN) && !stm32->own_timer)
+		return -EBUSY;
+
+	if (!stm32->own_timer) {
+		stm32->own_timer = true;
+		clk_enable(stm32->clk);
+	}
+
+	regmap_write(stm32->regmap, TIM_PSC, prescaler);
+	regmap_write(stm32->regmap, TIM_ARR, prd - 1);
+	regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
+
+	/* Force master mode to update mode */
+	regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, 0x20);
+
+	/* Make sure that registers are updated */
+	regmap_update_bits(stm32->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
+
+	/* Enable interrupt */
+	regmap_write(stm32->regmap, TIM_SR, 0);
+	regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, TIM_DIER_UIE);
+
+	/* Enable controller */
+	regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
+
+	return 0;
+}
+
+static int stm32_timer_stop(struct stm32_iio_timer_dev *stm32)
+{
+	if (!stm32->own_timer)
+		return 0;
+
+	/* Stop timer */
+	regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, 0);
+	regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, 0);
+	regmap_write(stm32->regmap, TIM_PSC, 0);
+	regmap_write(stm32->regmap, TIM_ARR, 0);
+
+	clk_disable(stm32->clk);
+
+	stm32->own_timer = false;
+	stm32->active_trigger = NULL;
+
+	return 0;
+}
+
+static int stm32_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+	struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
+
+	stm32->active_trigger = trig;
+
+	if (state)
+		return stm32_timer_start(stm32);
+	else
+		return stm32_timer_stop(stm32);
+}
+
+static irqreturn_t stm32_timer_irq_handler(int irq, void *private)
+{
+	struct stm32_iio_timer_dev *stm32 = private;
+	u32 sr;
+
+	regmap_read(stm32->regmap, TIM_SR, &sr);
+	regmap_write(stm32->regmap, TIM_SR, 0);
+
+	if ((sr & TIM_SR_UIF) && stm32->active_trigger)
+		iio_trigger_poll(stm32->active_trigger);
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_trigger_ops timer_trigger_ops = {
+	.owner = THIS_MODULE,
+	.set_trigger_state = stm32_set_trigger_state,
+};
+
+static int stm32_setup_iio_triggers(struct stm32_iio_timer_dev *stm32)
+{
+	int ret;
+	struct property *p;
+	const char *cur = NULL;
+
+	p = of_find_property(stm32->dev->of_node,
+			     "st,output-triggers-names", NULL);
+
+	while ((cur = of_prop_next_string(p, cur)) != NULL) {
+		struct iio_trigger *trig;
+
+		trig = devm_iio_trigger_alloc(stm32->dev, "%s", cur);
+		if  (!trig)
+			return -ENOMEM;
+
+		trig->dev.parent = stm32->dev->parent;
+		trig->ops = &timer_trigger_ops;
+		trig->dev.groups = stm32_trigger_attr_groups;
+		iio_trigger_set_drvdata(trig, stm32);
+
+		ret = devm_iio_trigger_register(stm32->dev, trig);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * is_stm32_iio_timer_trigger
+ * @trig: trigger to be checked
+ *
+ * return true if the trigger is a valid stm32 iio timer trigger
+ * either return false
+ */
+bool is_stm32_iio_timer_trigger(struct iio_trigger *trig)
+{
+	return (trig->ops == &timer_trigger_ops);
+}
+EXPORT_SYMBOL(is_stm32_iio_timer_trigger);
+
+static int stm32_validate_trigger(struct iio_dev *indio_dev,
+				  struct iio_trigger *trig)
+{
+	struct stm32_iio_timer_dev *dev = iio_priv(indio_dev);
+	int ret;
+
+	if (!is_stm32_iio_timer_trigger(trig))
+		return -EINVAL;
+
+	ret = of_property_match_string(dev->dev->of_node,
+				       "st,input-triggers-names",
+				       trig->name);
+
+	if (ret < 0)
+		return ret;
+
+	regmap_update_bits(dev->regmap, TIM_SMCR, TIM_SMCR_TS, ret << 4);
+
+	return 0;
+}
+
+static const struct iio_info stm32_trigger_info = {
+	.driver_module = THIS_MODULE,
+	.validate_trigger = stm32_validate_trigger,
+	.attrs = &stm32_timer_attr_group,
+};
+
+static struct stm32_iio_timer_dev *stm32_setup_iio_device(struct device *dev)
+{
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(struct stm32_iio_timer_dev));
+	if (!indio_dev)
+		return NULL;
+
+	indio_dev->name = dev_name(dev);
+	indio_dev->dev.parent = dev;
+	indio_dev->info = &stm32_trigger_info;
+	indio_dev->modes = INDIO_EVENT_TRIGGERED;
+	indio_dev->num_channels = 0;
+	indio_dev->dev.of_node = dev->of_node;
+
+	ret = iio_triggered_event_setup(indio_dev,
+					NULL,
+					stm32_timer_irq_handler);
+	if (ret)
+		return NULL;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret) {
+		iio_triggered_event_cleanup(indio_dev);
+		return NULL;
+	}
+
+	return iio_priv(indio_dev);
+}
+
+static int stm32_iio_timer_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct stm32_iio_timer_dev *stm32;
+	struct stm32_gptimer_dev *mfd = dev_get_drvdata(pdev->dev.parent);
+	int ret;
+
+	stm32 = stm32_setup_iio_device(dev);
+	if (!stm32)
+		return -ENOMEM;
+
+	stm32->dev = dev;
+	stm32->regmap = mfd->regmap;
+	stm32->clk = mfd->clk;
+
+	stm32->irq = platform_get_irq(pdev, 0);
+	if (stm32->irq < 0)
+		return -EINVAL;
+
+	ret = devm_request_irq(stm32->dev, stm32->irq,
+			       stm32_timer_irq_handler, IRQF_SHARED,
+			       "iiotimer_event", stm32);
+	if (ret)
+		return ret;
+
+	ret = stm32_setup_iio_triggers(stm32);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, stm32);
+
+	return 0;
+}
+
+static int stm32_iio_timer_remove(struct platform_device *pdev)
+{
+	struct stm32_iio_timer_dev *stm32 = platform_get_drvdata(pdev);
+
+	iio_triggered_event_cleanup((struct iio_dev *)stm32);
+
+	return 0;
+}
+
+static const struct of_device_id stm32_trig_of_match[] = {
+	{
+		.compatible = "st,stm32-iio-timer",
+	},
+};
+MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
+
+static struct platform_driver stm32_iio_timer_driver = {
+	.probe = stm32_iio_timer_probe,
+	.remove = stm32_iio_timer_remove,
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = stm32_trig_of_match,
+	},
+};
+module_platform_driver(stm32_iio_timer_driver);
+
+MODULE_ALIAS("platform:" DRIVER_NAME);
+MODULE_DESCRIPTION("STMicroelectronics STM32 iio timer driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
index 809b2e7..f2af4fe 100644
--- a/drivers/iio/trigger/Kconfig
+++ b/drivers/iio/trigger/Kconfig
@@ -46,5 +46,4 @@ config IIO_SYSFS_TRIGGER
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called iio-trig-sysfs.
-
 endmenu
diff --git a/include/dt-bindings/iio/timer/st,stm32-iio-timer.h b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
new file mode 100644
index 0000000..d39bf16
--- /dev/null
+++ b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
@@ -0,0 +1,23 @@
+/*
+ * st,stm32-iio-timer.h
+ *
+ * Copyright (C) STMicroelectronics 2016
+ * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#ifndef _DT_BINDINGS_IIO_TIMER_H_
+#define _DT_BINDINGS_IIO_TIMER_H_
+
+#define TIM1_TRGO	"tim1_trgo"
+#define TIM2_TRGO	"tim2_trgo"
+#define TIM3_TRGO	"tim3_trgo"
+#define TIM4_TRGO	"tim4_trgo"
+#define TIM5_TRGO	"tim5_trgo"
+#define TIM6_TRGO	"tim6_trgo"
+#define TIM7_TRGO	"tim7_trgo"
+#define TIM8_TRGO	"tim8_trgo"
+#define TIM9_TRGO	"tim9_trgo"
+#define TIM12_TRGO	"tim12_trgo"
+
+#endif
diff --git a/include/linux/iio/timer/stm32-iio-timers.h b/include/linux/iio/timer/stm32-iio-timers.h
new file mode 100644
index 0000000..5d1b86c
--- /dev/null
+++ b/include/linux/iio/timer/stm32-iio-timers.h
@@ -0,0 +1,16 @@
+/*
+ * stm32-iio-timers.h
+ *
+ * Copyright (C) STMicroelectronics 2016
+ * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#ifndef _STM32_IIO_TIMERS_H_
+#define _STM32_IIO_TIMERS_H_
+
+#include <dt-bindings/iio/timer/st,stm32-iio-timer.h>
+
+bool is_stm32_iio_timer_trigger(struct iio_trigger *trig);
+
+#endif
-- 
1.9.1

[PATCH v2 7/7] ARM: dts: stm32: add stm32 general purpose timer driver in DT

[Benjamin Gaignard]

Add general purpose timers and it sub-nodes into DT for stm32f4.
Define and enable pwm1 and pwm3 for stm32f469 discovery board

version 2:
- use parameters to describe hardware capabilities
- do not use references for pwm and iio timer subnodes

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 arch/arm/boot/dts/stm32f429.dtsi      | 305 +++++++++++++++++++++++++++++++++-
 arch/arm/boot/dts/stm32f469-disco.dts |  28 ++++
 2 files changed, 332 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index bca491d..8f217b1 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -48,7 +48,7 @@
 #include "skeleton.dtsi"
 #include "armv7-m.dtsi"
 #include <dt-bindings/pinctrl/stm32f429-pinfunc.h>
-
+#include <dt-bindings/iio/timer/st,stm32-iio-timer.h>
 / {
 	clocks {
 		clk_hse: clk-hse {
@@ -355,6 +355,21 @@
 					slew-rate = <2>;
 				};
 			};
+
+			pwm1_pins: pwm@1 {
+				pins {
+					pinmux = <STM32F429_PA8_FUNC_TIM1_CH1>,
+						 <STM32F429_PB13_FUNC_TIM1_CH1N>,
+						 <STM32F429_PB12_FUNC_TIM1_BKIN>;
+				};
+			};
+
+			pwm3_pins: pwm@3 {
+				pins {
+					pinmux = <STM32F429_PB4_FUNC_TIM3_CH1>,
+						 <STM32F429_PB5_FUNC_TIM3_CH2>;
+				};
+			};
 		};
 
 		rcc: rcc@40023810 {
@@ -426,6 +441,294 @@
 			interrupts = <80>;
 			clocks = <&rcc 0 38>;
 		};
+
+		gptimer1: gptimer1@40010000 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40010000 0x400>;
+			clocks = <&rcc 0 160>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm1@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <4>;
+				st,breakinput;
+				st,complementary;
+				status = "disabled";
+			};
+
+			iiotimer1@0 {
+				compatible = "st,stm32-iio-timer";
+				interrupts = <27>;
+				st,input-triggers-names = TIM5_TRGO,
+							  TIM2_TRGO,
+							  TIM4_TRGO,
+							  TIM3_TRGO;
+				st,output-triggers-names = TIM1_TRGO;
+				status = "disabled";
+			};
+		};
+
+		gptimer2: gptimer2@40000000 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40000000 0x400>;
+			clocks = <&rcc 0 128>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm2@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <4>;
+				st,32bits-counter;
+				status = "disabled";
+			};
+
+			iiotimer2@0 {
+				compatible = "st,stm32-iio-timer";
+				interrupts = <28>;
+				st,input-triggers-names = TIM1_TRGO,
+							  TIM8_TRGO,
+							  TIM3_TRGO,
+							  TIM4_TRGO;
+				st,output-triggers-names = TIM2_TRGO;
+				status = "disabled";
+			};
+		};
+
+		gptimer3: gptimer3@40000400 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40000400 0x400>;
+			clocks = <&rcc 0 129>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm3@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <4>;
+				status = "disabled";
+			};
+
+			iiotimer3@0 {
+				compatible = "st,stm32-iio-timer";
+				interrupts = <29>;
+				st,input-triggers-names = TIM1_TRGO,
+							  TIM8_TRGO,
+							  TIM5_TRGO,
+							  TIM4_TRGO;
+				st,output-triggers-names = TIM3_TRGO;
+				status = "disabled";
+			};
+		};
+
+		gptimer4: gptimer4@40000800 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40000800 0x400>;
+			clocks = <&rcc 0 130>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm4@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <4>;
+				status = "disabled";
+			};
+
+			iiotimer4@0 {
+				compatible = "st,stm32-iio-timer";
+				interrupts = <30>;
+				st,input-triggers-names = TIM1_TRGO,
+							  TIM2_TRGO,
+							  TIM3_TRGO,
+							  TIM8_TRGO;
+				st,output-triggers-names = TIM4_TRGO;
+				status = "disabled";
+			};
+		};
+
+		gptimer5: gptimer5@40000C00 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40000C00 0x400>;
+			clocks = <&rcc 0 131>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm5@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <4>;
+				st,32bits-counter;
+				status = "disabled";
+			};
+
+			iiotimer5@0 {
+				compatible = "st,stm32-iio-timer";
+				interrupts = <50>;
+				st,input-triggers-names = TIM2_TRGO,
+							  TIM3_TRGO,
+							  TIM4_TRGO,
+							  TIM8_TRGO;
+				st,output-triggers-names = TIM5_TRGO;
+				status = "disabled";
+			};
+		};
+
+		gptimer6: gptimer6@40001000 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40001000 0x400>;
+			clocks = <&rcc 0 132>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			iiotimer6@0 {
+				compatible = "st,stm32-iio-timer";
+				interrupts = <54>;
+				st,output-triggers-names = TIM6_TRGO;
+				status = "disabled";
+			};
+		};
+
+		gptimer7: gptimer7@40001400 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40001400 0x400>;
+			clocks = <&rcc 0 133>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			iiotimer7@0 {
+				compatible = "st,stm32-iio-timer";
+				interrupts = <55>;
+				st,output-triggers-names = TIM7_TRGO;
+				status = "disabled";
+			};
+		};
+
+		gptimer8: gptimer8@40010400 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40010400 0x400>;
+			clocks = <&rcc 0 161>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm8@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <4>;
+				st,complementary;
+				st,breakinput;
+				status = "disabled";
+			};
+
+			iiotimer8@0 {
+				compatible = "st,stm32-iio-timer";
+				interrupts = <46>;
+				st,input-triggers-names = TIM1_TRGO,
+							  TIM2_TRGO,
+							  TIM4_TRGO,
+							  TIM5_TRGO;
+				st,output-triggers-names = TIM8_TRGO;
+				status = "disabled";
+			};
+		};
+
+		gptimer9: gptimer9@40014000 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40014000 0x400>;
+			clocks = <&rcc 0 176>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm9@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <2>;
+				status = "disabled";
+			};
+
+			iiotimer9@0 {
+				compatible = "st,stm32-iio-timer";
+				interrupts = <24>;
+				st,input-triggers-names = TIM2_TRGO,
+							  TIM3_TRGO;
+				st,output-triggers-names = TIM9_TRGO;
+				status = "disabled";
+			};
+		};
+
+		gptimer10: gptimer10@40014400 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40014400 0x400>;
+			clocks = <&rcc 0 177>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm10@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <1>;
+				status = "disabled";
+			};
+		};
+
+		gptimer11: gptimer11@40014800 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40014800 0x400>;
+			clocks = <&rcc 0 178>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm11@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <1>;
+				status = "disabled";
+			};
+		};
+
+		gptimer12: gptimer12@40001800 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40001800 0x400>;
+			clocks = <&rcc 0 134>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm12@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <2>;
+				status = "disabled";
+			};
+
+			iiotimer12@0 {
+				compatible = "st,stm32-iio-timer";
+				interrupts = <43>;
+				st,input-triggers-names = TIM4_TRGO,
+							  TIM5_TRGO;
+				st,output-triggers-names = TIM12_TRGO;
+				status = "disabled";
+			};
+		};
+
+		gptimer13: gptimer13@40001C00 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40001C00 0x400>;
+			clocks = <&rcc 0 135>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm13@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <1>;
+				status = "disabled";
+			};
+		};
+
+		gptimer14: gptimer14@40002000 {
+			compatible = "st,stm32-gptimer";
+			reg = <0x40002000 0x400>;
+			clocks = <&rcc 0 136>;
+			clock-names = "clk_int";
+			status = "disabled";
+
+			pwm14@0 {
+				compatible = "st,stm32-pwm";
+				st,pwm-num-chan = <1>;
+				status = "disabled";
+			};
+		};
 	};
 };
 
diff --git a/arch/arm/boot/dts/stm32f469-disco.dts b/arch/arm/boot/dts/stm32f469-disco.dts
index 8a163d7..0c93846 100644
--- a/arch/arm/boot/dts/stm32f469-disco.dts
+++ b/arch/arm/boot/dts/stm32f469-disco.dts
@@ -81,3 +81,31 @@
 &usart3 {
 	status = "okay";
 };
+
+&gptimer1 {
+	status = "okay";
+
+	pwm1@0 {
+		pinctrl-0	= <&pwm1_pins>;
+		pinctrl-names	= "default";
+		status = "okay";
+	};
+
+	iiotimer1@0 {
+		status = "okay";
+	};
+};
+
+&gptimer3 {
+	status = "okay";
+
+	pwm3@0 {
+		pinctrl-0	= <&pwm3_pins>;
+		pinctrl-names	= "default";
+		status = "okay";
+	};
+
+	iiotimer3@0 {
+		status = "okay";
+	};
+};
-- 
1.9.1

Re: [PATCH v2 1/7] MFD: add bindings for stm32 general purpose timer driver

[Jonathan Cameron]

On 24/11/16 15:14, Benjamin Gaignard wrote:
> Add bindings information for stm32 general purpose timer
> 
> version 2:
> - rename stm32-mfd-timer to stm32-gptimer
> - only keep one compatible string
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  .../bindings/mfd/stm32-general-purpose-timer.txt   | 43 ++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
> new file mode 100644
> index 0000000..2f10e67
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
> @@ -0,0 +1,43 @@
> +STM32 general purpose timer driver
> +
> +Required parameters:
> +- compatible: must be "st,stm32-gptimer"
> +
> +- reg:			Physical base address and length of the controller's
> +			registers.
> +- clock-names: 		Set to "clk_int".
> +- clocks: 		Phandle to the clock used by the timer module.
> +			For Clk properties, please refer to ../clock/clock-bindings.txt
> +
> +Optional parameters:
> +- resets:		Phandle to the parent reset controller.
> +			See ..reset/st,stm32-rcc.txt
> +
> +Optional subnodes:
> +- pwm:			See ../pwm/pwm-stm32.txt
> +- iiotimer:		See ../iio/timer/stm32-iio-timer.txt
Naming issue here.  Can't mention IIO as that's a linux subsystem and all
bindings must be independent of OS. 

Perhaps adc-trigger-timer?
> +
> +Example:
> +	gptimer1: gptimer1@40010000 {
> +		compatible = "st,stm32-gptimer";
> +		reg = <0x40010000 0x400>;
> +		clocks = <&rcc 0 160>;
> +		clock-names = "clk_int";
> +
> +		pwm1@0 {
> +			compatible = "st,stm32-pwm";
> +			st,pwm-num-chan = <4>;
> +			st,breakinput;
> +			st,complementary;
> +		};
> +
> +		iiotimer1@0 {
> +			compatible = "st,stm32-iio-timer";
Again, avoid the use of iio in here (same issue you had with mfd in the previous
version).
> +			interrupts = <27>;
> +			st,input-triggers-names = TIM5_TRGO,
Docs for these should be introduced before they are used in an example.
Same for the PWM ones above.  Expand the detail fo the example as you add
the other elements.
> +						  TIM2_TRGO,
> +						  TIM4_TRGO,
> +						  TIM3_TRGO;
> +			st,output-triggers-names = TIM1_TRGO;
> +		};
> +	};
> 

Re: [PATCH v2 3/7] PWM: add pwm-stm32 DT bindings

[Jonathan Cameron]

On 24/11/16 15:14, Benjamin Gaignard wrote:
> Define bindings for pwm-stm32
> 
> version 2:
> - use parameters instead of compatible of handle the hardware configuration
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  .../devicetree/bindings/pwm/pwm-stm32.txt          | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> new file mode 100644
> index 0000000..36263f0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> @@ -0,0 +1,37 @@
> +STMicroelectronics PWM driver bindings for STM32
> +
> +Must be a sub-node of STM32 general purpose timer driver
> +
> +Required parameters:
> +- compatible:		Must be "st,stm32-pwm"
> +- pinctrl-names: 	Set to "default".
> +- pinctrl-0: 		List of phandles pointing to pin configuration nodes
> +			for PWM module.
> +			For Pinctrl properties, please refer to [1].
> +
> +Optional parameters:
> +- st,breakinput:	Set if the hardware have break input capabilities
> +- st,breakinput-polarity: Set break input polarity. Default is 0
> +			 The value define the active polarity:
> +			  - 0 (active LOW)
> +			  - 1 (active HIGH)
> +- st,pwm-num-chan:	Number of available PWM channels.  Default is 0.
> +- st,32bits-counter:	Set if the hardware have a 32 bits counter
> +- st,complementary:	Set if the hardware have complementary output channels
> +
> +[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +
> +Example:
> +	gptimer1: gptimer1@40010000 {
Given the example includes chunks for the other binding, make sure to have
explicit cross references in the document.

> +		compatible = "st,stm32-gptimer";
> +		reg = <0x40010000 0x400>;
> +		clocks = <&rcc 0 160>;
> +		clock-names = "clk_int";
> +
> +		pwm1@0 {
> +			compatible = "st,stm32-pwm";
> +			st,pwm-num-chan = <4>;
> +			st,breakinput;
> +			st,complementary;
> +		};
> +	};
> 

Re: [PATCH v2 5/7] IIO: add bindings for stm32 IIO timer driver

[Jonathan Cameron]

On 24/11/16 15:14, Benjamin Gaignard wrote:
> Define bindings for stm32 IIO timer
> 
> version 2:
> - only keep one compatible
> - add DT parameters to set lists of the triggers:
>   one list describe the triggers created by the device
>   another one give the triggers accepted by the device
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  .../bindings/iio/timer/stm32-iio-timer.txt         | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
> new file mode 100644
> index 0000000..840b417
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
> @@ -0,0 +1,41 @@
> +timer IIO trigger bindings for STM32
> +
> +Must be a sub-node of STM32 general purpose timer driver
Add a cross reference...
> +
> +Required parameters:
> +- compatible:		must be "st,stm32-iio-timer"
st,stm32-adc-timer or something like that.
> +- interrupts:		Interrupt for this device
> +			See ../interrupt-controller/st,stm32-exti.txt
> +
> +Optional parameters:
> +- st,input-triggers-names:	List of the possible input triggers for
> +				the device
> +- st,output-triggers-names:	List of the possible output triggers for
> +				the device
What are input / output triggers?
> +
> +Possible triggers are defined in include/dt-bindings/iio/timer/st,stm32-iio-timer.h
> +
> +Example:
> +	gptimer1: gptimer1@40010000 {
> +		compatible = "st,stm32-gptimer";
> +		reg = <0x40010000 0x400>;
> +		clocks = <&rcc 0 160>;
> +		clock-names = "clk_int";
> +
> +		pwm1@0 {
> +			compatible = "st,stm32-pwm";
> +			st,pwm-num-chan = <4>;
> +			st,breakinput;
> +			st,complementary;
> +		};
> +
> +		iiotimer1@0 {
> +			compatible = "st,stm32-iio-timer";
> +			interrupts = <27>;
> +			st,input-triggers-names = TIM5_TRGO,
> +						  TIM2_TRGO,
> +						  TIM4_TRGO,
> +						  TIM3_TRGO;
> +			st,output-triggers-names = TIM1_TRGO;
> +		};
> +	};
> 

Re: [PATCH v2 1/7] MFD: add bindings for stm32 general purpose timer driver

[Jonathan Cameron]

On 27/11/16 14:10, Jonathan Cameron wrote:
> On 24/11/16 15:14, Benjamin Gaignard wrote:
>> Add bindings information for stm32 general purpose timer
>>
>> version 2:
>> - rename stm32-mfd-timer to stm32-gptimer
>> - only keep one compatible string
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>>  .../bindings/mfd/stm32-general-purpose-timer.txt   | 43 ++++++++++++++++++++++
>>  1 file changed, 43 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>> new file mode 100644
>> index 0000000..2f10e67
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>> @@ -0,0 +1,43 @@
>> +STM32 general purpose timer driver
>> +
>> +Required parameters:
>> +- compatible: must be "st,stm32-gptimer"
>> +
>> +- reg:			Physical base address and length of the controller's
>> +			registers.
>> +- clock-names: 		Set to "clk_int".
>> +- clocks: 		Phandle to the clock used by the timer module.
>> +			For Clk properties, please refer to ../clock/clock-bindings.txt
>> +
>> +Optional parameters:
>> +- resets:		Phandle to the parent reset controller.
>> +			See ..reset/st,stm32-rcc.txt
>> +
>> +Optional subnodes:
>> +- pwm:			See ../pwm/pwm-stm32.txt
>> +- iiotimer:		See ../iio/timer/stm32-iio-timer.txt
> Naming issue here.  Can't mention IIO as that's a linux subsystem and all
> bindings must be independent of OS. 
> 
> Perhaps adc-trigger-timer?
>> +
>> +Example:
>> +	gptimer1: gptimer1@40010000 {
>> +		compatible = "st,stm32-gptimer";
>> +		reg = <0x40010000 0x400>;
>> +		clocks = <&rcc 0 160>;
>> +		clock-names = "clk_int";
>> +
>> +		pwm1@0 {
>> +			compatible = "st,stm32-pwm";
>> +			st,pwm-num-chan = <4>;
>> +			st,breakinput;
>> +			st,complementary;
>> +		};
>> +
>> +		iiotimer1@0 {
>> +			compatible = "st,stm32-iio-timer";
> Again, avoid the use of iio in here (same issue you had with mfd in the previous
> version).
>> +			interrupts = <27>;
>> +			st,input-triggers-names = TIM5_TRGO,
> Docs for these should be introduced before they are used in an example.
> Same for the PWM ones above.  Expand the detail fo the example as you add
> the other elements.

I've just dived into the datasheet for these timers.
http://www.st.com/content/ccc/resource/technical/document/reference_manual/3d/6d/5a/66/b4/99/40/d4/DM00031020.pdf/files/DM00031020.pdf/jcr:content/translations/en.DM00031020.pdf


I think you need a binding that describes the capabilities of each of the timers
explicitly.   Down to the level of whether there is a repetition counter or not.
Each should exists as a separate entity in device tree.

They then have an existence as timers separate to the description of what they
are used for.

Here the only way we are saying they exist is by their use which doesn't feel
right to me.

So I think you need to move back to what you had in the first place.  The key
thing is that ever timer needs describing fully.  They are different enough
that for example the datasheet doesn't even try to describe them in one section.
(it has 4 separate chapters covering different sets of these hardware blocks).
The naming isn't really based on index, we are talking different hardware
that the datasheet authors have decided not to give different names to!

If they'd called them 
advanced timers
generic timers
basic timers 
really basic timers meant for driving the DAC (6 and 7)

We'd all have been quite happy with different compatible strings giving away
what they can do. 

What you have here is far too specific to what you are trying to do with them
right now.

These things are separately capable of timing capture (which is I guess where
the IIO device later comes in).

So my expectation is that we end up potentially instantiating:

1) An MFD to handle the shared elements of the timers.
2) Up to 12ish timers each with separate existence as a device in the driver model
and in device tree.
(nasty corner cases here are using timers as perscalers for other timers - I'd be
tempted to leave that for now)
Note that each of these devices has a different register set I think? Any shared
bits are handled via the mfd above (if we even need that MFD which I'm starting
to doubt looking at the datasheet).

3) Up to N pwms again with there own existence in the device model.  These don't
do much other than wrap the timer and stick it in output mode.
4) Up to N iio triggers - this is basically using the timer as a periodic interrupt
(though without the interrupt having visibility to the kernel) which fires off
sampling on associated ADCs.
5) Up to N iio capture devices for all channels that support timing capture.
Note there is also hardware encoder capture support in these which should be
correctly handled as well.  This comes back to an ancient discussion on the
TI ecap units which have similar capabilities (driver never went anywhere but
I think that was because the author left TI).

Certainly for the IIO devices these should no be bound up into one instance
as you have done here.

Anyhow, I fear that right now this discussion is missing the key ingredient
that the hardware is horrendously variable in it's capabilities and really
is 4-5 different types of hardware that just happen to share a few bits of
their offsets in their register maps.

So after all that I'm almost more confused than I was at the start!

Jonathan


>> +						  TIM2_TRGO,
>> +						  TIM4_TRGO,
>> +						  TIM3_TRGO;
>> +			st,output-triggers-names = TIM1_TRGO;
>> +		};
>> +	};
>>
> 
> --
> 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
> 

Re: [PATCH v2 6/7] IIO: add STM32 IIO timer driver

[Jonathan Cameron]

I delved into the datasheet after trying to figure this out, so I think
I now sort of understand your intent, but please do answer the questions
inline.

On 24/11/16 15:14, Benjamin Gaignard wrote:
> Timers IPs can be used to generate triggers for other IPs like
> DAC, ADC or other timers.
> Each trigger may result of timer internals signals like counter enable,
> reset or edge, this configuration could be done through "master_mode"
> device attribute.
> 
> A timer device could be triggered by other timers, we use the trigger
> name and is_stm32_iio_timer_trigger() function to distinguish them
> and configure IP input switch.
The presence of an IIO device in here was a suprise.. What is it actually for?

I think this needs some examples of usage to make it clear what the aim is.

I was basically expecting to see a driver instantiating one iio trigger
per timer that can act as a trigger.  Those would each have sampling frequency
controls and basica enable / disable.

I'm seeing something much more complex here so additional explanation is
needed.
> 
> Timer may also decide on which event (edge, level) they could
> be activated by a trigger, this configuration is done by writing in
> "slave_mode" device attribute.
Really?  Sounds like magic numbers in sysfs which is never a good idea.
Please document those attributes / or break them up into elements that
don't require magic numbers.

> 
> Since triggers could also be used by DAC or ADC their names are defined
> in include/dt-bindings/iio/timer/st,stm32-iio-timer.h so those IPs will be able
> to configure themselves in valid_trigger function
> 
> Trigger have a "sampling_frequency" attribute which allow to configure
> timer sampling frequency without using pwm interface
> 
> version 2:
> - keep only one compatible
Hmm. I'm not sure I like this as such.  We are actually dealing with lots
of instances of a hardware block with only a small amount of shared
infrastrcuture (which is classic mfd teritory). So to my mind we
should have a separate device for each.

> - use st,input-triggers-names and st,output-triggers-names
>   to know which triggers are accepted and/or create by the device
I'm not following why we have this cascade setup?

These are triggers, not devices in the IIO context.  We need some detailed
description of why you have it setup like this. This would include the
ABI with examples of how you are using it.

Basically I don't currently understand what you are doing :(


Thanks,

Jonathan
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  drivers/iio/Kconfig                                |   2 +-
>  drivers/iio/Makefile                               |   1 +
>  drivers/iio/timer/Kconfig                          |  15 +
>  drivers/iio/timer/Makefile                         |   1 +
>  drivers/iio/timer/stm32-iio-timer.c                | 448 +++++++++++++++++++++
>  drivers/iio/trigger/Kconfig                        |   1 -
>  include/dt-bindings/iio/timer/st,stm32-iio-timer.h |  23 ++
>  include/linux/iio/timer/stm32-iio-timers.h         |  16 +
>  8 files changed, 505 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/iio/timer/Kconfig
>  create mode 100644 drivers/iio/timer/Makefile
>  create mode 100644 drivers/iio/timer/stm32-iio-timer.c
>  create mode 100644 include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>  create mode 100644 include/linux/iio/timer/stm32-iio-timers.h
> 
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 6743b18..2de2a80 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -90,5 +90,5 @@ source "drivers/iio/potentiometer/Kconfig"
>  source "drivers/iio/pressure/Kconfig"
>  source "drivers/iio/proximity/Kconfig"
>  source "drivers/iio/temperature/Kconfig"
> -
> +source "drivers/iio/timer/Kconfig"
>  endif # IIO
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 87e4c43..b797c08 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -32,4 +32,5 @@ obj-y += potentiometer/
>  obj-y += pressure/
>  obj-y += proximity/
>  obj-y += temperature/
> +obj-y += timer/
>  obj-y += trigger/
> diff --git a/drivers/iio/timer/Kconfig b/drivers/iio/timer/Kconfig
> new file mode 100644
> index 0000000..7a73bc6
> --- /dev/null
> +++ b/drivers/iio/timer/Kconfig
> @@ -0,0 +1,15 @@
> +#
> +# Timers drivers
> +
> +menu "Timers"
> +
> +config IIO_STM32_TIMER
> +	tristate "stm32 iio timer"
> +	depends on ARCH_STM32
> +	depends on OF
> +	select IIO_TRIGGERED_EVENT
> +	select MFD_STM32_GP_TIMER
> +	help
> +	  Select this option to enable stm32 timers hardware IPs
> +
> +endmenu
> diff --git a/drivers/iio/timer/Makefile b/drivers/iio/timer/Makefile
> new file mode 100644
> index 0000000..a360c9f
> --- /dev/null
> +++ b/drivers/iio/timer/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_IIO_STM32_TIMER) += stm32-iio-timer.o
> diff --git a/drivers/iio/timer/stm32-iio-timer.c b/drivers/iio/timer/stm32-iio-timer.c
> new file mode 100644
> index 0000000..35f2687
> --- /dev/null
> +++ b/drivers/iio/timer/stm32-iio-timer.c
> @@ -0,0 +1,448 @@
> +/*
> + * stm32-iio-timer.c
> + *
> + * Copyright (C) STMicroelectronics 2016
> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/timer/stm32-iio-timers.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_event.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/stm32-gptimer.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define DRIVER_NAME "stm32-iio-timer"
> +
> +struct stm32_iio_timer_dev {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct clk *clk;
> +	int irq;
> +	bool own_timer;
> +	unsigned int sampling_frequency;
> +	struct iio_trigger *active_trigger;
> +};
> +
> +static ssize_t _store_frequency(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t len)
> +{
> +	struct iio_trigger *trig = to_iio_trigger(dev);
> +	struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
> +	unsigned int freq;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &freq);
> +	if (ret)
> +		return ret;
> +
> +	stm32->sampling_frequency = freq;
> +
> +	return len;
> +}
> +
> +static ssize_t _read_frequency(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct iio_trigger *trig = to_iio_trigger(dev);
> +	struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
> +	unsigned long long freq = stm32->sampling_frequency;
> +	u32 psc, arr, cr1;
> +
> +	regmap_read(stm32->regmap, TIM_CR1, &cr1);
> +	regmap_read(stm32->regmap, TIM_PSC, &psc);
> +	regmap_read(stm32->regmap, TIM_ARR, &arr);
> +
> +	if (psc && arr && (cr1 & TIM_CR1_CEN)) {
> +		freq = (unsigned long long)clk_get_rate(stm32->clk);
> +		do_div(freq, psc);
> +		do_div(freq, arr);
> +	}
> +
> +	return sprintf(buf, "%d\n", (unsigned int)freq);
> +}
> +
> +static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
> +			      _read_frequency,
> +			      _store_frequency);
> +
> +static struct attribute *stm32_trigger_attrs[] = {
> +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group stm32_trigger_attr_group = {
> +	.attrs = stm32_trigger_attrs,
> +};
> +
> +static const struct attribute_group *stm32_trigger_attr_groups[] = {
> +	&stm32_trigger_attr_group,
> +	NULL,
> +};
> +
> +static
> +ssize_t _show_master_mode(struct device *dev,
> +			  struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
> +	u32 cr2;
> +
> +	regmap_read(stm32->regmap, TIM_CR2, &cr2);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", (cr2 >> 4) & 0x7);
> +}
> +
> +static
> +ssize_t _store_master_mode(struct device *dev,
> +			   struct device_attribute *attr,
> +			   const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
> +	u8 mode;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &mode);
> +	if (ret)
> +		return ret;
> +
> +	if (mode > 0x7)
> +		return -EINVAL;
> +
> +	regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, mode << 4);
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(master_mode, S_IRUGO | S_IWUSR,
> +		       _show_master_mode,
> +		       _store_master_mode,
> +		       0);
> +
> +static
> +ssize_t _show_slave_mode(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
> +	u32 smcr;
> +
> +	regmap_read(stm32->regmap, TIM_SMCR, &smcr);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", smcr & 0x3);
> +}
> +
> +static
> +ssize_t _store_slave_mode(struct device *dev,
> +			  struct device_attribute *attr,
> +			  const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
> +	u8 mode;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &mode);
> +	if (ret)
> +		return ret;
> +
> +	if (mode > 0x7)
> +		return -EINVAL;
How is something called slave mode going to be fed a number between 0 and 7?
Rule of thumb is no magic numbers in sysfs and right now this is looking
rather cryptic to say the least!
> +
> +	regmap_update_bits(stm32->regmap, TIM_SMCR, TIM_SMCR_SMS, mode);
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(slave_mode, S_IRUGO | S_IWUSR,
There is an iritating move (in terms of noise it's generating) to use values
directly instead fo these defines.  Still if you don't fix it here I'll only
get a patch 'fixing' it soon after...

> +		       _show_slave_mode,
> +		       _store_slave_mode,
> +		       0);
> +
> +static struct attribute *stm32_timer_attrs[] = {
> +	&iio_dev_attr_master_mode.dev_attr.attr,
> +	&iio_dev_attr_slave_mode.dev_attr.attr,
New ABI so must be documented under Documentation/ABI/testing/sysfs-bus-iio-*

> +	NULL,
> +};
> +
> +static const struct attribute_group stm32_timer_attr_group = {
> +	.attrs = stm32_timer_attrs,
> +};
> +
> +static int stm32_timer_start(struct stm32_iio_timer_dev *stm32)
> +{
> +	unsigned long long prd, div;
> +	int prescaler = 0;
> +	u32 max_arr = 0xFFFF, cr1;
> +
> +	if (stm32->sampling_frequency == 0)
> +		return 0;
> +
> +	/* Period and prescaler values depends of clock rate */
> +	div = (unsigned long long)clk_get_rate(stm32->clk);
> +
> +	do_div(div, stm32->sampling_frequency);
> +
> +	prd = div;
> +
> +	while (div > max_arr) {
> +		prescaler++;
> +		div = prd;
> +		do_div(div, (prescaler + 1));
> +	}
> +	prd = div;
> +
> +	if (prescaler > MAX_TIM_PSC) {
> +		dev_err(stm32->dev, "prescaler exceeds the maximum value\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Check that we own the timer */
> +	regmap_read(stm32->regmap, TIM_CR1, &cr1);
> +	if ((cr1 & TIM_CR1_CEN) && !stm32->own_timer)
> +		return -EBUSY;
> +
> +	if (!stm32->own_timer) {
> +		stm32->own_timer = true;
> +		clk_enable(stm32->clk);
> +	}
> +
> +	regmap_write(stm32->regmap, TIM_PSC, prescaler);
> +	regmap_write(stm32->regmap, TIM_ARR, prd - 1);
> +	regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
> +
> +	/* Force master mode to update mode */
> +	regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, 0x20);
> +
> +	/* Make sure that registers are updated */
> +	regmap_update_bits(stm32->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
> +
> +	/* Enable interrupt */
> +	regmap_write(stm32->regmap, TIM_SR, 0);
> +	regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, TIM_DIER_UIE);
> +
> +	/* Enable controller */
> +	regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
> +
> +	return 0;
> +}
> +
> +static int stm32_timer_stop(struct stm32_iio_timer_dev *stm32)
> +{
> +	if (!stm32->own_timer)
> +		return 0;
> +
> +	/* Stop timer */
> +	regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, 0);
> +	regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, 0);
> +	regmap_write(stm32->regmap, TIM_PSC, 0);
> +	regmap_write(stm32->regmap, TIM_ARR, 0);
> +
> +	clk_disable(stm32->clk);
> +
> +	stm32->own_timer = false;
> +	stm32->active_trigger = NULL;
> +
> +	return 0;
> +}
> +
> +static int stm32_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
> +
> +	stm32->active_trigger = trig;
> +
> +	if (state)
> +		return stm32_timer_start(stm32);
> +	else
> +		return stm32_timer_stop(stm32);
> +}
> +
> +static irqreturn_t stm32_timer_irq_handler(int irq, void *private)
> +{
> +	struct stm32_iio_timer_dev *stm32 = private;
> +	u32 sr;
> +
> +	regmap_read(stm32->regmap, TIM_SR, &sr);
> +	regmap_write(stm32->regmap, TIM_SR, 0);
> +
> +	if ((sr & TIM_SR_UIF) && stm32->active_trigger)
> +		iio_trigger_poll(stm32->active_trigger);
This is acting like a trigger cascading off another trigger?

Normally this interrupt handler would be directly associated with the
trigger hardware - in this case the timer.
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_trigger_ops timer_trigger_ops = {
> +	.owner = THIS_MODULE,
> +	.set_trigger_state = stm32_set_trigger_state,
> +};
> +
> +static int stm32_setup_iio_triggers(struct stm32_iio_timer_dev *stm32)
> +{
> +	int ret;
> +	struct property *p;
> +	const char *cur = NULL;
> +
> +	p = of_find_property(stm32->dev->of_node,
> +			     "st,output-triggers-names", NULL);
> +
> +	while ((cur = of_prop_next_string(p, cur)) != NULL) {
> +		struct iio_trigger *trig;
> +
> +		trig = devm_iio_trigger_alloc(stm32->dev, "%s", cur);
> +		if  (!trig)
> +			return -ENOMEM;
> +
> +		trig->dev.parent = stm32->dev->parent;
> +		trig->ops = &timer_trigger_ops;
> +		trig->dev.groups = stm32_trigger_attr_groups;
> +		iio_trigger_set_drvdata(trig, stm32);
> +
> +		ret = devm_iio_trigger_register(stm32->dev, trig);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * is_stm32_iio_timer_trigger
> + * @trig: trigger to be checked
> + *
> + * return true if the trigger is a valid stm32 iio timer trigger
> + * either return false
> + */
> +bool is_stm32_iio_timer_trigger(struct iio_trigger *trig)
> +{
> +	return (trig->ops == &timer_trigger_ops);
> +}
> +EXPORT_SYMBOL(is_stm32_iio_timer_trigger);
> +
> +static int stm32_validate_trigger(struct iio_dev *indio_dev,
> +				  struct iio_trigger *trig)
> +{
> +	struct stm32_iio_timer_dev *dev = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!is_stm32_iio_timer_trigger(trig))
> +		return -EINVAL;
> +
> +	ret = of_property_match_string(dev->dev->of_node,
> +				       "st,input-triggers-names",
> +				       trig->name);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	regmap_update_bits(dev->regmap, TIM_SMCR, TIM_SMCR_TS, ret << 4);
> +
> +	return 0;
> +}
> +
> +static const struct iio_info stm32_trigger_info = {
> +	.driver_module = THIS_MODULE,
> +	.validate_trigger = stm32_validate_trigger,
> +	.attrs = &stm32_timer_attr_group,
> +};
> +
> +static struct stm32_iio_timer_dev *stm32_setup_iio_device(struct device *dev)
> +{
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct stm32_iio_timer_dev));
> +	if (!indio_dev)
> +		return NULL;
This is 'unusual'.  Why does a trigger driver need an iio_dev at all?
> +
> +	indio_dev->name = dev_name(dev);
> +	indio_dev->dev.parent = dev;
> +	indio_dev->info = &stm32_trigger_info;
> +	indio_dev->modes = INDIO_EVENT_TRIGGERED;
> +	indio_dev->num_channels = 0;
> +	indio_dev->dev.of_node = dev->of_node;
> +
> +	ret = iio_triggered_event_setup(indio_dev,
> +					NULL,
> +					stm32_timer_irq_handler);
So the iio_dev exists to provide the ability to fire this interrupt from
another trigger?  Why do you want to do this?
> +	if (ret)
> +		return NULL;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret) {
> +		iio_triggered_event_cleanup(indio_dev);
> +		return NULL;
> +	}
> +
> +	return iio_priv(indio_dev);
> +}
> +
> +static int stm32_iio_timer_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct stm32_iio_timer_dev *stm32;
> +	struct stm32_gptimer_dev *mfd = dev_get_drvdata(pdev->dev.parent);
> +	int ret;
> +
> +	stm32 = stm32_setup_iio_device(dev);
> +	if (!stm32)
> +		return -ENOMEM;
> +
> +	stm32->dev = dev;
> +	stm32->regmap = mfd->regmap;
> +	stm32->clk = mfd->clk;
> +
> +	stm32->irq = platform_get_irq(pdev, 0);
> +	if (stm32->irq < 0)
> +		return -EINVAL;
> +
> +	ret = devm_request_irq(stm32->dev, stm32->irq,
> +			       stm32_timer_irq_handler, IRQF_SHARED,
> +			       "iiotimer_event", stm32);
> +	if (ret)
> +		return ret;
> +
> +	ret = stm32_setup_iio_triggers(stm32);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, stm32);
> +
> +	return 0;
> +}
> +
> +static int stm32_iio_timer_remove(struct platform_device *pdev)
> +{
> +	struct stm32_iio_timer_dev *stm32 = platform_get_drvdata(pdev);
> +
> +	iio_triggered_event_cleanup((struct iio_dev *)stm32);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id stm32_trig_of_match[] = {
> +	{
> +		.compatible = "st,stm32-iio-timer",
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
> +
> +static struct platform_driver stm32_iio_timer_driver = {
> +	.probe = stm32_iio_timer_probe,
> +	.remove = stm32_iio_timer_remove,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = stm32_trig_of_match,
> +	},
> +};
> +module_platform_driver(stm32_iio_timer_driver);
> +
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> +MODULE_DESCRIPTION("STMicroelectronics STM32 iio timer driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
> index 809b2e7..f2af4fe 100644
> --- a/drivers/iio/trigger/Kconfig
> +++ b/drivers/iio/trigger/Kconfig
> @@ -46,5 +46,4 @@ config IIO_SYSFS_TRIGGER
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called iio-trig-sysfs.
> -
Clear this out...
>  endmenu
> diff --git a/include/dt-bindings/iio/timer/st,stm32-iio-timer.h b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
> new file mode 100644
> index 0000000..d39bf16
> --- /dev/null
> +++ b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
> @@ -0,0 +1,23 @@
> +/*
> + * st,stm32-iio-timer.h
> + *
> + * Copyright (C) STMicroelectronics 2016
> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#ifndef _DT_BINDINGS_IIO_TIMER_H_
> +#define _DT_BINDINGS_IIO_TIMER_H_
> +
> +#define TIM1_TRGO	"tim1_trgo"
> +#define TIM2_TRGO	"tim2_trgo"
> +#define TIM3_TRGO	"tim3_trgo"
> +#define TIM4_TRGO	"tim4_trgo"
> +#define TIM5_TRGO	"tim5_trgo"
> +#define TIM6_TRGO	"tim6_trgo"
> +#define TIM7_TRGO	"tim7_trgo"
> +#define TIM8_TRGO	"tim8_trgo"
> +#define TIM9_TRGO	"tim9_trgo"
> +#define TIM12_TRGO	"tim12_trgo"
> +
> +#endif
> diff --git a/include/linux/iio/timer/stm32-iio-timers.h b/include/linux/iio/timer/stm32-iio-timers.h
> new file mode 100644
> index 0000000..5d1b86c
> --- /dev/null
> +++ b/include/linux/iio/timer/stm32-iio-timers.h
> @@ -0,0 +1,16 @@
> +/*
> + * stm32-iio-timers.h
> + *
> + * Copyright (C) STMicroelectronics 2016
> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#ifndef _STM32_IIO_TIMERS_H_
> +#define _STM32_IIO_TIMERS_H_
> +
> +#include <dt-bindings/iio/timer/st,stm32-iio-timer.h>
> +
> +bool is_stm32_iio_timer_trigger(struct iio_trigger *trig);
> +
> +#endif
> 

Re: [PATCH v2 5/7] IIO: add bindings for stm32 IIO timer driver

[Benjamin Gaignard]

2016-11-27 15:25 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
> On 24/11/16 15:14, Benjamin Gaignard wrote:
>> Define bindings for stm32 IIO timer
>>
>> version 2:
>> - only keep one compatible
>> - add DT parameters to set lists of the triggers:
>>   one list describe the triggers created by the device
>>   another one give the triggers accepted by the device
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>>  .../bindings/iio/timer/stm32-iio-timer.txt         | 41 ++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>> new file mode 100644
>> index 0000000..840b417
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>> @@ -0,0 +1,41 @@
>> +timer IIO trigger bindings for STM32
>> +
>> +Must be a sub-node of STM32 general purpose timer driver
> Add a cross reference...

I will add it in v3

>> +
>> +Required parameters:
>> +- compatible:                must be "st,stm32-iio-timer"
> st,stm32-adc-timer or something like that.

I would prefer use st,stm32-timer-trigger because triggers can be used
for multiple other devices (dac, adc, timers)

>> +- interrupts:                Interrupt for this device
>> +                     See ../interrupt-controller/st,stm32-exti.txt
>> +
>> +Optional parameters:
>> +- st,input-triggers-names:   List of the possible input triggers for
>> +                             the device
>> +- st,output-triggers-names:  List of the possible output triggers for
>> +                             the device
> What are input / output triggers?

each hardware block can be the source of triggers (output triggers) or customer
of some other trigger (input triggers).That what I have tried to
describe in those two
parameters

>> +
>> +Possible triggers are defined in include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>> +
>> +Example:
>> +     gptimer1: gptimer1@40010000 {
>> +             compatible = "st,stm32-gptimer";
>> +             reg = <0x40010000 0x400>;
>> +             clocks = <&rcc 0 160>;
>> +             clock-names = "clk_int";
>> +
>> +             pwm1@0 {
>> +                     compatible = "st,stm32-pwm";
>> +                     st,pwm-num-chan = <4>;
>> +                     st,breakinput;
>> +                     st,complementary;
>> +             };
>> +
>> +             iiotimer1@0 {
>> +                     compatible = "st,stm32-iio-timer";
>> +                     interrupts = <27>;
>> +                     st,input-triggers-names = TIM5_TRGO,
>> +                                               TIM2_TRGO,
>> +                                               TIM4_TRGO,
>> +                                               TIM3_TRGO;
>> +                     st,output-triggers-names = TIM1_TRGO;
>> +             };
>> +     };
>>
>

Re: [PATCH v2 5/7] IIO: add bindings for stm32 IIO timer driver

[Jonathan Cameron]

On 27/11/16 15:45, Benjamin Gaignard wrote:
> 2016-11-27 15:25 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
>> On 24/11/16 15:14, Benjamin Gaignard wrote:
>>> Define bindings for stm32 IIO timer
>>>
>>> version 2:
>>> - only keep one compatible
>>> - add DT parameters to set lists of the triggers:
>>>   one list describe the triggers created by the device
>>>   another one give the triggers accepted by the device
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>>> ---
>>>  .../bindings/iio/timer/stm32-iio-timer.txt         | 41 ++++++++++++++++++++++
>>>  1 file changed, 41 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>>> new file mode 100644
>>> index 0000000..840b417
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>>> @@ -0,0 +1,41 @@
>>> +timer IIO trigger bindings for STM32
>>> +
>>> +Must be a sub-node of STM32 general purpose timer driver
>> Add a cross reference...
> 
> I will add it in v3
> 
>>> +
>>> +Required parameters:
>>> +- compatible:                must be "st,stm32-iio-timer"
>> st,stm32-adc-timer or something like that.
> 
> I would prefer use st,stm32-timer-trigger because triggers can be used
> for multiple other devices (dac, adc, timers)
> 
>>> +- interrupts:                Interrupt for this device
>>> +                     See ../interrupt-controller/st,stm32-exti.txt
>>> +
>>> +Optional parameters:
>>> +- st,input-triggers-names:   List of the possible input triggers for
>>> +                             the device
>>> +- st,output-triggers-names:  List of the possible output triggers for
>>> +                             the device
>> What are input / output triggers?
> 
> each hardware block can be the source of triggers (output triggers) or customer
> of some other trigger (input triggers).That what I have tried to
> describe in those two
> parameters
So this is really about using one timer as a prescaler for another?

I'd be tempted in the first instance to drop that functionality and just
describe the ones that drive the device sampling.

It's complex to describe and there is enough complexity in here already
to keep things busy for a while!
> 
>>> +
>>> +Possible triggers are defined in include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>>> +
>>> +Example:
>>> +     gptimer1: gptimer1@40010000 {
>>> +             compatible = "st,stm32-gptimer";
>>> +             reg = <0x40010000 0x400>;
>>> +             clocks = <&rcc 0 160>;
>>> +             clock-names = "clk_int";
>>> +
>>> +             pwm1@0 {
>>> +                     compatible = "st,stm32-pwm";
>>> +                     st,pwm-num-chan = <4>;
>>> +                     st,breakinput;
>>> +                     st,complementary;
>>> +             };
>>> +
>>> +             iiotimer1@0 {
>>> +                     compatible = "st,stm32-iio-timer";
>>> +                     interrupts = <27>;
>>> +                     st,input-triggers-names = TIM5_TRGO,
>>> +                                               TIM2_TRGO,
>>> +                                               TIM4_TRGO,
>>> +                                               TIM3_TRGO;
>>> +                     st,output-triggers-names = TIM1_TRGO;
>>> +             };
>>> +     };
>>>
>>
> --
> 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
> 

Re: [PATCH v2 1/7] MFD: add bindings for stm32 general purpose timer driver

[Benjamin Gaignard]

2016-11-27 16:41 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
> On 27/11/16 14:10, Jonathan Cameron wrote:
>> On 24/11/16 15:14, Benjamin Gaignard wrote:
>>> Add bindings information for stm32 general purpose timer
>>>
>>> version 2:
>>> - rename stm32-mfd-timer to stm32-gptimer
>>> - only keep one compatible string
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>>> ---
>>>  .../bindings/mfd/stm32-general-purpose-timer.txt   | 43 ++++++++++++++++++++++
>>>  1 file changed, 43 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>>> new file mode 100644
>>> index 0000000..2f10e67
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>>> @@ -0,0 +1,43 @@
>>> +STM32 general purpose timer driver
>>> +
>>> +Required parameters:
>>> +- compatible: must be "st,stm32-gptimer"
>>> +
>>> +- reg:                      Physical base address and length of the controller's
>>> +                    registers.
>>> +- clock-names:              Set to "clk_int".
>>> +- clocks:           Phandle to the clock used by the timer module.
>>> +                    For Clk properties, please refer to ../clock/clock-bindings.txt
>>> +
>>> +Optional parameters:
>>> +- resets:           Phandle to the parent reset controller.
>>> +                    See ..reset/st,stm32-rcc.txt
>>> +
>>> +Optional subnodes:
>>> +- pwm:                      See ../pwm/pwm-stm32.txt
>>> +- iiotimer:         See ../iio/timer/stm32-iio-timer.txt
>> Naming issue here.  Can't mention IIO as that's a linux subsystem and all
>> bindings must be independent of OS.
>>
>> Perhaps adc-trigger-timer?
>>> +
>>> +Example:
>>> +    gptimer1: gptimer1@40010000 {
>>> +            compatible = "st,stm32-gptimer";
>>> +            reg = <0x40010000 0x400>;
>>> +            clocks = <&rcc 0 160>;
>>> +            clock-names = "clk_int";
>>> +
>>> +            pwm1@0 {
>>> +                    compatible = "st,stm32-pwm";
>>> +                    st,pwm-num-chan = <4>;
>>> +                    st,breakinput;
>>> +                    st,complementary;
>>> +            };
>>> +
>>> +            iiotimer1@0 {
>>> +                    compatible = "st,stm32-iio-timer";
>> Again, avoid the use of iio in here (same issue you had with mfd in the previous
>> version).
>>> +                    interrupts = <27>;
>>> +                    st,input-triggers-names = TIM5_TRGO,
>> Docs for these should be introduced before they are used in an example.
>> Same for the PWM ones above.  Expand the detail fo the example as you add
>> the other elements.
>
> I've just dived into the datasheet for these timers.
> http://www.st.com/content/ccc/resource/technical/document/reference_manual/3d/6d/5a/66/b4/99/40/d4/DM00031020.pdf/files/DM00031020.pdf/jcr:content/translations/en.DM00031020.pdf
>

I really appreciate that you do this effort, thanks.

>
> I think you need a binding that describes the capabilities of each of the timers
> explicitly.   Down to the level of whether there is a repetition counter or not.
> Each should exists as a separate entity in device tree.
>
> They then have an existence as timers separate to the description of what they
> are used for.
>
> Here the only way we are saying they exist is by their use which doesn't feel
> right to me.
>
> So I think you need to move back to what you had in the first place.  The key
> thing is that ever timer needs describing fully.  They are different enough
> that for example the datasheet doesn't even try to describe them in one section.
> (it has 4 separate chapters covering different sets of these hardware blocks).
> The naming isn't really based on index, we are talking different hardware
> that the datasheet authors have decided not to give different names to!

Even if the hardware are named differently in the documentation they
all share the
same registers definitions and mapping but configurations are
different for each device.

>
> If they'd called them
> advanced timers
> generic timers
> basic timers
> really basic timers meant for driving the DAC (6 and 7)
>
> We'd all have been quite happy with different compatible strings giving away
> what they can do.

4 compatible strings will not be enough to describe devices
configuration, for example
in the documentation general purpose timers could have a 16 or 32 bit
counter, for 1 to 4
pwm channels and triggers (accepted or generated by the device) are
different for each device.

DAC could be drive by timers 2, 4, 5, 6, 7 and 8.
ADC could be driver by 32 triggers

> What you have here is far too specific to what you are trying to do with them
> right now.
>
> These things are separately capable of timing capture (which is I guess where
> the IIO device later comes in).
>
> So my expectation is that we end up potentially instantiating:
>
> 1) An MFD to handle the shared elements of the timers.
> 2) Up to 12ish timers each with separate existence as a device in the driver model
> and in device tree.
> (nasty corner cases here are using timers as perscalers for other timers - I'd be
> tempted to leave that for now)
> Note that each of these devices has a different register set I think? Any shared
> bits are handled via the mfd above (if we even need that MFD which I'm starting
> to doubt looking at the datasheet).
>

pwm and trigger share the same registers but not the same bits.
With regmap write functions I don't have sharing problems.

> 3) Up to N pwms again with there own existence in the device model.  These don't
> do much other than wrap the timer and stick it in output mode.
> 4) Up to N iio triggers - this is basically using the timer as a periodic interrupt
> (though without the interrupt having visibility to the kernel) which fires off
> sampling on associated ADCs.
> 5) Up to N iio capture devices for all channels that support timing capture.
> Note there is also hardware encoder capture support in these which should be
> correctly handled as well.  This comes back to an ancient discussion on the
> TI ecap units which have similar capabilities (driver never went anywhere but
> I think that was because the author left TI).
>
> Certainly for the IIO devices these should no be bound up into one instance
> as you have done here.
>
> Anyhow, I fear that right now this discussion is missing the key ingredient
> that the hardware is horrendously variable in it's capabilities and really
> is 4-5 different types of hardware that just happen to share a few bits of
> their offsets in their register maps.

Hardware really share the same registers mapping that why I have wrote
one only driver
per framework. Only the configurations are different

>
> So after all that I'm almost more confused than I was at the start!
>
> Jonathan
>
>
>>> +                                              TIM2_TRGO,
>>> +                                              TIM4_TRGO,
>>> +                                              TIM3_TRGO;
>>> +                    st,output-triggers-names = TIM1_TRGO;
>>> +            };
>>> +    };
>>>
>>
>> --
>> 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
>>
>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 6/7] IIO: add STM32 IIO timer driver

[Benjamin Gaignard]

2016-11-27 16:42 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
> I delved into the datasheet after trying to figure this out, so I think
> I now sort of understand your intent, but please do answer the questions
> inline.
>
> On 24/11/16 15:14, Benjamin Gaignard wrote:
>> Timers IPs can be used to generate triggers for other IPs like
>> DAC, ADC or other timers.
>> Each trigger may result of timer internals signals like counter enable,
>> reset or edge, this configuration could be done through "master_mode"
>> device attribute.
>>
>> A timer device could be triggered by other timers, we use the trigger
>> name and is_stm32_iio_timer_trigger() function to distinguish them
>> and configure IP input switch.
> The presence of an IIO device in here was a suprise.. What is it actually for?

IIO device is needed to be able to valid the input triggers, which
aren't the same than
those generated by the device.
Since I use triggers name to distinguish them I have introduced
is_stm32_iio_timer_trigger()
function to be sure that triggers are coming for a valid hardware and
not from a fake one
using the same name.

>
> I think this needs some examples of usage to make it clear what the aim is.

In the hardware block there is switch in input to select which trigger
will drive the IP.
For example that allow to start multiple pwm exactly that the same
time or to start/stop
it on master edges.

>
> I was basically expecting to see a driver instantiating one iio trigger
> per timer that can act as a trigger.  Those would each have sampling frequency
> controls and basica enable / disable.

An hardware device could have up to 5 triggers: timX_trgo, timX_ch1, timX_ch2,
timX_ch3, timX_ch4.
Until now I have try to simplify the problem and just use timX_trgo trigger.
I have added a "sampling_frequency" attribute on the trigger to
control the frequence
and I use trigger set_state function to enable disable it.

>
> I'm seeing something much more complex here so additional explanation is
> needed.
>>
>> Timer may also decide on which event (edge, level) they could
>> be activated by a trigger, this configuration is done by writing in
>> "slave_mode" device attribute.
> Really?  Sounds like magic numbers in sysfs which is never a good idea.
> Please document those attributes / or break them up into elements that
> don't require magic numbers.

I would like to use strings here, it is possible to use IIO_CONST_ATTR
to describe them ?

>>
>> Since triggers could also be used by DAC or ADC their names are defined
>> in include/dt-bindings/iio/timer/st,stm32-iio-timer.h so those IPs will be able
>> to configure themselves in valid_trigger function
>>
>> Trigger have a "sampling_frequency" attribute which allow to configure
>> timer sampling frequency without using pwm interface
>>
>> version 2:
>> - keep only one compatible
> Hmm. I'm not sure I like this as such.  We are actually dealing with lots
> of instances of a hardware block with only a small amount of shared
> infrastrcuture (which is classic mfd teritory). So to my mind we
> should have a separate device for each.

Registers mapping and offset are the same, from triggers point of view
only the configuration of the input switch is different.

>
>> - use st,input-triggers-names and st,output-triggers-names
>>   to know which triggers are accepted and/or create by the device
> I'm not following why we have this cascade setup?
>
> These are triggers, not devices in the IIO context.  We need some detailed
> description of why you have it setup like this. This would include the
> ABI with examples of how you are using it.

I had put example of usage on the cover letter, I will duplicate them
in this commit
message.

>
> Basically I don't currently understand what you are doing :(
>
>
> Thanks,
>
> Jonathan
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>>  drivers/iio/Kconfig                                |   2 +-
>>  drivers/iio/Makefile                               |   1 +
>>  drivers/iio/timer/Kconfig                          |  15 +
>>  drivers/iio/timer/Makefile                         |   1 +
>>  drivers/iio/timer/stm32-iio-timer.c                | 448 +++++++++++++++++++++
>>  drivers/iio/trigger/Kconfig                        |   1 -
>>  include/dt-bindings/iio/timer/st,stm32-iio-timer.h |  23 ++
>>  include/linux/iio/timer/stm32-iio-timers.h         |  16 +
>>  8 files changed, 505 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/iio/timer/Kconfig
>>  create mode 100644 drivers/iio/timer/Makefile
>>  create mode 100644 drivers/iio/timer/stm32-iio-timer.c
>>  create mode 100644 include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>>  create mode 100644 include/linux/iio/timer/stm32-iio-timers.h
>>
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index 6743b18..2de2a80 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -90,5 +90,5 @@ source "drivers/iio/potentiometer/Kconfig"
>>  source "drivers/iio/pressure/Kconfig"
>>  source "drivers/iio/proximity/Kconfig"
>>  source "drivers/iio/temperature/Kconfig"
>> -
>> +source "drivers/iio/timer/Kconfig"
>>  endif # IIO
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index 87e4c43..b797c08 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -32,4 +32,5 @@ obj-y += potentiometer/
>>  obj-y += pressure/
>>  obj-y += proximity/
>>  obj-y += temperature/
>> +obj-y += timer/
>>  obj-y += trigger/
>> diff --git a/drivers/iio/timer/Kconfig b/drivers/iio/timer/Kconfig
>> new file mode 100644
>> index 0000000..7a73bc6
>> --- /dev/null
>> +++ b/drivers/iio/timer/Kconfig
>> @@ -0,0 +1,15 @@
>> +#
>> +# Timers drivers
>> +
>> +menu "Timers"
>> +
>> +config IIO_STM32_TIMER
>> +     tristate "stm32 iio timer"
>> +     depends on ARCH_STM32
>> +     depends on OF
>> +     select IIO_TRIGGERED_EVENT
>> +     select MFD_STM32_GP_TIMER
>> +     help
>> +       Select this option to enable stm32 timers hardware IPs
>> +
>> +endmenu
>> diff --git a/drivers/iio/timer/Makefile b/drivers/iio/timer/Makefile
>> new file mode 100644
>> index 0000000..a360c9f
>> --- /dev/null
>> +++ b/drivers/iio/timer/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_IIO_STM32_TIMER) += stm32-iio-timer.o
>> diff --git a/drivers/iio/timer/stm32-iio-timer.c b/drivers/iio/timer/stm32-iio-timer.c
>> new file mode 100644
>> index 0000000..35f2687
>> --- /dev/null
>> +++ b/drivers/iio/timer/stm32-iio-timer.c
>> @@ -0,0 +1,448 @@
>> +/*
>> + * stm32-iio-timer.c
>> + *
>> + * Copyright (C) STMicroelectronics 2016
>> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
>> + * License terms:  GNU General Public License (GPL), version 2
>> + */
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/timer/stm32-iio-timers.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/triggered_event.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/stm32-gptimer.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define DRIVER_NAME "stm32-iio-timer"
>> +
>> +struct stm32_iio_timer_dev {
>> +     struct device *dev;
>> +     struct regmap *regmap;
>> +     struct clk *clk;
>> +     int irq;
>> +     bool own_timer;
>> +     unsigned int sampling_frequency;
>> +     struct iio_trigger *active_trigger;
>> +};
>> +
>> +static ssize_t _store_frequency(struct device *dev,
>> +                             struct device_attribute *attr,
>> +                             const char *buf, size_t len)
>> +{
>> +     struct iio_trigger *trig = to_iio_trigger(dev);
>> +     struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
>> +     unsigned int freq;
>> +     int ret;
>> +
>> +     ret = kstrtouint(buf, 10, &freq);
>> +     if (ret)
>> +             return ret;
>> +
>> +     stm32->sampling_frequency = freq;
>> +
>> +     return len;
>> +}
>> +
>> +static ssize_t _read_frequency(struct device *dev,
>> +                            struct device_attribute *attr, char *buf)
>> +{
>> +     struct iio_trigger *trig = to_iio_trigger(dev);
>> +     struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
>> +     unsigned long long freq = stm32->sampling_frequency;
>> +     u32 psc, arr, cr1;
>> +
>> +     regmap_read(stm32->regmap, TIM_CR1, &cr1);
>> +     regmap_read(stm32->regmap, TIM_PSC, &psc);
>> +     regmap_read(stm32->regmap, TIM_ARR, &arr);
>> +
>> +     if (psc && arr && (cr1 & TIM_CR1_CEN)) {
>> +             freq = (unsigned long long)clk_get_rate(stm32->clk);
>> +             do_div(freq, psc);
>> +             do_div(freq, arr);
>> +     }
>> +
>> +     return sprintf(buf, "%d\n", (unsigned int)freq);
>> +}
>> +
>> +static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
>> +                           _read_frequency,
>> +                           _store_frequency);
>> +
>> +static struct attribute *stm32_trigger_attrs[] = {
>> +     &iio_dev_attr_sampling_frequency.dev_attr.attr,
>> +     NULL,
>> +};
>> +
>> +static const struct attribute_group stm32_trigger_attr_group = {
>> +     .attrs = stm32_trigger_attrs,
>> +};
>> +
>> +static const struct attribute_group *stm32_trigger_attr_groups[] = {
>> +     &stm32_trigger_attr_group,
>> +     NULL,
>> +};
>> +
>> +static
>> +ssize_t _show_master_mode(struct device *dev,
>> +                       struct device_attribute *attr, char *buf)
>> +{
>> +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> +     struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>> +     u32 cr2;
>> +
>> +     regmap_read(stm32->regmap, TIM_CR2, &cr2);
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%d\n", (cr2 >> 4) & 0x7);
>> +}
>> +
>> +static
>> +ssize_t _store_master_mode(struct device *dev,
>> +                        struct device_attribute *attr,
>> +                        const char *buf, size_t len)
>> +{
>> +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> +     struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>> +     u8 mode;
>> +     int ret;
>> +
>> +     ret = kstrtou8(buf, 10, &mode);
>> +     if (ret)
>> +             return ret;
>> +
>> +     if (mode > 0x7)
>> +             return -EINVAL;
>> +
>> +     regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, mode << 4);
>> +
>> +     return len;
>> +}
>> +
>> +static IIO_DEVICE_ATTR(master_mode, S_IRUGO | S_IWUSR,
>> +                    _show_master_mode,
>> +                    _store_master_mode,
>> +                    0);
>> +
>> +static
>> +ssize_t _show_slave_mode(struct device *dev,
>> +                      struct device_attribute *attr, char *buf)
>> +{
>> +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> +     struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>> +     u32 smcr;
>> +
>> +     regmap_read(stm32->regmap, TIM_SMCR, &smcr);
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%d\n", smcr & 0x3);
>> +}
>> +
>> +static
>> +ssize_t _store_slave_mode(struct device *dev,
>> +                       struct device_attribute *attr,
>> +                       const char *buf, size_t len)
>> +{
>> +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> +     struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>> +     u8 mode;
>> +     int ret;
>> +
>> +     ret = kstrtou8(buf, 10, &mode);
>> +     if (ret)
>> +             return ret;
>> +
>> +     if (mode > 0x7)
>> +             return -EINVAL;
> How is something called slave mode going to be fed a number between 0 and 7?
> Rule of thumb is no magic numbers in sysfs and right now this is looking
> rather cryptic to say the least!

I would like to use strings here, it is possible to use IIO_CONST_ATTR
to describe them ?
In documentation slave modes are describe that this:
000: Slave mode disabled - if CEN = ‘1’ then the prescaler is clocked
directly by the internal clock.
001: Encoder mode 1 - Counter counts up/down on TI2FP1 edge depending
on TI1FP2 level.
010: Encoder mode 2 - Counter counts up/down on TI1FP2 edge depending
on TI2FP1 level.
011: Encoder mode 3 - Counter counts up/down on both TI1FP1 and TI2FP2
edges depending on the level of the other input.
100: Reset Mode - Rising edge of the selected trigger input (TRGI)
reinitializes the counter and generates an update of the registers.
101: Gated Mode - The counter clock is enabled when the trigger input
(TRGI) is high.
        The counter stops (but is not reset) as soon as the trigger becomes low.
         Both start and stop of the counter are controlled.
110: Trigger Mode - The counter starts at a rising edge of the trigger
TRGI (but it is notreset).
         Only the start of the counter is controlled.
111: External Clock Mode 1 - Rising edges of the selected trigger
(TRGI) clock the counter.

>> +
>> +     regmap_update_bits(stm32->regmap, TIM_SMCR, TIM_SMCR_SMS, mode);
>> +
>> +     return len;
>> +}
>> +
>> +static IIO_DEVICE_ATTR(slave_mode, S_IRUGO | S_IWUSR,
> There is an iritating move (in terms of noise it's generating) to use values
> directly instead fo these defines.  Still if you don't fix it here I'll only
> get a patch 'fixing' it soon after...

I will fix at in version 3

>
>> +                    _show_slave_mode,
>> +                    _store_slave_mode,
>> +                    0);
>> +
>> +static struct attribute *stm32_timer_attrs[] = {
>> +     &iio_dev_attr_master_mode.dev_attr.attr,
>> +     &iio_dev_attr_slave_mode.dev_attr.attr,
> New ABI so must be documented under Documentation/ABI/testing/sysfs-bus-iio-*

OK

>> +     NULL,
>> +};
>> +
>> +static const struct attribute_group stm32_timer_attr_group = {
>> +     .attrs = stm32_timer_attrs,
>> +};
>> +
>> +static int stm32_timer_start(struct stm32_iio_timer_dev *stm32)
>> +{
>> +     unsigned long long prd, div;
>> +     int prescaler = 0;
>> +     u32 max_arr = 0xFFFF, cr1;
>> +
>> +     if (stm32->sampling_frequency == 0)
>> +             return 0;
>> +
>> +     /* Period and prescaler values depends of clock rate */
>> +     div = (unsigned long long)clk_get_rate(stm32->clk);
>> +
>> +     do_div(div, stm32->sampling_frequency);
>> +
>> +     prd = div;
>> +
>> +     while (div > max_arr) {
>> +             prescaler++;
>> +             div = prd;
>> +             do_div(div, (prescaler + 1));
>> +     }
>> +     prd = div;
>> +
>> +     if (prescaler > MAX_TIM_PSC) {
>> +             dev_err(stm32->dev, "prescaler exceeds the maximum value\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* Check that we own the timer */
>> +     regmap_read(stm32->regmap, TIM_CR1, &cr1);
>> +     if ((cr1 & TIM_CR1_CEN) && !stm32->own_timer)
>> +             return -EBUSY;
>> +
>> +     if (!stm32->own_timer) {
>> +             stm32->own_timer = true;
>> +             clk_enable(stm32->clk);
>> +     }
>> +
>> +     regmap_write(stm32->regmap, TIM_PSC, prescaler);
>> +     regmap_write(stm32->regmap, TIM_ARR, prd - 1);
>> +     regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
>> +
>> +     /* Force master mode to update mode */
>> +     regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, 0x20);
>> +
>> +     /* Make sure that registers are updated */
>> +     regmap_update_bits(stm32->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
>> +
>> +     /* Enable interrupt */
>> +     regmap_write(stm32->regmap, TIM_SR, 0);
>> +     regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, TIM_DIER_UIE);
>> +
>> +     /* Enable controller */
>> +     regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
>> +
>> +     return 0;
>> +}
>> +
>> +static int stm32_timer_stop(struct stm32_iio_timer_dev *stm32)
>> +{
>> +     if (!stm32->own_timer)
>> +             return 0;
>> +
>> +     /* Stop timer */
>> +     regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, 0);
>> +     regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>> +     regmap_write(stm32->regmap, TIM_PSC, 0);
>> +     regmap_write(stm32->regmap, TIM_ARR, 0);
>> +
>> +     clk_disable(stm32->clk);
>> +
>> +     stm32->own_timer = false;
>> +     stm32->active_trigger = NULL;
>> +
>> +     return 0;
>> +}
>> +
>> +static int stm32_set_trigger_state(struct iio_trigger *trig, bool state)
>> +{
>> +     struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
>> +
>> +     stm32->active_trigger = trig;
>> +
>> +     if (state)
>> +             return stm32_timer_start(stm32);
>> +     else
>> +             return stm32_timer_stop(stm32);
>> +}
>> +
>> +static irqreturn_t stm32_timer_irq_handler(int irq, void *private)
>> +{
>> +     struct stm32_iio_timer_dev *stm32 = private;
>> +     u32 sr;
>> +
>> +     regmap_read(stm32->regmap, TIM_SR, &sr);
>> +     regmap_write(stm32->regmap, TIM_SR, 0);
>> +
>> +     if ((sr & TIM_SR_UIF) && stm32->active_trigger)
>> +             iio_trigger_poll(stm32->active_trigger);
> This is acting like a trigger cascading off another trigger?

Not only a trigger but ADC or DAC too.

>
> Normally this interrupt handler would be directly associated with the
> trigger hardware - in this case the timer.

>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static const struct iio_trigger_ops timer_trigger_ops = {
>> +     .owner = THIS_MODULE,
>> +     .set_trigger_state = stm32_set_trigger_state,
>> +};
>> +
>> +static int stm32_setup_iio_triggers(struct stm32_iio_timer_dev *stm32)
>> +{
>> +     int ret;
>> +     struct property *p;
>> +     const char *cur = NULL;
>> +
>> +     p = of_find_property(stm32->dev->of_node,
>> +                          "st,output-triggers-names", NULL);
>> +
>> +     while ((cur = of_prop_next_string(p, cur)) != NULL) {
>> +             struct iio_trigger *trig;
>> +
>> +             trig = devm_iio_trigger_alloc(stm32->dev, "%s", cur);
>> +             if  (!trig)
>> +                     return -ENOMEM;
>> +
>> +             trig->dev.parent = stm32->dev->parent;
>> +             trig->ops = &timer_trigger_ops;
>> +             trig->dev.groups = stm32_trigger_attr_groups;
>> +             iio_trigger_set_drvdata(trig, stm32);
>> +
>> +             ret = devm_iio_trigger_register(stm32->dev, trig);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * is_stm32_iio_timer_trigger
>> + * @trig: trigger to be checked
>> + *
>> + * return true if the trigger is a valid stm32 iio timer trigger
>> + * either return false
>> + */
>> +bool is_stm32_iio_timer_trigger(struct iio_trigger *trig)
>> +{
>> +     return (trig->ops == &timer_trigger_ops);
>> +}
>> +EXPORT_SYMBOL(is_stm32_iio_timer_trigger);
>> +
>> +static int stm32_validate_trigger(struct iio_dev *indio_dev,
>> +                               struct iio_trigger *trig)
>> +{
>> +     struct stm32_iio_timer_dev *dev = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     if (!is_stm32_iio_timer_trigger(trig))
>> +             return -EINVAL;
>> +
>> +     ret = of_property_match_string(dev->dev->of_node,
>> +                                    "st,input-triggers-names",
>> +                                    trig->name);
>> +
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     regmap_update_bits(dev->regmap, TIM_SMCR, TIM_SMCR_TS, ret << 4);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct iio_info stm32_trigger_info = {
>> +     .driver_module = THIS_MODULE,
>> +     .validate_trigger = stm32_validate_trigger,
>> +     .attrs = &stm32_timer_attr_group,
>> +};
>> +
>> +static struct stm32_iio_timer_dev *stm32_setup_iio_device(struct device *dev)
>> +{
>> +     struct iio_dev *indio_dev;
>> +     int ret;
>> +
>> +     indio_dev = devm_iio_device_alloc(dev, sizeof(struct stm32_iio_timer_dev));
>> +     if (!indio_dev)
>> +             return NULL;
> This is 'unusual'.  Why does a trigger driver need an iio_dev at all?

Trigger doesn't need it but for configuring the input switch when
validating the triggers I need a device

>> +
>> +     indio_dev->name = dev_name(dev);
>> +     indio_dev->dev.parent = dev;
>> +     indio_dev->info = &stm32_trigger_info;
>> +     indio_dev->modes = INDIO_EVENT_TRIGGERED;
>> +     indio_dev->num_channels = 0;
>> +     indio_dev->dev.of_node = dev->of_node;
>> +
>> +     ret = iio_triggered_event_setup(indio_dev,
>> +                                     NULL,
>> +                                     stm32_timer_irq_handler);
> So the iio_dev exists to provide the ability to fire this interrupt from
> another trigger?  Why do you want to do this?

I need interrupt because I use set_trigger_state() to enable/disable
the sampling frequency.
As far I understand and test set_trigger_state() is only called when
indio_dev->modes = INDIO_EVENT_TRIGGERED
and iio_triggered_event_setup have been called to create register an
irq handler.
I just need irq declaration for this last condition, I don't need the
irq to fire in kernel to drive other hardware block.

If I could use set_trigger_state() without calling using
iio_triggered_event_setup() I should remove all
irq code from the driver.

One possible solution would be to add calls to set_trigger_state() in
iio_trigger_write_current() function
at the same level than iio_trigger_detach_poll_func() or
iio_trigger_attach_poll_func() calls:

if (indio_dev->modes = INDIO_DIRECT_MODE && oldtrig->ops->set_trigger_state)
     oldtrig->ops->set_trigger_state(oldtrig, false);

if (indio_dev->modes = INDIO_DIRECT_MODE &&
indio_dev->trig->ops->set_trigger_state)
     indio_dev->trig->ops->set_trigger_state(indio_dev->trig, true);

I'm to new in IIO framework to understand if that it possible or not

>> +     if (ret)
>> +             return NULL;
>> +
>> +     ret = devm_iio_device_register(dev, indio_dev);
>> +     if (ret) {
>> +             iio_triggered_event_cleanup(indio_dev);
>> +             return NULL;
>> +     }
>> +
>> +     return iio_priv(indio_dev);
>> +}
>> +
>> +static int stm32_iio_timer_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct stm32_iio_timer_dev *stm32;
>> +     struct stm32_gptimer_dev *mfd = dev_get_drvdata(pdev->dev.parent);
>> +     int ret;
>> +
>> +     stm32 = stm32_setup_iio_device(dev);
>> +     if (!stm32)
>> +             return -ENOMEM;
>> +
>> +     stm32->dev = dev;
>> +     stm32->regmap = mfd->regmap;
>> +     stm32->clk = mfd->clk;
>> +
>> +     stm32->irq = platform_get_irq(pdev, 0);
>> +     if (stm32->irq < 0)
>> +             return -EINVAL;
>> +
>> +     ret = devm_request_irq(stm32->dev, stm32->irq,
>> +                            stm32_timer_irq_handler, IRQF_SHARED,
>> +                            "iiotimer_event", stm32);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = stm32_setup_iio_triggers(stm32);
>> +     if (ret)
>> +             return ret;
>> +
>> +     platform_set_drvdata(pdev, stm32);
>> +
>> +     return 0;
>> +}
>> +
>> +static int stm32_iio_timer_remove(struct platform_device *pdev)
>> +{
>> +     struct stm32_iio_timer_dev *stm32 = platform_get_drvdata(pdev);
>> +
>> +     iio_triggered_event_cleanup((struct iio_dev *)stm32);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id stm32_trig_of_match[] = {
>> +     {
>> +             .compatible = "st,stm32-iio-timer",
>> +     },
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
>> +
>> +static struct platform_driver stm32_iio_timer_driver = {
>> +     .probe = stm32_iio_timer_probe,
>> +     .remove = stm32_iio_timer_remove,
>> +     .driver = {
>> +             .name = DRIVER_NAME,
>> +             .of_match_table = stm32_trig_of_match,
>> +     },
>> +};
>> +module_platform_driver(stm32_iio_timer_driver);
>> +
>> +MODULE_ALIAS("platform:" DRIVER_NAME);
>> +MODULE_DESCRIPTION("STMicroelectronics STM32 iio timer driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
>> index 809b2e7..f2af4fe 100644
>> --- a/drivers/iio/trigger/Kconfig
>> +++ b/drivers/iio/trigger/Kconfig
>> @@ -46,5 +46,4 @@ config IIO_SYSFS_TRIGGER
>>
>>         To compile this driver as a module, choose M here: the
>>         module will be called iio-trig-sysfs.
>> -
> Clear this out...
>>  endmenu
>> diff --git a/include/dt-bindings/iio/timer/st,stm32-iio-timer.h b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>> new file mode 100644
>> index 0000000..d39bf16
>> --- /dev/null
>> +++ b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>> @@ -0,0 +1,23 @@
>> +/*
>> + * st,stm32-iio-timer.h
>> + *
>> + * Copyright (C) STMicroelectronics 2016
>> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
>> + * License terms:  GNU General Public License (GPL), version 2
>> + */
>> +
>> +#ifndef _DT_BINDINGS_IIO_TIMER_H_
>> +#define _DT_BINDINGS_IIO_TIMER_H_
>> +
>> +#define TIM1_TRGO    "tim1_trgo"
>> +#define TIM2_TRGO    "tim2_trgo"
>> +#define TIM3_TRGO    "tim3_trgo"
>> +#define TIM4_TRGO    "tim4_trgo"
>> +#define TIM5_TRGO    "tim5_trgo"
>> +#define TIM6_TRGO    "tim6_trgo"
>> +#define TIM7_TRGO    "tim7_trgo"
>> +#define TIM8_TRGO    "tim8_trgo"
>> +#define TIM9_TRGO    "tim9_trgo"
>> +#define TIM12_TRGO   "tim12_trgo"
>> +
>> +#endif
>> diff --git a/include/linux/iio/timer/stm32-iio-timers.h b/include/linux/iio/timer/stm32-iio-timers.h
>> new file mode 100644
>> index 0000000..5d1b86c
>> --- /dev/null
>> +++ b/include/linux/iio/timer/stm32-iio-timers.h
>> @@ -0,0 +1,16 @@
>> +/*
>> + * stm32-iio-timers.h
>> + *
>> + * Copyright (C) STMicroelectronics 2016
>> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
>> + * License terms:  GNU General Public License (GPL), version 2
>> + */
>> +
>> +#ifndef _STM32_IIO_TIMERS_H_
>> +#define _STM32_IIO_TIMERS_H_
>> +
>> +#include <dt-bindings/iio/timer/st,stm32-iio-timer.h>
>> +
>> +bool is_stm32_iio_timer_trigger(struct iio_trigger *trig);
>> +
>> +#endif
>>
>

Re: [PATCH v2 3/7] PWM: add pwm-stm32 DT bindings

[Rob Herring]

On Thu, Nov 24, 2016 at 04:14:19PM +0100, Benjamin Gaignard wrote:
> Define bindings for pwm-stm32
> 
> version 2:
> - use parameters instead of compatible of handle the hardware configuration
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  .../devicetree/bindings/pwm/pwm-stm32.txt          | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> new file mode 100644
> index 0000000..36263f0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> @@ -0,0 +1,37 @@
> +STMicroelectronics PWM driver bindings for STM32
> +
> +Must be a sub-node of STM32 general purpose timer driver
> +
> +Required parameters:
> +- compatible:		Must be "st,stm32-pwm"
> +- pinctrl-names: 	Set to "default".
> +- pinctrl-0: 		List of phandles pointing to pin configuration nodes
> +			for PWM module.
> +			For Pinctrl properties, please refer to [1].
> +
> +Optional parameters:
> +- st,breakinput:	Set if the hardware have break input capabilities
> +- st,breakinput-polarity: Set break input polarity. Default is 0
> +			 The value define the active polarity:
> +			  - 0 (active LOW)
> +			  - 1 (active HIGH)
> +- st,pwm-num-chan:	Number of available PWM channels.  Default is 0.
> +- st,32bits-counter:	Set if the hardware have a 32 bits counter
> +- st,complementary:	Set if the hardware have complementary output channels

What does complementary mean here?

> +
> +[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +
> +Example:
> +	gptimer1: gptimer1@40010000 {

timer@...

> +		compatible = "st,stm32-gptimer";
> +		reg = <0x40010000 0x400>;
> +		clocks = <&rcc 0 160>;
> +		clock-names = "clk_int";
> +
> +		pwm1@0 {

pwm {

Is there more than one?

> +			compatible = "st,stm32-pwm";
> +			st,pwm-num-chan = <4>;
> +			st,breakinput;
> +			st,complementary;
> +		};
> +	};
> -- 
> 1.9.1
> 

Re: [PATCH v2 3/7] PWM: add pwm-stm32 DT bindings

[Benjamin Gaignard]

2016-11-30 22:20 GMT+01:00 Rob Herring <robh@kernel.org>:
> On Thu, Nov 24, 2016 at 04:14:19PM +0100, Benjamin Gaignard wrote:
>> Define bindings for pwm-stm32
>>
>> version 2:
>> - use parameters instead of compatible of handle the hardware configuration
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>>  .../devicetree/bindings/pwm/pwm-stm32.txt          | 37 ++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> new file mode 100644
>> index 0000000..36263f0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> @@ -0,0 +1,37 @@
>> +STMicroelectronics PWM driver bindings for STM32
>> +
>> +Must be a sub-node of STM32 general purpose timer driver
>> +
>> +Required parameters:
>> +- compatible:                Must be "st,stm32-pwm"
>> +- pinctrl-names:     Set to "default".
>> +- pinctrl-0:                 List of phandles pointing to pin configuration nodes
>> +                     for PWM module.
>> +                     For Pinctrl properties, please refer to [1].
>> +
>> +Optional parameters:
>> +- st,breakinput:     Set if the hardware have break input capabilities
>> +- st,breakinput-polarity: Set break input polarity. Default is 0
>> +                      The value define the active polarity:
>> +                       - 0 (active LOW)
>> +                       - 1 (active HIGH)
>> +- st,pwm-num-chan:   Number of available PWM channels.  Default is 0.
>> +- st,32bits-counter: Set if the hardware have a 32 bits counter
>> +- st,complementary:  Set if the hardware have complementary output channels
>
> What does complementary mean here?

Complementary channels are pwm channels where the signal level is inverted
compare to the original channel.
This parameter indicate that the hardware have this kind of outputs.
If the polarity of the original channel change then polarity of
complementary channel
change too.

>
>> +
>> +[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> +
>> +Example:
>> +     gptimer1: gptimer1@40010000 {
>
> timer@...

I would like keep "timer" for timer-trigger sub-node

>
>> +             compatible = "st,stm32-gptimer";
>> +             reg = <0x40010000 0x400>;
>> +             clocks = <&rcc 0 160>;
>> +             clock-names = "clk_int";
>> +
>> +             pwm1@0 {
>
> pwm {
>
> Is there more than one?

Not per hardware block but their is 12 of them in the SoC.
Adding a number (which match with SoC documentation) help to find
the wanted pwm in sysfs either we only have the address.

>
>> +                     compatible = "st,stm32-pwm";
>> +                     st,pwm-num-chan = <4>;
>> +                     st,breakinput;
>> +                     st,complementary;
>> +             };
>> +     };
>> --
>> 1.9.1
>>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 1/7] MFD: add bindings for stm32 general purpose timer driver

[Jonathan Cameron]

On 29/11/16 08:48, Benjamin Gaignard wrote:
> 2016-11-27 16:41 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
>> On 27/11/16 14:10, Jonathan Cameron wrote:
>>> On 24/11/16 15:14, Benjamin Gaignard wrote:
>>>> Add bindings information for stm32 general purpose timer
>>>>
>>>> version 2:
>>>> - rename stm32-mfd-timer to stm32-gptimer
>>>> - only keep one compatible string
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>>>> ---
>>>>  .../bindings/mfd/stm32-general-purpose-timer.txt   | 43 ++++++++++++++++++++++
>>>>  1 file changed, 43 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>>>> new file mode 100644
>>>> index 0000000..2f10e67
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>>>> @@ -0,0 +1,43 @@
>>>> +STM32 general purpose timer driver
>>>> +
>>>> +Required parameters:
>>>> +- compatible: must be "st,stm32-gptimer"
>>>> +
>>>> +- reg:                      Physical base address and length of the controller's
>>>> +                    registers.
>>>> +- clock-names:              Set to "clk_int".
>>>> +- clocks:           Phandle to the clock used by the timer module.
>>>> +                    For Clk properties, please refer to ../clock/clock-bindings.txt
>>>> +
>>>> +Optional parameters:
>>>> +- resets:           Phandle to the parent reset controller.
>>>> +                    See ..reset/st,stm32-rcc.txt
>>>> +
>>>> +Optional subnodes:
>>>> +- pwm:                      See ../pwm/pwm-stm32.txt
>>>> +- iiotimer:         See ../iio/timer/stm32-iio-timer.txt
>>> Naming issue here.  Can't mention IIO as that's a linux subsystem and all
>>> bindings must be independent of OS.
>>>
>>> Perhaps adc-trigger-timer?
>>>> +
>>>> +Example:
>>>> +    gptimer1: gptimer1@40010000 {
>>>> +            compatible = "st,stm32-gptimer";
>>>> +            reg = <0x40010000 0x400>;
>>>> +            clocks = <&rcc 0 160>;
>>>> +            clock-names = "clk_int";
>>>> +
>>>> +            pwm1@0 {
>>>> +                    compatible = "st,stm32-pwm";
>>>> +                    st,pwm-num-chan = <4>;
>>>> +                    st,breakinput;
>>>> +                    st,complementary;
>>>> +            };
>>>> +
>>>> +            iiotimer1@0 {
>>>> +                    compatible = "st,stm32-iio-timer";
>>> Again, avoid the use of iio in here (same issue you had with mfd in the previous
>>> version).
>>>> +                    interrupts = <27>;
>>>> +                    st,input-triggers-names = TIM5_TRGO,
>>> Docs for these should be introduced before they are used in an example.
>>> Same for the PWM ones above.  Expand the detail fo the example as you add
>>> the other elements.
>>
>> I've just dived into the datasheet for these timers.
>> http://www.st.com/content/ccc/resource/technical/document/reference_manual/3d/6d/5a/66/b4/99/40/d4/DM00031020.pdf/files/DM00031020.pdf/jcr:content/translations/en.DM00031020.pdf
>>
> 
> I really appreciate that you do this effort, thanks.
> 
>>
>> I think you need a binding that describes the capabilities of each of the timers
>> explicitly.   Down to the level of whether there is a repetition counter or not.
>> Each should exists as a separate entity in device tree.
>>
>> They then have an existence as timers separate to the description of what they
>> are used for.
>>
>> Here the only way we are saying they exist is by their use which doesn't feel
>> right to me.
>>
>> So I think you need to move back to what you had in the first place.  The key
>> thing is that ever timer needs describing fully.  They are different enough
>> that for example the datasheet doesn't even try to describe them in one section.
>> (it has 4 separate chapters covering different sets of these hardware blocks).
>> The naming isn't really based on index, we are talking different hardware
>> that the datasheet authors have decided not to give different names to!
> 
> Even if the hardware are named differently in the documentation they
> all share the
> same registers definitions and mapping but configurations are
> different for each device.
> 
>>
>> If they'd called them
>> advanced timers
>> generic timers
>> basic timers
>> really basic timers meant for driving the DAC (6 and 7)
>>
>> We'd all have been quite happy with different compatible strings giving away
>> what they can do.
> 
> 4 compatible strings will not be enough to describe devices
> configuration, for example
> in the documentation general purpose timers could have a 16 or 32 bit
> counter, for 1 to 4
> pwm channels and triggers (accepted or generated by the device) are
> different for each device.
> 
> DAC could be drive by timers 2, 4, 5, 6, 7 and 8.
> ADC could be driver by 32 triggers
My point was more about the fact that though the naming appears to (and kind
of does describe an index) these devices are really as different as for example
different part numbers would imply on a range of ADCs (say the multitude supported
by the max1363 driver - all of which are very nearly register compatible)

Hence I'd be less quick to dismiss the option of a number of compatible strings
rather than the wealth of description you'll otherwise have to put in the device
tree.
> 
>> What you have here is far too specific to what you are trying to do with them
>> right now.
>>
>> These things are separately capable of timing capture (which is I guess where
>> the IIO device later comes in).
>>
>> So my expectation is that we end up potentially instantiating:
>>
>> 1) An MFD to handle the shared elements of the timers.
>> 2) Up to 12ish timers each with separate existence as a device in the driver model
>> and in device tree.
>> (nasty corner cases here are using timers as perscalers for other timers - I'd be
>> tempted to leave that for now)
>> Note that each of these devices has a different register set I think? Any shared
>> bits are handled via the mfd above (if we even need that MFD which I'm starting
>> to doubt looking at the datasheet).
>>
> 
> pwm and trigger share the same registers but not the same bits.
> With regmap write functions I don't have sharing problems.
> 
>> 3) Up to N pwms again with there own existence in the device model.  These don't
>> do much other than wrap the timer and stick it in output mode.
>> 4) Up to N iio triggers - this is basically using the timer as a periodic interrupt
>> (though without the interrupt having visibility to the kernel) which fires off
>> sampling on associated ADCs.
>> 5) Up to N iio capture devices for all channels that support timing capture.
>> Note there is also hardware encoder capture support in these which should be
>> correctly handled as well.  This comes back to an ancient discussion on the
>> TI ecap units which have similar capabilities (driver never went anywhere but
>> I think that was because the author left TI).
>>
>> Certainly for the IIO devices these should no be bound up into one instance
>> as you have done here.
>>
>> Anyhow, I fear that right now this discussion is missing the key ingredient
>> that the hardware is horrendously variable in it's capabilities and really
>> is 4-5 different types of hardware that just happen to share a few bits of
>> their offsets in their register maps.
> 
> Hardware really share the same registers mapping that why I have wrote
> one only driver
> per framework. Only the configurations are different
One driver is fine, but the difference to my mind are sufficient that
we may need to use compatible strings for the various options.  Worth
a go at trying to fully describe them first though!
> 
>>
>> So after all that I'm almost more confused than I was at the start!
>>
>> Jonathan
>>
>>
>>>> +                                              TIM2_TRGO,
>>>> +                                              TIM4_TRGO,
>>>> +                                              TIM3_TRGO;
>>>> +                    st,output-triggers-names = TIM1_TRGO;
>>>> +            };
>>>> +    };
>>>>
>>>
>>> --
>>> 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
>>>
>>
> 
> 
> 

Re: [PATCH v2 6/7] IIO: add STM32 IIO timer driver

[Jonathan Cameron]

On 29/11/16 09:46, Benjamin Gaignard wrote:
> 2016-11-27 16:42 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
>> I delved into the datasheet after trying to figure this out, so I think
>> I now sort of understand your intent, but please do answer the questions
>> inline.
>>
>> On 24/11/16 15:14, Benjamin Gaignard wrote:
>>> Timers IPs can be used to generate triggers for other IPs like
>>> DAC, ADC or other timers.
>>> Each trigger may result of timer internals signals like counter enable,
>>> reset or edge, this configuration could be done through "master_mode"
>>> device attribute.
>>>
>>> A timer device could be triggered by other timers, we use the trigger
>>> name and is_stm32_iio_timer_trigger() function to distinguish them
>>> and configure IP input switch.
>> The presence of an IIO device in here was a suprise.. What is it actually for?
> 
> IIO device is needed to be able to valid the input triggers, which
> aren't the same than
> those generated by the device.
> Since I use triggers name to distinguish them I have introduced
> is_stm32_iio_timer_trigger()
> function to be sure that triggers are coming for a valid hardware and
> not from a fake one
> using the same name.
> 
>>
>> I think this needs some examples of usage to make it clear what the aim is.
> 
> In the hardware block there is switch in input to select which trigger
> will drive the IP.
> For example that allow to start multiple pwm exactly that the same
> time or to start/stop
> it on master edges.
Hmm. OK. We need to think about how to represent this concept of a tree 
of triggers - independent of having an IIO device as that is down right
misleading.

In the first instance the tree is full supported by this one driver I think?
If so lets use it as a testbed and try and put together a simple tree between
the triggers.

So the child triggers (started on the parent firing) can perhaps have a
'parent' attribute? (might be better naming possible!)

> 
>>
>> I was basically expecting to see a driver instantiating one iio trigger
>> per timer that can act as a trigger.  Those would each have sampling frequency
>> controls and basica enable / disable.
> 
> An hardware device could have up to 5 triggers: timX_trgo, timX_ch1, timX_ch2,
> timX_ch3, timX_ch4.
On a train so I don't have the datasheet.  Which of these would actually make
sense when driving an adc scan?
> Until now I have try to simplify the problem and just use timX_trgo trigger.
> I have added a "sampling_frequency" attribute on the trigger to
> control the frequence
> and I use trigger set_state function to enable disable it.
Wise move!  Makes sense to build this up in baby steps if possible.
> 
>>
>> I'm seeing something much more complex here so additional explanation is
>> needed.
>>>
>>> Timer may also decide on which event (edge, level) they could
>>> be activated by a trigger, this configuration is done by writing in
>>> "slave_mode" device attribute.
>> Really?  Sounds like magic numbers in sysfs which is never a good idea.
>> Please document those attributes / or break them up into elements that
>> don't require magic numbers.
> 
> I would like to use strings here, it is possible to use IIO_CONST_ATTR
> to describe them ?
If it's on the iio device use the iio_ext_info stuff that has support
for enums.  If you need this for the trigger feel free to add equivalent
support to the core as needed.

Note that we are still evolving IIO so if we need new stuff to support
your usecase, never be afraid of proposing it!  The only element
I am keen on is keeping anything that is opaque to drivers opaque 
unless there is a VERY good reason to do otherwise.  Mostly this
just means using access functions etc.  That makes messing around
with the core internals (as still happens from time to time) a lot
less painful!
> 
>>>
>>> Since triggers could also be used by DAC or ADC their names are defined
>>> in include/dt-bindings/iio/timer/st,stm32-iio-timer.h so those IPs will be able
>>> to configure themselves in valid_trigger function
>>>
>>> Trigger have a "sampling_frequency" attribute which allow to configure
>>> timer sampling frequency without using pwm interface
>>>
>>> version 2:
>>> - keep only one compatible
>> Hmm. I'm not sure I like this as such.  We are actually dealing with lots
>> of instances of a hardware block with only a small amount of shared
>> infrastrcuture (which is classic mfd teritory). So to my mind we
>> should have a separate device for each.
> 
> Registers mapping and offset are the same, from triggers point of view
> only the configuration of the input switch is different.
> 
>>
>>> - use st,input-triggers-names and st,output-triggers-names
>>>   to know which triggers are accepted and/or create by the device
>> I'm not following why we have this cascade setup?
>>
>> These are triggers, not devices in the IIO context.  We need some detailed
>> description of why you have it setup like this. This would include the
>> ABI with examples of how you are using it.
> 
> I had put example of usage on the cover letter, I will duplicate them
> in this commit
> message.
Ooops. I didn't ready that ;) Sorry.
> 
>>
>> Basically I don't currently understand what you are doing :(
>>
>>
>> Thanks,
>>
>> Jonathan
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>>> ---
>>>  drivers/iio/Kconfig                                |   2 +-
>>>  drivers/iio/Makefile                               |   1 +
>>>  drivers/iio/timer/Kconfig                          |  15 +
>>>  drivers/iio/timer/Makefile                         |   1 +
>>>  drivers/iio/timer/stm32-iio-timer.c                | 448 +++++++++++++++++++++
>>>  drivers/iio/trigger/Kconfig                        |   1 -
>>>  include/dt-bindings/iio/timer/st,stm32-iio-timer.h |  23 ++
>>>  include/linux/iio/timer/stm32-iio-timers.h         |  16 +
>>>  8 files changed, 505 insertions(+), 2 deletions(-)
>>>  create mode 100644 drivers/iio/timer/Kconfig
>>>  create mode 100644 drivers/iio/timer/Makefile
>>>  create mode 100644 drivers/iio/timer/stm32-iio-timer.c
>>>  create mode 100644 include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>>>  create mode 100644 include/linux/iio/timer/stm32-iio-timers.h
>>>
>>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>>> index 6743b18..2de2a80 100644
>>> --- a/drivers/iio/Kconfig
>>> +++ b/drivers/iio/Kconfig
>>> @@ -90,5 +90,5 @@ source "drivers/iio/potentiometer/Kconfig"
>>>  source "drivers/iio/pressure/Kconfig"
>>>  source "drivers/iio/proximity/Kconfig"
>>>  source "drivers/iio/temperature/Kconfig"
>>> -
>>> +source "drivers/iio/timer/Kconfig"
>>>  endif # IIO
>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>> index 87e4c43..b797c08 100644
>>> --- a/drivers/iio/Makefile
>>> +++ b/drivers/iio/Makefile
>>> @@ -32,4 +32,5 @@ obj-y += potentiometer/
>>>  obj-y += pressure/
>>>  obj-y += proximity/
>>>  obj-y += temperature/
>>> +obj-y += timer/
>>>  obj-y += trigger/
>>> diff --git a/drivers/iio/timer/Kconfig b/drivers/iio/timer/Kconfig
>>> new file mode 100644
>>> index 0000000..7a73bc6
>>> --- /dev/null
>>> +++ b/drivers/iio/timer/Kconfig
>>> @@ -0,0 +1,15 @@
>>> +#
>>> +# Timers drivers
>>> +
>>> +menu "Timers"
>>> +
>>> +config IIO_STM32_TIMER
>>> +     tristate "stm32 iio timer"
>>> +     depends on ARCH_STM32
>>> +     depends on OF
>>> +     select IIO_TRIGGERED_EVENT
>>> +     select MFD_STM32_GP_TIMER
>>> +     help
>>> +       Select this option to enable stm32 timers hardware IPs
>>> +
>>> +endmenu
>>> diff --git a/drivers/iio/timer/Makefile b/drivers/iio/timer/Makefile
>>> new file mode 100644
>>> index 0000000..a360c9f
>>> --- /dev/null
>>> +++ b/drivers/iio/timer/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-$(CONFIG_IIO_STM32_TIMER) += stm32-iio-timer.o
>>> diff --git a/drivers/iio/timer/stm32-iio-timer.c b/drivers/iio/timer/stm32-iio-timer.c
>>> new file mode 100644
>>> index 0000000..35f2687
>>> --- /dev/null
>>> +++ b/drivers/iio/timer/stm32-iio-timer.c
>>> @@ -0,0 +1,448 @@
>>> +/*
>>> + * stm32-iio-timer.c
>>> + *
>>> + * Copyright (C) STMicroelectronics 2016
>>> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
>>> + * License terms:  GNU General Public License (GPL), version 2
>>> + */
>>> +
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +#include <linux/iio/timer/stm32-iio-timers.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/triggered_event.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/mfd/stm32-gptimer.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#define DRIVER_NAME "stm32-iio-timer"
>>> +
>>> +struct stm32_iio_timer_dev {
>>> +     struct device *dev;
>>> +     struct regmap *regmap;
>>> +     struct clk *clk;
>>> +     int irq;
>>> +     bool own_timer;
>>> +     unsigned int sampling_frequency;
>>> +     struct iio_trigger *active_trigger;
>>> +};
>>> +
>>> +static ssize_t _store_frequency(struct device *dev,
>>> +                             struct device_attribute *attr,
>>> +                             const char *buf, size_t len)
>>> +{
>>> +     struct iio_trigger *trig = to_iio_trigger(dev);
>>> +     struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
>>> +     unsigned int freq;
>>> +     int ret;
>>> +
>>> +     ret = kstrtouint(buf, 10, &freq);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     stm32->sampling_frequency = freq;
>>> +
>>> +     return len;
>>> +}
>>> +
>>> +static ssize_t _read_frequency(struct device *dev,
>>> +                            struct device_attribute *attr, char *buf)
>>> +{
>>> +     struct iio_trigger *trig = to_iio_trigger(dev);
>>> +     struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
>>> +     unsigned long long freq = stm32->sampling_frequency;
>>> +     u32 psc, arr, cr1;
>>> +
>>> +     regmap_read(stm32->regmap, TIM_CR1, &cr1);
>>> +     regmap_read(stm32->regmap, TIM_PSC, &psc);
>>> +     regmap_read(stm32->regmap, TIM_ARR, &arr);
>>> +
>>> +     if (psc && arr && (cr1 & TIM_CR1_CEN)) {
>>> +             freq = (unsigned long long)clk_get_rate(stm32->clk);
>>> +             do_div(freq, psc);
>>> +             do_div(freq, arr);
>>> +     }
>>> +
>>> +     return sprintf(buf, "%d\n", (unsigned int)freq);
>>> +}
>>> +
>>> +static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
>>> +                           _read_frequency,
>>> +                           _store_frequency);
>>> +
>>> +static struct attribute *stm32_trigger_attrs[] = {
>>> +     &iio_dev_attr_sampling_frequency.dev_attr.attr,
>>> +     NULL,
>>> +};
>>> +
>>> +static const struct attribute_group stm32_trigger_attr_group = {
>>> +     .attrs = stm32_trigger_attrs,
>>> +};
>>> +
>>> +static const struct attribute_group *stm32_trigger_attr_groups[] = {
>>> +     &stm32_trigger_attr_group,
>>> +     NULL,
>>> +};
>>> +
>>> +static
>>> +ssize_t _show_master_mode(struct device *dev,
>>> +                       struct device_attribute *attr, char *buf)
>>> +{
>>> +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +     struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>>> +     u32 cr2;
>>> +
>>> +     regmap_read(stm32->regmap, TIM_CR2, &cr2);
>>> +
>>> +     return snprintf(buf, PAGE_SIZE, "%d\n", (cr2 >> 4) & 0x7);
>>> +}
>>> +
>>> +static
>>> +ssize_t _store_master_mode(struct device *dev,
>>> +                        struct device_attribute *attr,
>>> +                        const char *buf, size_t len)
>>> +{
>>> +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +     struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>>> +     u8 mode;
>>> +     int ret;
>>> +
>>> +     ret = kstrtou8(buf, 10, &mode);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (mode > 0x7)
>>> +             return -EINVAL;
>>> +
>>> +     regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, mode << 4);
>>> +
>>> +     return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(master_mode, S_IRUGO | S_IWUSR,
>>> +                    _show_master_mode,
>>> +                    _store_master_mode,
>>> +                    0);
>>> +
>>> +static
>>> +ssize_t _show_slave_mode(struct device *dev,
>>> +                      struct device_attribute *attr, char *buf)
>>> +{
>>> +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +     struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>>> +     u32 smcr;
>>> +
>>> +     regmap_read(stm32->regmap, TIM_SMCR, &smcr);
>>> +
>>> +     return snprintf(buf, PAGE_SIZE, "%d\n", smcr & 0x3);
>>> +}
>>> +
>>> +static
>>> +ssize_t _store_slave_mode(struct device *dev,
>>> +                       struct device_attribute *attr,
>>> +                       const char *buf, size_t len)
>>> +{
>>> +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +     struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>>> +     u8 mode;
>>> +     int ret;
>>> +
>>> +     ret = kstrtou8(buf, 10, &mode);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (mode > 0x7)
>>> +             return -EINVAL;
>> How is something called slave mode going to be fed a number between 0 and 7?
>> Rule of thumb is no magic numbers in sysfs and right now this is looking
>> rather cryptic to say the least!
> 
> I would like to use strings here, it is possible to use IIO_CONST_ATTR
> to describe them ?
> In documentation slave modes are describe that this:
> 000: Slave mode disabled - if CEN = ‘1’ then the prescaler is clocked
> directly by the internal clock.
> 001: Encoder mode 1 - Counter counts up/down on TI2FP1 edge depending
> on TI1FP2 level.
> 010: Encoder mode 2 - Counter counts up/down on TI1FP2 edge depending
> on TI2FP1 level.
> 011: Encoder mode 3 - Counter counts up/down on both TI1FP1 and TI2FP2
> edges depending on the level of the other input.
> 100: Reset Mode - Rising edge of the selected trigger input (TRGI)
> reinitializes the counter and generates an update of the registers.
> 101: Gated Mode - The counter clock is enabled when the trigger input
> (TRGI) is high.
>         The counter stops (but is not reset) as soon as the trigger becomes low.
>          Both start and stop of the counter are controlled.
> 110: Trigger Mode - The counter starts at a rising edge of the trigger
> TRGI (but it is notreset).
>          Only the start of the counter is controlled.
> 111: External Clock Mode 1 - Rising edges of the selected trigger
> (TRGI) clock the counter.
> 
>>> +
>>> +     regmap_update_bits(stm32->regmap, TIM_SMCR, TIM_SMCR_SMS, mode);
>>> +
>>> +     return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(slave_mode, S_IRUGO | S_IWUSR,
>> There is an iritating move (in terms of noise it's generating) to use values
>> directly instead fo these defines.  Still if you don't fix it here I'll only
>> get a patch 'fixing' it soon after...
> 
> I will fix at in version 3
> 
>>
>>> +                    _show_slave_mode,
>>> +                    _store_slave_mode,
>>> +                    0);
>>> +
>>> +static struct attribute *stm32_timer_attrs[] = {
>>> +     &iio_dev_attr_master_mode.dev_attr.attr,
>>> +     &iio_dev_attr_slave_mode.dev_attr.attr,
>> New ABI so must be documented under Documentation/ABI/testing/sysfs-bus-iio-*
> 
> OK
> 
>>> +     NULL,
>>> +};
>>> +
>>> +static const struct attribute_group stm32_timer_attr_group = {
>>> +     .attrs = stm32_timer_attrs,
>>> +};
>>> +
>>> +static int stm32_timer_start(struct stm32_iio_timer_dev *stm32)
>>> +{
>>> +     unsigned long long prd, div;
>>> +     int prescaler = 0;
>>> +     u32 max_arr = 0xFFFF, cr1;
>>> +
>>> +     if (stm32->sampling_frequency == 0)
>>> +             return 0;
>>> +
>>> +     /* Period and prescaler values depends of clock rate */
>>> +     div = (unsigned long long)clk_get_rate(stm32->clk);
>>> +
>>> +     do_div(div, stm32->sampling_frequency);
>>> +
>>> +     prd = div;
>>> +
>>> +     while (div > max_arr) {
>>> +             prescaler++;
>>> +             div = prd;
>>> +             do_div(div, (prescaler + 1));
>>> +     }
>>> +     prd = div;
>>> +
>>> +     if (prescaler > MAX_TIM_PSC) {
>>> +             dev_err(stm32->dev, "prescaler exceeds the maximum value\n");
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     /* Check that we own the timer */
>>> +     regmap_read(stm32->regmap, TIM_CR1, &cr1);
>>> +     if ((cr1 & TIM_CR1_CEN) && !stm32->own_timer)
>>> +             return -EBUSY;
>>> +
>>> +     if (!stm32->own_timer) {
>>> +             stm32->own_timer = true;
>>> +             clk_enable(stm32->clk);
>>> +     }
>>> +
>>> +     regmap_write(stm32->regmap, TIM_PSC, prescaler);
>>> +     regmap_write(stm32->regmap, TIM_ARR, prd - 1);
>>> +     regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
>>> +
>>> +     /* Force master mode to update mode */
>>> +     regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, 0x20);
>>> +
>>> +     /* Make sure that registers are updated */
>>> +     regmap_update_bits(stm32->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
>>> +
>>> +     /* Enable interrupt */
>>> +     regmap_write(stm32->regmap, TIM_SR, 0);
>>> +     regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, TIM_DIER_UIE);
>>> +
>>> +     /* Enable controller */
>>> +     regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int stm32_timer_stop(struct stm32_iio_timer_dev *stm32)
>>> +{
>>> +     if (!stm32->own_timer)
>>> +             return 0;
>>> +
>>> +     /* Stop timer */
>>> +     regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, 0);
>>> +     regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>>> +     regmap_write(stm32->regmap, TIM_PSC, 0);
>>> +     regmap_write(stm32->regmap, TIM_ARR, 0);
>>> +
>>> +     clk_disable(stm32->clk);
>>> +
>>> +     stm32->own_timer = false;
>>> +     stm32->active_trigger = NULL;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int stm32_set_trigger_state(struct iio_trigger *trig, bool state)
>>> +{
>>> +     struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
>>> +
>>> +     stm32->active_trigger = trig;
>>> +
>>> +     if (state)
>>> +             return stm32_timer_start(stm32);
>>> +     else
>>> +             return stm32_timer_stop(stm32);
>>> +}
>>> +
>>> +static irqreturn_t stm32_timer_irq_handler(int irq, void *private)
>>> +{
>>> +     struct stm32_iio_timer_dev *stm32 = private;
>>> +     u32 sr;
>>> +
>>> +     regmap_read(stm32->regmap, TIM_SR, &sr);
>>> +     regmap_write(stm32->regmap, TIM_SR, 0);
>>> +
>>> +     if ((sr & TIM_SR_UIF) && stm32->active_trigger)
>>> +             iio_trigger_poll(stm32->active_trigger);
>> This is acting like a trigger cascading off another trigger?
> 
> Not only a trigger but ADC or DAC too.
> 
>>
>> Normally this interrupt handler would be directly associated with the
>> trigger hardware - in this case the timer.
> 
>>> +
>>> +     return IRQ_HANDLED;
>>> +}
>>> +
>>> +static const struct iio_trigger_ops timer_trigger_ops = {
>>> +     .owner = THIS_MODULE,
>>> +     .set_trigger_state = stm32_set_trigger_state,
>>> +};
>>> +
>>> +static int stm32_setup_iio_triggers(struct stm32_iio_timer_dev *stm32)
>>> +{
>>> +     int ret;
>>> +     struct property *p;
>>> +     const char *cur = NULL;
>>> +
>>> +     p = of_find_property(stm32->dev->of_node,
>>> +                          "st,output-triggers-names", NULL);
>>> +
>>> +     while ((cur = of_prop_next_string(p, cur)) != NULL) {
>>> +             struct iio_trigger *trig;
>>> +
>>> +             trig = devm_iio_trigger_alloc(stm32->dev, "%s", cur);
>>> +             if  (!trig)
>>> +                     return -ENOMEM;
>>> +
>>> +             trig->dev.parent = stm32->dev->parent;
>>> +             trig->ops = &timer_trigger_ops;
>>> +             trig->dev.groups = stm32_trigger_attr_groups;
>>> +             iio_trigger_set_drvdata(trig, stm32);
>>> +
>>> +             ret = devm_iio_trigger_register(stm32->dev, trig);
>>> +             if (ret)
>>> +                     return ret;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +/**
>>> + * is_stm32_iio_timer_trigger
>>> + * @trig: trigger to be checked
>>> + *
>>> + * return true if the trigger is a valid stm32 iio timer trigger
>>> + * either return false
>>> + */
>>> +bool is_stm32_iio_timer_trigger(struct iio_trigger *trig)
>>> +{
>>> +     return (trig->ops == &timer_trigger_ops);
>>> +}
>>> +EXPORT_SYMBOL(is_stm32_iio_timer_trigger);
>>> +
>>> +static int stm32_validate_trigger(struct iio_dev *indio_dev,
>>> +                               struct iio_trigger *trig)
>>> +{
>>> +     struct stm32_iio_timer_dev *dev = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     if (!is_stm32_iio_timer_trigger(trig))
>>> +             return -EINVAL;
>>> +
>>> +     ret = of_property_match_string(dev->dev->of_node,
>>> +                                    "st,input-triggers-names",
>>> +                                    trig->name);
>>> +
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     regmap_update_bits(dev->regmap, TIM_SMCR, TIM_SMCR_TS, ret << 4);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct iio_info stm32_trigger_info = {
>>> +     .driver_module = THIS_MODULE,
>>> +     .validate_trigger = stm32_validate_trigger,
>>> +     .attrs = &stm32_timer_attr_group,
>>> +};
>>> +
>>> +static struct stm32_iio_timer_dev *stm32_setup_iio_device(struct device *dev)
>>> +{
>>> +     struct iio_dev *indio_dev;
>>> +     int ret;
>>> +
>>> +     indio_dev = devm_iio_device_alloc(dev, sizeof(struct stm32_iio_timer_dev));
>>> +     if (!indio_dev)
>>> +             return NULL;
>> This is 'unusual'.  Why does a trigger driver need an iio_dev at all?
> 
> Trigger doesn't need it but for configuring the input switch when
> validating the triggers I need a device
As suggested above, lets pull this trigger cascade clear of involving devices
at all.  It's nice functionality to have anyway.  Once we've figured it
out for this driver, I'd like to generalize it as I think the same stuff could
be used to do clean setup of oscilloscope sampling approaches for more
complex sensor setups...
> 
>>> +
>>> +     indio_dev->name = dev_name(dev);
>>> +     indio_dev->dev.parent = dev;
>>> +     indio_dev->info = &stm32_trigger_info;
>>> +     indio_dev->modes = INDIO_EVENT_TRIGGERED;
>>> +     indio_dev->num_channels = 0;
>>> +     indio_dev->dev.of_node = dev->of_node;
>>> +
>>> +     ret = iio_triggered_event_setup(indio_dev,
>>> +                                     NULL,
>>> +                                     stm32_timer_irq_handler);
>> So the iio_dev exists to provide the ability to fire this interrupt from
>> another trigger?  Why do you want to do this?
> 
> I need interrupt because I use set_trigger_state() to enable/disable
> the sampling frequency.
> As far I understand and test set_trigger_state() is only called when
> indio_dev->modes = INDIO_EVENT_TRIGGERED
> and iio_triggered_event_setup have been called to create register an
> irq handler.
> I just need irq declaration for this last condition, I don't need the
> irq to fire in kernel to drive other hardware block.
> 
> If I could use set_trigger_state() without calling using
> iio_triggered_event_setup() I should remove all
> irq code from the driver.
> 
> One possible solution would be to add calls to set_trigger_state() in
> iio_trigger_write_current() function
> at the same level than iio_trigger_detach_poll_func() or
> iio_trigger_attach_poll_func() calls:
I fear this may introduce race conditions in drivers that assume this stuff
can't change whilst the trigger is enabled.

Bit too risky a change to my mind.

If you need to add functions to explicitly do such a trigger enable, then
feel free to propose them.  I never have a problem with adding core
functionality if that is the best way to solve a particular issue.
(subject to standard questions of maintainability and insisting they have
good documentation  - do as I say, not as I did years ago ;)
> 
> if (indio_dev->modes = INDIO_DIRECT_MODE && oldtrig->ops->set_trigger_state)
>      oldtrig->ops->set_trigger_state(oldtrig, false);
> 
> if (indio_dev->modes = INDIO_DIRECT_MODE &&
> indio_dev->trig->ops->set_trigger_state)
>      indio_dev->trig->ops->set_trigger_state(indio_dev->trig, true);
> 
> I'm to new in IIO framework to understand if that it possible or not
> 
>>> +     if (ret)
>>> +             return NULL;
>>> +
>>> +     ret = devm_iio_device_register(dev, indio_dev);
>>> +     if (ret) {
>>> +             iio_triggered_event_cleanup(indio_dev);
>>> +             return NULL;
>>> +     }
>>> +
>>> +     return iio_priv(indio_dev);
>>> +}
>>> +
>>> +static int stm32_iio_timer_probe(struct platform_device *pdev)
>>> +{
>>> +     struct device *dev = &pdev->dev;
>>> +     struct stm32_iio_timer_dev *stm32;
>>> +     struct stm32_gptimer_dev *mfd = dev_get_drvdata(pdev->dev.parent);
>>> +     int ret;
>>> +
>>> +     stm32 = stm32_setup_iio_device(dev);
>>> +     if (!stm32)
>>> +             return -ENOMEM;
>>> +
>>> +     stm32->dev = dev;
>>> +     stm32->regmap = mfd->regmap;
>>> +     stm32->clk = mfd->clk;
>>> +
>>> +     stm32->irq = platform_get_irq(pdev, 0);
>>> +     if (stm32->irq < 0)
>>> +             return -EINVAL;
>>> +
>>> +     ret = devm_request_irq(stm32->dev, stm32->irq,
>>> +                            stm32_timer_irq_handler, IRQF_SHARED,
>>> +                            "iiotimer_event", stm32);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = stm32_setup_iio_triggers(stm32);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     platform_set_drvdata(pdev, stm32);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int stm32_iio_timer_remove(struct platform_device *pdev)
>>> +{
>>> +     struct stm32_iio_timer_dev *stm32 = platform_get_drvdata(pdev);
>>> +
>>> +     iio_triggered_event_cleanup((struct iio_dev *)stm32);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct of_device_id stm32_trig_of_match[] = {
>>> +     {
>>> +             .compatible = "st,stm32-iio-timer",
>>> +     },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
>>> +
>>> +static struct platform_driver stm32_iio_timer_driver = {
>>> +     .probe = stm32_iio_timer_probe,
>>> +     .remove = stm32_iio_timer_remove,
>>> +     .driver = {
>>> +             .name = DRIVER_NAME,
>>> +             .of_match_table = stm32_trig_of_match,
>>> +     },
>>> +};
>>> +module_platform_driver(stm32_iio_timer_driver);
>>> +
>>> +MODULE_ALIAS("platform:" DRIVER_NAME);
>>> +MODULE_DESCRIPTION("STMicroelectronics STM32 iio timer driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
>>> index 809b2e7..f2af4fe 100644
>>> --- a/drivers/iio/trigger/Kconfig
>>> +++ b/drivers/iio/trigger/Kconfig
>>> @@ -46,5 +46,4 @@ config IIO_SYSFS_TRIGGER
>>>
>>>         To compile this driver as a module, choose M here: the
>>>         module will be called iio-trig-sysfs.
>>> -
>> Clear this out...
>>>  endmenu
>>> diff --git a/include/dt-bindings/iio/timer/st,stm32-iio-timer.h b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>>> new file mode 100644
>>> index 0000000..d39bf16
>>> --- /dev/null
>>> +++ b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>>> @@ -0,0 +1,23 @@
>>> +/*
>>> + * st,stm32-iio-timer.h
>>> + *
>>> + * Copyright (C) STMicroelectronics 2016
>>> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
>>> + * License terms:  GNU General Public License (GPL), version 2
>>> + */
>>> +
>>> +#ifndef _DT_BINDINGS_IIO_TIMER_H_
>>> +#define _DT_BINDINGS_IIO_TIMER_H_
>>> +
>>> +#define TIM1_TRGO    "tim1_trgo"
>>> +#define TIM2_TRGO    "tim2_trgo"
>>> +#define TIM3_TRGO    "tim3_trgo"
>>> +#define TIM4_TRGO    "tim4_trgo"
>>> +#define TIM5_TRGO    "tim5_trgo"
>>> +#define TIM6_TRGO    "tim6_trgo"
>>> +#define TIM7_TRGO    "tim7_trgo"
>>> +#define TIM8_TRGO    "tim8_trgo"
>>> +#define TIM9_TRGO    "tim9_trgo"
>>> +#define TIM12_TRGO   "tim12_trgo"
>>> +
>>> +#endif
>>> diff --git a/include/linux/iio/timer/stm32-iio-timers.h b/include/linux/iio/timer/stm32-iio-timers.h
>>> new file mode 100644
>>> index 0000000..5d1b86c
>>> --- /dev/null
>>> +++ b/include/linux/iio/timer/stm32-iio-timers.h
>>> @@ -0,0 +1,16 @@
>>> +/*
>>> + * stm32-iio-timers.h
>>> + *
>>> + * Copyright (C) STMicroelectronics 2016
>>> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
>>> + * License terms:  GNU General Public License (GPL), version 2
>>> + */
>>> +
>>> +#ifndef _STM32_IIO_TIMERS_H_
>>> +#define _STM32_IIO_TIMERS_H_
>>> +
>>> +#include <dt-bindings/iio/timer/st,stm32-iio-timer.h>
>>> +
>>> +bool is_stm32_iio_timer_trigger(struct iio_trigger *trig);
>>> +
>>> +#endif
>>>
>>