[PATCH 0/4] Add STM32H7 DAC driver

[PATCH 0/4] Add STM32H7 DAC driver

[Fabrice Gasnier]

This patchset adds support for the STM32H7 DAC controller

It's a 12-bit, voltage output digital-to-analog converter. It has two
output channels, each with its own converter, trigger sources and
waveform generator.

Each channel can be used independently, so common resources are managed
in stm32-dac-core driver (e.g. clock, reset, regulator, registers).
One IIO device is instantiated per DAC output channel, in stm32-dac
driver, so each channel can have its own trigger.

Please find bellow basic examples, using this driver to:
- generate DC voltage output on channel1
- generate a triangle waveform on channel2

# set max DC voltage / enable / min DC voltage / disable on out1:
cd /sys/bus/iio/devices/iio\:device0
echo 4095 > out_voltage1_raw 
echo 1 > out_voltage1_en
echo 0 > out_voltage1_raw
echo 0 > out_voltage1_en

# configure timer trigger, and set triangle waveform with max
# amplitude on out2:
cd /sys/bus/iio/devices/trigger9
cat name
tim2_trgo
echo 10000 > sampling_frequency

cd /sys/bus/iio/devices/iio\:device1
echo triangle > wave
echo 11 > mamp
echo tim2_trgo > trigger/current_trigger
echo 1 > out_voltage2_en

Fabrice Gasnier (4):
  dt-bindings: iio: stm32-dac: Add support for STM32 DAC
  iio: dac: add support for stm32 DAC
  iio: dac: stm32: add support for trigger events
  iio: dac: stm32: add support for waveform generator

 Documentation/ABI/testing/sysfs-bus-iio-dac-stm32  |  32 ++
 .../devicetree/bindings/iio/dac/st,stm32-dac.txt   |  56 +++
 drivers/iio/dac/Kconfig                            |  18 +
 drivers/iio/dac/Makefile                           |   2 +
 drivers/iio/dac/stm32-dac-core.c                   | 180 +++++++
 drivers/iio/dac/stm32-dac-core.h                   |  63 +++
 drivers/iio/dac/stm32-dac.c                        | 538 +++++++++++++++++++++
 7 files changed, 889 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
 create mode 100644 Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
 create mode 100644 drivers/iio/dac/stm32-dac-core.c
 create mode 100644 drivers/iio/dac/stm32-dac-core.h
 create mode 100644 drivers/iio/dac/stm32-dac.c

-- 
1.9.1

[PATCH 1/4] dt-bindings: iio: stm32-dac: Add support for STM32 DAC

[Fabrice Gasnier]

Document STMicroelectronics STM32 DAC (digital-to-analog converter).

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 .../devicetree/bindings/iio/dac/st,stm32-dac.txt   | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt

diff --git a/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
new file mode 100644
index 0000000..1981983
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
@@ -0,0 +1,56 @@
+STMicroelectronics STM32 DAC
+
+The STM32 DAC is a 12-bit voltage output digital-to-analog converter. The DAC
+can be configured in 8- or 12-bit mode. In 12-bit mode, the data could be
+left- or right-aligned. It has two output channels, each with its own converter.
+It has built-in noise and triangle waveform generator and supports external
+triggers for conversions. The DAC's output buffer allows a high drive output
+current.
+
+Contents of a stm32 dac root node:
+-----------------------------------
+Required properties:
+- compatible: Must be "st,stm32h7-dac-core".
+- reg: Offset and length of the device's register set.
+- clocks: Must contain an entry for pclk (which feeds the peripheral bus
+  interface)
+- clock-names: Must be "pclk".
+- vref-supply: Phandle to the vref+ input analog reference supply.
+
+Optional properties:
+- resets: Must contain the phandle to the reset controller.
+- A pinctrl state named "default" for each DAC channel may be defined to set
+  DAC_OUTx pin in mode of operation for analog output on external pin.
+
+Contents of a stm32 dac child node:
+-----------------------------------
+DAC core node should contain at least one subnode, representing a
+DAC instance/channel available on the machine.
+
+Required properties:
+- compatible: Must be "st,stm32-dac".
+- #io-channel-cells = <1>: See the IIO bindings section "IIO consumers" in
+  Documentation/devicetree/bindings/iio/iio-bindings.txt
+- st,dac-channel: Must be either 1 or 2, to define channel in use (e.g.
+  single channels: 1 or 2)
+
+Example:
+	dac: dac@40007400 {
+		compatible = "st,stm32h7-dac-core";
+		reg = <0x40007400 0x400>;
+		clocks = <&clk>;
+		clock-names = "pclk";
+		vref-supply = <&reg_vref>;
+
+		dac1: dac@1 {
+			compatible = "st,stm32-dac";
+			#io-channels-cells = <1>;
+			st,dac-channel = <1>;
+		};
+
+		dac2: dac@2 {
+			compatible = "st,stm32-dac";
+			#io-channels-cells = <1>;
+			st,dac-channel = <2>;
+		};
+	};
-- 
1.9.1

Re: [PATCH 1/4] dt-bindings: iio: stm32-dac: Add support for STM32 DAC

[Jonathan Cameron]

On 31/03/17 12:45, Fabrice Gasnier wrote:
> Document STMicroelectronics STM32 DAC (digital-to-analog converter).
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
>  .../devicetree/bindings/iio/dac/st,stm32-dac.txt   | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
> new file mode 100644
> index 0000000..1981983
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
> @@ -0,0 +1,56 @@
> +STMicroelectronics STM32 DAC
> +
> +The STM32 DAC is a 12-bit voltage output digital-to-analog converter. The DAC
> +can be configured in 8- or 12-bit mode. In 12-bit mode, the data could be
> +left- or right-aligned.
Whilst possibly true, do we care about the alignment?  That'll all get wrapped
up in the driver.

> It has two output channels, each with its own converter.
> +It has built-in noise and triangle waveform generator and supports external
> +triggers for conversions. The DAC's output buffer allows a high drive output
> +current.
Ah.. This is going to be fun :) More unusual hardware to find in an SoC.
> +
> +Contents of a stm32 dac root node:
> +-----------------------------------
> +Required properties:
> +- compatible: Must be "st,stm32h7-dac-core".
> +- reg: Offset and length of the device's register set.
> +- clocks: Must contain an entry for pclk (which feeds the peripheral bus
> +  interface)
> +- clock-names: Must be "pclk".
> +- vref-supply: Phandle to the vref+ input analog reference supply.
> +
> +Optional properties:
> +- resets: Must contain the phandle to the reset controller.
> +- A pinctrl state named "default" for each DAC channel may be defined to set
> +  DAC_OUTx pin in mode of operation for analog output on external pin.
> +
> +Contents of a stm32 dac child node:
> +-----------------------------------
> +DAC core node should contain at least one subnode, representing a
> +DAC instance/channel available on the machine.
> +
> +Required properties:
> +- compatible: Must be "st,stm32-dac".
> +- #io-channel-cells = <1>: See the IIO bindings section "IIO consumers" in
> +  Documentation/devicetree/bindings/iio/iio-bindings.txt
> +- st,dac-channel: Must be either 1 or 2, to define channel in use (e.g.
> +  single channels: 1 or 2)
> +
> +Example:
> +	dac: dac@40007400 {
> +		compatible = "st,stm32h7-dac-core";
> +		reg = <0x40007400 0x400>;
> +		clocks = <&clk>;
> +		clock-names = "pclk";
> +		vref-supply = <&reg_vref>;
> +
> +		dac1: dac@1 {
> +			compatible = "st,stm32-dac";
> +			#io-channels-cells = <1>;
> +			st,dac-channel = <1>;
> +		};
> +
> +		dac2: dac@2 {
> +			compatible = "st,stm32-dac";
> +			#io-channels-cells = <1>;
> +			st,dac-channel = <2>;
> +		};
> +	};
> 

Re: [PATCH 1/4] dt-bindings: iio: stm32-dac: Add support for STM32 DAC

[Fabrice Gasnier]

On 04/02/2017 01:16 PM, Jonathan Cameron wrote:
> On 31/03/17 12:45, Fabrice Gasnier wrote:
>> Document STMicroelectronics STM32 DAC (digital-to-analog converter).
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> ---
>>  .../devicetree/bindings/iio/dac/st,stm32-dac.txt   | 56 ++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
>> new file mode 100644
>> index 0000000..1981983
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
>> @@ -0,0 +1,56 @@
>> +STMicroelectronics STM32 DAC
>> +
>> +The STM32 DAC is a 12-bit voltage output digital-to-analog converter. The DAC
>> +can be configured in 8- or 12-bit mode. In 12-bit mode, the data could be
>> +left- or right-aligned.
> Whilst possibly true, do we care about the alignment?  That'll all get wrapped
> up in the driver.
Hi Jonathan,

I'll update this description in V2

> 
>> It has two output channels, each with its own converter.
>> +It has built-in noise and triangle waveform generator and supports external
>> +triggers for conversions. The DAC's output buffer allows a high drive output
>> +current.
> Ah.. This is going to be fun :) More unusual hardware to find in an SoC.
Yes, let's discuss this on following patches.

Thanks your review,
Best Regards,
Fabrice

>> +
>> +Contents of a stm32 dac root node:
>> +-----------------------------------
>> +Required properties:
>> +- compatible: Must be "st,stm32h7-dac-core".
>> +- reg: Offset and length of the device's register set.
>> +- clocks: Must contain an entry for pclk (which feeds the peripheral bus
>> +  interface)
>> +- clock-names: Must be "pclk".
>> +- vref-supply: Phandle to the vref+ input analog reference supply.
>> +
>> +Optional properties:
>> +- resets: Must contain the phandle to the reset controller.
>> +- A pinctrl state named "default" for each DAC channel may be defined to set
>> +  DAC_OUTx pin in mode of operation for analog output on external pin.
>> +
>> +Contents of a stm32 dac child node:
>> +-----------------------------------
>> +DAC core node should contain at least one subnode, representing a
>> +DAC instance/channel available on the machine.
>> +
>> +Required properties:
>> +- compatible: Must be "st,stm32-dac".
>> +- #io-channel-cells = <1>: See the IIO bindings section "IIO consumers" in
>> +  Documentation/devicetree/bindings/iio/iio-bindings.txt
>> +- st,dac-channel: Must be either 1 or 2, to define channel in use (e.g.
>> +  single channels: 1 or 2)
>> +
>> +Example:
>> +	dac: dac@40007400 {
>> +		compatible = "st,stm32h7-dac-core";
>> +		reg = <0x40007400 0x400>;
>> +		clocks = <&clk>;
>> +		clock-names = "pclk";
>> +		vref-supply = <&reg_vref>;
>> +
>> +		dac1: dac@1 {
>> +			compatible = "st,stm32-dac";
>> +			#io-channels-cells = <1>;
>> +			st,dac-channel = <1>;
>> +		};
>> +
>> +		dac2: dac@2 {
>> +			compatible = "st,stm32-dac";
>> +			#io-channels-cells = <1>;
>> +			st,dac-channel = <2>;
>> +		};
>> +	};
>>
> 

Re: [PATCH 1/4] dt-bindings: iio: stm32-dac: Add support for STM32 DAC

[Rob Herring]

On Fri, Mar 31, 2017 at 01:45:04PM +0200, Fabrice Gasnier wrote:
> Document STMicroelectronics STM32 DAC (digital-to-analog converter).
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
>  .../devicetree/bindings/iio/dac/st,stm32-dac.txt   | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
> new file mode 100644
> index 0000000..1981983
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
> @@ -0,0 +1,56 @@
> +STMicroelectronics STM32 DAC
> +
> +The STM32 DAC is a 12-bit voltage output digital-to-analog converter. The DAC
> +can be configured in 8- or 12-bit mode. In 12-bit mode, the data could be
> +left- or right-aligned. It has two output channels, each with its own converter.
> +It has built-in noise and triangle waveform generator and supports external
> +triggers for conversions. The DAC's output buffer allows a high drive output
> +current.
> +
> +Contents of a stm32 dac root node:
> +-----------------------------------
> +Required properties:
> +- compatible: Must be "st,stm32h7-dac-core".
> +- reg: Offset and length of the device's register set.
> +- clocks: Must contain an entry for pclk (which feeds the peripheral bus
> +  interface)
> +- clock-names: Must be "pclk".
> +- vref-supply: Phandle to the vref+ input analog reference supply.
> +
> +Optional properties:
> +- resets: Must contain the phandle to the reset controller.
> +- A pinctrl state named "default" for each DAC channel may be defined to set
> +  DAC_OUTx pin in mode of operation for analog output on external pin.
> +
> +Contents of a stm32 dac child node:
> +-----------------------------------
> +DAC core node should contain at least one subnode, representing a
> +DAC instance/channel available on the machine.
> +
> +Required properties:
> +- compatible: Must be "st,stm32-dac".
> +- #io-channel-cells = <1>: See the IIO bindings section "IIO consumers" in
> +  Documentation/devicetree/bindings/iio/iio-bindings.txt
> +- st,dac-channel: Must be either 1 or 2, to define channel in use (e.g.
> +  single channels: 1 or 2)

Use "reg" instead.

> +
> +Example:
> +	dac: dac@40007400 {
> +		compatible = "st,stm32h7-dac-core";
> +		reg = <0x40007400 0x400>;
> +		clocks = <&clk>;
> +		clock-names = "pclk";
> +		vref-supply = <&reg_vref>;
> +
> +		dac1: dac@1 {
> +			compatible = "st,stm32-dac";
> +			#io-channels-cells = <1>;
> +			st,dac-channel = <1>;
> +		};
> +
> +		dac2: dac@2 {
> +			compatible = "st,stm32-dac";
> +			#io-channels-cells = <1>;
> +			st,dac-channel = <2>;
> +		};
> +	};
> -- 
> 1.9.1
> 

Re: [PATCH 1/4] dt-bindings: iio: stm32-dac: Add support for STM32 DAC

[Fabrice Gasnier]

On 04/03/2017 06:42 PM, Rob Herring wrote:
> On Fri, Mar 31, 2017 at 01:45:04PM +0200, Fabrice Gasnier wrote:
>> Document STMicroelectronics STM32 DAC (digital-to-analog converter).
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> ---
>>  .../devicetree/bindings/iio/dac/st,stm32-dac.txt   | 56 ++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
>> new file mode 100644
>> index 0000000..1981983
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
>> @@ -0,0 +1,56 @@
>> +STMicroelectronics STM32 DAC
>> +
>> +The STM32 DAC is a 12-bit voltage output digital-to-analog converter. The DAC
>> +can be configured in 8- or 12-bit mode. In 12-bit mode, the data could be
>> +left- or right-aligned. It has two output channels, each with its own converter.
>> +It has built-in noise and triangle waveform generator and supports external
>> +triggers for conversions. The DAC's output buffer allows a high drive output
>> +current.
>> +
>> +Contents of a stm32 dac root node:
>> +-----------------------------------
>> +Required properties:
>> +- compatible: Must be "st,stm32h7-dac-core".
>> +- reg: Offset and length of the device's register set.
>> +- clocks: Must contain an entry for pclk (which feeds the peripheral bus
>> +  interface)
>> +- clock-names: Must be "pclk".
>> +- vref-supply: Phandle to the vref+ input analog reference supply.
>> +
>> +Optional properties:
>> +- resets: Must contain the phandle to the reset controller.
>> +- A pinctrl state named "default" for each DAC channel may be defined to set
>> +  DAC_OUTx pin in mode of operation for analog output on external pin.
>> +
>> +Contents of a stm32 dac child node:
>> +-----------------------------------
>> +DAC core node should contain at least one subnode, representing a
>> +DAC instance/channel available on the machine.
>> +
>> +Required properties:
>> +- compatible: Must be "st,stm32-dac".
>> +- #io-channel-cells = <1>: See the IIO bindings section "IIO consumers" in
>> +  Documentation/devicetree/bindings/iio/iio-bindings.txt
>> +- st,dac-channel: Must be either 1 or 2, to define channel in use (e.g.
>> +  single channels: 1 or 2)
> 
> Use "reg" instead.

Hi Rob,

Thanks your reviewing. I'll update this in V2

Best Regards,
Fabrice

> 
>> +
>> +Example:
>> +	dac: dac@40007400 {
>> +		compatible = "st,stm32h7-dac-core";
>> +		reg = <0x40007400 0x400>;
>> +		clocks = <&clk>;
>> +		clock-names = "pclk";
>> +		vref-supply = <&reg_vref>;
>> +
>> +		dac1: dac@1 {
>> +			compatible = "st,stm32-dac";
>> +			#io-channels-cells = <1>;
>> +			st,dac-channel = <1>;
>> +		};
>> +
>> +		dac2: dac@2 {
>> +			compatible = "st,stm32-dac";
>> +			#io-channels-cells = <1>;
>> +			st,dac-channel = <2>;
>> +		};
>> +	};
>> -- 
>> 1.9.1
>>

[PATCH 2/4] iio: dac: add support for stm32 DAC

[Fabrice Gasnier]

Add support for STMicroelectronics STM32 DAC. It's a 12-bit, voltage
output digital-to-analog converter. It has two output channels, each
with its own converter.
It supports 8 bits or 12bits left/right aligned data format. Only
12bits right-aligned is used here. It has built-in noise or
triangle waveform generator, and supports external triggers for
conversions.
Each channel can be used independently, with separate trigger, then
separate IIO devices are used to handle this. Core driver is intended
to share common resources such as clock, reset, reference voltage and
registers.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/iio/dac/Kconfig          |  15 ++
 drivers/iio/dac/Makefile         |   2 +
 drivers/iio/dac/stm32-dac-core.c | 180 ++++++++++++++++++++++++
 drivers/iio/dac/stm32-dac-core.h |  51 +++++++
 drivers/iio/dac/stm32-dac.c      | 296 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 544 insertions(+)
 create mode 100644 drivers/iio/dac/stm32-dac-core.c
 create mode 100644 drivers/iio/dac/stm32-dac-core.h
 create mode 100644 drivers/iio/dac/stm32-dac.c

diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index d3084028..7198648 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -274,6 +274,21 @@ config MCP4922
 	  To compile this driver as a module, choose M here: the module
 	  will be called mcp4922.
 
+config STM32_DAC
+	tristate "STMicroelectronics STM32 DAC"
+	depends on (ARCH_STM32 && OF) || COMPILE_TEST
+	depends on REGULATOR
+	select STM32_DAC_CORE
+	help
+	  Say yes here to build support for STMicroelectronics STM32 Digital
+	  to Analog Converter (DAC).
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called stm32-dac.
+
+config STM32_DAC_CORE
+	tristate
+
 config VF610_DAC
 	tristate "Vybrid vf610 DAC driver"
 	depends on OF
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index f01bf4a..afe8ae7 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -29,4 +29,6 @@ obj-$(CONFIG_MAX517) += max517.o
 obj-$(CONFIG_MAX5821) += max5821.o
 obj-$(CONFIG_MCP4725) += mcp4725.o
 obj-$(CONFIG_MCP4922) += mcp4922.o
+obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
+obj-$(CONFIG_STM32_DAC) += stm32-dac.o
 obj-$(CONFIG_VF610_DAC) += vf610_dac.o
diff --git a/drivers/iio/dac/stm32-dac-core.c b/drivers/iio/dac/stm32-dac-core.c
new file mode 100644
index 0000000..75e4878
--- /dev/null
+++ b/drivers/iio/dac/stm32-dac-core.c
@@ -0,0 +1,180 @@
+/*
+ * This file is part of STM32 DAC driver
+ *
+ * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
+ * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
+ *
+ * License type: GPLv2
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+
+#include "stm32-dac-core.h"
+
+/**
+ * struct stm32_dac_priv - stm32 DAC core private data
+ * @pclk:		peripheral clock common for all DACs
+ * @rst:		peripheral reset control
+ * @vref:		regulator reference
+ * @common:		Common data for all DAC instances
+ */
+struct stm32_dac_priv {
+	struct clk *pclk;
+	struct reset_control *rst;
+	struct regulator *vref;
+	struct stm32_dac_common common;
+};
+
+static struct stm32_dac_priv *to_stm32_dac_priv(struct stm32_dac_common *com)
+{
+	return container_of(com, struct stm32_dac_priv, common);
+}
+
+static const struct regmap_config stm32_dac_regmap_cfg = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = sizeof(u32),
+	.max_register = 0x3fc,
+};
+
+static int stm32_dac_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct stm32_dac_priv *priv;
+	struct regmap *regmap;
+	struct resource *res;
+	void __iomem *mmio;
+	int ret;
+
+	if (!dev->of_node)
+		return -ENODEV;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mmio = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mmio))
+		return PTR_ERR(mmio);
+
+	regmap = devm_regmap_init_mmio(dev, mmio, &stm32_dac_regmap_cfg);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+	priv->common.regmap = regmap;
+
+	priv->vref = devm_regulator_get(dev, "vref");
+	if (IS_ERR(priv->vref)) {
+		ret = PTR_ERR(priv->vref);
+		dev_err(dev, "vref get failed, %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_enable(priv->vref);
+	if (ret < 0) {
+		dev_err(dev, "vref enable failed\n");
+		return ret;
+	}
+
+	ret = regulator_get_voltage(priv->vref);
+	if (ret < 0) {
+		dev_err(dev, "vref get voltage failed, %d\n", ret);
+		goto err_vref;
+	}
+	priv->common.vref_mv = ret / 1000;
+	dev_dbg(dev, "vref+=%dmV\n", priv->common.vref_mv);
+
+	priv->pclk = devm_clk_get(dev, "pclk");
+	if (IS_ERR(priv->pclk)) {
+		ret = PTR_ERR(priv->pclk);
+		dev_err(dev, "pclk get failed\n");
+		goto err_vref;
+	}
+
+	ret = clk_prepare_enable(priv->pclk);
+	if (ret < 0) {
+		dev_err(dev, "pclk enable failed\n");
+		goto err_vref;
+	}
+
+	priv->rst = devm_reset_control_get(dev, NULL);
+	if (!IS_ERR(priv->rst)) {
+		reset_control_assert(priv->rst);
+		udelay(2);
+		reset_control_deassert(priv->rst);
+	}
+
+	/* When clock speed is higher than 80MHz, set HFSEL */
+	priv->common.hfsel = (clk_get_rate(priv->pclk) > 80000000UL);
+	ret = regmap_update_bits(regmap, STM32_DAC_CR, STM32H7_DAC_CR_HFSEL,
+				 priv->common.hfsel ? STM32H7_DAC_CR_HFSEL : 0);
+	if (ret)
+		goto err_pclk;
+
+	platform_set_drvdata(pdev, &priv->common);
+
+	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to populate DT children\n");
+		goto err_pclk;
+	}
+
+	return 0;
+
+err_pclk:
+	clk_disable_unprepare(priv->pclk);
+err_vref:
+	regulator_disable(priv->vref);
+
+	return ret;
+}
+
+static int stm32_dac_remove(struct platform_device *pdev)
+{
+	struct stm32_dac_common *common = platform_get_drvdata(pdev);
+	struct stm32_dac_priv *priv = to_stm32_dac_priv(common);
+
+	of_platform_depopulate(&pdev->dev);
+	clk_disable_unprepare(priv->pclk);
+	regulator_disable(priv->vref);
+
+	return 0;
+}
+
+static const struct of_device_id stm32_dac_of_match[] = {
+	{ .compatible = "st,stm32h7-dac-core", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
+
+static struct platform_driver stm32_dac_driver = {
+	.probe = stm32_dac_probe,
+	.remove = stm32_dac_remove,
+	.driver = {
+		.name = "stm32-dac-core",
+		.of_match_table = stm32_dac_of_match,
+	},
+};
+module_platform_driver(stm32_dac_driver);
+
+MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
+MODULE_DESCRIPTION("STMicroelectronics STM32 DAC core driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:stm32-dac-core");
diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
new file mode 100644
index 0000000..d3099f7
--- /dev/null
+++ b/drivers/iio/dac/stm32-dac-core.h
@@ -0,0 +1,51 @@
+/*
+ * This file is part of STM32 DAC driver
+ *
+ * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
+ * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
+ *
+ * License type: GPLv2
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __STM32_DAC_CORE_H
+#define __STM32_DAC_CORE_H
+
+#include <linux/regmap.h>
+
+/* STM32 DAC registers */
+#define STM32_DAC_CR		0x00
+#define STM32_DAC_DHR12R1	0x08
+#define STM32_DAC_DHR12R2	0x14
+#define STM32_DAC_DOR1		0x2C
+#define STM32_DAC_DOR2		0x30
+
+/* STM32_DAC_CR bit fields */
+#define STM32_DAC_CR_EN1		BIT(0)
+#define STM32H7_DAC_CR_HFSEL		BIT(15)
+#define STM32_DAC_CR_EN2		BIT(16)
+
+/**
+ * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
+ * @regmap: DAC registers shared via regmap
+ * @vref_mv: reference voltage (mv)
+ * @hfsel: high speed bus clock
+ */
+struct stm32_dac_common {
+	struct regmap			*regmap;
+	int				vref_mv;
+	bool				hfsel;
+};
+
+#endif
diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
new file mode 100644
index 0000000..ee9711d
--- /dev/null
+++ b/drivers/iio/dac/stm32-dac.c
@@ -0,0 +1,296 @@
+/*
+ * This file is part of STM32 DAC driver
+ *
+ * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
+ * Authors: Amelie Delaunay <amelie.delaunay@st.com>
+ *	    Fabrice Gasnier <fabrice.gasnier@st.com>
+ *
+ * License type: GPLv2
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include "stm32-dac-core.h"
+
+#define STM32_DAC_CHANNEL_1		1
+#define STM32_DAC_CHANNEL_2		2
+
+/**
+ * struct stm32_dac - private data of DAC driver
+ * @common:		reference to DAC common data
+ */
+struct stm32_dac {
+	struct stm32_dac_common *common;
+};
+
+static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
+{
+	u32 en, val;
+	int ret;
+
+	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
+	if (ret < 0)
+		return ret;
+	if (channel == STM32_DAC_CHANNEL_1)
+		en = FIELD_GET(STM32_DAC_CR_EN1, val);
+	else
+		en = FIELD_GET(STM32_DAC_CR_EN2, val);
+
+	return !!en;
+}
+
+static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
+		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
+	int ret;
+
+	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev, "Enable failed\n");
+		return ret;
+	}
+
+	/*
+	 * When HFSEL is set, it is not allowed to write the DHRx register
+	 * during 8 clock cycles after the ENx bit is set. It is not allowed
+	 * to make software/hardware trigger during this period neither.
+	 */
+	if (dac->common->hfsel)
+		udelay(1);
+
+	return 0;
+}
+
+static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
+		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
+	int ret;
+
+	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
+	if (ret)
+		dev_err(&indio_dev->dev, "Disable failed\n");
+
+	return ret;
+}
+
+static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
+{
+	int ret;
+
+	if (channel == STM32_DAC_CHANNEL_1)
+		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR1, val);
+	else
+		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR2, val);
+
+	return ret ? ret : IIO_VAL_INT;
+}
+
+static int stm32_dac_set_value(struct stm32_dac *dac, int channel, int val)
+{
+	int ret;
+
+	if (channel == STM32_DAC_CHANNEL_1)
+		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R1, val);
+	else
+		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R2, val);
+
+	return ret;
+}
+
+static int stm32_dac_read_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int *val, int *val2, long mask)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return stm32_dac_get_value(dac, chan->channel, val);
+	case IIO_CHAN_INFO_SCALE:
+		*val = dac->common->vref_mv;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_ENABLE:
+		ret = stm32_dac_is_enabled(dac, chan->channel);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int stm32_dac_write_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int val, int val2, long mask)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return stm32_dac_set_value(dac, chan->channel, val);
+	case IIO_CHAN_INFO_ENABLE:
+		if (!!val)
+			return stm32_dac_enable(indio_dev, chan->channel);
+		else
+			return stm32_dac_disable(indio_dev, chan->channel);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
+					unsigned reg, unsigned writeval,
+					unsigned *readval)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+
+	if (!readval)
+		return regmap_write(dac->common->regmap, reg, writeval);
+	else
+		return regmap_read(dac->common->regmap, reg, readval);
+}
+
+static const struct iio_info stm32_dac_iio_info = {
+	.read_raw = &stm32_dac_read_raw,
+	.write_raw = &stm32_dac_write_raw,
+	.debugfs_reg_access = &stm32_dac_debugfs_reg_access,
+	.driver_module = THIS_MODULE,
+};
+
+#define STM32_DAC_CHANNEL(chan, name) {		\
+	.type = IIO_VOLTAGE,			\
+	.indexed = 1,				\
+	.output = 1,				\
+	.channel = chan,			\
+	.info_mask_separate =			\
+		BIT(IIO_CHAN_INFO_RAW) |	\
+		BIT(IIO_CHAN_INFO_ENABLE) |	\
+		BIT(IIO_CHAN_INFO_SCALE),	\
+	.scan_type = {				\
+		.sign = 'u',			\
+		.realbits = 12,			\
+		.storagebits = 16,		\
+	},					\
+	.datasheet_name = name,			\
+}
+
+static const struct iio_chan_spec stm32_dac_channels[] = {
+	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_1, "out1"),
+	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_2, "out2"),
+};
+
+static int stm32_dac_chan_of_init(struct iio_dev *indio_dev)
+{
+	struct device_node *np = indio_dev->dev.of_node;
+	unsigned int i;
+	u32 channel;
+	int ret;
+
+	ret = of_property_read_u32(np, "st,dac-channel", &channel);
+	if (ret) {
+		dev_err(&indio_dev->dev, "Failed to read st,dac-channel\n");
+		return ret;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(stm32_dac_channels); i++) {
+		if (stm32_dac_channels[i].channel == channel)
+			break;
+	}
+	if (i >= ARRAY_SIZE(stm32_dac_channels)) {
+		dev_err(&indio_dev->dev, "Invalid st,dac-channel\n");
+		return -EINVAL;
+	}
+
+	indio_dev->channels = &stm32_dac_channels[i];
+	indio_dev->num_channels = 1;
+
+	return 0;
+};
+
+static int stm32_dac_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct iio_dev *indio_dev;
+	struct stm32_dac *dac;
+	int ret;
+
+	if (!np)
+		return -ENODEV;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*dac));
+	if (!indio_dev)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, indio_dev);
+
+	dac = iio_priv(indio_dev);
+	dac->common = dev_get_drvdata(pdev->dev.parent);
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->dev.of_node = pdev->dev.of_node;
+	indio_dev->info = &stm32_dac_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = stm32_dac_chan_of_init(indio_dev);
+	if (ret < 0)
+		return ret;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int stm32_dac_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+
+	iio_device_unregister(indio_dev);
+
+	return 0;
+}
+
+static const struct of_device_id stm32_dac_of_match[] = {
+	{ .compatible = "st,stm32-dac", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
+
+static struct platform_driver stm32_dac_driver = {
+	.probe = stm32_dac_probe,
+	.remove = stm32_dac_remove,
+	.driver = {
+		.name = "stm32-dac",
+		.of_match_table = stm32_dac_of_match,
+	},
+};
+module_platform_driver(stm32_dac_driver);
+
+MODULE_ALIAS("platform:stm32-dac");
+MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
+MODULE_DESCRIPTION("STMicroelectronics STM32 DAC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH 2/4] iio: dac: add support for stm32 DAC

[Peter Meerwald-Stadler]

On Fri, 31 Mar 2017, Fabrice Gasnier wrote:

> Add support for STMicroelectronics STM32 DAC. It's a 12-bit, voltage
> output digital-to-analog converter. It has two output channels, each
> with its own converter.
> It supports 8 bits or 12bits left/right aligned data format. Only
> 12bits right-aligned is used here. It has built-in noise or
> triangle waveform generator, and supports external triggers for
> conversions.
> Each channel can be used independently, with separate trigger, then
> separate IIO devices are used to handle this. Core driver is intended
> to share common resources such as clock, reset, reference voltage and
> registers.

comments below
 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
>  drivers/iio/dac/Kconfig          |  15 ++
>  drivers/iio/dac/Makefile         |   2 +
>  drivers/iio/dac/stm32-dac-core.c | 180 ++++++++++++++++++++++++
>  drivers/iio/dac/stm32-dac-core.h |  51 +++++++
>  drivers/iio/dac/stm32-dac.c      | 296 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 544 insertions(+)
>  create mode 100644 drivers/iio/dac/stm32-dac-core.c
>  create mode 100644 drivers/iio/dac/stm32-dac-core.h
>  create mode 100644 drivers/iio/dac/stm32-dac.c
> 
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index d3084028..7198648 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -274,6 +274,21 @@ config MCP4922
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called mcp4922.
>  
> +config STM32_DAC
> +	tristate "STMicroelectronics STM32 DAC"
> +	depends on (ARCH_STM32 && OF) || COMPILE_TEST
> +	depends on REGULATOR
> +	select STM32_DAC_CORE
> +	help
> +	  Say yes here to build support for STMicroelectronics STM32 Digital
> +	  to Analog Converter (DAC).
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called stm32-dac.
> +
> +config STM32_DAC_CORE
> +	tristate
> +
>  config VF610_DAC
>  	tristate "Vybrid vf610 DAC driver"
>  	depends on OF
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index f01bf4a..afe8ae7 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -29,4 +29,6 @@ obj-$(CONFIG_MAX517) += max517.o
>  obj-$(CONFIG_MAX5821) += max5821.o
>  obj-$(CONFIG_MCP4725) += mcp4725.o
>  obj-$(CONFIG_MCP4922) += mcp4922.o
> +obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
> +obj-$(CONFIG_STM32_DAC) += stm32-dac.o
>  obj-$(CONFIG_VF610_DAC) += vf610_dac.o
> diff --git a/drivers/iio/dac/stm32-dac-core.c b/drivers/iio/dac/stm32-dac-core.c
> new file mode 100644
> index 0000000..75e4878
> --- /dev/null
> +++ b/drivers/iio/dac/stm32-dac-core.c
> @@ -0,0 +1,180 @@
> +/*
> + * This file is part of STM32 DAC driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
> + *
> + * License type: GPLv2
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +
> +#include "stm32-dac-core.h"
> +
> +/**
> + * struct stm32_dac_priv - stm32 DAC core private data
> + * @pclk:		peripheral clock common for all DACs
> + * @rst:		peripheral reset control
> + * @vref:		regulator reference
> + * @common:		Common data for all DAC instances
> + */
> +struct stm32_dac_priv {
> +	struct clk *pclk;
> +	struct reset_control *rst;
> +	struct regulator *vref;
> +	struct stm32_dac_common common;
> +};
> +
> +static struct stm32_dac_priv *to_stm32_dac_priv(struct stm32_dac_common *com)
> +{
> +	return container_of(com, struct stm32_dac_priv, common);
> +}
> +
> +static const struct regmap_config stm32_dac_regmap_cfg = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = sizeof(u32),
> +	.max_register = 0x3fc,
> +};
> +
> +static int stm32_dac_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct stm32_dac_priv *priv;
> +	struct regmap *regmap;
> +	struct resource *res;
> +	void __iomem *mmio;
> +	int ret;
> +
> +	if (!dev->of_node)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mmio = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(mmio))
> +		return PTR_ERR(mmio);
> +
> +	regmap = devm_regmap_init_mmio(dev, mmio, &stm32_dac_regmap_cfg);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +	priv->common.regmap = regmap;
> +
> +	priv->vref = devm_regulator_get(dev, "vref");
> +	if (IS_ERR(priv->vref)) {
> +		ret = PTR_ERR(priv->vref);
> +		dev_err(dev, "vref get failed, %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(priv->vref);
> +	if (ret < 0) {
> +		dev_err(dev, "vref enable failed\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_get_voltage(priv->vref);
> +	if (ret < 0) {
> +		dev_err(dev, "vref get voltage failed, %d\n", ret);
> +		goto err_vref;
> +	}
> +	priv->common.vref_mv = ret / 1000;
> +	dev_dbg(dev, "vref+=%dmV\n", priv->common.vref_mv);
> +
> +	priv->pclk = devm_clk_get(dev, "pclk");
> +	if (IS_ERR(priv->pclk)) {
> +		ret = PTR_ERR(priv->pclk);
> +		dev_err(dev, "pclk get failed\n");
> +		goto err_vref;
> +	}
> +
> +	ret = clk_prepare_enable(priv->pclk);
> +	if (ret < 0) {
> +		dev_err(dev, "pclk enable failed\n");
> +		goto err_vref;
> +	}
> +
> +	priv->rst = devm_reset_control_get(dev, NULL);
> +	if (!IS_ERR(priv->rst)) {
> +		reset_control_assert(priv->rst);
> +		udelay(2);
> +		reset_control_deassert(priv->rst);
> +	}
> +
> +	/* When clock speed is higher than 80MHz, set HFSEL */
> +	priv->common.hfsel = (clk_get_rate(priv->pclk) > 80000000UL);
> +	ret = regmap_update_bits(regmap, STM32_DAC_CR, STM32H7_DAC_CR_HFSEL,
> +				 priv->common.hfsel ? STM32H7_DAC_CR_HFSEL : 0);
> +	if (ret)
> +		goto err_pclk;
> +
> +	platform_set_drvdata(pdev, &priv->common);
> +
> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to populate DT children\n");
> +		goto err_pclk;
> +	}
> +
> +	return 0;
> +
> +err_pclk:
> +	clk_disable_unprepare(priv->pclk);
> +err_vref:
> +	regulator_disable(priv->vref);
> +
> +	return ret;
> +}
> +
> +static int stm32_dac_remove(struct platform_device *pdev)
> +{
> +	struct stm32_dac_common *common = platform_get_drvdata(pdev);
> +	struct stm32_dac_priv *priv = to_stm32_dac_priv(common);
> +
> +	of_platform_depopulate(&pdev->dev);
> +	clk_disable_unprepare(priv->pclk);
> +	regulator_disable(priv->vref);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id stm32_dac_of_match[] = {
> +	{ .compatible = "st,stm32h7-dac-core", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
> +
> +static struct platform_driver stm32_dac_driver = {
> +	.probe = stm32_dac_probe,
> +	.remove = stm32_dac_remove,
> +	.driver = {
> +		.name = "stm32-dac-core",
> +		.of_match_table = stm32_dac_of_match,
> +	},
> +};
> +module_platform_driver(stm32_dac_driver);
> +
> +MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
> +MODULE_DESCRIPTION("STMicroelectronics STM32 DAC core driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:stm32-dac-core");
> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
> new file mode 100644
> index 0000000..d3099f7
> --- /dev/null
> +++ b/drivers/iio/dac/stm32-dac-core.h
> @@ -0,0 +1,51 @@
> +/*
> + * This file is part of STM32 DAC driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
> + *
> + * License type: GPLv2
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __STM32_DAC_CORE_H
> +#define __STM32_DAC_CORE_H
> +
> +#include <linux/regmap.h>
> +
> +/* STM32 DAC registers */
> +#define STM32_DAC_CR		0x00
> +#define STM32_DAC_DHR12R1	0x08
> +#define STM32_DAC_DHR12R2	0x14
> +#define STM32_DAC_DOR1		0x2C
> +#define STM32_DAC_DOR2		0x30
> +
> +/* STM32_DAC_CR bit fields */
> +#define STM32_DAC_CR_EN1		BIT(0)
> +#define STM32H7_DAC_CR_HFSEL		BIT(15)
> +#define STM32_DAC_CR_EN2		BIT(16)
> +
> +/**
> + * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
> + * @regmap: DAC registers shared via regmap
> + * @vref_mv: reference voltage (mv)
> + * @hfsel: high speed bus clock

probably better "high speed bus clock selected"

> + */
> +struct stm32_dac_common {
> +	struct regmap			*regmap;
> +	int				vref_mv;
> +	bool				hfsel;
> +};
> +
> +#endif
> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
> new file mode 100644
> index 0000000..ee9711d
> --- /dev/null
> +++ b/drivers/iio/dac/stm32-dac.c
> @@ -0,0 +1,296 @@
> +/*
> + * This file is part of STM32 DAC driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Authors: Amelie Delaunay <amelie.delaunay@st.com>
> + *	    Fabrice Gasnier <fabrice.gasnier@st.com>
> + *
> + * License type: GPLv2
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include "stm32-dac-core.h"
> +
> +#define STM32_DAC_CHANNEL_1		1
> +#define STM32_DAC_CHANNEL_2		2
> +
> +/**
> + * struct stm32_dac - private data of DAC driver
> + * @common:		reference to DAC common data
> + */
> +struct stm32_dac {
> +	struct stm32_dac_common *common;
> +};
> +
> +static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
> +{
> +	u32 en, val;
> +	int ret;
> +
> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
> +	if (ret < 0)
> +		return ret;
> +	if (channel == STM32_DAC_CHANNEL_1)
> +		en = FIELD_GET(STM32_DAC_CR_EN1, val);
> +	else
> +		en = FIELD_GET(STM32_DAC_CR_EN2, val);
> +
> +	return !!en;
> +}
> +
> +static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
> +		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;

could #define a macro to select CR_EN1 or CR_EN2 depending on channel; to 
use here and above and below

or maybe a table to select all the channel-specific constants indexed by 
channel

> +	int ret;
> +
> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "Enable failed\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * When HFSEL is set, it is not allowed to write the DHRx register
> +	 * during 8 clock cycles after the ENx bit is set. It is not allowed
> +	 * to make software/hardware trigger during this period neither.

neither -> either

> +	 */
> +	if (dac->common->hfsel)
> +		udelay(1);
> +
> +	return 0;
> +}
> +
> +static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
> +		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
> +	int ret;
> +
> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
> +	if (ret)
> +		dev_err(&indio_dev->dev, "Disable failed\n");
> +
> +	return ret;
> +}
> +
> +static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
> +{
> +	int ret;
> +
> +	if (channel == STM32_DAC_CHANNEL_1)
> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR1, val);
> +	else
> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR2, val);
> +
> +	return ret ? ret : IIO_VAL_INT;
> +}
> +
> +static int stm32_dac_set_value(struct stm32_dac *dac, int channel, int val)
> +{
> +	int ret;
> +
> +	if (channel == STM32_DAC_CHANNEL_1)
> +		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R1, val);
> +	else
> +		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R2, val);
> +
> +	return ret;
> +}
> +
> +static int stm32_dac_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int *val, int *val2, long mask)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return stm32_dac_get_value(dac, chan->channel, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = dac->common->vref_mv;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_ENABLE:
> +		ret = stm32_dac_is_enabled(dac, chan->channel);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int stm32_dac_write_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       int val, int val2, long mask)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return stm32_dac_set_value(dac, chan->channel, val);
> +	case IIO_CHAN_INFO_ENABLE:
> +		if (!!val)
> +			return stm32_dac_enable(indio_dev, chan->channel);
> +		else
> +			return stm32_dac_disable(indio_dev, chan->channel);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
> +					unsigned reg, unsigned writeval,
> +					unsigned *readval)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +
> +	if (!readval)
> +		return regmap_write(dac->common->regmap, reg, writeval);
> +	else
> +		return regmap_read(dac->common->regmap, reg, readval);
> +}
> +
> +static const struct iio_info stm32_dac_iio_info = {
> +	.read_raw = &stm32_dac_read_raw,

no & for functions (matter of taste)

> +	.write_raw = &stm32_dac_write_raw,
> +	.debugfs_reg_access = &stm32_dac_debugfs_reg_access,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +#define STM32_DAC_CHANNEL(chan, name) {		\
> +	.type = IIO_VOLTAGE,			\
> +	.indexed = 1,				\
> +	.output = 1,				\
> +	.channel = chan,			\
> +	.info_mask_separate =			\
> +		BIT(IIO_CHAN_INFO_RAW) |	\
> +		BIT(IIO_CHAN_INFO_ENABLE) |	\
> +		BIT(IIO_CHAN_INFO_SCALE),	\
> +	.scan_type = {				\

scan_type is no needed (yet) and scan_index is missing (or always 0 which 
becomes clear below, a comment may be helpful)

> +		.sign = 'u',			\
> +		.realbits = 12,			\
> +		.storagebits = 16,		\
> +	},					\
> +	.datasheet_name = name,			\
> +}
> +
> +static const struct iio_chan_spec stm32_dac_channels[] = {
> +	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_1, "out1"),
> +	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_2, "out2"),
> +};
> +
> +static int stm32_dac_chan_of_init(struct iio_dev *indio_dev)
> +{
> +	struct device_node *np = indio_dev->dev.of_node;
> +	unsigned int i;
> +	u32 channel;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "st,dac-channel", &channel);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Failed to read st,dac-channel\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(stm32_dac_channels); i++) {
> +		if (stm32_dac_channels[i].channel == channel)
> +			break;
> +	}
> +	if (i >= ARRAY_SIZE(stm32_dac_channels)) {
> +		dev_err(&indio_dev->dev, "Invalid st,dac-channel\n");
> +		return -EINVAL;
> +	}
> +
> +	indio_dev->channels = &stm32_dac_channels[i];
> +	indio_dev->num_channels = 1;

this is unusual, a comment would be helpful
why not expose both channels?

> +
> +	return 0;
> +};
> +
> +static int stm32_dac_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct iio_dev *indio_dev;
> +	struct stm32_dac *dac;
> +	int ret;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*dac));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	dac = iio_priv(indio_dev);
> +	dac->common = dev_get_drvdata(pdev->dev.parent);
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->dev.of_node = pdev->dev.of_node;
> +	indio_dev->info = &stm32_dac_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = stm32_dac_chan_of_init(indio_dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);

just 
return iio_device_register();

candidate for devm_ due to empty _remove()

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int stm32_dac_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id stm32_dac_of_match[] = {
> +	{ .compatible = "st,stm32-dac", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
> +
> +static struct platform_driver stm32_dac_driver = {
> +	.probe = stm32_dac_probe,
> +	.remove = stm32_dac_remove,
> +	.driver = {
> +		.name = "stm32-dac",
> +		.of_match_table = stm32_dac_of_match,
> +	},
> +};
> +module_platform_driver(stm32_dac_driver);
> +
> +MODULE_ALIAS("platform:stm32-dac");
> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
> +MODULE_DESCRIPTION("STMicroelectronics STM32 DAC driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Re: [PATCH 2/4] iio: dac: add support for stm32 DAC

[Fabrice Gasnier]

On 04/01/2017 02:34 PM, Peter Meerwald-Stadler wrote:
> On Fri, 31 Mar 2017, Fabrice Gasnier wrote:
> 
>> Add support for STMicroelectronics STM32 DAC. It's a 12-bit, voltage
>> output digital-to-analog converter. It has two output channels, each
>> with its own converter.
>> It supports 8 bits or 12bits left/right aligned data format. Only
>> 12bits right-aligned is used here. It has built-in noise or
>> triangle waveform generator, and supports external triggers for
>> conversions.
>> Each channel can be used independently, with separate trigger, then
>> separate IIO devices are used to handle this. Core driver is intended
>> to share common resources such as clock, reset, reference voltage and
>> registers.
> 
> comments below

Hi Peter,

Thanks for reviewing.
I'll update V2 with all your remarks (comments bellow).

Best Regards,
Fabrice

>  
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> ---
>>  drivers/iio/dac/Kconfig          |  15 ++
>>  drivers/iio/dac/Makefile         |   2 +
>>  drivers/iio/dac/stm32-dac-core.c | 180 ++++++++++++++++++++++++
>>  drivers/iio/dac/stm32-dac-core.h |  51 +++++++
>>  drivers/iio/dac/stm32-dac.c      | 296 +++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 544 insertions(+)
>>  create mode 100644 drivers/iio/dac/stm32-dac-core.c
>>  create mode 100644 drivers/iio/dac/stm32-dac-core.h
>>  create mode 100644 drivers/iio/dac/stm32-dac.c
>>
>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>> index d3084028..7198648 100644
>> --- a/drivers/iio/dac/Kconfig
>> +++ b/drivers/iio/dac/Kconfig
>> @@ -274,6 +274,21 @@ config MCP4922
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called mcp4922.
>>  
>> +config STM32_DAC
>> +	tristate "STMicroelectronics STM32 DAC"
>> +	depends on (ARCH_STM32 && OF) || COMPILE_TEST
>> +	depends on REGULATOR
>> +	select STM32_DAC_CORE
>> +	help
>> +	  Say yes here to build support for STMicroelectronics STM32 Digital
>> +	  to Analog Converter (DAC).
>> +
>> +	  This driver can also be built as a module.  If so, the module
>> +	  will be called stm32-dac.
>> +
>> +config STM32_DAC_CORE
>> +	tristate
>> +
>>  config VF610_DAC
>>  	tristate "Vybrid vf610 DAC driver"
>>  	depends on OF
>> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
>> index f01bf4a..afe8ae7 100644
>> --- a/drivers/iio/dac/Makefile
>> +++ b/drivers/iio/dac/Makefile
>> @@ -29,4 +29,6 @@ obj-$(CONFIG_MAX517) += max517.o
>>  obj-$(CONFIG_MAX5821) += max5821.o
>>  obj-$(CONFIG_MCP4725) += mcp4725.o
>>  obj-$(CONFIG_MCP4922) += mcp4922.o
>> +obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
>> +obj-$(CONFIG_STM32_DAC) += stm32-dac.o
>>  obj-$(CONFIG_VF610_DAC) += vf610_dac.o
>> diff --git a/drivers/iio/dac/stm32-dac-core.c b/drivers/iio/dac/stm32-dac-core.c
>> new file mode 100644
>> index 0000000..75e4878
>> --- /dev/null
>> +++ b/drivers/iio/dac/stm32-dac-core.c
>> @@ -0,0 +1,180 @@
>> +/*
>> + * This file is part of STM32 DAC driver
>> + *
>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>> + * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
>> + *
>> + * License type: GPLv2
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>> + * or FITNESS FOR A PARTICULAR PURPOSE.
>> + * See the GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/reset.h>
>> +
>> +#include "stm32-dac-core.h"
>> +
>> +/**
>> + * struct stm32_dac_priv - stm32 DAC core private data
>> + * @pclk:		peripheral clock common for all DACs
>> + * @rst:		peripheral reset control
>> + * @vref:		regulator reference
>> + * @common:		Common data for all DAC instances
>> + */
>> +struct stm32_dac_priv {
>> +	struct clk *pclk;
>> +	struct reset_control *rst;
>> +	struct regulator *vref;
>> +	struct stm32_dac_common common;
>> +};
>> +
>> +static struct stm32_dac_priv *to_stm32_dac_priv(struct stm32_dac_common *com)
>> +{
>> +	return container_of(com, struct stm32_dac_priv, common);
>> +}
>> +
>> +static const struct regmap_config stm32_dac_regmap_cfg = {
>> +	.reg_bits = 32,
>> +	.val_bits = 32,
>> +	.reg_stride = sizeof(u32),
>> +	.max_register = 0x3fc,
>> +};
>> +
>> +static int stm32_dac_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct stm32_dac_priv *priv;
>> +	struct regmap *regmap;
>> +	struct resource *res;
>> +	void __iomem *mmio;
>> +	int ret;
>> +
>> +	if (!dev->of_node)
>> +		return -ENODEV;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	mmio = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(mmio))
>> +		return PTR_ERR(mmio);
>> +
>> +	regmap = devm_regmap_init_mmio(dev, mmio, &stm32_dac_regmap_cfg);
>> +	if (IS_ERR(regmap))
>> +		return PTR_ERR(regmap);
>> +	priv->common.regmap = regmap;
>> +
>> +	priv->vref = devm_regulator_get(dev, "vref");
>> +	if (IS_ERR(priv->vref)) {
>> +		ret = PTR_ERR(priv->vref);
>> +		dev_err(dev, "vref get failed, %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = regulator_enable(priv->vref);
>> +	if (ret < 0) {
>> +		dev_err(dev, "vref enable failed\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = regulator_get_voltage(priv->vref);
>> +	if (ret < 0) {
>> +		dev_err(dev, "vref get voltage failed, %d\n", ret);
>> +		goto err_vref;
>> +	}
>> +	priv->common.vref_mv = ret / 1000;
>> +	dev_dbg(dev, "vref+=%dmV\n", priv->common.vref_mv);
>> +
>> +	priv->pclk = devm_clk_get(dev, "pclk");
>> +	if (IS_ERR(priv->pclk)) {
>> +		ret = PTR_ERR(priv->pclk);
>> +		dev_err(dev, "pclk get failed\n");
>> +		goto err_vref;
>> +	}
>> +
>> +	ret = clk_prepare_enable(priv->pclk);
>> +	if (ret < 0) {
>> +		dev_err(dev, "pclk enable failed\n");
>> +		goto err_vref;
>> +	}
>> +
>> +	priv->rst = devm_reset_control_get(dev, NULL);
>> +	if (!IS_ERR(priv->rst)) {
>> +		reset_control_assert(priv->rst);
>> +		udelay(2);
>> +		reset_control_deassert(priv->rst);
>> +	}
>> +
>> +	/* When clock speed is higher than 80MHz, set HFSEL */
>> +	priv->common.hfsel = (clk_get_rate(priv->pclk) > 80000000UL);
>> +	ret = regmap_update_bits(regmap, STM32_DAC_CR, STM32H7_DAC_CR_HFSEL,
>> +				 priv->common.hfsel ? STM32H7_DAC_CR_HFSEL : 0);
>> +	if (ret)
>> +		goto err_pclk;
>> +
>> +	platform_set_drvdata(pdev, &priv->common);
>> +
>> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to populate DT children\n");
>> +		goto err_pclk;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_pclk:
>> +	clk_disable_unprepare(priv->pclk);
>> +err_vref:
>> +	regulator_disable(priv->vref);
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_dac_remove(struct platform_device *pdev)
>> +{
>> +	struct stm32_dac_common *common = platform_get_drvdata(pdev);
>> +	struct stm32_dac_priv *priv = to_stm32_dac_priv(common);
>> +
>> +	of_platform_depopulate(&pdev->dev);
>> +	clk_disable_unprepare(priv->pclk);
>> +	regulator_disable(priv->vref);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id stm32_dac_of_match[] = {
>> +	{ .compatible = "st,stm32h7-dac-core", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
>> +
>> +static struct platform_driver stm32_dac_driver = {
>> +	.probe = stm32_dac_probe,
>> +	.remove = stm32_dac_remove,
>> +	.driver = {
>> +		.name = "stm32-dac-core",
>> +		.of_match_table = stm32_dac_of_match,
>> +	},
>> +};
>> +module_platform_driver(stm32_dac_driver);
>> +
>> +MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
>> +MODULE_DESCRIPTION("STMicroelectronics STM32 DAC core driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:stm32-dac-core");
>> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
>> new file mode 100644
>> index 0000000..d3099f7
>> --- /dev/null
>> +++ b/drivers/iio/dac/stm32-dac-core.h
>> @@ -0,0 +1,51 @@
>> +/*
>> + * This file is part of STM32 DAC driver
>> + *
>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>> + * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
>> + *
>> + * License type: GPLv2
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>> + * or FITNESS FOR A PARTICULAR PURPOSE.
>> + * See the GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __STM32_DAC_CORE_H
>> +#define __STM32_DAC_CORE_H
>> +
>> +#include <linux/regmap.h>
>> +
>> +/* STM32 DAC registers */
>> +#define STM32_DAC_CR		0x00
>> +#define STM32_DAC_DHR12R1	0x08
>> +#define STM32_DAC_DHR12R2	0x14
>> +#define STM32_DAC_DOR1		0x2C
>> +#define STM32_DAC_DOR2		0x30
>> +
>> +/* STM32_DAC_CR bit fields */
>> +#define STM32_DAC_CR_EN1		BIT(0)
>> +#define STM32H7_DAC_CR_HFSEL		BIT(15)
>> +#define STM32_DAC_CR_EN2		BIT(16)
>> +
>> +/**
>> + * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
>> + * @regmap: DAC registers shared via regmap
>> + * @vref_mv: reference voltage (mv)
>> + * @hfsel: high speed bus clock
> 
> probably better "high speed bus clock selected"
Ok

> 
>> + */
>> +struct stm32_dac_common {
>> +	struct regmap			*regmap;
>> +	int				vref_mv;
>> +	bool				hfsel;
>> +};
>> +
>> +#endif
>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>> new file mode 100644
>> index 0000000..ee9711d
>> --- /dev/null
>> +++ b/drivers/iio/dac/stm32-dac.c
>> @@ -0,0 +1,296 @@
>> +/*
>> + * This file is part of STM32 DAC driver
>> + *
>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>> + * Authors: Amelie Delaunay <amelie.delaunay@st.com>
>> + *	    Fabrice Gasnier <fabrice.gasnier@st.com>
>> + *
>> + * License type: GPLv2
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>> + * or FITNESS FOR A PARTICULAR PURPOSE.
>> + * See the GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/delay.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "stm32-dac-core.h"
>> +
>> +#define STM32_DAC_CHANNEL_1		1
>> +#define STM32_DAC_CHANNEL_2		2
>> +
>> +/**
>> + * struct stm32_dac - private data of DAC driver
>> + * @common:		reference to DAC common data
>> + */
>> +struct stm32_dac {
>> +	struct stm32_dac_common *common;
>> +};
>> +
>> +static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
>> +{
>> +	u32 en, val;
>> +	int ret;
>> +
>> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +	if (channel == STM32_DAC_CHANNEL_1)
>> +		en = FIELD_GET(STM32_DAC_CR_EN1, val);
>> +	else
>> +		en = FIELD_GET(STM32_DAC_CR_EN2, val);
>> +
>> +	return !!en;
>> +}
>> +
>> +static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
>> +		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
> 
> could #define a macro to select CR_EN1 or CR_EN2 depending on channel; to 
> use here and above and below

I also thought about it earlier, but... I'll define a macro in V2.
> 
> or maybe a table to select all the channel-specific constants indexed by 
> channel
> 
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
>> +	if (ret < 0) {
>> +		dev_err(&indio_dev->dev, "Enable failed\n");
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * When HFSEL is set, it is not allowed to write the DHRx register
>> +	 * during 8 clock cycles after the ENx bit is set. It is not allowed
>> +	 * to make software/hardware trigger during this period neither.
> 
> neither -> either
Ok
> 
>> +	 */
>> +	if (dac->common->hfsel)
>> +		udelay(1);
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
>> +		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
>> +	if (ret)
>> +		dev_err(&indio_dev->dev, "Disable failed\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
>> +{
>> +	int ret;
>> +
>> +	if (channel == STM32_DAC_CHANNEL_1)
>> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR1, val);
>> +	else
>> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR2, val);
>> +
>> +	return ret ? ret : IIO_VAL_INT;
>> +}
>> +
>> +static int stm32_dac_set_value(struct stm32_dac *dac, int channel, int val)
>> +{
>> +	int ret;
>> +
>> +	if (channel == STM32_DAC_CHANNEL_1)
>> +		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R1, val);
>> +	else
>> +		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R2, val);
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_dac_read_raw(struct iio_dev *indio_dev,
>> +			      struct iio_chan_spec const *chan,
>> +			      int *val, int *val2, long mask)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		return stm32_dac_get_value(dac, chan->channel, val);
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = dac->common->vref_mv;
>> +		*val2 = chan->scan_type.realbits;
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>> +	case IIO_CHAN_INFO_ENABLE:
>> +		ret = stm32_dac_is_enabled(dac, chan->channel);
>> +		if (ret < 0)
>> +			return ret;
>> +		*val = ret;
>> +		return IIO_VAL_INT;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int stm32_dac_write_raw(struct iio_dev *indio_dev,
>> +			       struct iio_chan_spec const *chan,
>> +			       int val, int val2, long mask)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		return stm32_dac_set_value(dac, chan->channel, val);
>> +	case IIO_CHAN_INFO_ENABLE:
>> +		if (!!val)
>> +			return stm32_dac_enable(indio_dev, chan->channel);
>> +		else
>> +			return stm32_dac_disable(indio_dev, chan->channel);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>> +					unsigned reg, unsigned writeval,
>> +					unsigned *readval)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +
>> +	if (!readval)
>> +		return regmap_write(dac->common->regmap, reg, writeval);
>> +	else
>> +		return regmap_read(dac->common->regmap, reg, readval);
>> +}
>> +
>> +static const struct iio_info stm32_dac_iio_info = {
>> +	.read_raw = &stm32_dac_read_raw,
> 
> no & for functions (matter of taste)
I'll remove these.
> 
>> +	.write_raw = &stm32_dac_write_raw,
>> +	.debugfs_reg_access = &stm32_dac_debugfs_reg_access,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +#define STM32_DAC_CHANNEL(chan, name) {		\
>> +	.type = IIO_VOLTAGE,			\
>> +	.indexed = 1,				\
>> +	.output = 1,				\
>> +	.channel = chan,			\
>> +	.info_mask_separate =			\
>> +		BIT(IIO_CHAN_INFO_RAW) |	\
>> +		BIT(IIO_CHAN_INFO_ENABLE) |	\
>> +		BIT(IIO_CHAN_INFO_SCALE),	\
>> +	.scan_type = {				\
> 
> scan_type is no needed (yet) and scan_index is missing (or always 0 which 
> becomes clear below, a comment may be helpful)
> 
>> +		.sign = 'u',			\
>> +		.realbits = 12,			\
>> +		.storagebits = 16,		\
>> +	},					\
>> +	.datasheet_name = name,			\
>> +}
>> +
>> +static const struct iio_chan_spec stm32_dac_channels[] = {
>> +	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_1, "out1"),
>> +	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_2, "out2"),
>> +};
>> +
>> +static int stm32_dac_chan_of_init(struct iio_dev *indio_dev)
>> +{
>> +	struct device_node *np = indio_dev->dev.of_node;
>> +	unsigned int i;
>> +	u32 channel;
>> +	int ret;
>> +
>> +	ret = of_property_read_u32(np, "st,dac-channel", &channel);
>> +	if (ret) {
>> +		dev_err(&indio_dev->dev, "Failed to read st,dac-channel\n");
>> +		return ret;
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(stm32_dac_channels); i++) {
>> +		if (stm32_dac_channels[i].channel == channel)
>> +			break;
>> +	}
>> +	if (i >= ARRAY_SIZE(stm32_dac_channels)) {
>> +		dev_err(&indio_dev->dev, "Invalid st,dac-channel\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	indio_dev->channels = &stm32_dac_channels[i];
>> +	indio_dev->num_channels = 1;
> 
> this is unusual, a comment would be helpful
> why not expose both channels?
Got it, I'll comment about this.

> 
>> +
>> +	return 0;
>> +};
>> +
>> +static int stm32_dac_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	struct iio_dev *indio_dev;
>> +	struct stm32_dac *dac;
>> +	int ret;
>> +
>> +	if (!np)
>> +		return -ENODEV;
>> +
>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*dac));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +	platform_set_drvdata(pdev, indio_dev);
>> +
>> +	dac = iio_priv(indio_dev);
>> +	dac->common = dev_get_drvdata(pdev->dev.parent);
>> +	indio_dev->name = dev_name(&pdev->dev);
>> +	indio_dev->dev.parent = &pdev->dev;
>> +	indio_dev->dev.of_node = pdev->dev.of_node;
>> +	indio_dev->info = &stm32_dac_iio_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	ret = stm32_dac_chan_of_init(indio_dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = iio_device_register(indio_dev);
> 
> just 
> return iio_device_register();
> 
> candidate for devm_ due to empty _remove()
You're right.

I didn't use devm_ due to patch 3: It needs .remove routine.
But okay, I'll do this anyway :-)

> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_dac_remove(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +
>> +	iio_device_unregister(indio_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id stm32_dac_of_match[] = {
>> +	{ .compatible = "st,stm32-dac", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
>> +
>> +static struct platform_driver stm32_dac_driver = {
>> +	.probe = stm32_dac_probe,
>> +	.remove = stm32_dac_remove,
>> +	.driver = {
>> +		.name = "stm32-dac",
>> +		.of_match_table = stm32_dac_of_match,
>> +	},
>> +};
>> +module_platform_driver(stm32_dac_driver);
>> +
>> +MODULE_ALIAS("platform:stm32-dac");
>> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
>> +MODULE_DESCRIPTION("STMicroelectronics STM32 DAC driver");
>> +MODULE_LICENSE("GPL v2");
>>
> 

Re: [PATCH 2/4] iio: dac: add support for stm32 DAC

[Jonathan Cameron]

On 31/03/17 12:45, Fabrice Gasnier wrote:
> Add support for STMicroelectronics STM32 DAC. It's a 12-bit, voltage
> output digital-to-analog converter. It has two output channels, each
> with its own converter.
> It supports 8 bits or 12bits left/right aligned data format. Only
> 12bits right-aligned is used here. It has built-in noise or
> triangle waveform generator, and supports external triggers for
> conversions.
> Each channel can be used independently, with separate trigger, then
> separate IIO devices are used to handle this. Core driver is intended
> to share common resources such as clock, reset, reference voltage and
> registers.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Annoyingly my laptop just crashed mid way through reviewing this..

Ah well, hopefully I'll remember everything (there wasn't much).

For DACs the 'enable' attribute is not normally used. Rather we
use the powerdown one.  The reasoning being that we care about what
the state is when it is powered down.  Even if that isn't controllable
I would expect to see it exported as powerdown_mode with a fixed value.

Other than that - looks pretty good to me.

Jonathan

> ---
>  drivers/iio/dac/Kconfig          |  15 ++
>  drivers/iio/dac/Makefile         |   2 +
>  drivers/iio/dac/stm32-dac-core.c | 180 ++++++++++++++++++++++++
>  drivers/iio/dac/stm32-dac-core.h |  51 +++++++
>  drivers/iio/dac/stm32-dac.c      | 296 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 544 insertions(+)
>  create mode 100644 drivers/iio/dac/stm32-dac-core.c
>  create mode 100644 drivers/iio/dac/stm32-dac-core.h
>  create mode 100644 drivers/iio/dac/stm32-dac.c
> 
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index d3084028..7198648 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -274,6 +274,21 @@ config MCP4922
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called mcp4922.
>  
> +config STM32_DAC
> +	tristate "STMicroelectronics STM32 DAC"
> +	depends on (ARCH_STM32 && OF) || COMPILE_TEST
> +	depends on REGULATOR
> +	select STM32_DAC_CORE
> +	help
> +	  Say yes here to build support for STMicroelectronics STM32 Digital
> +	  to Analog Converter (DAC).
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called stm32-dac.
> +
> +config STM32_DAC_CORE
> +	tristate
> +
>  config VF610_DAC
>  	tristate "Vybrid vf610 DAC driver"
>  	depends on OF
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index f01bf4a..afe8ae7 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -29,4 +29,6 @@ obj-$(CONFIG_MAX517) += max517.o
>  obj-$(CONFIG_MAX5821) += max5821.o
>  obj-$(CONFIG_MCP4725) += mcp4725.o
>  obj-$(CONFIG_MCP4922) += mcp4922.o
> +obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
> +obj-$(CONFIG_STM32_DAC) += stm32-dac.o
>  obj-$(CONFIG_VF610_DAC) += vf610_dac.o
> diff --git a/drivers/iio/dac/stm32-dac-core.c b/drivers/iio/dac/stm32-dac-core.c
> new file mode 100644
> index 0000000..75e4878
> --- /dev/null
> +++ b/drivers/iio/dac/stm32-dac-core.c
> @@ -0,0 +1,180 @@
> +/*
> + * This file is part of STM32 DAC driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
> + *
> + * License type: GPLv2
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +
> +#include "stm32-dac-core.h"
> +
> +/**
> + * struct stm32_dac_priv - stm32 DAC core private data
> + * @pclk:		peripheral clock common for all DACs
> + * @rst:		peripheral reset control
> + * @vref:		regulator reference
> + * @common:		Common data for all DAC instances
> + */
> +struct stm32_dac_priv {
> +	struct clk *pclk;
> +	struct reset_control *rst;
> +	struct regulator *vref;
> +	struct stm32_dac_common common;
> +};
> +
> +static struct stm32_dac_priv *to_stm32_dac_priv(struct stm32_dac_common *com)
> +{
> +	return container_of(com, struct stm32_dac_priv, common);
> +}
> +
> +static const struct regmap_config stm32_dac_regmap_cfg = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = sizeof(u32),
> +	.max_register = 0x3fc,
> +};
> +
> +static int stm32_dac_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct stm32_dac_priv *priv;
> +	struct regmap *regmap;
> +	struct resource *res;
> +	void __iomem *mmio;
> +	int ret;
> +
> +	if (!dev->of_node)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mmio = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(mmio))
> +		return PTR_ERR(mmio);
> +
> +	regmap = devm_regmap_init_mmio(dev, mmio, &stm32_dac_regmap_cfg);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +	priv->common.regmap = regmap;
> +
> +	priv->vref = devm_regulator_get(dev, "vref");
> +	if (IS_ERR(priv->vref)) {
> +		ret = PTR_ERR(priv->vref);
> +		dev_err(dev, "vref get failed, %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(priv->vref);
> +	if (ret < 0) {
> +		dev_err(dev, "vref enable failed\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_get_voltage(priv->vref);
> +	if (ret < 0) {
> +		dev_err(dev, "vref get voltage failed, %d\n", ret);
> +		goto err_vref;
> +	}
> +	priv->common.vref_mv = ret / 1000;
> +	dev_dbg(dev, "vref+=%dmV\n", priv->common.vref_mv);
> +
> +	priv->pclk = devm_clk_get(dev, "pclk");
> +	if (IS_ERR(priv->pclk)) {
> +		ret = PTR_ERR(priv->pclk);
> +		dev_err(dev, "pclk get failed\n");
> +		goto err_vref;
> +	}
> +
> +	ret = clk_prepare_enable(priv->pclk);
> +	if (ret < 0) {
> +		dev_err(dev, "pclk enable failed\n");
> +		goto err_vref;
> +	}
> +
> +	priv->rst = devm_reset_control_get(dev, NULL);
> +	if (!IS_ERR(priv->rst)) {
> +		reset_control_assert(priv->rst);
> +		udelay(2);
> +		reset_control_deassert(priv->rst);
> +	}
> +
> +	/* When clock speed is higher than 80MHz, set HFSEL */
> +	priv->common.hfsel = (clk_get_rate(priv->pclk) > 80000000UL);
> +	ret = regmap_update_bits(regmap, STM32_DAC_CR, STM32H7_DAC_CR_HFSEL,
> +				 priv->common.hfsel ? STM32H7_DAC_CR_HFSEL : 0);
> +	if (ret)
> +		goto err_pclk;
> +
> +	platform_set_drvdata(pdev, &priv->common);
> +
> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to populate DT children\n");
> +		goto err_pclk;
> +	}
> +
> +	return 0;
> +
> +err_pclk:
> +	clk_disable_unprepare(priv->pclk);
> +err_vref:
> +	regulator_disable(priv->vref);
> +
> +	return ret;
> +}
> +
> +static int stm32_dac_remove(struct platform_device *pdev)
> +{
> +	struct stm32_dac_common *common = platform_get_drvdata(pdev);
> +	struct stm32_dac_priv *priv = to_stm32_dac_priv(common);
> +
> +	of_platform_depopulate(&pdev->dev);
> +	clk_disable_unprepare(priv->pclk);
> +	regulator_disable(priv->vref);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id stm32_dac_of_match[] = {
> +	{ .compatible = "st,stm32h7-dac-core", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
> +
> +static struct platform_driver stm32_dac_driver = {
> +	.probe = stm32_dac_probe,
> +	.remove = stm32_dac_remove,
> +	.driver = {
> +		.name = "stm32-dac-core",
> +		.of_match_table = stm32_dac_of_match,
> +	},
> +};
> +module_platform_driver(stm32_dac_driver);
> +
> +MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
> +MODULE_DESCRIPTION("STMicroelectronics STM32 DAC core driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:stm32-dac-core");
> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
> new file mode 100644
> index 0000000..d3099f7
> --- /dev/null
> +++ b/drivers/iio/dac/stm32-dac-core.h
> @@ -0,0 +1,51 @@
> +/*
> + * This file is part of STM32 DAC driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
> + *
> + * License type: GPLv2
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __STM32_DAC_CORE_H
> +#define __STM32_DAC_CORE_H
> +
> +#include <linux/regmap.h>
> +
> +/* STM32 DAC registers */
> +#define STM32_DAC_CR		0x00
> +#define STM32_DAC_DHR12R1	0x08
> +#define STM32_DAC_DHR12R2	0x14
> +#define STM32_DAC_DOR1		0x2C
> +#define STM32_DAC_DOR2		0x30
> +
> +/* STM32_DAC_CR bit fields */
> +#define STM32_DAC_CR_EN1		BIT(0)
> +#define STM32H7_DAC_CR_HFSEL		BIT(15)
> +#define STM32_DAC_CR_EN2		BIT(16)
> +
> +/**
> + * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
> + * @regmap: DAC registers shared via regmap
> + * @vref_mv: reference voltage (mv)
> + * @hfsel: high speed bus clock
> + */
> +struct stm32_dac_common {
> +	struct regmap			*regmap;
> +	int				vref_mv;
> +	bool				hfsel;
> +};
> +
> +#endif
> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
> new file mode 100644
> index 0000000..ee9711d
> --- /dev/null
> +++ b/drivers/iio/dac/stm32-dac.c
> @@ -0,0 +1,296 @@
> +/*
> + * This file is part of STM32 DAC driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Authors: Amelie Delaunay <amelie.delaunay@st.com>
> + *	    Fabrice Gasnier <fabrice.gasnier@st.com>
> + *
> + * License type: GPLv2
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include "stm32-dac-core.h"
> +
> +#define STM32_DAC_CHANNEL_1		1
> +#define STM32_DAC_CHANNEL_2		2
> +
> +/**
> + * struct stm32_dac - private data of DAC driver
> + * @common:		reference to DAC common data
> + */
> +struct stm32_dac {
> +	struct stm32_dac_common *common;
> +};
> +
> +static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
> +{
> +	u32 en, val;
> +	int ret;
> +
> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
> +	if (ret < 0)
> +		return ret;
> +	if (channel == STM32_DAC_CHANNEL_1)
> +		en = FIELD_GET(STM32_DAC_CR_EN1, val);
> +	else
> +		en = FIELD_GET(STM32_DAC_CR_EN2, val);
> +
> +	return !!en;
> +}
> +
> +static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
> +		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
> +	int ret;
> +
> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "Enable failed\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * When HFSEL is set, it is not allowed to write the DHRx register
> +	 * during 8 clock cycles after the ENx bit is set. It is not allowed
> +	 * to make software/hardware trigger during this period neither.
> +	 */
> +	if (dac->common->hfsel)
> +		udelay(1);
> +
> +	return 0;
> +}
> +
> +static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
> +		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
> +	int ret;
> +
> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
> +	if (ret)
> +		dev_err(&indio_dev->dev, "Disable failed\n");
> +
> +	return ret;
> +}
> +
> +static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
> +{
> +	int ret;
> +
> +	if (channel == STM32_DAC_CHANNEL_1)
> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR1, val);
> +	else
> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR2, val);
> +
> +	return ret ? ret : IIO_VAL_INT;
> +}
> +
> +static int stm32_dac_set_value(struct stm32_dac *dac, int channel, int val)
> +{
> +	int ret;
> +
> +	if (channel == STM32_DAC_CHANNEL_1)
> +		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R1, val);
> +	else
> +		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R2, val);
> +
> +	return ret;
> +}
> +
> +static int stm32_dac_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int *val, int *val2, long mask)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return stm32_dac_get_value(dac, chan->channel, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = dac->common->vref_mv;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_ENABLE:
> +		ret = stm32_dac_is_enabled(dac, chan->channel);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int stm32_dac_write_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       int val, int val2, long mask)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return stm32_dac_set_value(dac, chan->channel, val);
> +	case IIO_CHAN_INFO_ENABLE:
> +		if (!!val)
> +			return stm32_dac_enable(indio_dev, chan->channel);
> +		else
> +			return stm32_dac_disable(indio_dev, chan->channel);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
> +					unsigned reg, unsigned writeval,
> +					unsigned *readval)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +
> +	if (!readval)
> +		return regmap_write(dac->common->regmap, reg, writeval);
> +	else
> +		return regmap_read(dac->common->regmap, reg, readval);
> +}
> +
> +static const struct iio_info stm32_dac_iio_info = {
> +	.read_raw = &stm32_dac_read_raw,
> +	.write_raw = &stm32_dac_write_raw,
> +	.debugfs_reg_access = &stm32_dac_debugfs_reg_access,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +#define STM32_DAC_CHANNEL(chan, name) {		\
> +	.type = IIO_VOLTAGE,			\
> +	.indexed = 1,				\
> +	.output = 1,				\
> +	.channel = chan,			\
> +	.info_mask_separate =			\
> +		BIT(IIO_CHAN_INFO_RAW) |	\
> +		BIT(IIO_CHAN_INFO_ENABLE) |	\
> +		BIT(IIO_CHAN_INFO_SCALE),	\
> +	.scan_type = {				\
> +		.sign = 'u',			\
> +		.realbits = 12,			\
> +		.storagebits = 16,		\
> +	},					\
> +	.datasheet_name = name,			\
> +}
> +
> +static const struct iio_chan_spec stm32_dac_channels[] = {
> +	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_1, "out1"),
> +	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_2, "out2"),
> +};
> +
> +static int stm32_dac_chan_of_init(struct iio_dev *indio_dev)
> +{
> +	struct device_node *np = indio_dev->dev.of_node;
> +	unsigned int i;
> +	u32 channel;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "st,dac-channel", &channel);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Failed to read st,dac-channel\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(stm32_dac_channels); i++) {
> +		if (stm32_dac_channels[i].channel == channel)
> +			break;
> +	}
> +	if (i >= ARRAY_SIZE(stm32_dac_channels)) {
> +		dev_err(&indio_dev->dev, "Invalid st,dac-channel\n");
> +		return -EINVAL;
> +	}
> +
> +	indio_dev->channels = &stm32_dac_channels[i];
> +	indio_dev->num_channels = 1;
> +
> +	return 0;
> +};
> +
> +static int stm32_dac_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct iio_dev *indio_dev;
> +	struct stm32_dac *dac;
> +	int ret;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*dac));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	dac = iio_priv(indio_dev);
> +	dac->common = dev_get_drvdata(pdev->dev.parent);
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->dev.of_node = pdev->dev.of_node;
> +	indio_dev->info = &stm32_dac_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = stm32_dac_chan_of_init(indio_dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int stm32_dac_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> +	iio_device_unregister(indio_dev);
use devm_iio_device_register and drop the remove entirely
(I guess this may make no sense once I've looked at later
patches however!)
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id stm32_dac_of_match[] = {
> +	{ .compatible = "st,stm32-dac", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
> +
> +static struct platform_driver stm32_dac_driver = {
> +	.probe = stm32_dac_probe,
> +	.remove = stm32_dac_remove,
> +	.driver = {
> +		.name = "stm32-dac",
> +		.of_match_table = stm32_dac_of_match,
> +	},
> +};
> +module_platform_driver(stm32_dac_driver);
> +
> +MODULE_ALIAS("platform:stm32-dac");
> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
> +MODULE_DESCRIPTION("STMicroelectronics STM32 DAC driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH 2/4] iio: dac: add support for stm32 DAC

[Fabrice Gasnier]

On 04/02/2017 01:32 PM, Jonathan Cameron wrote:
> On 31/03/17 12:45, Fabrice Gasnier wrote:
>> Add support for STMicroelectronics STM32 DAC. It's a 12-bit, voltage
>> output digital-to-analog converter. It has two output channels, each
>> with its own converter.
>> It supports 8 bits or 12bits left/right aligned data format. Only
>> 12bits right-aligned is used here. It has built-in noise or
>> triangle waveform generator, and supports external triggers for
>> conversions.
>> Each channel can be used independently, with separate trigger, then
>> separate IIO devices are used to handle this. Core driver is intended
>> to share common resources such as clock, reset, reference voltage and
>> registers.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Annoyingly my laptop just crashed mid way through reviewing this..
> 
Hi Jonathan,
I hope I have nothing to do with this ;-)

> Ah well, hopefully I'll remember everything (there wasn't much).
> 
> For DACs the 'enable' attribute is not normally used. Rather we
> use the powerdown one.  The reasoning being that we care about what
> the state is when it is powered down.  Even if that isn't controllable
> I would expect to see it exported as powerdown_mode with a fixed value.
> 
Ok, I'll try to use powerdown_mode in V2 as other DACs do. For now,
basically, I'll remap same functionality as 'enable' of this patch.

What do you mean by 'fixed value' ?

But, this also raise me one question:
Current patch use 'enable' to set EN bits in control register. Then, DAC
output goes from Hi-Z to buffered output.
There is also other power modes available. One of them is 'unbuffered':
output buffer can be disabled/bypassed.
This typically can save power, but it only makes sense to use it
depending on output load impedance (This is explained in AN3126 as you
pointed out in later patch).
Current patch uses buffered output (which suits all needs regarding
output load impedance). And the question is...

Should I expose this power modes to userland by using 'powerdown_mode' ?

OR... I'd rather rely on a dt property like st,dac-output-mode to manage
this, because buffered/unbuffered output modes depends on HW output load
impedance. Do you agree with this approach to use:
- powerdown_mode as Hi-Z / enable switch, with dedicated dt property to
set output power mode ?

Please let me know your opinion.

But, I think this can be part of another patchset...

> Other than that - looks pretty good to me.
> 
> Jonathan
> 
>> ---
>>  drivers/iio/dac/Kconfig          |  15 ++
>>  drivers/iio/dac/Makefile         |   2 +
>>  drivers/iio/dac/stm32-dac-core.c | 180 ++++++++++++++++++++++++
>>  drivers/iio/dac/stm32-dac-core.h |  51 +++++++
>>  drivers/iio/dac/stm32-dac.c      | 296 +++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 544 insertions(+)
>>  create mode 100644 drivers/iio/dac/stm32-dac-core.c
>>  create mode 100644 drivers/iio/dac/stm32-dac-core.h
>>  create mode 100644 drivers/iio/dac/stm32-dac.c
>>
>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>> index d3084028..7198648 100644
>> --- a/drivers/iio/dac/Kconfig
>> +++ b/drivers/iio/dac/Kconfig
>> @@ -274,6 +274,21 @@ config MCP4922
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called mcp4922.
>>  
>> +config STM32_DAC
>> +	tristate "STMicroelectronics STM32 DAC"
>> +	depends on (ARCH_STM32 && OF) || COMPILE_TEST
>> +	depends on REGULATOR
>> +	select STM32_DAC_CORE
>> +	help
>> +	  Say yes here to build support for STMicroelectronics STM32 Digital
>> +	  to Analog Converter (DAC).
>> +
>> +	  This driver can also be built as a module.  If so, the module
>> +	  will be called stm32-dac.
>> +
>> +config STM32_DAC_CORE
>> +	tristate
>> +
>>  config VF610_DAC
>>  	tristate "Vybrid vf610 DAC driver"
>>  	depends on OF
>> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
>> index f01bf4a..afe8ae7 100644
>> --- a/drivers/iio/dac/Makefile
>> +++ b/drivers/iio/dac/Makefile
>> @@ -29,4 +29,6 @@ obj-$(CONFIG_MAX517) += max517.o
>>  obj-$(CONFIG_MAX5821) += max5821.o
>>  obj-$(CONFIG_MCP4725) += mcp4725.o
>>  obj-$(CONFIG_MCP4922) += mcp4922.o
>> +obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
>> +obj-$(CONFIG_STM32_DAC) += stm32-dac.o
>>  obj-$(CONFIG_VF610_DAC) += vf610_dac.o
>> diff --git a/drivers/iio/dac/stm32-dac-core.c b/drivers/iio/dac/stm32-dac-core.c
>> new file mode 100644
>> index 0000000..75e4878
>> --- /dev/null
>> +++ b/drivers/iio/dac/stm32-dac-core.c
>> @@ -0,0 +1,180 @@
>> +/*
>> + * This file is part of STM32 DAC driver
>> + *
>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>> + * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
>> + *
>> + * License type: GPLv2
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>> + * or FITNESS FOR A PARTICULAR PURPOSE.
>> + * See the GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/reset.h>
>> +
>> +#include "stm32-dac-core.h"
>> +
>> +/**
>> + * struct stm32_dac_priv - stm32 DAC core private data
>> + * @pclk:		peripheral clock common for all DACs
>> + * @rst:		peripheral reset control
>> + * @vref:		regulator reference
>> + * @common:		Common data for all DAC instances
>> + */
>> +struct stm32_dac_priv {
>> +	struct clk *pclk;
>> +	struct reset_control *rst;
>> +	struct regulator *vref;
>> +	struct stm32_dac_common common;
>> +};
>> +
>> +static struct stm32_dac_priv *to_stm32_dac_priv(struct stm32_dac_common *com)
>> +{
>> +	return container_of(com, struct stm32_dac_priv, common);
>> +}
>> +
>> +static const struct regmap_config stm32_dac_regmap_cfg = {
>> +	.reg_bits = 32,
>> +	.val_bits = 32,
>> +	.reg_stride = sizeof(u32),
>> +	.max_register = 0x3fc,
>> +};
>> +
>> +static int stm32_dac_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct stm32_dac_priv *priv;
>> +	struct regmap *regmap;
>> +	struct resource *res;
>> +	void __iomem *mmio;
>> +	int ret;
>> +
>> +	if (!dev->of_node)
>> +		return -ENODEV;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	mmio = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(mmio))
>> +		return PTR_ERR(mmio);
>> +
>> +	regmap = devm_regmap_init_mmio(dev, mmio, &stm32_dac_regmap_cfg);
>> +	if (IS_ERR(regmap))
>> +		return PTR_ERR(regmap);
>> +	priv->common.regmap = regmap;
>> +
>> +	priv->vref = devm_regulator_get(dev, "vref");
>> +	if (IS_ERR(priv->vref)) {
>> +		ret = PTR_ERR(priv->vref);
>> +		dev_err(dev, "vref get failed, %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = regulator_enable(priv->vref);
>> +	if (ret < 0) {
>> +		dev_err(dev, "vref enable failed\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = regulator_get_voltage(priv->vref);
>> +	if (ret < 0) {
>> +		dev_err(dev, "vref get voltage failed, %d\n", ret);
>> +		goto err_vref;
>> +	}
>> +	priv->common.vref_mv = ret / 1000;
>> +	dev_dbg(dev, "vref+=%dmV\n", priv->common.vref_mv);
>> +
>> +	priv->pclk = devm_clk_get(dev, "pclk");
>> +	if (IS_ERR(priv->pclk)) {
>> +		ret = PTR_ERR(priv->pclk);
>> +		dev_err(dev, "pclk get failed\n");
>> +		goto err_vref;
>> +	}
>> +
>> +	ret = clk_prepare_enable(priv->pclk);
>> +	if (ret < 0) {
>> +		dev_err(dev, "pclk enable failed\n");
>> +		goto err_vref;
>> +	}
>> +
>> +	priv->rst = devm_reset_control_get(dev, NULL);
>> +	if (!IS_ERR(priv->rst)) {
>> +		reset_control_assert(priv->rst);
>> +		udelay(2);
>> +		reset_control_deassert(priv->rst);
>> +	}
>> +
>> +	/* When clock speed is higher than 80MHz, set HFSEL */
>> +	priv->common.hfsel = (clk_get_rate(priv->pclk) > 80000000UL);
>> +	ret = regmap_update_bits(regmap, STM32_DAC_CR, STM32H7_DAC_CR_HFSEL,
>> +				 priv->common.hfsel ? STM32H7_DAC_CR_HFSEL : 0);
>> +	if (ret)
>> +		goto err_pclk;
>> +
>> +	platform_set_drvdata(pdev, &priv->common);
>> +
>> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to populate DT children\n");
>> +		goto err_pclk;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_pclk:
>> +	clk_disable_unprepare(priv->pclk);
>> +err_vref:
>> +	regulator_disable(priv->vref);
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_dac_remove(struct platform_device *pdev)
>> +{
>> +	struct stm32_dac_common *common = platform_get_drvdata(pdev);
>> +	struct stm32_dac_priv *priv = to_stm32_dac_priv(common);
>> +
>> +	of_platform_depopulate(&pdev->dev);
>> +	clk_disable_unprepare(priv->pclk);
>> +	regulator_disable(priv->vref);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id stm32_dac_of_match[] = {
>> +	{ .compatible = "st,stm32h7-dac-core", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
>> +
>> +static struct platform_driver stm32_dac_driver = {
>> +	.probe = stm32_dac_probe,
>> +	.remove = stm32_dac_remove,
>> +	.driver = {
>> +		.name = "stm32-dac-core",
>> +		.of_match_table = stm32_dac_of_match,
>> +	},
>> +};
>> +module_platform_driver(stm32_dac_driver);
>> +
>> +MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
>> +MODULE_DESCRIPTION("STMicroelectronics STM32 DAC core driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:stm32-dac-core");
>> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
>> new file mode 100644
>> index 0000000..d3099f7
>> --- /dev/null
>> +++ b/drivers/iio/dac/stm32-dac-core.h
>> @@ -0,0 +1,51 @@
>> +/*
>> + * This file is part of STM32 DAC driver
>> + *
>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>> + * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
>> + *
>> + * License type: GPLv2
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>> + * or FITNESS FOR A PARTICULAR PURPOSE.
>> + * See the GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __STM32_DAC_CORE_H
>> +#define __STM32_DAC_CORE_H
>> +
>> +#include <linux/regmap.h>
>> +
>> +/* STM32 DAC registers */
>> +#define STM32_DAC_CR		0x00
>> +#define STM32_DAC_DHR12R1	0x08
>> +#define STM32_DAC_DHR12R2	0x14
>> +#define STM32_DAC_DOR1		0x2C
>> +#define STM32_DAC_DOR2		0x30
>> +
>> +/* STM32_DAC_CR bit fields */
>> +#define STM32_DAC_CR_EN1		BIT(0)
>> +#define STM32H7_DAC_CR_HFSEL		BIT(15)
>> +#define STM32_DAC_CR_EN2		BIT(16)
>> +
>> +/**
>> + * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
>> + * @regmap: DAC registers shared via regmap
>> + * @vref_mv: reference voltage (mv)
>> + * @hfsel: high speed bus clock
>> + */
>> +struct stm32_dac_common {
>> +	struct regmap			*regmap;
>> +	int				vref_mv;
>> +	bool				hfsel;
>> +};
>> +
>> +#endif
>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>> new file mode 100644
>> index 0000000..ee9711d
>> --- /dev/null
>> +++ b/drivers/iio/dac/stm32-dac.c
>> @@ -0,0 +1,296 @@
>> +/*
>> + * This file is part of STM32 DAC driver
>> + *
>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>> + * Authors: Amelie Delaunay <amelie.delaunay@st.com>
>> + *	    Fabrice Gasnier <fabrice.gasnier@st.com>
>> + *
>> + * License type: GPLv2
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>> + * or FITNESS FOR A PARTICULAR PURPOSE.
>> + * See the GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/delay.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "stm32-dac-core.h"
>> +
>> +#define STM32_DAC_CHANNEL_1		1
>> +#define STM32_DAC_CHANNEL_2		2
>> +
>> +/**
>> + * struct stm32_dac - private data of DAC driver
>> + * @common:		reference to DAC common data
>> + */
>> +struct stm32_dac {
>> +	struct stm32_dac_common *common;
>> +};
>> +
>> +static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
>> +{
>> +	u32 en, val;
>> +	int ret;
>> +
>> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +	if (channel == STM32_DAC_CHANNEL_1)
>> +		en = FIELD_GET(STM32_DAC_CR_EN1, val);
>> +	else
>> +		en = FIELD_GET(STM32_DAC_CR_EN2, val);
>> +
>> +	return !!en;
>> +}
>> +
>> +static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
>> +		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
>> +	if (ret < 0) {
>> +		dev_err(&indio_dev->dev, "Enable failed\n");
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * When HFSEL is set, it is not allowed to write the DHRx register
>> +	 * during 8 clock cycles after the ENx bit is set. It is not allowed
>> +	 * to make software/hardware trigger during this period neither.
>> +	 */
>> +	if (dac->common->hfsel)
>> +		udelay(1);
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
>> +		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
>> +	if (ret)
>> +		dev_err(&indio_dev->dev, "Disable failed\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
>> +{
>> +	int ret;
>> +
>> +	if (channel == STM32_DAC_CHANNEL_1)
>> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR1, val);
>> +	else
>> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR2, val);
>> +
>> +	return ret ? ret : IIO_VAL_INT;
>> +}
>> +
>> +static int stm32_dac_set_value(struct stm32_dac *dac, int channel, int val)
>> +{
>> +	int ret;
>> +
>> +	if (channel == STM32_DAC_CHANNEL_1)
>> +		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R1, val);
>> +	else
>> +		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R2, val);
>> +
>> +	return ret;
>> +}
>> +
>> +static int stm32_dac_read_raw(struct iio_dev *indio_dev,
>> +			      struct iio_chan_spec const *chan,
>> +			      int *val, int *val2, long mask)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		return stm32_dac_get_value(dac, chan->channel, val);
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = dac->common->vref_mv;
>> +		*val2 = chan->scan_type.realbits;
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>> +	case IIO_CHAN_INFO_ENABLE:
>> +		ret = stm32_dac_is_enabled(dac, chan->channel);
>> +		if (ret < 0)
>> +			return ret;
>> +		*val = ret;
>> +		return IIO_VAL_INT;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int stm32_dac_write_raw(struct iio_dev *indio_dev,
>> +			       struct iio_chan_spec const *chan,
>> +			       int val, int val2, long mask)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		return stm32_dac_set_value(dac, chan->channel, val);
>> +	case IIO_CHAN_INFO_ENABLE:
>> +		if (!!val)
>> +			return stm32_dac_enable(indio_dev, chan->channel);
>> +		else
>> +			return stm32_dac_disable(indio_dev, chan->channel);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>> +					unsigned reg, unsigned writeval,
>> +					unsigned *readval)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +
>> +	if (!readval)
>> +		return regmap_write(dac->common->regmap, reg, writeval);
>> +	else
>> +		return regmap_read(dac->common->regmap, reg, readval);
>> +}
>> +
>> +static const struct iio_info stm32_dac_iio_info = {
>> +	.read_raw = &stm32_dac_read_raw,
>> +	.write_raw = &stm32_dac_write_raw,
>> +	.debugfs_reg_access = &stm32_dac_debugfs_reg_access,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +#define STM32_DAC_CHANNEL(chan, name) {		\
>> +	.type = IIO_VOLTAGE,			\
>> +	.indexed = 1,				\
>> +	.output = 1,				\
>> +	.channel = chan,			\
>> +	.info_mask_separate =			\
>> +		BIT(IIO_CHAN_INFO_RAW) |	\
>> +		BIT(IIO_CHAN_INFO_ENABLE) |	\
>> +		BIT(IIO_CHAN_INFO_SCALE),	\
>> +	.scan_type = {				\
>> +		.sign = 'u',			\
>> +		.realbits = 12,			\
>> +		.storagebits = 16,		\
>> +	},					\
>> +	.datasheet_name = name,			\
>> +}
>> +
>> +static const struct iio_chan_spec stm32_dac_channels[] = {
>> +	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_1, "out1"),
>> +	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_2, "out2"),
>> +};
>> +
>> +static int stm32_dac_chan_of_init(struct iio_dev *indio_dev)
>> +{
>> +	struct device_node *np = indio_dev->dev.of_node;
>> +	unsigned int i;
>> +	u32 channel;
>> +	int ret;
>> +
>> +	ret = of_property_read_u32(np, "st,dac-channel", &channel);
>> +	if (ret) {
>> +		dev_err(&indio_dev->dev, "Failed to read st,dac-channel\n");
>> +		return ret;
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(stm32_dac_channels); i++) {
>> +		if (stm32_dac_channels[i].channel == channel)
>> +			break;
>> +	}
>> +	if (i >= ARRAY_SIZE(stm32_dac_channels)) {
>> +		dev_err(&indio_dev->dev, "Invalid st,dac-channel\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	indio_dev->channels = &stm32_dac_channels[i];
>> +	indio_dev->num_channels = 1;
>> +
>> +	return 0;
>> +};
>> +
>> +static int stm32_dac_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	struct iio_dev *indio_dev;
>> +	struct stm32_dac *dac;
>> +	int ret;
>> +
>> +	if (!np)
>> +		return -ENODEV;
>> +
>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*dac));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +	platform_set_drvdata(pdev, indio_dev);
>> +
>> +	dac = iio_priv(indio_dev);
>> +	dac->common = dev_get_drvdata(pdev->dev.parent);
>> +	indio_dev->name = dev_name(&pdev->dev);
>> +	indio_dev->dev.parent = &pdev->dev;
>> +	indio_dev->dev.of_node = pdev->dev.of_node;
>> +	indio_dev->info = &stm32_dac_iio_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	ret = stm32_dac_chan_of_init(indio_dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_dac_remove(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +
>> +	iio_device_unregister(indio_dev);
> use devm_iio_device_register and drop the remove entirely
> (I guess this may make no sense once I've looked at later
> patches however!)
Good guess ;-)
But okay, I'll use devm_ anyway, regardless of other patches.

Thanks & Regards,
Fabrice

>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id stm32_dac_of_match[] = {
>> +	{ .compatible = "st,stm32-dac", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
>> +
>> +static struct platform_driver stm32_dac_driver = {
>> +	.probe = stm32_dac_probe,
>> +	.remove = stm32_dac_remove,
>> +	.driver = {
>> +		.name = "stm32-dac",
>> +		.of_match_table = stm32_dac_of_match,
>> +	},
>> +};
>> +module_platform_driver(stm32_dac_driver);
>> +
>> +MODULE_ALIAS("platform:stm32-dac");
>> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
>> +MODULE_DESCRIPTION("STMicroelectronics STM32 DAC driver");
>> +MODULE_LICENSE("GPL v2");
>>
> 

Re: [PATCH 2/4] iio: dac: add support for stm32 DAC

[Jonathan Cameron]

On 05/04/17 16:48, Fabrice Gasnier wrote:
> On 04/02/2017 01:32 PM, Jonathan Cameron wrote:
>> On 31/03/17 12:45, Fabrice Gasnier wrote:
>>> Add support for STMicroelectronics STM32 DAC. It's a 12-bit, voltage
>>> output digital-to-analog converter. It has two output channels, each
>>> with its own converter.
>>> It supports 8 bits or 12bits left/right aligned data format. Only
>>> 12bits right-aligned is used here. It has built-in noise or
>>> triangle waveform generator, and supports external triggers for
>>> conversions.
>>> Each channel can be used independently, with separate trigger, then
>>> separate IIO devices are used to handle this. Core driver is intended
>>> to share common resources such as clock, reset, reference voltage and
>>> registers.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> Annoyingly my laptop just crashed mid way through reviewing this..
>>
> Hi Jonathan,
> I hope I have nothing to do with this ;-)
> 
>> Ah well, hopefully I'll remember everything (there wasn't much).
>>
>> For DACs the 'enable' attribute is not normally used. Rather we
>> use the powerdown one.  The reasoning being that we care about what
>> the state is when it is powered down.  Even if that isn't controllable
>> I would expect to see it exported as powerdown_mode with a fixed value.
>>
> Ok, I'll try to use powerdown_mode in V2 as other DACs do. For now,
> basically, I'll remap same functionality as 'enable' of this patch.
> 
> What do you mean by 'fixed value' ?
Read only with only one value.
> 
> But, this also raise me one question:
> Current patch use 'enable' to set EN bits in control register. Then, DAC
> output goes from Hi-Z to buffered output.
> There is also other power modes available. One of them is 'unbuffered':
> output buffer can be disabled/bypassed.
> This typically can save power, but it only makes sense to use it
> depending on output load impedance (This is explained in AN3126 as you
> pointed out in later patch).
> Current patch uses buffered output (which suits all needs regarding
> output load impedance). And the question is...
> 
> Should I expose this power modes to userland by using 'powerdown_mode' ?
> 
> OR... I'd rather rely on a dt property like st,dac-output-mode to manage
> this, because buffered/unbuffered output modes depends on HW output load
> impedance. Do you agree with this approach to use:
> - powerdown_mode as Hi-Z / enable switch, with dedicated dt property to
> set output power mode ?
Yes.  If it's hardware dependant like this it belongs in dt to my mind.
> 
> Please let me know your opinion.
> 
> But, I think this can be part of another patchset...
Absolutely.  No need to support all the bells and whistles
from the start!
> 
>> Other than that - looks pretty good to me.
>>
>> Jonathan
>>
>>> ---
>>>  drivers/iio/dac/Kconfig          |  15 ++
>>>  drivers/iio/dac/Makefile         |   2 +
>>>  drivers/iio/dac/stm32-dac-core.c | 180 ++++++++++++++++++++++++
>>>  drivers/iio/dac/stm32-dac-core.h |  51 +++++++
>>>  drivers/iio/dac/stm32-dac.c      | 296 +++++++++++++++++++++++++++++++++++++++
>>>  5 files changed, 544 insertions(+)
>>>  create mode 100644 drivers/iio/dac/stm32-dac-core.c
>>>  create mode 100644 drivers/iio/dac/stm32-dac-core.h
>>>  create mode 100644 drivers/iio/dac/stm32-dac.c
>>>
>>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>>> index d3084028..7198648 100644
>>> --- a/drivers/iio/dac/Kconfig
>>> +++ b/drivers/iio/dac/Kconfig
>>> @@ -274,6 +274,21 @@ config MCP4922
>>>  	  To compile this driver as a module, choose M here: the module
>>>  	  will be called mcp4922.
>>>  
>>> +config STM32_DAC
>>> +	tristate "STMicroelectronics STM32 DAC"
>>> +	depends on (ARCH_STM32 && OF) || COMPILE_TEST
>>> +	depends on REGULATOR
>>> +	select STM32_DAC_CORE
>>> +	help
>>> +	  Say yes here to build support for STMicroelectronics STM32 Digital
>>> +	  to Analog Converter (DAC).
>>> +
>>> +	  This driver can also be built as a module.  If so, the module
>>> +	  will be called stm32-dac.
>>> +
>>> +config STM32_DAC_CORE
>>> +	tristate
>>> +
>>>  config VF610_DAC
>>>  	tristate "Vybrid vf610 DAC driver"
>>>  	depends on OF
>>> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
>>> index f01bf4a..afe8ae7 100644
>>> --- a/drivers/iio/dac/Makefile
>>> +++ b/drivers/iio/dac/Makefile
>>> @@ -29,4 +29,6 @@ obj-$(CONFIG_MAX517) += max517.o
>>>  obj-$(CONFIG_MAX5821) += max5821.o
>>>  obj-$(CONFIG_MCP4725) += mcp4725.o
>>>  obj-$(CONFIG_MCP4922) += mcp4922.o
>>> +obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
>>> +obj-$(CONFIG_STM32_DAC) += stm32-dac.o
>>>  obj-$(CONFIG_VF610_DAC) += vf610_dac.o
>>> diff --git a/drivers/iio/dac/stm32-dac-core.c b/drivers/iio/dac/stm32-dac-core.c
>>> new file mode 100644
>>> index 0000000..75e4878
>>> --- /dev/null
>>> +++ b/drivers/iio/dac/stm32-dac-core.c
>>> @@ -0,0 +1,180 @@
>>> +/*
>>> + * This file is part of STM32 DAC driver
>>> + *
>>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>>> + * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
>>> + *
>>> + * License type: GPLv2
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License version 2 as published by
>>> + * the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but
>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>>> + * or FITNESS FOR A PARTICULAR PURPOSE.
>>> + * See the GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along with
>>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/reset.h>
>>> +
>>> +#include "stm32-dac-core.h"
>>> +
>>> +/**
>>> + * struct stm32_dac_priv - stm32 DAC core private data
>>> + * @pclk:		peripheral clock common for all DACs
>>> + * @rst:		peripheral reset control
>>> + * @vref:		regulator reference
>>> + * @common:		Common data for all DAC instances
>>> + */
>>> +struct stm32_dac_priv {
>>> +	struct clk *pclk;
>>> +	struct reset_control *rst;
>>> +	struct regulator *vref;
>>> +	struct stm32_dac_common common;
>>> +};
>>> +
>>> +static struct stm32_dac_priv *to_stm32_dac_priv(struct stm32_dac_common *com)
>>> +{
>>> +	return container_of(com, struct stm32_dac_priv, common);
>>> +}
>>> +
>>> +static const struct regmap_config stm32_dac_regmap_cfg = {
>>> +	.reg_bits = 32,
>>> +	.val_bits = 32,
>>> +	.reg_stride = sizeof(u32),
>>> +	.max_register = 0x3fc,
>>> +};
>>> +
>>> +static int stm32_dac_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct stm32_dac_priv *priv;
>>> +	struct regmap *regmap;
>>> +	struct resource *res;
>>> +	void __iomem *mmio;
>>> +	int ret;
>>> +
>>> +	if (!dev->of_node)
>>> +		return -ENODEV;
>>> +
>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	mmio = devm_ioremap_resource(dev, res);
>>> +	if (IS_ERR(mmio))
>>> +		return PTR_ERR(mmio);
>>> +
>>> +	regmap = devm_regmap_init_mmio(dev, mmio, &stm32_dac_regmap_cfg);
>>> +	if (IS_ERR(regmap))
>>> +		return PTR_ERR(regmap);
>>> +	priv->common.regmap = regmap;
>>> +
>>> +	priv->vref = devm_regulator_get(dev, "vref");
>>> +	if (IS_ERR(priv->vref)) {
>>> +		ret = PTR_ERR(priv->vref);
>>> +		dev_err(dev, "vref get failed, %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = regulator_enable(priv->vref);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "vref enable failed\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = regulator_get_voltage(priv->vref);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "vref get voltage failed, %d\n", ret);
>>> +		goto err_vref;
>>> +	}
>>> +	priv->common.vref_mv = ret / 1000;
>>> +	dev_dbg(dev, "vref+=%dmV\n", priv->common.vref_mv);
>>> +
>>> +	priv->pclk = devm_clk_get(dev, "pclk");
>>> +	if (IS_ERR(priv->pclk)) {
>>> +		ret = PTR_ERR(priv->pclk);
>>> +		dev_err(dev, "pclk get failed\n");
>>> +		goto err_vref;
>>> +	}
>>> +
>>> +	ret = clk_prepare_enable(priv->pclk);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "pclk enable failed\n");
>>> +		goto err_vref;
>>> +	}
>>> +
>>> +	priv->rst = devm_reset_control_get(dev, NULL);
>>> +	if (!IS_ERR(priv->rst)) {
>>> +		reset_control_assert(priv->rst);
>>> +		udelay(2);
>>> +		reset_control_deassert(priv->rst);
>>> +	}
>>> +
>>> +	/* When clock speed is higher than 80MHz, set HFSEL */
>>> +	priv->common.hfsel = (clk_get_rate(priv->pclk) > 80000000UL);
>>> +	ret = regmap_update_bits(regmap, STM32_DAC_CR, STM32H7_DAC_CR_HFSEL,
>>> +				 priv->common.hfsel ? STM32H7_DAC_CR_HFSEL : 0);
>>> +	if (ret)
>>> +		goto err_pclk;
>>> +
>>> +	platform_set_drvdata(pdev, &priv->common);
>>> +
>>> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "failed to populate DT children\n");
>>> +		goto err_pclk;
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +err_pclk:
>>> +	clk_disable_unprepare(priv->pclk);
>>> +err_vref:
>>> +	regulator_disable(priv->vref);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int stm32_dac_remove(struct platform_device *pdev)
>>> +{
>>> +	struct stm32_dac_common *common = platform_get_drvdata(pdev);
>>> +	struct stm32_dac_priv *priv = to_stm32_dac_priv(common);
>>> +
>>> +	of_platform_depopulate(&pdev->dev);
>>> +	clk_disable_unprepare(priv->pclk);
>>> +	regulator_disable(priv->vref);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id stm32_dac_of_match[] = {
>>> +	{ .compatible = "st,stm32h7-dac-core", },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
>>> +
>>> +static struct platform_driver stm32_dac_driver = {
>>> +	.probe = stm32_dac_probe,
>>> +	.remove = stm32_dac_remove,
>>> +	.driver = {
>>> +		.name = "stm32-dac-core",
>>> +		.of_match_table = stm32_dac_of_match,
>>> +	},
>>> +};
>>> +module_platform_driver(stm32_dac_driver);
>>> +
>>> +MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
>>> +MODULE_DESCRIPTION("STMicroelectronics STM32 DAC core driver");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_ALIAS("platform:stm32-dac-core");
>>> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
>>> new file mode 100644
>>> index 0000000..d3099f7
>>> --- /dev/null
>>> +++ b/drivers/iio/dac/stm32-dac-core.h
>>> @@ -0,0 +1,51 @@
>>> +/*
>>> + * This file is part of STM32 DAC driver
>>> + *
>>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>>> + * Author: Fabrice Gasnier <fabrice.gasnier@st.com>.
>>> + *
>>> + * License type: GPLv2
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License version 2 as published by
>>> + * the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but
>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>>> + * or FITNESS FOR A PARTICULAR PURPOSE.
>>> + * See the GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along with
>>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#ifndef __STM32_DAC_CORE_H
>>> +#define __STM32_DAC_CORE_H
>>> +
>>> +#include <linux/regmap.h>
>>> +
>>> +/* STM32 DAC registers */
>>> +#define STM32_DAC_CR		0x00
>>> +#define STM32_DAC_DHR12R1	0x08
>>> +#define STM32_DAC_DHR12R2	0x14
>>> +#define STM32_DAC_DOR1		0x2C
>>> +#define STM32_DAC_DOR2		0x30
>>> +
>>> +/* STM32_DAC_CR bit fields */
>>> +#define STM32_DAC_CR_EN1		BIT(0)
>>> +#define STM32H7_DAC_CR_HFSEL		BIT(15)
>>> +#define STM32_DAC_CR_EN2		BIT(16)
>>> +
>>> +/**
>>> + * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
>>> + * @regmap: DAC registers shared via regmap
>>> + * @vref_mv: reference voltage (mv)
>>> + * @hfsel: high speed bus clock
>>> + */
>>> +struct stm32_dac_common {
>>> +	struct regmap			*regmap;
>>> +	int				vref_mv;
>>> +	bool				hfsel;
>>> +};
>>> +
>>> +#endif
>>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>>> new file mode 100644
>>> index 0000000..ee9711d
>>> --- /dev/null
>>> +++ b/drivers/iio/dac/stm32-dac.c
>>> @@ -0,0 +1,296 @@
>>> +/*
>>> + * This file is part of STM32 DAC driver
>>> + *
>>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>>> + * Authors: Amelie Delaunay <amelie.delaunay@st.com>
>>> + *	    Fabrice Gasnier <fabrice.gasnier@st.com>
>>> + *
>>> + * License type: GPLv2
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License version 2 as published by
>>> + * the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but
>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>>> + * or FITNESS FOR A PARTICULAR PURPOSE.
>>> + * See the GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along with
>>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include <linux/bitfield.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#include "stm32-dac-core.h"
>>> +
>>> +#define STM32_DAC_CHANNEL_1		1
>>> +#define STM32_DAC_CHANNEL_2		2
>>> +
>>> +/**
>>> + * struct stm32_dac - private data of DAC driver
>>> + * @common:		reference to DAC common data
>>> + */
>>> +struct stm32_dac {
>>> +	struct stm32_dac_common *common;
>>> +};
>>> +
>>> +static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
>>> +{
>>> +	u32 en, val;
>>> +	int ret;
>>> +
>>> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +	if (channel == STM32_DAC_CHANNEL_1)
>>> +		en = FIELD_GET(STM32_DAC_CR_EN1, val);
>>> +	else
>>> +		en = FIELD_GET(STM32_DAC_CR_EN2, val);
>>> +
>>> +	return !!en;
>>> +}
>>> +
>>> +static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
>>> +		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>>> +	int ret;
>>> +
>>> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
>>> +	if (ret < 0) {
>>> +		dev_err(&indio_dev->dev, "Enable failed\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	/*
>>> +	 * When HFSEL is set, it is not allowed to write the DHRx register
>>> +	 * during 8 clock cycles after the ENx bit is set. It is not allowed
>>> +	 * to make software/hardware trigger during this period neither.
>>> +	 */
>>> +	if (dac->common->hfsel)
>>> +		udelay(1);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +	u32 en = (channel == STM32_DAC_CHANNEL_1) ?
>>> +		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>>> +	int ret;
>>> +
>>> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
>>> +	if (ret)
>>> +		dev_err(&indio_dev->dev, "Disable failed\n");
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (channel == STM32_DAC_CHANNEL_1)
>>> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR1, val);
>>> +	else
>>> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DOR2, val);
>>> +
>>> +	return ret ? ret : IIO_VAL_INT;
>>> +}
>>> +
>>> +static int stm32_dac_set_value(struct stm32_dac *dac, int channel, int val)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (channel == STM32_DAC_CHANNEL_1)
>>> +		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R1, val);
>>> +	else
>>> +		ret = regmap_write(dac->common->regmap, STM32_DAC_DHR12R2, val);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int stm32_dac_read_raw(struct iio_dev *indio_dev,
>>> +			      struct iio_chan_spec const *chan,
>>> +			      int *val, int *val2, long mask)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +	int ret;
>>> +
>>> +	switch (mask) {
>>> +	case IIO_CHAN_INFO_RAW:
>>> +		return stm32_dac_get_value(dac, chan->channel, val);
>>> +	case IIO_CHAN_INFO_SCALE:
>>> +		*val = dac->common->vref_mv;
>>> +		*val2 = chan->scan_type.realbits;
>>> +		return IIO_VAL_FRACTIONAL_LOG2;
>>> +	case IIO_CHAN_INFO_ENABLE:
>>> +		ret = stm32_dac_is_enabled(dac, chan->channel);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +		*val = ret;
>>> +		return IIO_VAL_INT;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +}
>>> +
>>> +static int stm32_dac_write_raw(struct iio_dev *indio_dev,
>>> +			       struct iio_chan_spec const *chan,
>>> +			       int val, int val2, long mask)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +
>>> +	switch (mask) {
>>> +	case IIO_CHAN_INFO_RAW:
>>> +		return stm32_dac_set_value(dac, chan->channel, val);
>>> +	case IIO_CHAN_INFO_ENABLE:
>>> +		if (!!val)
>>> +			return stm32_dac_enable(indio_dev, chan->channel);
>>> +		else
>>> +			return stm32_dac_disable(indio_dev, chan->channel);
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +}
>>> +
>>> +static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>>> +					unsigned reg, unsigned writeval,
>>> +					unsigned *readval)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +
>>> +	if (!readval)
>>> +		return regmap_write(dac->common->regmap, reg, writeval);
>>> +	else
>>> +		return regmap_read(dac->common->regmap, reg, readval);
>>> +}
>>> +
>>> +static const struct iio_info stm32_dac_iio_info = {
>>> +	.read_raw = &stm32_dac_read_raw,
>>> +	.write_raw = &stm32_dac_write_raw,
>>> +	.debugfs_reg_access = &stm32_dac_debugfs_reg_access,
>>> +	.driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +#define STM32_DAC_CHANNEL(chan, name) {		\
>>> +	.type = IIO_VOLTAGE,			\
>>> +	.indexed = 1,				\
>>> +	.output = 1,				\
>>> +	.channel = chan,			\
>>> +	.info_mask_separate =			\
>>> +		BIT(IIO_CHAN_INFO_RAW) |	\
>>> +		BIT(IIO_CHAN_INFO_ENABLE) |	\
>>> +		BIT(IIO_CHAN_INFO_SCALE),	\
>>> +	.scan_type = {				\
>>> +		.sign = 'u',			\
>>> +		.realbits = 12,			\
>>> +		.storagebits = 16,		\
>>> +	},					\
>>> +	.datasheet_name = name,			\
>>> +}
>>> +
>>> +static const struct iio_chan_spec stm32_dac_channels[] = {
>>> +	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_1, "out1"),
>>> +	STM32_DAC_CHANNEL(STM32_DAC_CHANNEL_2, "out2"),
>>> +};
>>> +
>>> +static int stm32_dac_chan_of_init(struct iio_dev *indio_dev)
>>> +{
>>> +	struct device_node *np = indio_dev->dev.of_node;
>>> +	unsigned int i;
>>> +	u32 channel;
>>> +	int ret;
>>> +
>>> +	ret = of_property_read_u32(np, "st,dac-channel", &channel);
>>> +	if (ret) {
>>> +		dev_err(&indio_dev->dev, "Failed to read st,dac-channel\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(stm32_dac_channels); i++) {
>>> +		if (stm32_dac_channels[i].channel == channel)
>>> +			break;
>>> +	}
>>> +	if (i >= ARRAY_SIZE(stm32_dac_channels)) {
>>> +		dev_err(&indio_dev->dev, "Invalid st,dac-channel\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	indio_dev->channels = &stm32_dac_channels[i];
>>> +	indio_dev->num_channels = 1;
>>> +
>>> +	return 0;
>>> +};
>>> +
>>> +static int stm32_dac_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device_node *np = pdev->dev.of_node;
>>> +	struct iio_dev *indio_dev;
>>> +	struct stm32_dac *dac;
>>> +	int ret;
>>> +
>>> +	if (!np)
>>> +		return -ENODEV;
>>> +
>>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*dac));
>>> +	if (!indio_dev)
>>> +		return -ENOMEM;
>>> +	platform_set_drvdata(pdev, indio_dev);
>>> +
>>> +	dac = iio_priv(indio_dev);
>>> +	dac->common = dev_get_drvdata(pdev->dev.parent);
>>> +	indio_dev->name = dev_name(&pdev->dev);
>>> +	indio_dev->dev.parent = &pdev->dev;
>>> +	indio_dev->dev.of_node = pdev->dev.of_node;
>>> +	indio_dev->info = &stm32_dac_iio_info;
>>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>>> +
>>> +	ret = stm32_dac_chan_of_init(indio_dev);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	ret = iio_device_register(indio_dev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int stm32_dac_remove(struct platform_device *pdev)
>>> +{
>>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> +
>>> +	iio_device_unregister(indio_dev);
>> use devm_iio_device_register and drop the remove entirely
>> (I guess this may make no sense once I've looked at later
>> patches however!)
> Good guess ;-)
> But okay, I'll use devm_ anyway, regardless of other patches.
> 
> Thanks & Regards,
> Fabrice
> 
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id stm32_dac_of_match[] = {
>>> +	{ .compatible = "st,stm32-dac", },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, stm32_dac_of_match);
>>> +
>>> +static struct platform_driver stm32_dac_driver = {
>>> +	.probe = stm32_dac_probe,
>>> +	.remove = stm32_dac_remove,
>>> +	.driver = {
>>> +		.name = "stm32-dac",
>>> +		.of_match_table = stm32_dac_of_match,
>>> +	},
>>> +};
>>> +module_platform_driver(stm32_dac_driver);
>>> +
>>> +MODULE_ALIAS("platform:stm32-dac");
>>> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
>>> +MODULE_DESCRIPTION("STMicroelectronics STM32 DAC driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
> --
> 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
> 

[PATCH 3/4] iio: dac: stm32: add support for trigger events

[Fabrice Gasnier]

STM32 DAC supports triggers to synchronize conversions. When trigger
occurs, data is transferred from DHR (data holding register) to DOR
(data output register) so output voltage is updated.
Both hardware and software triggers are supported.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/iio/dac/Kconfig          |   3 +
 drivers/iio/dac/stm32-dac-core.h |  12 ++++
 drivers/iio/dac/stm32-dac.c      | 124 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 136 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 7198648..786c38b 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -278,6 +278,9 @@ config STM32_DAC
 	tristate "STMicroelectronics STM32 DAC"
 	depends on (ARCH_STM32 && OF) || COMPILE_TEST
 	depends on REGULATOR
+	select IIO_TRIGGERED_EVENT
+	select IIO_STM32_TIMER_TRIGGER
+	select MFD_STM32_TIMERS
 	select STM32_DAC_CORE
 	help
 	  Say yes here to build support for STMicroelectronics STM32 Digital
diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
index d3099f7..3bf211c 100644
--- a/drivers/iio/dac/stm32-dac-core.h
+++ b/drivers/iio/dac/stm32-dac-core.h
@@ -26,6 +26,7 @@
 
 /* STM32 DAC registers */
 #define STM32_DAC_CR		0x00
+#define STM32_DAC_SWTRIGR	0x04
 #define STM32_DAC_DHR12R1	0x08
 #define STM32_DAC_DHR12R2	0x14
 #define STM32_DAC_DOR1		0x2C
@@ -33,8 +34,19 @@
 
 /* STM32_DAC_CR bit fields */
 #define STM32_DAC_CR_EN1		BIT(0)
+#define STM32H7_DAC_CR_TEN1		BIT(1)
+#define STM32H7_DAC_CR_TSEL1_SHIFT	2
+#define STM32H7_DAC_CR_TSEL1		GENMASK(5, 2)
+#define STM32_DAC_CR_WAVE1		GENMASK(7, 6)
+#define STM32_DAC_CR_MAMP1		GENMASK(11, 8)
 #define STM32H7_DAC_CR_HFSEL		BIT(15)
 #define STM32_DAC_CR_EN2		BIT(16)
+#define STM32_DAC_CR_WAVE2		GENMASK(23, 22)
+#define STM32_DAC_CR_MAMP2		GENMASK(27, 24)
+
+/* STM32_DAC_SWTRIGR bit fields */
+#define STM32_DAC_SWTRIGR_SWTRIG1	BIT(0)
+#define STM32_DAC_SWTRIGR_SWTRIG2	BIT(1)
 
 /**
  * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
index ee9711d..62e43e9 100644
--- a/drivers/iio/dac/stm32-dac.c
+++ b/drivers/iio/dac/stm32-dac.c
@@ -23,6 +23,10 @@
 #include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/timer/stm32-timer-trigger.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_event.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -31,15 +35,112 @@
 
 #define STM32_DAC_CHANNEL_1		1
 #define STM32_DAC_CHANNEL_2		2
+/* channel2 shift */
+#define STM32_DAC_CHAN2_SHIFT		16
 
 /**
  * struct stm32_dac - private data of DAC driver
  * @common:		reference to DAC common data
+ * @swtrig:		Using software trigger
  */
 struct stm32_dac {
 	struct stm32_dac_common *common;
+	bool swtrig;
 };
 
+/**
+ * struct stm32_dac_trig_info - DAC trigger info
+ * @name: name of the trigger, corresponding to its source
+ * @tsel: trigger selection, value to be configured in DAC_CR.TSELx
+ */
+struct stm32_dac_trig_info {
+	const char *name;
+	u32 tsel;
+};
+
+static const struct stm32_dac_trig_info stm32h7_dac_trinfo[] = {
+	{ "swtrig", 0 },
+	{ TIM1_TRGO, 1 },
+	{ TIM2_TRGO, 2 },
+	{ TIM4_TRGO, 3 },
+	{ TIM5_TRGO, 4 },
+	{ TIM6_TRGO, 5 },
+	{ TIM7_TRGO, 6 },
+	{ TIM8_TRGO, 7 },
+	{},
+};
+
+static irqreturn_t stm32_dac_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct stm32_dac *dac = iio_priv(indio_dev);
+	int channel = indio_dev->channels[0].channel;
+
+	/* Using software trigger? Then, trigger it now */
+	if (dac->swtrig) {
+		u32 swtrig;
+
+		if (channel == STM32_DAC_CHANNEL_1)
+			swtrig = STM32_DAC_SWTRIGR_SWTRIG1;
+		else
+			swtrig = STM32_DAC_SWTRIGR_SWTRIG2;
+		regmap_update_bits(dac->common->regmap, STM32_DAC_SWTRIGR,
+				   swtrig, swtrig);
+	}
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static unsigned int stm32_dac_get_trig_tsel(struct stm32_dac *dac,
+					    struct iio_trigger *trig)
+{
+	unsigned int i;
+
+	/* skip 1st trigger that should be swtrig */
+	for (i = 1; stm32h7_dac_trinfo[i].name; i++) {
+		/*
+		 * Checking both stm32 timer trigger type and trig name
+		 * should be safe against arbitrary trigger names.
+		 */
+		if (is_stm32_timer_trigger(trig) &&
+		    !strcmp(stm32h7_dac_trinfo[i].name, trig->name)) {
+			return stm32h7_dac_trinfo[i].tsel;
+		}
+	}
+
+	/* When no trigger has been found, default to software trigger */
+	dac->swtrig = true;
+
+	return stm32h7_dac_trinfo[0].tsel;
+}
+
+static int stm32_dac_set_trig(struct stm32_dac *dac, struct iio_trigger *trig,
+			      int channel)
+{
+	struct iio_dev *indio_dev = iio_priv_to_dev(dac);
+	u32 shift = channel == STM32_DAC_CHANNEL_1 ? 0 : STM32_DAC_CHAN2_SHIFT;
+	u32 val = 0, tsel;
+	u32 msk = (STM32H7_DAC_CR_TEN1 | STM32H7_DAC_CR_TSEL1) << shift;
+
+	dac->swtrig = false;
+	if (trig) {
+		/* select & enable trigger (tsel / ten) */
+		tsel = stm32_dac_get_trig_tsel(dac, trig);
+		val = tsel << STM32H7_DAC_CR_TSEL1_SHIFT;
+		val = (val | STM32H7_DAC_CR_TEN1) << shift;
+	}
+
+	if (trig)
+		dev_dbg(&indio_dev->dev, "enable trigger: %s\n", trig->name);
+	else
+		dev_dbg(&indio_dev->dev, "disable trigger\n");
+
+	return regmap_update_bits(dac->common->regmap, STM32_DAC_CR, msk, val);
+}
+
 static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
 {
 	u32 en, val;
@@ -63,9 +164,16 @@ static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
 		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
 	int ret;
 
+	ret = stm32_dac_set_trig(dac, indio_dev->trig, channel);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev, "Trigger setup failed\n");
+		return ret;
+	}
+
 	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
 	if (ret < 0) {
 		dev_err(&indio_dev->dev, "Enable failed\n");
+		stm32_dac_set_trig(dac, NULL, channel);
 		return ret;
 	}
 
@@ -88,10 +196,12 @@ static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
 	int ret;
 
 	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
-	if (ret)
+	if (ret) {
 		dev_err(&indio_dev->dev, "Disable failed\n");
+		return ret;
+	}
 
-	return ret;
+	return stm32_dac_set_trig(dac, NULL, channel);
 }
 
 static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
@@ -258,10 +368,17 @@ static int stm32_dac_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	ret = iio_device_register(indio_dev);
+	ret = iio_triggered_event_setup(indio_dev, NULL,
+					stm32_dac_trigger_handler);
 	if (ret)
 		return ret;
 
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		iio_triggered_event_cleanup(indio_dev);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -269,6 +386,7 @@ static int stm32_dac_remove(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 
+	iio_triggered_event_cleanup(indio_dev);
 	iio_device_unregister(indio_dev);
 
 	return 0;
-- 
1.9.1

Re: [PATCH 3/4] iio: dac: stm32: add support for trigger events

[Jonathan Cameron]

On 31/03/17 12:45, Fabrice Gasnier wrote:
> STM32 DAC supports triggers to synchronize conversions. When trigger
> occurs, data is transferred from DHR (data holding register) to DOR
> (data output register) so output voltage is updated.
> Both hardware and software triggers are supported.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Hmm. This is a somewhat different use of triggered event from normal...

What you have here is rather closer to the output buffers stuff that Analog
have in their tree which hasn't made it upstream yet.
To that end I'll want Lars to have a look at this...  I've completely
lost track of where they are with this.
Perhaps Lars can give us a quick update?

If that was in place (or what I have in my head was true anyway),
it would look like the reverse of the triggered buffer input devices.
You'd be able to write to a software buffer and it would clock them
out as the trigger fires (here I think it would have to keep updating
the DHR whenever the trigger occurs).

Even if it's not there, we aren't necessarily looking at terribly big job
to implement it in the core and that would make this handling a lot more
'standard' and consistent.

Jonathan

> ---
>  drivers/iio/dac/Kconfig          |   3 +
>  drivers/iio/dac/stm32-dac-core.h |  12 ++++
>  drivers/iio/dac/stm32-dac.c      | 124 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 136 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 7198648..786c38b 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -278,6 +278,9 @@ config STM32_DAC
>  	tristate "STMicroelectronics STM32 DAC"
>  	depends on (ARCH_STM32 && OF) || COMPILE_TEST
>  	depends on REGULATOR
> +	select IIO_TRIGGERED_EVENT
> +	select IIO_STM32_TIMER_TRIGGER
> +	select MFD_STM32_TIMERS
>  	select STM32_DAC_CORE
>  	help
>  	  Say yes here to build support for STMicroelectronics STM32 Digital
> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
> index d3099f7..3bf211c 100644
> --- a/drivers/iio/dac/stm32-dac-core.h
> +++ b/drivers/iio/dac/stm32-dac-core.h
> @@ -26,6 +26,7 @@
>  
>  /* STM32 DAC registers */
>  #define STM32_DAC_CR		0x00
> +#define STM32_DAC_SWTRIGR	0x04
>  #define STM32_DAC_DHR12R1	0x08
>  #define STM32_DAC_DHR12R2	0x14
>  #define STM32_DAC_DOR1		0x2C
> @@ -33,8 +34,19 @@
>  
>  /* STM32_DAC_CR bit fields */
>  #define STM32_DAC_CR_EN1		BIT(0)
> +#define STM32H7_DAC_CR_TEN1		BIT(1)
> +#define STM32H7_DAC_CR_TSEL1_SHIFT	2
> +#define STM32H7_DAC_CR_TSEL1		GENMASK(5, 2)
> +#define STM32_DAC_CR_WAVE1		GENMASK(7, 6)
> +#define STM32_DAC_CR_MAMP1		GENMASK(11, 8)
>  #define STM32H7_DAC_CR_HFSEL		BIT(15)
>  #define STM32_DAC_CR_EN2		BIT(16)
> +#define STM32_DAC_CR_WAVE2		GENMASK(23, 22)
> +#define STM32_DAC_CR_MAMP2		GENMASK(27, 24)
> +
> +/* STM32_DAC_SWTRIGR bit fields */
> +#define STM32_DAC_SWTRIGR_SWTRIG1	BIT(0)
> +#define STM32_DAC_SWTRIGR_SWTRIG2	BIT(1)
>  
>  /**
>   * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
> index ee9711d..62e43e9 100644
> --- a/drivers/iio/dac/stm32-dac.c
> +++ b/drivers/iio/dac/stm32-dac.c
> @@ -23,6 +23,10 @@
>  #include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/timer/stm32-timer-trigger.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_event.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> @@ -31,15 +35,112 @@
>  
>  #define STM32_DAC_CHANNEL_1		1
>  #define STM32_DAC_CHANNEL_2		2
> +/* channel2 shift */
> +#define STM32_DAC_CHAN2_SHIFT		16
>  
>  /**
>   * struct stm32_dac - private data of DAC driver
>   * @common:		reference to DAC common data
> + * @swtrig:		Using software trigger
>   */
>  struct stm32_dac {
>  	struct stm32_dac_common *common;
> +	bool swtrig;
>  };
>  
> +/**
> + * struct stm32_dac_trig_info - DAC trigger info
> + * @name: name of the trigger, corresponding to its source
> + * @tsel: trigger selection, value to be configured in DAC_CR.TSELx
> + */
> +struct stm32_dac_trig_info {
> +	const char *name;
> +	u32 tsel;
> +};
> +
> +static const struct stm32_dac_trig_info stm32h7_dac_trinfo[] = {
> +	{ "swtrig", 0 },
> +	{ TIM1_TRGO, 1 },
> +	{ TIM2_TRGO, 2 },
> +	{ TIM4_TRGO, 3 },
> +	{ TIM5_TRGO, 4 },
> +	{ TIM6_TRGO, 5 },
> +	{ TIM7_TRGO, 6 },
> +	{ TIM8_TRGO, 7 },
> +	{},
> +};
> +
> +static irqreturn_t stm32_dac_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	int channel = indio_dev->channels[0].channel;
> +
> +	/* Using software trigger? Then, trigger it now */
> +	if (dac->swtrig) {
> +		u32 swtrig;
> +
> +		if (channel == STM32_DAC_CHANNEL_1)
> +			swtrig = STM32_DAC_SWTRIGR_SWTRIG1;
> +		else
> +			swtrig = STM32_DAC_SWTRIGR_SWTRIG2;
> +		regmap_update_bits(dac->common->regmap, STM32_DAC_SWTRIGR,
> +				   swtrig, swtrig);
> +	}
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static unsigned int stm32_dac_get_trig_tsel(struct stm32_dac *dac,
> +					    struct iio_trigger *trig)
> +{
> +	unsigned int i;
> +
> +	/* skip 1st trigger that should be swtrig */
> +	for (i = 1; stm32h7_dac_trinfo[i].name; i++) {
> +		/*
> +		 * Checking both stm32 timer trigger type and trig name
> +		 * should be safe against arbitrary trigger names.
> +		 */
> +		if (is_stm32_timer_trigger(trig) &&
> +		    !strcmp(stm32h7_dac_trinfo[i].name, trig->name)) {
> +			return stm32h7_dac_trinfo[i].tsel;
> +		}
> +	}
> +
> +	/* When no trigger has been found, default to software trigger */
> +	dac->swtrig = true;
> +
> +	return stm32h7_dac_trinfo[0].tsel;
> +}
> +
> +static int stm32_dac_set_trig(struct stm32_dac *dac, struct iio_trigger *trig,
> +			      int channel)
> +{
> +	struct iio_dev *indio_dev = iio_priv_to_dev(dac);
> +	u32 shift = channel == STM32_DAC_CHANNEL_1 ? 0 : STM32_DAC_CHAN2_SHIFT;
> +	u32 val = 0, tsel;
> +	u32 msk = (STM32H7_DAC_CR_TEN1 | STM32H7_DAC_CR_TSEL1) << shift;
> +
> +	dac->swtrig = false;
> +	if (trig) {
> +		/* select & enable trigger (tsel / ten) */
> +		tsel = stm32_dac_get_trig_tsel(dac, trig);
> +		val = tsel << STM32H7_DAC_CR_TSEL1_SHIFT;
> +		val = (val | STM32H7_DAC_CR_TEN1) << shift;
> +	}
> +
> +	if (trig)
> +		dev_dbg(&indio_dev->dev, "enable trigger: %s\n", trig->name);
> +	else
> +		dev_dbg(&indio_dev->dev, "disable trigger\n");
> +
> +	return regmap_update_bits(dac->common->regmap, STM32_DAC_CR, msk, val);
> +}
> +
>  static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
>  {
>  	u32 en, val;
> @@ -63,9 +164,16 @@ static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>  		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>  	int ret;
>  
> +	ret = stm32_dac_set_trig(dac, indio_dev->trig, channel);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "Trigger setup failed\n");
> +		return ret;
> +	}
> +
>  	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
>  	if (ret < 0) {
>  		dev_err(&indio_dev->dev, "Enable failed\n");
> +		stm32_dac_set_trig(dac, NULL, channel);
>  		return ret;
>  	}
>  
> @@ -88,10 +196,12 @@ static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
>  	int ret;
>  
>  	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
> -	if (ret)
> +	if (ret) {
>  		dev_err(&indio_dev->dev, "Disable failed\n");
> +		return ret;
> +	}
>  
> -	return ret;
> +	return stm32_dac_set_trig(dac, NULL, channel);
>  }
>  
>  static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
> @@ -258,10 +368,17 @@ static int stm32_dac_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = iio_triggered_event_setup(indio_dev, NULL,
> +					stm32_dac_trigger_handler);
>  	if (ret)
>  		return ret;
>  
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		iio_triggered_event_cleanup(indio_dev);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -269,6 +386,7 @@ static int stm32_dac_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  
> +	iio_triggered_event_cleanup(indio_dev);
>  	iio_device_unregister(indio_dev);
>  
>  	return 0;
> 

Re: [PATCH 3/4] iio: dac: stm32: add support for trigger events

[Jonathan Cameron]

On 02/04/17 12:45, Jonathan Cameron wrote:
> On 31/03/17 12:45, Fabrice Gasnier wrote:
>> STM32 DAC supports triggers to synchronize conversions. When trigger
>> occurs, data is transferred from DHR (data holding register) to DOR
>> (data output register) so output voltage is updated.
>> Both hardware and software triggers are supported.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Hmm. This is a somewhat different use of triggered event from normal...
> 
> What you have here is rather closer to the output buffers stuff that Analog
> have in their tree which hasn't made it upstream yet.
> To that end I'll want Lars to have a look at this...  I've completely
> lost track of where they are with this.
> Perhaps Lars can give us a quick update?
> 
> If that was in place (or what I have in my head was true anyway),
> it would look like the reverse of the triggered buffer input devices.
> You'd be able to write to a software buffer and it would clock them
> out as the trigger fires (here I think it would have to keep updating
> the DHR whenever the trigger occurs).
> 
> Even if it's not there, we aren't necessarily looking at terribly big job
> to implement it in the core and that would make this handling a lot more
> 'standard' and consistent.

Having tracked down some limited docs (AN3126 - Audio and waveform
generation using the DAC in STM32 microcontrollers) the fact this
can also be driven by DMA definitely argues in favour of working with
Analog on getting the output buffers support upstream.

*crosses fingers people have the time!*
> 
> Jonathan
> 
>> ---
>>  drivers/iio/dac/Kconfig          |   3 +
>>  drivers/iio/dac/stm32-dac-core.h |  12 ++++
>>  drivers/iio/dac/stm32-dac.c      | 124 ++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 136 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>> index 7198648..786c38b 100644
>> --- a/drivers/iio/dac/Kconfig
>> +++ b/drivers/iio/dac/Kconfig
>> @@ -278,6 +278,9 @@ config STM32_DAC
>>  	tristate "STMicroelectronics STM32 DAC"
>>  	depends on (ARCH_STM32 && OF) || COMPILE_TEST
>>  	depends on REGULATOR
>> +	select IIO_TRIGGERED_EVENT
>> +	select IIO_STM32_TIMER_TRIGGER
>> +	select MFD_STM32_TIMERS
>>  	select STM32_DAC_CORE
>>  	help
>>  	  Say yes here to build support for STMicroelectronics STM32 Digital
>> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
>> index d3099f7..3bf211c 100644
>> --- a/drivers/iio/dac/stm32-dac-core.h
>> +++ b/drivers/iio/dac/stm32-dac-core.h
>> @@ -26,6 +26,7 @@
>>  
>>  /* STM32 DAC registers */
>>  #define STM32_DAC_CR		0x00
>> +#define STM32_DAC_SWTRIGR	0x04
>>  #define STM32_DAC_DHR12R1	0x08
>>  #define STM32_DAC_DHR12R2	0x14
>>  #define STM32_DAC_DOR1		0x2C
>> @@ -33,8 +34,19 @@
>>  
>>  /* STM32_DAC_CR bit fields */
>>  #define STM32_DAC_CR_EN1		BIT(0)
>> +#define STM32H7_DAC_CR_TEN1		BIT(1)
>> +#define STM32H7_DAC_CR_TSEL1_SHIFT	2
>> +#define STM32H7_DAC_CR_TSEL1		GENMASK(5, 2)
>> +#define STM32_DAC_CR_WAVE1		GENMASK(7, 6)
>> +#define STM32_DAC_CR_MAMP1		GENMASK(11, 8)
>>  #define STM32H7_DAC_CR_HFSEL		BIT(15)
>>  #define STM32_DAC_CR_EN2		BIT(16)
>> +#define STM32_DAC_CR_WAVE2		GENMASK(23, 22)
>> +#define STM32_DAC_CR_MAMP2		GENMASK(27, 24)
>> +
>> +/* STM32_DAC_SWTRIGR bit fields */
>> +#define STM32_DAC_SWTRIGR_SWTRIG1	BIT(0)
>> +#define STM32_DAC_SWTRIGR_SWTRIG2	BIT(1)
>>  
>>  /**
>>   * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>> index ee9711d..62e43e9 100644
>> --- a/drivers/iio/dac/stm32-dac.c
>> +++ b/drivers/iio/dac/stm32-dac.c
>> @@ -23,6 +23,10 @@
>>  #include <linux/bitfield.h>
>>  #include <linux/delay.h>
>>  #include <linux/iio/iio.h>
>> +#include <linux/iio/timer/stm32-timer-trigger.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_event.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>> @@ -31,15 +35,112 @@
>>  
>>  #define STM32_DAC_CHANNEL_1		1
>>  #define STM32_DAC_CHANNEL_2		2
>> +/* channel2 shift */
>> +#define STM32_DAC_CHAN2_SHIFT		16
>>  
>>  /**
>>   * struct stm32_dac - private data of DAC driver
>>   * @common:		reference to DAC common data
>> + * @swtrig:		Using software trigger
>>   */
>>  struct stm32_dac {
>>  	struct stm32_dac_common *common;
>> +	bool swtrig;
>>  };
>>  
>> +/**
>> + * struct stm32_dac_trig_info - DAC trigger info
>> + * @name: name of the trigger, corresponding to its source
>> + * @tsel: trigger selection, value to be configured in DAC_CR.TSELx
>> + */
>> +struct stm32_dac_trig_info {
>> +	const char *name;
>> +	u32 tsel;
>> +};
>> +
>> +static const struct stm32_dac_trig_info stm32h7_dac_trinfo[] = {
>> +	{ "swtrig", 0 },
>> +	{ TIM1_TRGO, 1 },
>> +	{ TIM2_TRGO, 2 },
>> +	{ TIM4_TRGO, 3 },
>> +	{ TIM5_TRGO, 4 },
>> +	{ TIM6_TRGO, 5 },
>> +	{ TIM7_TRGO, 6 },
>> +	{ TIM8_TRGO, 7 },
>> +	{},
>> +};
>> +
>> +static irqreturn_t stm32_dac_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	int channel = indio_dev->channels[0].channel;
>> +
>> +	/* Using software trigger? Then, trigger it now */
>> +	if (dac->swtrig) {
>> +		u32 swtrig;
>> +
>> +		if (channel == STM32_DAC_CHANNEL_1)
>> +			swtrig = STM32_DAC_SWTRIGR_SWTRIG1;
>> +		else
>> +			swtrig = STM32_DAC_SWTRIGR_SWTRIG2;
>> +		regmap_update_bits(dac->common->regmap, STM32_DAC_SWTRIGR,
>> +				   swtrig, swtrig);
>> +	}
>> +
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static unsigned int stm32_dac_get_trig_tsel(struct stm32_dac *dac,
>> +					    struct iio_trigger *trig)
>> +{
>> +	unsigned int i;
>> +
>> +	/* skip 1st trigger that should be swtrig */
>> +	for (i = 1; stm32h7_dac_trinfo[i].name; i++) {
>> +		/*
>> +		 * Checking both stm32 timer trigger type and trig name
>> +		 * should be safe against arbitrary trigger names.
>> +		 */
>> +		if (is_stm32_timer_trigger(trig) &&
>> +		    !strcmp(stm32h7_dac_trinfo[i].name, trig->name)) {
>> +			return stm32h7_dac_trinfo[i].tsel;
>> +		}
>> +	}
>> +
>> +	/* When no trigger has been found, default to software trigger */
>> +	dac->swtrig = true;
>> +
>> +	return stm32h7_dac_trinfo[0].tsel;
>> +}
>> +
>> +static int stm32_dac_set_trig(struct stm32_dac *dac, struct iio_trigger *trig,
>> +			      int channel)
>> +{
>> +	struct iio_dev *indio_dev = iio_priv_to_dev(dac);
>> +	u32 shift = channel == STM32_DAC_CHANNEL_1 ? 0 : STM32_DAC_CHAN2_SHIFT;
>> +	u32 val = 0, tsel;
>> +	u32 msk = (STM32H7_DAC_CR_TEN1 | STM32H7_DAC_CR_TSEL1) << shift;
>> +
>> +	dac->swtrig = false;
>> +	if (trig) {
>> +		/* select & enable trigger (tsel / ten) */
>> +		tsel = stm32_dac_get_trig_tsel(dac, trig);
>> +		val = tsel << STM32H7_DAC_CR_TSEL1_SHIFT;
>> +		val = (val | STM32H7_DAC_CR_TEN1) << shift;
>> +	}
>> +
>> +	if (trig)
>> +		dev_dbg(&indio_dev->dev, "enable trigger: %s\n", trig->name);
>> +	else
>> +		dev_dbg(&indio_dev->dev, "disable trigger\n");
>> +
>> +	return regmap_update_bits(dac->common->regmap, STM32_DAC_CR, msk, val);
>> +}
>> +
>>  static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
>>  {
>>  	u32 en, val;
>> @@ -63,9 +164,16 @@ static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>>  		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>>  	int ret;
>>  
>> +	ret = stm32_dac_set_trig(dac, indio_dev->trig, channel);
>> +	if (ret < 0) {
>> +		dev_err(&indio_dev->dev, "Trigger setup failed\n");
>> +		return ret;
>> +	}
>> +
>>  	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
>>  	if (ret < 0) {
>>  		dev_err(&indio_dev->dev, "Enable failed\n");
>> +		stm32_dac_set_trig(dac, NULL, channel);
>>  		return ret;
>>  	}
>>  
>> @@ -88,10 +196,12 @@ static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
>>  	int ret;
>>  
>>  	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
>> -	if (ret)
>> +	if (ret) {
>>  		dev_err(&indio_dev->dev, "Disable failed\n");
>> +		return ret;
>> +	}
>>  
>> -	return ret;
>> +	return stm32_dac_set_trig(dac, NULL, channel);
>>  }
>>  
>>  static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
>> @@ -258,10 +368,17 @@ static int stm32_dac_probe(struct platform_device *pdev)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	ret = iio_device_register(indio_dev);
>> +	ret = iio_triggered_event_setup(indio_dev, NULL,
>> +					stm32_dac_trigger_handler);
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret) {
>> +		iio_triggered_event_cleanup(indio_dev);
>> +		return ret;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> @@ -269,6 +386,7 @@ static int stm32_dac_remove(struct platform_device *pdev)
>>  {
>>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>  
>> +	iio_triggered_event_cleanup(indio_dev);
>>  	iio_device_unregister(indio_dev);
>>  
>>  	return 0;
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/4] iio: dac: stm32: add support for trigger events

[Fabrice Gasnier]

On 04/02/2017 02:21 PM, Jonathan Cameron wrote:
> On 02/04/17 12:45, Jonathan Cameron wrote:
>> On 31/03/17 12:45, Fabrice Gasnier wrote:
>>> STM32 DAC supports triggers to synchronize conversions. When trigger
>>> occurs, data is transferred from DHR (data holding register) to DOR
>>> (data output register) so output voltage is updated.
>>> Both hardware and software triggers are supported.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> Hmm. This is a somewhat different use of triggered event from normal...
>>
Waveform generator in STM32 DAC requires a trigger to increment /
decrement internal counter in case of triangle generator. Noise
generator is a bit different, but same trigger usage applies. I agree
this is unusual.
Is it acceptable to use event trigger for this use ?

>> What you have here is rather closer to the output buffers stuff that Analog
>> have in their tree which hasn't made it upstream yet.
>> To that end I'll want Lars to have a look at this...  I've completely
>> lost track of where they are with this.
>> Perhaps Lars can give us a quick update?
>>
>> If that was in place (or what I have in my head was true anyway),
>> it would look like the reverse of the triggered buffer input devices.
>> You'd be able to write to a software buffer and it would clock them
>> out as the trigger fires (here I think it would have to keep updating
>> the DHR whenever the trigger occurs).

Hmm.. for waveform generator mode, there is no need for data buffer. DAC
generate samples itself, using trigger. But, i agree it would be nice
for playing data samples (write DHR registers, or dma), yes.

>>
>> Even if it's not there, we aren't necessarily looking at terribly big job
>> to implement it in the core and that would make this handling a lot more
>> 'standard' and consistent.
> 
> Having tracked down some limited docs (AN3126 - Audio and waveform
> generation using the DAC in STM32 microcontrollers) the fact this
> can also be driven by DMA definitely argues in favour of working with
> Analog on getting the output buffers support upstream.
> 
> *crosses fingers people have the time!*

Hopefully this can happen.

For the time being, I'll propose a similar patch in V2. I found out this
patch is missing a clear path to (re-)assign trigger, once set by
userland. Also, driver never gets informed in case trigger gets changed
or removed, without re-enabling it:
e.g. like echo "" > trigger/current_trigger
I'll propose a small change. Hope you agree with this approach.

Thanks,
Fabrice

>>
>> Jonathan
>>
>>> ---
>>>  drivers/iio/dac/Kconfig          |   3 +
>>>  drivers/iio/dac/stm32-dac-core.h |  12 ++++
>>>  drivers/iio/dac/stm32-dac.c      | 124 ++++++++++++++++++++++++++++++++++++++-
>>>  3 files changed, 136 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>>> index 7198648..786c38b 100644
>>> --- a/drivers/iio/dac/Kconfig
>>> +++ b/drivers/iio/dac/Kconfig
>>> @@ -278,6 +278,9 @@ config STM32_DAC
>>>  	tristate "STMicroelectronics STM32 DAC"
>>>  	depends on (ARCH_STM32 && OF) || COMPILE_TEST
>>>  	depends on REGULATOR
>>> +	select IIO_TRIGGERED_EVENT
>>> +	select IIO_STM32_TIMER_TRIGGER
>>> +	select MFD_STM32_TIMERS
>>>  	select STM32_DAC_CORE
>>>  	help
>>>  	  Say yes here to build support for STMicroelectronics STM32 Digital
>>> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
>>> index d3099f7..3bf211c 100644
>>> --- a/drivers/iio/dac/stm32-dac-core.h
>>> +++ b/drivers/iio/dac/stm32-dac-core.h
>>> @@ -26,6 +26,7 @@
>>>  
>>>  /* STM32 DAC registers */
>>>  #define STM32_DAC_CR		0x00
>>> +#define STM32_DAC_SWTRIGR	0x04
>>>  #define STM32_DAC_DHR12R1	0x08
>>>  #define STM32_DAC_DHR12R2	0x14
>>>  #define STM32_DAC_DOR1		0x2C
>>> @@ -33,8 +34,19 @@
>>>  
>>>  /* STM32_DAC_CR bit fields */
>>>  #define STM32_DAC_CR_EN1		BIT(0)
>>> +#define STM32H7_DAC_CR_TEN1		BIT(1)
>>> +#define STM32H7_DAC_CR_TSEL1_SHIFT	2
>>> +#define STM32H7_DAC_CR_TSEL1		GENMASK(5, 2)
>>> +#define STM32_DAC_CR_WAVE1		GENMASK(7, 6)
>>> +#define STM32_DAC_CR_MAMP1		GENMASK(11, 8)
>>>  #define STM32H7_DAC_CR_HFSEL		BIT(15)
>>>  #define STM32_DAC_CR_EN2		BIT(16)
>>> +#define STM32_DAC_CR_WAVE2		GENMASK(23, 22)
>>> +#define STM32_DAC_CR_MAMP2		GENMASK(27, 24)
>>> +
>>> +/* STM32_DAC_SWTRIGR bit fields */
>>> +#define STM32_DAC_SWTRIGR_SWTRIG1	BIT(0)
>>> +#define STM32_DAC_SWTRIGR_SWTRIG2	BIT(1)
>>>  
>>>  /**
>>>   * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
>>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>>> index ee9711d..62e43e9 100644
>>> --- a/drivers/iio/dac/stm32-dac.c
>>> +++ b/drivers/iio/dac/stm32-dac.c
>>> @@ -23,6 +23,10 @@
>>>  #include <linux/bitfield.h>
>>>  #include <linux/delay.h>
>>>  #include <linux/iio/iio.h>
>>> +#include <linux/iio/timer/stm32-timer-trigger.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/triggered_event.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/module.h>
>>>  #include <linux/platform_device.h>
>>> @@ -31,15 +35,112 @@
>>>  
>>>  #define STM32_DAC_CHANNEL_1		1
>>>  #define STM32_DAC_CHANNEL_2		2
>>> +/* channel2 shift */
>>> +#define STM32_DAC_CHAN2_SHIFT		16
>>>  
>>>  /**
>>>   * struct stm32_dac - private data of DAC driver
>>>   * @common:		reference to DAC common data
>>> + * @swtrig:		Using software trigger
>>>   */
>>>  struct stm32_dac {
>>>  	struct stm32_dac_common *common;
>>> +	bool swtrig;
>>>  };
>>>  
>>> +/**
>>> + * struct stm32_dac_trig_info - DAC trigger info
>>> + * @name: name of the trigger, corresponding to its source
>>> + * @tsel: trigger selection, value to be configured in DAC_CR.TSELx
>>> + */
>>> +struct stm32_dac_trig_info {
>>> +	const char *name;
>>> +	u32 tsel;
>>> +};
>>> +
>>> +static const struct stm32_dac_trig_info stm32h7_dac_trinfo[] = {
>>> +	{ "swtrig", 0 },
>>> +	{ TIM1_TRGO, 1 },
>>> +	{ TIM2_TRGO, 2 },
>>> +	{ TIM4_TRGO, 3 },
>>> +	{ TIM5_TRGO, 4 },
>>> +	{ TIM6_TRGO, 5 },
>>> +	{ TIM7_TRGO, 6 },
>>> +	{ TIM8_TRGO, 7 },
>>> +	{},
>>> +};
>>> +
>>> +static irqreturn_t stm32_dac_trigger_handler(int irq, void *p)
>>> +{
>>> +	struct iio_poll_func *pf = p;
>>> +	struct iio_dev *indio_dev = pf->indio_dev;
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +	int channel = indio_dev->channels[0].channel;
>>> +
>>> +	/* Using software trigger? Then, trigger it now */
>>> +	if (dac->swtrig) {
>>> +		u32 swtrig;
>>> +
>>> +		if (channel == STM32_DAC_CHANNEL_1)
>>> +			swtrig = STM32_DAC_SWTRIGR_SWTRIG1;
>>> +		else
>>> +			swtrig = STM32_DAC_SWTRIGR_SWTRIG2;
>>> +		regmap_update_bits(dac->common->regmap, STM32_DAC_SWTRIGR,
>>> +				   swtrig, swtrig);
>>> +	}
>>> +
>>> +	iio_trigger_notify_done(indio_dev->trig);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static unsigned int stm32_dac_get_trig_tsel(struct stm32_dac *dac,
>>> +					    struct iio_trigger *trig)
>>> +{
>>> +	unsigned int i;
>>> +
>>> +	/* skip 1st trigger that should be swtrig */
>>> +	for (i = 1; stm32h7_dac_trinfo[i].name; i++) {
>>> +		/*
>>> +		 * Checking both stm32 timer trigger type and trig name
>>> +		 * should be safe against arbitrary trigger names.
>>> +		 */
>>> +		if (is_stm32_timer_trigger(trig) &&
>>> +		    !strcmp(stm32h7_dac_trinfo[i].name, trig->name)) {
>>> +			return stm32h7_dac_trinfo[i].tsel;
>>> +		}
>>> +	}
>>> +
>>> +	/* When no trigger has been found, default to software trigger */
>>> +	dac->swtrig = true;
>>> +
>>> +	return stm32h7_dac_trinfo[0].tsel;
>>> +}
>>> +
>>> +static int stm32_dac_set_trig(struct stm32_dac *dac, struct iio_trigger *trig,
>>> +			      int channel)
>>> +{
>>> +	struct iio_dev *indio_dev = iio_priv_to_dev(dac);
>>> +	u32 shift = channel == STM32_DAC_CHANNEL_1 ? 0 : STM32_DAC_CHAN2_SHIFT;
>>> +	u32 val = 0, tsel;
>>> +	u32 msk = (STM32H7_DAC_CR_TEN1 | STM32H7_DAC_CR_TSEL1) << shift;
>>> +
>>> +	dac->swtrig = false;
>>> +	if (trig) {
>>> +		/* select & enable trigger (tsel / ten) */
>>> +		tsel = stm32_dac_get_trig_tsel(dac, trig);
>>> +		val = tsel << STM32H7_DAC_CR_TSEL1_SHIFT;
>>> +		val = (val | STM32H7_DAC_CR_TEN1) << shift;
>>> +	}
>>> +
>>> +	if (trig)
>>> +		dev_dbg(&indio_dev->dev, "enable trigger: %s\n", trig->name);
>>> +	else
>>> +		dev_dbg(&indio_dev->dev, "disable trigger\n");
>>> +
>>> +	return regmap_update_bits(dac->common->regmap, STM32_DAC_CR, msk, val);
>>> +}
>>> +
>>>  static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
>>>  {
>>>  	u32 en, val;
>>> @@ -63,9 +164,16 @@ static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>>>  		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>>>  	int ret;
>>>  
>>> +	ret = stm32_dac_set_trig(dac, indio_dev->trig, channel);
>>> +	if (ret < 0) {
>>> +		dev_err(&indio_dev->dev, "Trigger setup failed\n");
>>> +		return ret;
>>> +	}
>>> +
>>>  	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
>>>  	if (ret < 0) {
>>>  		dev_err(&indio_dev->dev, "Enable failed\n");
>>> +		stm32_dac_set_trig(dac, NULL, channel);
>>>  		return ret;
>>>  	}
>>>  
>>> @@ -88,10 +196,12 @@ static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
>>>  	int ret;
>>>  
>>>  	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
>>> -	if (ret)
>>> +	if (ret) {
>>>  		dev_err(&indio_dev->dev, "Disable failed\n");
>>> +		return ret;
>>> +	}
>>>  
>>> -	return ret;
>>> +	return stm32_dac_set_trig(dac, NULL, channel);
>>>  }
>>>  
>>>  static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
>>> @@ -258,10 +368,17 @@ static int stm32_dac_probe(struct platform_device *pdev)
>>>  	if (ret < 0)
>>>  		return ret;
>>>  
>>> -	ret = iio_device_register(indio_dev);
>>> +	ret = iio_triggered_event_setup(indio_dev, NULL,
>>> +					stm32_dac_trigger_handler);
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> +	ret = iio_device_register(indio_dev);
>>> +	if (ret) {
>>> +		iio_triggered_event_cleanup(indio_dev);
>>> +		return ret;
>>> +	}
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> @@ -269,6 +386,7 @@ static int stm32_dac_remove(struct platform_device *pdev)
>>>  {
>>>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>>  
>>> +	iio_triggered_event_cleanup(indio_dev);
>>>  	iio_device_unregister(indio_dev);
>>>  
>>>  	return 0;
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

Re: [PATCH 3/4] iio: dac: stm32: add support for trigger events

[Jonathan Cameron]

On 05/04/17 17:44, Fabrice Gasnier wrote:
> On 04/02/2017 02:21 PM, Jonathan Cameron wrote:
>> On 02/04/17 12:45, Jonathan Cameron wrote:
>>> On 31/03/17 12:45, Fabrice Gasnier wrote:
>>>> STM32 DAC supports triggers to synchronize conversions. When trigger
>>>> occurs, data is transferred from DHR (data holding register) to DOR
>>>> (data output register) so output voltage is updated.
>>>> Both hardware and software triggers are supported.
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>> Hmm. This is a somewhat different use of triggered event from normal...
>>>
> Waveform generator in STM32 DAC requires a trigger to increment /
> decrement internal counter in case of triangle generator. Noise
> generator is a bit different, but same trigger usage applies. I agree
> this is unusual.
> Is it acceptable to use event trigger for this use ?
Not sure. It's kind of a like an output trigger but with the buffer
tied to a hardware source (a bit like the hardware consumers we have
in the other direction).

I think it's closer to an output trigger than an event trigger certainly.
Question is whether it should be something else entirely...
> 
>>> What you have here is rather closer to the output buffers stuff that Analog
>>> have in their tree which hasn't made it upstream yet.
>>> To that end I'll want Lars to have a look at this...  I've completely
>>> lost track of where they are with this.
>>> Perhaps Lars can give us a quick update?
>>>
>>> If that was in place (or what I have in my head was true anyway),
>>> it would look like the reverse of the triggered buffer input devices.
>>> You'd be able to write to a software buffer and it would clock them
>>> out as the trigger fires (here I think it would have to keep updating
>>> the DHR whenever the trigger occurs).
> 
> Hmm.. for waveform generator mode, there is no need for data buffer. DAC
> generate samples itself, using trigger. But, i agree it would be nice
> for playing data samples (write DHR registers, or dma), yes.
Definitely in the nice to have category - except wrt to having a
hardware_buffer_provider or similar to provide the data stream.
This is kind of similar to when we have a data pipeline on incoming data
where the data is never actually visible to some IIO device because it's
pushed into some processing engine directly.

It's a bit of an oddity that we need to think about when looking at output
triggering.  The previous DDS devices I have seen have all be directly
clocked...
> 
>>>
>>> Even if it's not there, we aren't necessarily looking at terribly big job
>>> to implement it in the core and that would make this handling a lot more
>>> 'standard' and consistent.
>>
>> Having tracked down some limited docs (AN3126 - Audio and waveform
>> generation using the DAC in STM32 microcontrollers) the fact this
>> can also be driven by DMA definitely argues in favour of working with
>> Analog on getting the output buffers support upstream.
>>
>> *crosses fingers people have the time!*
> 
> Hopefully this can happen.
> 
> For the time being, I'll propose a similar patch in V2. I found out this
> patch is missing a clear path to (re-)assign trigger, once set by
> userland. Also, driver never gets informed in case trigger gets changed
> or removed, without re-enabling it:
> e.g. like echo "" > trigger/current_trigger
> I'll propose a small change. Hope you agree with this approach.
> 
Cool.  Definitely looking for some analog devices input on this.
They have a quite a few similar out of tree drivers beyond those currently
in staging...

Michael / Lars?

I briefly discussed output buffers with Lars earlier in the week and the
main outstanding issues were around userspace ABI.  Last time we talked
about this in detail was quite a few years ago IIRC so time for a revisit.
+ hopefully some progress.

Jonathan
> Thanks,
> Fabrice
> 
>>>
>>> Jonathan
>>>
>>>> ---
>>>>  drivers/iio/dac/Kconfig          |   3 +
>>>>  drivers/iio/dac/stm32-dac-core.h |  12 ++++
>>>>  drivers/iio/dac/stm32-dac.c      | 124 ++++++++++++++++++++++++++++++++++++++-
>>>>  3 files changed, 136 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>>>> index 7198648..786c38b 100644
>>>> --- a/drivers/iio/dac/Kconfig
>>>> +++ b/drivers/iio/dac/Kconfig
>>>> @@ -278,6 +278,9 @@ config STM32_DAC
>>>>  	tristate "STMicroelectronics STM32 DAC"
>>>>  	depends on (ARCH_STM32 && OF) || COMPILE_TEST
>>>>  	depends on REGULATOR
>>>> +	select IIO_TRIGGERED_EVENT
>>>> +	select IIO_STM32_TIMER_TRIGGER
>>>> +	select MFD_STM32_TIMERS
>>>>  	select STM32_DAC_CORE
>>>>  	help
>>>>  	  Say yes here to build support for STMicroelectronics STM32 Digital
>>>> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
>>>> index d3099f7..3bf211c 100644
>>>> --- a/drivers/iio/dac/stm32-dac-core.h
>>>> +++ b/drivers/iio/dac/stm32-dac-core.h
>>>> @@ -26,6 +26,7 @@
>>>>  
>>>>  /* STM32 DAC registers */
>>>>  #define STM32_DAC_CR		0x00
>>>> +#define STM32_DAC_SWTRIGR	0x04
>>>>  #define STM32_DAC_DHR12R1	0x08
>>>>  #define STM32_DAC_DHR12R2	0x14
>>>>  #define STM32_DAC_DOR1		0x2C
>>>> @@ -33,8 +34,19 @@
>>>>  
>>>>  /* STM32_DAC_CR bit fields */
>>>>  #define STM32_DAC_CR_EN1		BIT(0)
>>>> +#define STM32H7_DAC_CR_TEN1		BIT(1)
>>>> +#define STM32H7_DAC_CR_TSEL1_SHIFT	2
>>>> +#define STM32H7_DAC_CR_TSEL1		GENMASK(5, 2)
>>>> +#define STM32_DAC_CR_WAVE1		GENMASK(7, 6)
>>>> +#define STM32_DAC_CR_MAMP1		GENMASK(11, 8)
>>>>  #define STM32H7_DAC_CR_HFSEL		BIT(15)
>>>>  #define STM32_DAC_CR_EN2		BIT(16)
>>>> +#define STM32_DAC_CR_WAVE2		GENMASK(23, 22)
>>>> +#define STM32_DAC_CR_MAMP2		GENMASK(27, 24)
>>>> +
>>>> +/* STM32_DAC_SWTRIGR bit fields */
>>>> +#define STM32_DAC_SWTRIGR_SWTRIG1	BIT(0)
>>>> +#define STM32_DAC_SWTRIGR_SWTRIG2	BIT(1)
>>>>  
>>>>  /**
>>>>   * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
>>>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>>>> index ee9711d..62e43e9 100644
>>>> --- a/drivers/iio/dac/stm32-dac.c
>>>> +++ b/drivers/iio/dac/stm32-dac.c
>>>> @@ -23,6 +23,10 @@
>>>>  #include <linux/bitfield.h>
>>>>  #include <linux/delay.h>
>>>>  #include <linux/iio/iio.h>
>>>> +#include <linux/iio/timer/stm32-timer-trigger.h>
>>>> +#include <linux/iio/trigger.h>
>>>> +#include <linux/iio/trigger_consumer.h>
>>>> +#include <linux/iio/triggered_event.h>
>>>>  #include <linux/kernel.h>
>>>>  #include <linux/module.h>
>>>>  #include <linux/platform_device.h>
>>>> @@ -31,15 +35,112 @@
>>>>  
>>>>  #define STM32_DAC_CHANNEL_1		1
>>>>  #define STM32_DAC_CHANNEL_2		2
>>>> +/* channel2 shift */
>>>> +#define STM32_DAC_CHAN2_SHIFT		16
>>>>  
>>>>  /**
>>>>   * struct stm32_dac - private data of DAC driver
>>>>   * @common:		reference to DAC common data
>>>> + * @swtrig:		Using software trigger
>>>>   */
>>>>  struct stm32_dac {
>>>>  	struct stm32_dac_common *common;
>>>> +	bool swtrig;
>>>>  };
>>>>  
>>>> +/**
>>>> + * struct stm32_dac_trig_info - DAC trigger info
>>>> + * @name: name of the trigger, corresponding to its source
>>>> + * @tsel: trigger selection, value to be configured in DAC_CR.TSELx
>>>> + */
>>>> +struct stm32_dac_trig_info {
>>>> +	const char *name;
>>>> +	u32 tsel;
>>>> +};
>>>> +
>>>> +static const struct stm32_dac_trig_info stm32h7_dac_trinfo[] = {
>>>> +	{ "swtrig", 0 },
>>>> +	{ TIM1_TRGO, 1 },
>>>> +	{ TIM2_TRGO, 2 },
>>>> +	{ TIM4_TRGO, 3 },
>>>> +	{ TIM5_TRGO, 4 },
>>>> +	{ TIM6_TRGO, 5 },
>>>> +	{ TIM7_TRGO, 6 },
>>>> +	{ TIM8_TRGO, 7 },
>>>> +	{},
>>>> +};
>>>> +
>>>> +static irqreturn_t stm32_dac_trigger_handler(int irq, void *p)
>>>> +{
>>>> +	struct iio_poll_func *pf = p;
>>>> +	struct iio_dev *indio_dev = pf->indio_dev;
>>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>>> +	int channel = indio_dev->channels[0].channel;
>>>> +
>>>> +	/* Using software trigger? Then, trigger it now */
>>>> +	if (dac->swtrig) {
>>>> +		u32 swtrig;
>>>> +
>>>> +		if (channel == STM32_DAC_CHANNEL_1)
>>>> +			swtrig = STM32_DAC_SWTRIGR_SWTRIG1;
>>>> +		else
>>>> +			swtrig = STM32_DAC_SWTRIGR_SWTRIG2;
>>>> +		regmap_update_bits(dac->common->regmap, STM32_DAC_SWTRIGR,
>>>> +				   swtrig, swtrig);
>>>> +	}
>>>> +
>>>> +	iio_trigger_notify_done(indio_dev->trig);
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static unsigned int stm32_dac_get_trig_tsel(struct stm32_dac *dac,
>>>> +					    struct iio_trigger *trig)
>>>> +{
>>>> +	unsigned int i;
>>>> +
>>>> +	/* skip 1st trigger that should be swtrig */
>>>> +	for (i = 1; stm32h7_dac_trinfo[i].name; i++) {
>>>> +		/*
>>>> +		 * Checking both stm32 timer trigger type and trig name
>>>> +		 * should be safe against arbitrary trigger names.
>>>> +		 */
>>>> +		if (is_stm32_timer_trigger(trig) &&
>>>> +		    !strcmp(stm32h7_dac_trinfo[i].name, trig->name)) {
>>>> +			return stm32h7_dac_trinfo[i].tsel;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	/* When no trigger has been found, default to software trigger */
>>>> +	dac->swtrig = true;
>>>> +
>>>> +	return stm32h7_dac_trinfo[0].tsel;
>>>> +}
>>>> +
>>>> +static int stm32_dac_set_trig(struct stm32_dac *dac, struct iio_trigger *trig,
>>>> +			      int channel)
>>>> +{
>>>> +	struct iio_dev *indio_dev = iio_priv_to_dev(dac);
>>>> +	u32 shift = channel == STM32_DAC_CHANNEL_1 ? 0 : STM32_DAC_CHAN2_SHIFT;
>>>> +	u32 val = 0, tsel;
>>>> +	u32 msk = (STM32H7_DAC_CR_TEN1 | STM32H7_DAC_CR_TSEL1) << shift;
>>>> +
>>>> +	dac->swtrig = false;
>>>> +	if (trig) {
>>>> +		/* select & enable trigger (tsel / ten) */
>>>> +		tsel = stm32_dac_get_trig_tsel(dac, trig);
>>>> +		val = tsel << STM32H7_DAC_CR_TSEL1_SHIFT;
>>>> +		val = (val | STM32H7_DAC_CR_TEN1) << shift;
>>>> +	}
>>>> +
>>>> +	if (trig)
>>>> +		dev_dbg(&indio_dev->dev, "enable trigger: %s\n", trig->name);
>>>> +	else
>>>> +		dev_dbg(&indio_dev->dev, "disable trigger\n");
>>>> +
>>>> +	return regmap_update_bits(dac->common->regmap, STM32_DAC_CR, msk, val);
>>>> +}
>>>> +
>>>>  static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
>>>>  {
>>>>  	u32 en, val;
>>>> @@ -63,9 +164,16 @@ static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>>>>  		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>>>>  	int ret;
>>>>  
>>>> +	ret = stm32_dac_set_trig(dac, indio_dev->trig, channel);
>>>> +	if (ret < 0) {
>>>> +		dev_err(&indio_dev->dev, "Trigger setup failed\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>>  	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
>>>>  	if (ret < 0) {
>>>>  		dev_err(&indio_dev->dev, "Enable failed\n");
>>>> +		stm32_dac_set_trig(dac, NULL, channel);
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> @@ -88,10 +196,12 @@ static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
>>>>  	int ret;
>>>>  
>>>>  	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
>>>> -	if (ret)
>>>> +	if (ret) {
>>>>  		dev_err(&indio_dev->dev, "Disable failed\n");
>>>> +		return ret;
>>>> +	}
>>>>  
>>>> -	return ret;
>>>> +	return stm32_dac_set_trig(dac, NULL, channel);
>>>>  }
>>>>  
>>>>  static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
>>>> @@ -258,10 +368,17 @@ static int stm32_dac_probe(struct platform_device *pdev)
>>>>  	if (ret < 0)
>>>>  		return ret;
>>>>  
>>>> -	ret = iio_device_register(indio_dev);
>>>> +	ret = iio_triggered_event_setup(indio_dev, NULL,
>>>> +					stm32_dac_trigger_handler);
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> +	ret = iio_device_register(indio_dev);
>>>> +	if (ret) {
>>>> +		iio_triggered_event_cleanup(indio_dev);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>>  	return 0;
>>>>  }
>>>>  
>>>> @@ -269,6 +386,7 @@ static int stm32_dac_remove(struct platform_device *pdev)
>>>>  {
>>>>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>>>  
>>>> +	iio_triggered_event_cleanup(indio_dev);
>>>>  	iio_device_unregister(indio_dev);
>>>>  
>>>>  	return 0;
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> --
> 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
> 

[PATCH 4/4] iio: dac: stm32: add support for waveform generator

[Fabrice Gasnier]

STM32 DAC has built-in noise or triangle waveform generator.
Waveform generator requires trigger to be configured.
- "wave" extended attribute selects noise or triangle.
- "mamp" extended attribute selects either LFSR (linear feedback
  shift register) mask for noise waveform, OR triangle amplitude.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 |  32 ++++++
 drivers/iio/dac/stm32-dac.c                       | 124 ++++++++++++++++++++++
 2 files changed, 156 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-stm32

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
new file mode 100644
index 0000000..c2432e1
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
@@ -0,0 +1,32 @@
+What:		/sys/bus/iio/devices/iio:deviceX/wave
+What:		/sys/bus/iio/devices/iio:deviceX/wave_available
+KernelVersion:	4.12
+Contact:	fabrice.gasnier@st.com
+Description:
+		List and/or select waveform generation provided by STM32 DAC:
+		- "none": (default) means normal DAC operations
+		- "noise": select noise waveform
+		- "triangle": select triangle waveform
+		Note: when waveform generator is used, writing _raw sysfs entry
+		adds a DC offset to generated waveform. Reading it reports
+		current output value.
+
+What:		/sys/bus/iio/devices/iio:deviceX/mamp
+What:		/sys/bus/iio/devices/iio:deviceX/mamp_available
+KernelVersion:	4.12
+Contact:	fabrice.gasnier@st.com
+Description:
+		List and select mask/amplitude used for noise/triangle waveform
+		generator, which are:
+		- "0": unmask bit 0 of LFSR / triangle amplitude equal to 1
+		- "1": unmask bit [1:0] of LFSR / triangle amplitude equal to 3
+		- "2": unmask bit [2:0] of LFSR / triangle amplitude equal to 7
+		- "3": unmask bit [3:0] of LFSR / triangle amplitude equal to 15
+		- "4": unmask bit [4:0] of LFSR / triangle amplitude equal to 31
+		- "5": unmask bit [5:0] of LFSR / triangle amplitude equal to 63
+		- "6": unmask bit [6:0] of LFSR / triangle amplitude equal to 127
+		- "7": unmask bit [7:0] of LFSR / triangle amplitude equal to 255
+		- "8": unmask bit [8:0] of LFSR / triangle amplitude equal to 511
+		- "9": unmask bit [9:0] of LFSR / triangle amplitude equal to 1023
+		- "10": unmask bit [10:0] of LFSR / triangle amplitude equal to 2047
+		- "11": unmask bit [11:0] of LFSR / triangle amplitude equal to 4095
diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
index 62e43e9..d7dda78 100644
--- a/drivers/iio/dac/stm32-dac.c
+++ b/drivers/iio/dac/stm32-dac.c
@@ -41,10 +41,14 @@
 /**
  * struct stm32_dac - private data of DAC driver
  * @common:		reference to DAC common data
+ * @wave:		waveform generator
+ * @mamp:		waveform mask/amplitude
  * @swtrig:		Using software trigger
  */
 struct stm32_dac {
 	struct stm32_dac_common *common;
+	u32 wave;
+	u32 mamp;
 	bool swtrig;
 };
 
@@ -157,6 +161,24 @@ static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
 	return !!en;
 }
 
+static int stm32_dac_wavegen(struct stm32_dac *dac, int channel)
+{
+	struct regmap *regmap = dac->common->regmap;
+	u32 mask, val;
+
+	if (channel == STM32_DAC_CHANNEL_1) {
+		val = FIELD_PREP(STM32_DAC_CR_WAVE1, dac->wave) |
+			FIELD_PREP(STM32_DAC_CR_MAMP1, dac->mamp);
+		mask = STM32_DAC_CR_WAVE1 | STM32_DAC_CR_MAMP1;
+	} else {
+		val = FIELD_PREP(STM32_DAC_CR_WAVE2, dac->wave) |
+			FIELD_PREP(STM32_DAC_CR_MAMP2, dac->mamp);
+		mask = STM32_DAC_CR_WAVE2 | STM32_DAC_CR_MAMP2;
+	}
+
+	return regmap_update_bits(regmap, STM32_DAC_CR, mask, val);
+}
+
 static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
 {
 	struct stm32_dac *dac = iio_priv(indio_dev);
@@ -164,6 +186,17 @@ static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
 		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
 	int ret;
 
+	if (dac->wave && !indio_dev->trig) {
+		dev_err(&indio_dev->dev, "Wavegen requires a trigger\n");
+		return -EINVAL;
+	}
+
+	ret = stm32_dac_wavegen(dac, channel);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev, "Wavegen setup failed\n");
+		return ret;
+	}
+
 	ret = stm32_dac_set_trig(dac, indio_dev->trig, channel);
 	if (ret < 0) {
 		dev_err(&indio_dev->dev, "Trigger setup failed\n");
@@ -291,6 +324,96 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
 	.driver_module = THIS_MODULE,
 };
 
+/* waveform generator wave selection */
+static const char * const stm32_dac_wave_desc[] = {
+	"none",
+	"noise",
+	"triangle",
+};
+
+static int stm32_dac_set_wave(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan,
+			      unsigned int type)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+
+	if (stm32_dac_is_enabled(dac, chan->channel))
+		return -EBUSY;
+	dac->wave = type;
+
+	return 0;
+}
+
+static int stm32_dac_get_wave(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+
+	return dac->wave;
+}
+
+static const struct iio_enum stm32_dac_wave_enum = {
+	.items = stm32_dac_wave_desc,
+	.num_items = ARRAY_SIZE(stm32_dac_wave_desc),
+	.get = stm32_dac_get_wave,
+	.set = stm32_dac_set_wave,
+};
+
+/*
+ * waveform generator mask/amplitude selection:
+ * - noise: LFSR mask (linear feedback shift register, umasks bit 0, [1:0]...)
+ * - triangle: amplitude (equal to 1, 3, 5, 7... 4095)
+ */
+static const char * const stm32_dac_mamp_desc[] = {
+	"0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11",
+};
+
+static int stm32_dac_set_mamp(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan,
+			      unsigned int type)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+
+	if (stm32_dac_is_enabled(dac, chan->channel))
+		return -EBUSY;
+	dac->mamp = type;
+
+	return 0;
+}
+
+static int  stm32_dac_get_mamp(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+
+	return dac->mamp;
+}
+
+static const struct iio_enum stm32_dac_mamp_enum = {
+	.items = stm32_dac_mamp_desc,
+	.num_items = ARRAY_SIZE(stm32_dac_mamp_desc),
+	.get = stm32_dac_get_mamp,
+	.set = stm32_dac_set_mamp,
+};
+
+static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
+	IIO_ENUM("wave", IIO_SHARED_BY_ALL, &stm32_dac_wave_enum),
+	{
+		.name = "wave_available",
+		.shared = IIO_SHARED_BY_ALL,
+		.read = iio_enum_available_read,
+		.private = (uintptr_t)&stm32_dac_wave_enum,
+	},
+	IIO_ENUM("mamp", IIO_SHARED_BY_ALL, &stm32_dac_mamp_enum),
+	{
+		.name = "mamp_available",
+		.shared = IIO_SHARED_BY_ALL,
+		.read = iio_enum_available_read,
+		.private = (uintptr_t)&stm32_dac_mamp_enum,
+	},
+	{},
+};
+
 #define STM32_DAC_CHANNEL(chan, name) {		\
 	.type = IIO_VOLTAGE,			\
 	.indexed = 1,				\
@@ -306,6 +429,7 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
 		.storagebits = 16,		\
 	},					\
 	.datasheet_name = name,			\
+	.ext_info = stm32_dac_ext_info		\
 }
 
 static const struct iio_chan_spec stm32_dac_channels[] = {
-- 
1.9.1

Re: [PATCH 4/4] iio: dac: stm32: add support for waveform generator

[Jonathan Cameron]

On 31/03/17 12:45, Fabrice Gasnier wrote:
> STM32 DAC has built-in noise or triangle waveform generator.
> Waveform generator requires trigger to be configured.
> - "wave" extended attribute selects noise or triangle.
> - "mamp" extended attribute selects either LFSR (linear feedback
>   shift register) mask for noise waveform, OR triangle amplitude.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>

Looks like AN3126 is the relevant doc.
(a quick note from this relevant to earlier patches- doc says
1-3 channels - perhaps build that from the start with that
possibility in mind).

As you probably know, this wanders into a large chunk of 'poorly'
defined ABI within IIO as it stands.

Note there are a number of waveform generators still in staging.
Not a lot of movement on getting them out of staging unfortunately
(so far!)

However, let us keep those drivers in mind as we work on ABI and
I definitely want some input from someone at Analog. 
Lars, who is best for this? I see at least some of these were
originally Michael's work.

They do have partial docs under
drivers/staging/iio/Documentation/sysfs-bus-iio-dds
I'll highlight thoughts from there as I look through this...


> ---
>  Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 |  32 ++++++
>  drivers/iio/dac/stm32-dac.c                       | 124 ++++++++++++++++++++++
>  2 files changed, 156 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
> new file mode 100644
> index 0000000..c2432e1
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
> @@ -0,0 +1,32 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/wave
> +What:		/sys/bus/iio/devices/iio:deviceX/wave_available
Needs to be channel associated. Whilst in your case you have basically
a pair of single channel devices, in more general case, it's not usual
to have multiple parallel waveform generators clocked together.

Old ABI is:
What:		/sys/bus/iio/devices/.../out_altvoltageX_outY_wavetype etc


> +KernelVersion:	4.12
> +Contact:	fabrice.gasnier@st.com
> +Description:
> +		List and/or select waveform generation provided by STM32 DAC:
> +		- "none": (default) means normal DAC operations
none kind of hints at nothing coming out.  Perhaps 'flat' would be closer?
i.e. only changes when someone tells it to.

> +		- "noise": select noise waveform
> +		- "triangle": select triangle waveform
> +		Note: when waveform generator is used, writing _raw sysfs entry
> +		adds a DC offset to generated waveform. Reading it reports
> +		current output value.
Interesting.  This gets fiddly but one option would be to describe the whole
device as a dds.

Then we have flat type above, combined with an _offset.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/mamp
> +What:		/sys/bus/iio/devices/iio:deviceX/mamp_available
> +KernelVersion:	4.12
> +Contact:	fabrice.gasnier@st.com
> +Description:
> +		List and select mask/amplitude used for noise/triangle waveform
> +		generator, which are:
> +		- "0": unmask bit 0 of LFSR / triangle amplitude equal to 1
> +		- "1": unmask bit [1:0] of LFSR / triangle amplitude equal to 3
> +		- "2": unmask bit [2:0] of LFSR / triangle amplitude equal to 7
> +		- "3": unmask bit [3:0] of LFSR / triangle amplitude equal to 15
> +		- "4": unmask bit [4:0] of LFSR / triangle amplitude equal to 31
> +		- "5": unmask bit [5:0] of LFSR / triangle amplitude equal to 63
> +		- "6": unmask bit [6:0] of LFSR / triangle amplitude equal to 127
> +		- "7": unmask bit [7:0] of LFSR / triangle amplitude equal to 255
> +		- "8": unmask bit [8:0] of LFSR / triangle amplitude equal to 511
> +		- "9": unmask bit [9:0] of LFSR / triangle amplitude equal to 1023
> +		- "10": unmask bit [10:0] of LFSR / triangle amplitude equal to 2047
> +		- "11": unmask bit [11:0] of LFSR / triangle amplitude equal to 4095
I don't fully understand what is going on here - so I'm guessing somewhat.


Let us try describing these generically.  If we define standard 'forms' of each
waveform type.  Say a 0 to 1 V peak to peak, then we could use _scale to control
this nicely.

> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
> index 62e43e9..d7dda78 100644
> --- a/drivers/iio/dac/stm32-dac.c
> +++ b/drivers/iio/dac/stm32-dac.c
> @@ -41,10 +41,14 @@
>  /**
>   * struct stm32_dac - private data of DAC driver
>   * @common:		reference to DAC common data
> + * @wave:		waveform generator
> + * @mamp:		waveform mask/amplitude
>   * @swtrig:		Using software trigger
>   */
>  struct stm32_dac {
>  	struct stm32_dac_common *common;
> +	u32 wave;
> +	u32 mamp;
>  	bool swtrig;
>  };
>  
> @@ -157,6 +161,24 @@ static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
>  	return !!en;
>  }
>  
> +static int stm32_dac_wavegen(struct stm32_dac *dac, int channel)
> +{
> +	struct regmap *regmap = dac->common->regmap;
> +	u32 mask, val;
> +
> +	if (channel == STM32_DAC_CHANNEL_1) {
> +		val = FIELD_PREP(STM32_DAC_CR_WAVE1, dac->wave) |
> +			FIELD_PREP(STM32_DAC_CR_MAMP1, dac->mamp);
> +		mask = STM32_DAC_CR_WAVE1 | STM32_DAC_CR_MAMP1;
> +	} else {
> +		val = FIELD_PREP(STM32_DAC_CR_WAVE2, dac->wave) |
> +			FIELD_PREP(STM32_DAC_CR_MAMP2, dac->mamp);
> +		mask = STM32_DAC_CR_WAVE2 | STM32_DAC_CR_MAMP2;
> +	}
> +
> +	return regmap_update_bits(regmap, STM32_DAC_CR, mask, val);
> +}
> +
>  static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>  {
>  	struct stm32_dac *dac = iio_priv(indio_dev);
> @@ -164,6 +186,17 @@ static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>  		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>  	int ret;
>  
> +	if (dac->wave && !indio_dev->trig) {
> +		dev_err(&indio_dev->dev, "Wavegen requires a trigger\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = stm32_dac_wavegen(dac, channel);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "Wavegen setup failed\n");
> +		return ret;
> +	}
> +
>  	ret = stm32_dac_set_trig(dac, indio_dev->trig, channel);
>  	if (ret < 0) {
>  		dev_err(&indio_dev->dev, "Trigger setup failed\n");
> @@ -291,6 +324,96 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>  	.driver_module = THIS_MODULE,
>  };
>  
> +/* waveform generator wave selection */
> +static const char * const stm32_dac_wave_desc[] = {
> +	"none",
> +	"noise",
> +	"triangle",
> +};
> +
> +static int stm32_dac_set_wave(struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan,
> +			      unsigned int type)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +
> +	if (stm32_dac_is_enabled(dac, chan->channel))
> +		return -EBUSY;
> +	dac->wave = type;
> +
> +	return 0;
> +}
> +
> +static int stm32_dac_get_wave(struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +
> +	return dac->wave;
> +}
> +
> +static const struct iio_enum stm32_dac_wave_enum = {
> +	.items = stm32_dac_wave_desc,
> +	.num_items = ARRAY_SIZE(stm32_dac_wave_desc),
> +	.get = stm32_dac_get_wave,
> +	.set = stm32_dac_set_wave,
> +};
> +
> +/*
> + * waveform generator mask/amplitude selection:
> + * - noise: LFSR mask (linear feedback shift register, umasks bit 0, [1:0]...)
> + * - triangle: amplitude (equal to 1, 3, 5, 7... 4095)
> + */
> +static const char * const stm32_dac_mamp_desc[] = {
> +	"0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11",
> +};
> +
> +static int stm32_dac_set_mamp(struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan,
> +			      unsigned int type)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +
> +	if (stm32_dac_is_enabled(dac, chan->channel))
> +		return -EBUSY;
> +	dac->mamp = type;
> +
> +	return 0;
> +}
> +
> +static int  stm32_dac_get_mamp(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +
> +	return dac->mamp;
> +}
> +
> +static const struct iio_enum stm32_dac_mamp_enum = {
> +	.items = stm32_dac_mamp_desc,
> +	.num_items = ARRAY_SIZE(stm32_dac_mamp_desc),
> +	.get = stm32_dac_get_mamp,
> +	.set = stm32_dac_set_mamp,
> +};
> +
> +static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
> +	IIO_ENUM("wave", IIO_SHARED_BY_ALL, &stm32_dac_wave_enum),
> +	{
> +		.name = "wave_available",
> +		.shared = IIO_SHARED_BY_ALL,
> +		.read = iio_enum_available_read,
> +		.private = (uintptr_t)&stm32_dac_wave_enum,
> +	},
> +	IIO_ENUM("mamp", IIO_SHARED_BY_ALL, &stm32_dac_mamp_enum),
> +	{
> +		.name = "mamp_available",
> +		.shared = IIO_SHARED_BY_ALL,
> +		.read = iio_enum_available_read,
> +		.private = (uintptr_t)&stm32_dac_mamp_enum,
> +	},
> +	{},
> +};
> +
>  #define STM32_DAC_CHANNEL(chan, name) {		\
>  	.type = IIO_VOLTAGE,			\
>  	.indexed = 1,				\
> @@ -306,6 +429,7 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>  		.storagebits = 16,		\
>  	},					\
>  	.datasheet_name = name,			\
> +	.ext_info = stm32_dac_ext_info		\
>  }
>  
>  static const struct iio_chan_spec stm32_dac_channels[] = {
> 

Re: [PATCH 4/4] iio: dac: stm32: add support for waveform generator

[Fabrice Gasnier]

JonathanOn 04/02/2017 02:19 PM, Jonathan Cameron wrote:
> On 31/03/17 12:45, Fabrice Gasnier wrote:
>> STM32 DAC has built-in noise or triangle waveform generator.
>> Waveform generator requires trigger to be configured.
>> - "wave" extended attribute selects noise or triangle.
>> - "mamp" extended attribute selects either LFSR (linear feedback
>>   shift register) mask for noise waveform, OR triangle amplitude.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> 
> Looks like AN3126 is the relevant doc.
> (a quick note from this relevant to earlier patches- doc says
> 1-3 channels - perhaps build that from the start with that
> possibility in mind).
Hi Jonathan,

Just to clarify this, some products like STM32F334xx have 3 channels,
yes. Several STM32 DAC IPs (& so registers) are instantiated: DAC1 have
two outputs (dac1_out1 & dac1_out2), DAC2 have one output (e.g.
dac2_out1). Driver can be instantiated several times. Is it ok ?

> 
> As you probably know, this wanders into a large chunk of 'poorly'
> defined ABI within IIO as it stands.
> 
> Note there are a number of waveform generators still in staging.
> Not a lot of movement on getting them out of staging unfortunately
> (so far!)
> 
> However, let us keep those drivers in mind as we work on ABI and
> I definitely want some input from someone at Analog. 
> Lars, who is best for this? I see at least some of these were
> originally Michael's work.
> 
> They do have partial docs under
> drivers/staging/iio/Documentation/sysfs-bus-iio-dds
> I'll highlight thoughts from there as I look through this...

Thanks for pointing this out.

> 
> 
>> ---
>>  Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 |  32 ++++++
>>  drivers/iio/dac/stm32-dac.c                       | 124 ++++++++++++++++++++++
>>  2 files changed, 156 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
>> new file mode 100644
>> index 0000000..c2432e1
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
>> @@ -0,0 +1,32 @@
>> +What:		/sys/bus/iio/devices/iio:deviceX/wave
>> +What:		/sys/bus/iio/devices/iio:deviceX/wave_available
> Needs to be channel associated. Whilst in your case you have basically
> a pair of single channel devices, in more general case, it's not usual
> to have multiple parallel waveform generators clocked together.
> 
> Old ABI is:
> What:		/sys/bus/iio/devices/.../out_altvoltageX_outY_wavetype etc
> 
I'll rework this in V2.
> 
>> +KernelVersion:	4.12
>> +Contact:	fabrice.gasnier@st.com
>> +Description:
>> +		List and/or select waveform generation provided by STM32 DAC:
>> +		- "none": (default) means normal DAC operations
> none kind of hints at nothing coming out.  Perhaps 'flat' would be closer?
> i.e. only changes when someone tells it to.
> 
>> +		- "noise": select noise waveform
>> +		- "triangle": select triangle waveform
>> +		Note: when waveform generator is used, writing _raw sysfs entry
>> +		adds a DC offset to generated waveform. Reading it reports
>> +		current output value.
> Interesting.  This gets fiddly but one option would be to describe the whole
> device as a dds.
> 
> Then we have flat type above, combined with an _offset.

I'll update from 'none' to 'flat' in V2, and use _offset.
> 
>> +
>> +What:		/sys/bus/iio/devices/iio:deviceX/mamp
>> +What:		/sys/bus/iio/devices/iio:deviceX/mamp_available
>> +KernelVersion:	4.12
>> +Contact:	fabrice.gasnier@st.com
>> +Description:
>> +		List and select mask/amplitude used for noise/triangle waveform
>> +		generator, which are:
>> +		- "0": unmask bit 0 of LFSR / triangle amplitude equal to 1
>> +		- "1": unmask bit [1:0] of LFSR / triangle amplitude equal to 3
>> +		- "2": unmask bit [2:0] of LFSR / triangle amplitude equal to 7
>> +		- "3": unmask bit [3:0] of LFSR / triangle amplitude equal to 15
>> +		- "4": unmask bit [4:0] of LFSR / triangle amplitude equal to 31
>> +		- "5": unmask bit [5:0] of LFSR / triangle amplitude equal to 63
>> +		- "6": unmask bit [6:0] of LFSR / triangle amplitude equal to 127
>> +		- "7": unmask bit [7:0] of LFSR / triangle amplitude equal to 255
>> +		- "8": unmask bit [8:0] of LFSR / triangle amplitude equal to 511
>> +		- "9": unmask bit [9:0] of LFSR / triangle amplitude equal to 1023
>> +		- "10": unmask bit [10:0] of LFSR / triangle amplitude equal to 2047
>> +		- "11": unmask bit [11:0] of LFSR / triangle amplitude equal to 4095
> I don't fully understand what is going on here - so I'm guessing somewhat.
Sorry for this, this is basically amplitude.

I think best is to rename above to something like 'amplitude' and
'amplitude_available'.

I'll rework this in V2.
> 
> 
> Let us try describing these generically.  If we define standard 'forms' of each
> waveform type.  Say a 0 to 1 V peak to peak, then we could use _scale to control
> this nicely.
> 
>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>> index 62e43e9..d7dda78 100644
>> --- a/drivers/iio/dac/stm32-dac.c
>> +++ b/drivers/iio/dac/stm32-dac.c
>> @@ -41,10 +41,14 @@
>>  /**
>>   * struct stm32_dac - private data of DAC driver
>>   * @common:		reference to DAC common data
>> + * @wave:		waveform generator
>> + * @mamp:		waveform mask/amplitude
>>   * @swtrig:		Using software trigger
>>   */
>>  struct stm32_dac {
>>  	struct stm32_dac_common *common;
>> +	u32 wave;
>> +	u32 mamp;
>>  	bool swtrig;
>>  };
>>  
>> @@ -157,6 +161,24 @@ static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
>>  	return !!en;
>>  }
>>  
>> +static int stm32_dac_wavegen(struct stm32_dac *dac, int channel)
>> +{
>> +	struct regmap *regmap = dac->common->regmap;
>> +	u32 mask, val;
>> +
>> +	if (channel == STM32_DAC_CHANNEL_1) {
>> +		val = FIELD_PREP(STM32_DAC_CR_WAVE1, dac->wave) |
>> +			FIELD_PREP(STM32_DAC_CR_MAMP1, dac->mamp);
>> +		mask = STM32_DAC_CR_WAVE1 | STM32_DAC_CR_MAMP1;
>> +	} else {
>> +		val = FIELD_PREP(STM32_DAC_CR_WAVE2, dac->wave) |
>> +			FIELD_PREP(STM32_DAC_CR_MAMP2, dac->mamp);
>> +		mask = STM32_DAC_CR_WAVE2 | STM32_DAC_CR_MAMP2;
>> +	}
>> +
>> +	return regmap_update_bits(regmap, STM32_DAC_CR, mask, val);
>> +}
>> +
>>  static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>>  {
>>  	struct stm32_dac *dac = iio_priv(indio_dev);
>> @@ -164,6 +186,17 @@ static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>>  		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>>  	int ret;
>>  
>> +	if (dac->wave && !indio_dev->trig) {
>> +		dev_err(&indio_dev->dev, "Wavegen requires a trigger\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = stm32_dac_wavegen(dac, channel);
>> +	if (ret < 0) {
>> +		dev_err(&indio_dev->dev, "Wavegen setup failed\n");
>> +		return ret;
>> +	}
>> +
>>  	ret = stm32_dac_set_trig(dac, indio_dev->trig, channel);
>>  	if (ret < 0) {
>>  		dev_err(&indio_dev->dev, "Trigger setup failed\n");
>> @@ -291,6 +324,96 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>>  	.driver_module = THIS_MODULE,
>>  };
>>  
>> +/* waveform generator wave selection */
>> +static const char * const stm32_dac_wave_desc[] = {
>> +	"none",
>> +	"noise",
>> +	"triangle",
>> +};
>> +
>> +static int stm32_dac_set_wave(struct iio_dev *indio_dev,
>> +			      const struct iio_chan_spec *chan,
>> +			      unsigned int type)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +
>> +	if (stm32_dac_is_enabled(dac, chan->channel))
>> +		return -EBUSY;
>> +	dac->wave = type;
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_dac_get_wave(struct iio_dev *indio_dev,
>> +			      const struct iio_chan_spec *chan)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +
>> +	return dac->wave;
>> +}
>> +
>> +static const struct iio_enum stm32_dac_wave_enum = {
>> +	.items = stm32_dac_wave_desc,
>> +	.num_items = ARRAY_SIZE(stm32_dac_wave_desc),
>> +	.get = stm32_dac_get_wave,
>> +	.set = stm32_dac_set_wave,
>> +};
>> +
>> +/*
>> + * waveform generator mask/amplitude selection:
>> + * - noise: LFSR mask (linear feedback shift register, umasks bit 0, [1:0]...)
>> + * - triangle: amplitude (equal to 1, 3, 5, 7... 4095)
>> + */
>> +static const char * const stm32_dac_mamp_desc[] = {
>> +	"0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11",
>> +};
>> +
>> +static int stm32_dac_set_mamp(struct iio_dev *indio_dev,
>> +			      const struct iio_chan_spec *chan,
>> +			      unsigned int type)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +
>> +	if (stm32_dac_is_enabled(dac, chan->channel))
>> +		return -EBUSY;
>> +	dac->mamp = type;
>> +
>> +	return 0;
>> +}
>> +
>> +static int  stm32_dac_get_mamp(struct iio_dev *indio_dev,
>> +			       const struct iio_chan_spec *chan)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +
>> +	return dac->mamp;
>> +}
>> +
>> +static const struct iio_enum stm32_dac_mamp_enum = {
>> +	.items = stm32_dac_mamp_desc,
>> +	.num_items = ARRAY_SIZE(stm32_dac_mamp_desc),
>> +	.get = stm32_dac_get_mamp,
>> +	.set = stm32_dac_set_mamp,
>> +};
>> +
>> +static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
>> +	IIO_ENUM("wave", IIO_SHARED_BY_ALL, &stm32_dac_wave_enum),
>> +	{
>> +		.name = "wave_available",
>> +		.shared = IIO_SHARED_BY_ALL,
>> +		.read = iio_enum_available_read,
>> +		.private = (uintptr_t)&stm32_dac_wave_enum,
>> +	},
>> +	IIO_ENUM("mamp", IIO_SHARED_BY_ALL, &stm32_dac_mamp_enum),
>> +	{
>> +		.name = "mamp_available",
>> +		.shared = IIO_SHARED_BY_ALL,
>> +		.read = iio_enum_available_read,
>> +		.private = (uintptr_t)&stm32_dac_mamp_enum,
>> +	},
>> +	{},
>> +};
>> +
>>  #define STM32_DAC_CHANNEL(chan, name) {		\
>>  	.type = IIO_VOLTAGE,			\
>>  	.indexed = 1,				\
>> @@ -306,6 +429,7 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>>  		.storagebits = 16,		\
>>  	},					\
>>  	.datasheet_name = name,			\
>> +	.ext_info = stm32_dac_ext_info		\
>>  }
>>  
>>  static const struct iio_chan_spec stm32_dac_channels[] = {
>>
> 

Re: [PATCH 4/4] iio: dac: stm32: add support for waveform generator

[Jonathan Cameron]

On 05/04/17 17:46, Fabrice Gasnier wrote:
> JonathanOn 04/02/2017 02:19 PM, Jonathan Cameron wrote:
>> On 31/03/17 12:45, Fabrice Gasnier wrote:
>>> STM32 DAC has built-in noise or triangle waveform generator.
>>> Waveform generator requires trigger to be configured.
>>> - "wave" extended attribute selects noise or triangle.
>>> - "mamp" extended attribute selects either LFSR (linear feedback
>>>   shift register) mask for noise waveform, OR triangle amplitude.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>
>> Looks like AN3126 is the relevant doc.
>> (a quick note from this relevant to earlier patches- doc says
>> 1-3 channels - perhaps build that from the start with that
>> possibility in mind).
> Hi Jonathan,
> 
> Just to clarify this, some products like STM32F334xx have 3 channels,
> yes. Several STM32 DAC IPs (& so registers) are instantiated: DAC1 have
> two outputs (dac1_out1 & dac1_out2), DAC2 have one output (e.g.
> dac2_out1). Driver can be instantiated several times. Is it ok ?
Sure - I'd missed the complex structure when reading the doc.
> 
>>
>> As you probably know, this wanders into a large chunk of 'poorly'
>> defined ABI within IIO as it stands.
>>
>> Note there are a number of waveform generators still in staging.
>> Not a lot of movement on getting them out of staging unfortunately
>> (so far!)
>>
>> However, let us keep those drivers in mind as we work on ABI and
>> I definitely want some input from someone at Analog. 
>> Lars, who is best for this? I see at least some of these were
>> originally Michael's work.
>>
>> They do have partial docs under
>> drivers/staging/iio/Documentation/sysfs-bus-iio-dds
>> I'll highlight thoughts from there as I look through this...
> 
> Thanks for pointing this out.
> 
>>
>>
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 |  32 ++++++
>>>  drivers/iio/dac/stm32-dac.c                       | 124 ++++++++++++++++++++++
>>>  2 files changed, 156 insertions(+)
>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
>>> new file mode 100644
>>> index 0000000..c2432e1
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
>>> @@ -0,0 +1,32 @@
>>> +What:		/sys/bus/iio/devices/iio:deviceX/wave
>>> +What:		/sys/bus/iio/devices/iio:deviceX/wave_available
>> Needs to be channel associated. Whilst in your case you have basically
>> a pair of single channel devices, in more general case, it's not usual
>> to have multiple parallel waveform generators clocked together.
>>
>> Old ABI is:
>> What:		/sys/bus/iio/devices/.../out_altvoltageX_outY_wavetype etc
>>
> I'll rework this in V2.
>>
>>> +KernelVersion:	4.12
>>> +Contact:	fabrice.gasnier@st.com
>>> +Description:
>>> +		List and/or select waveform generation provided by STM32 DAC:
>>> +		- "none": (default) means normal DAC operations
>> none kind of hints at nothing coming out.  Perhaps 'flat' would be closer?
>> i.e. only changes when someone tells it to.
>>
>>> +		- "noise": select noise waveform
>>> +		- "triangle": select triangle waveform
>>> +		Note: when waveform generator is used, writing _raw sysfs entry
>>> +		adds a DC offset to generated waveform. Reading it reports
>>> +		current output value.
>> Interesting.  This gets fiddly but one option would be to describe the whole
>> device as a dds.
>>
>> Then we have flat type above, combined with an _offset.
> 
> I'll update from 'none' to 'flat' in V2, and use _offset.
It's a bit ugly and doesn't generalize well to driving this via DMA for example...

J
>>
>>> +
>>> +What:		/sys/bus/iio/devices/iio:deviceX/mamp
>>> +What:		/sys/bus/iio/devices/iio:deviceX/mamp_available
>>> +KernelVersion:	4.12
>>> +Contact:	fabrice.gasnier@st.com
>>> +Description:
>>> +		List and select mask/amplitude used for noise/triangle waveform
>>> +		generator, which are:
>>> +		- "0": unmask bit 0 of LFSR / triangle amplitude equal to 1
>>> +		- "1": unmask bit [1:0] of LFSR / triangle amplitude equal to 3
>>> +		- "2": unmask bit [2:0] of LFSR / triangle amplitude equal to 7
>>> +		- "3": unmask bit [3:0] of LFSR / triangle amplitude equal to 15
>>> +		- "4": unmask bit [4:0] of LFSR / triangle amplitude equal to 31
>>> +		- "5": unmask bit [5:0] of LFSR / triangle amplitude equal to 63
>>> +		- "6": unmask bit [6:0] of LFSR / triangle amplitude equal to 127
>>> +		- "7": unmask bit [7:0] of LFSR / triangle amplitude equal to 255
>>> +		- "8": unmask bit [8:0] of LFSR / triangle amplitude equal to 511
>>> +		- "9": unmask bit [9:0] of LFSR / triangle amplitude equal to 1023
>>> +		- "10": unmask bit [10:0] of LFSR / triangle amplitude equal to 2047
>>> +		- "11": unmask bit [11:0] of LFSR / triangle amplitude equal to 4095
>> I don't fully understand what is going on here - so I'm guessing somewhat.
> Sorry for this, this is basically amplitude.
> 
> I think best is to rename above to something like 'amplitude' and
> 'amplitude_available'.
> 
> I'll rework this in V2.
>>
>>
>> Let us try describing these generically.  If we define standard 'forms' of each
>> waveform type.  Say a 0 to 1 V peak to peak, then we could use _scale to control
>> this nicely.
>>
>>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>>> index 62e43e9..d7dda78 100644
>>> --- a/drivers/iio/dac/stm32-dac.c
>>> +++ b/drivers/iio/dac/stm32-dac.c
>>> @@ -41,10 +41,14 @@
>>>  /**
>>>   * struct stm32_dac - private data of DAC driver
>>>   * @common:		reference to DAC common data
>>> + * @wave:		waveform generator
>>> + * @mamp:		waveform mask/amplitude
>>>   * @swtrig:		Using software trigger
>>>   */
>>>  struct stm32_dac {
>>>  	struct stm32_dac_common *common;
>>> +	u32 wave;
>>> +	u32 mamp;
>>>  	bool swtrig;
>>>  };
>>>  
>>> @@ -157,6 +161,24 @@ static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
>>>  	return !!en;
>>>  }
>>>  
>>> +static int stm32_dac_wavegen(struct stm32_dac *dac, int channel)
>>> +{
>>> +	struct regmap *regmap = dac->common->regmap;
>>> +	u32 mask, val;
>>> +
>>> +	if (channel == STM32_DAC_CHANNEL_1) {
>>> +		val = FIELD_PREP(STM32_DAC_CR_WAVE1, dac->wave) |
>>> +			FIELD_PREP(STM32_DAC_CR_MAMP1, dac->mamp);
>>> +		mask = STM32_DAC_CR_WAVE1 | STM32_DAC_CR_MAMP1;
>>> +	} else {
>>> +		val = FIELD_PREP(STM32_DAC_CR_WAVE2, dac->wave) |
>>> +			FIELD_PREP(STM32_DAC_CR_MAMP2, dac->mamp);
>>> +		mask = STM32_DAC_CR_WAVE2 | STM32_DAC_CR_MAMP2;
>>> +	}
>>> +
>>> +	return regmap_update_bits(regmap, STM32_DAC_CR, mask, val);
>>> +}
>>> +
>>>  static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>>>  {
>>>  	struct stm32_dac *dac = iio_priv(indio_dev);
>>> @@ -164,6 +186,17 @@ static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>>>  		STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>>>  	int ret;
>>>  
>>> +	if (dac->wave && !indio_dev->trig) {
>>> +		dev_err(&indio_dev->dev, "Wavegen requires a trigger\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = stm32_dac_wavegen(dac, channel);
>>> +	if (ret < 0) {
>>> +		dev_err(&indio_dev->dev, "Wavegen setup failed\n");
>>> +		return ret;
>>> +	}
>>> +
>>>  	ret = stm32_dac_set_trig(dac, indio_dev->trig, channel);
>>>  	if (ret < 0) {
>>>  		dev_err(&indio_dev->dev, "Trigger setup failed\n");
>>> @@ -291,6 +324,96 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>>>  	.driver_module = THIS_MODULE,
>>>  };
>>>  
>>> +/* waveform generator wave selection */
>>> +static const char * const stm32_dac_wave_desc[] = {
>>> +	"none",
>>> +	"noise",
>>> +	"triangle",
>>> +};
>>> +
>>> +static int stm32_dac_set_wave(struct iio_dev *indio_dev,
>>> +			      const struct iio_chan_spec *chan,
>>> +			      unsigned int type)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +
>>> +	if (stm32_dac_is_enabled(dac, chan->channel))
>>> +		return -EBUSY;
>>> +	dac->wave = type;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int stm32_dac_get_wave(struct iio_dev *indio_dev,
>>> +			      const struct iio_chan_spec *chan)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +
>>> +	return dac->wave;
>>> +}
>>> +
>>> +static const struct iio_enum stm32_dac_wave_enum = {
>>> +	.items = stm32_dac_wave_desc,
>>> +	.num_items = ARRAY_SIZE(stm32_dac_wave_desc),
>>> +	.get = stm32_dac_get_wave,
>>> +	.set = stm32_dac_set_wave,
>>> +};
>>> +
>>> +/*
>>> + * waveform generator mask/amplitude selection:
>>> + * - noise: LFSR mask (linear feedback shift register, umasks bit 0, [1:0]...)
>>> + * - triangle: amplitude (equal to 1, 3, 5, 7... 4095)
>>> + */
>>> +static const char * const stm32_dac_mamp_desc[] = {
>>> +	"0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11",
>>> +};
>>> +
>>> +static int stm32_dac_set_mamp(struct iio_dev *indio_dev,
>>> +			      const struct iio_chan_spec *chan,
>>> +			      unsigned int type)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +
>>> +	if (stm32_dac_is_enabled(dac, chan->channel))
>>> +		return -EBUSY;
>>> +	dac->mamp = type;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int  stm32_dac_get_mamp(struct iio_dev *indio_dev,
>>> +			       const struct iio_chan_spec *chan)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +
>>> +	return dac->mamp;
>>> +}
>>> +
>>> +static const struct iio_enum stm32_dac_mamp_enum = {
>>> +	.items = stm32_dac_mamp_desc,
>>> +	.num_items = ARRAY_SIZE(stm32_dac_mamp_desc),
>>> +	.get = stm32_dac_get_mamp,
>>> +	.set = stm32_dac_set_mamp,
>>> +};
>>> +
>>> +static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
>>> +	IIO_ENUM("wave", IIO_SHARED_BY_ALL, &stm32_dac_wave_enum),
>>> +	{
>>> +		.name = "wave_available",
>>> +		.shared = IIO_SHARED_BY_ALL,
>>> +		.read = iio_enum_available_read,
>>> +		.private = (uintptr_t)&stm32_dac_wave_enum,
>>> +	},
>>> +	IIO_ENUM("mamp", IIO_SHARED_BY_ALL, &stm32_dac_mamp_enum),
>>> +	{
>>> +		.name = "mamp_available",
>>> +		.shared = IIO_SHARED_BY_ALL,
>>> +		.read = iio_enum_available_read,
>>> +		.private = (uintptr_t)&stm32_dac_mamp_enum,
>>> +	},
>>> +	{},
>>> +};
>>> +
>>>  #define STM32_DAC_CHANNEL(chan, name) {		\
>>>  	.type = IIO_VOLTAGE,			\
>>>  	.indexed = 1,				\
>>> @@ -306,6 +429,7 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>>>  		.storagebits = 16,		\
>>>  	},					\
>>>  	.datasheet_name = name,			\
>>> +	.ext_info = stm32_dac_ext_info		\
>>>  }
>>>  
>>>  static const struct iio_chan_spec stm32_dac_channels[] = {
>>>
>>
> --
> 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
>