[PATCH v2 0/5] Add STM32H7 DAC driver

[PATCH v2 0/5] 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_powerdown
echo 0 > out_voltage1_raw
echo 0 > out_voltage1_powerdown
cat out_voltage_powerdown_mode_available
Hi-Z enable

# configure timer trigger, and set triangle waveform with half
# amplitude and DC offset 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 > out_voltage2_wavetype
echo 2047 > out_voltage2_amplitude
echo 1024 > out_voltage2_offset
echo tim2_trgo > trigger/current_trigger
echo 1 > out_voltage2_powerdown

---
Changes in v2:
- Update dt binding, use 'reg' property to select channel
- Use 'powerdown' attribute instead of 'enable'
- Added set_trigger callback
- Use 'offset' attribute in waveform generation mode to add DC offset
- rework ABI for waveform generation mode
- Various typos, comments

Fabrice Gasnier (5):
  dt-bindings: iio: stm32-dac: Add support for STM32 DAC
  iio: dac: add support for stm32 DAC
  iio: trigger: add set_trigger callback to notify device
  iio: dac: stm32: add support for trigger events
  iio: dac: stm32: add support for waveform generator

 Documentation/ABI/testing/sysfs-bus-iio-dac-stm32  |  16 +
 .../devicetree/bindings/iio/dac/st,stm32-dac.txt   |  61 ++
 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                        | 631 +++++++++++++++++++++
 drivers/iio/industrialio-trigger.c                 |   6 +
 include/linux/iio/iio.h                            |   2 +
 9 files changed, 979 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 v2 1/5] 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-qxv4g6HH51o@public.gmane.org>
---
Changes in v2:
- use 'reg' instead of 'st,dac-channel' property
- remove alignment from description
---
 .../devicetree/bindings/iio/dac/st,stm32-dac.txt   | 61 ++++++++++++++++++++++
 1 file changed, 61 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..bcee71f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
@@ -0,0 +1,61 @@
+STMicroelectronics STM32 DAC
+
+The STM32 DAC is a 12-bit voltage output digital-to-analog converter. The DAC
+may be configured in 8 or 12-bit mode. 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.
+- #address-cells = <1>;
+- #size-cells = <0>;
+
+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".
+- reg: Must be either 1 or 2, to define (single) channel in use
+- #io-channel-cells = <1>: See the IIO bindings section "IIO consumers" in
+  Documentation/devicetree/bindings/iio/iio-bindings.txt
+
+Example:
+	dac: dac@40007400 {
+		compatible = "st,stm32h7-dac-core";
+		reg = <0x40007400 0x400>;
+		clocks = <&clk>;
+		clock-names = "pclk";
+		vref-supply = <&reg_vref>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&dac_out1 &dac_out2>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		dac1: dac@1 {
+			compatible = "st,stm32-dac";
+			#io-channels-cells = <1>;
+			reg = <1>;
+		};
+
+		dac2: dac@2 {
+			compatible = "st,stm32-dac";
+			#io-channels-cells = <1>;
+			reg = <2>;
+		};
+	};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/5] 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-qxv4g6HH51o@public.gmane.org>
---
Changes in v2:
- Define 'Hi-Z'/'enable' powerdown modes instead of using 'enable'
  attribute normally not used for DACs.
- use 'reg' instead of 'st,dac-channel' property
- Use macro to differentiate channels
- Fix typos, remove leading '&' for functions
- Add comments on single channel per device
- Use devm_iio_device_register variant, removes need for .remove
---
 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      | 350 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 598 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-qxv4g6HH51o@public.gmane.org>.
+ *
+ * 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-qxv4g6HH51o@public.gmane.org>");
+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..daf0993
--- /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-qxv4g6HH51o@public.gmane.org>.
+ *
+ * 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 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..c0d993a
--- /dev/null
+++ b/drivers/iio/dac/stm32-dac.c
@@ -0,0 +1,350 @@
+/*
+ * This file is part of STM32 DAC driver
+ *
+ * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
+ * Authors: Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org>
+ *	    Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
+ *
+ * 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
+#define STM32_DAC_IS_CHAN_1(ch)		((ch) & STM32_DAC_CHANNEL_1)
+
+/**
+ * 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 iio_dev *indio_dev, int channel)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+	u32 en, val;
+	int ret;
+
+	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
+	if (ret < 0)
+		return ret;
+	if (STM32_DAC_IS_CHAN_1(channel))
+		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 = STM32_DAC_IS_CHAN_1(channel) ?
+		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 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 = STM32_DAC_IS_CHAN_1(channel) ?
+		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 (STM32_DAC_IS_CHAN_1(channel))
+		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 (STM32_DAC_IS_CHAN_1(channel))
+		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);
+
+	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;
+	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);
+	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,
+};
+
+static const char * const stm32_dac_powerdown_modes[] = {
+	"Hi-Z",
+	"enable",
+};
+
+static int stm32_dac_get_powerdown_mode(struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan)
+{
+	return stm32_dac_is_enabled(indio_dev, chan->channel);
+}
+
+static int stm32_dac_set_powerdown_mode(struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan,
+					unsigned int type)
+{
+	if (type)
+		return stm32_dac_enable(indio_dev, chan->channel);
+	else
+		return stm32_dac_disable(indio_dev, chan->channel);
+}
+
+static ssize_t stm32_dac_read_powerdown(struct iio_dev *indio_dev,
+					uintptr_t private,
+					const struct iio_chan_spec *chan,
+					char *buf)
+{
+	int ret = stm32_dac_is_enabled(indio_dev, chan->channel);
+
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", ret);
+}
+
+static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
+					 uintptr_t private,
+					 const struct iio_chan_spec *chan,
+					 const char *buf, size_t len)
+{
+	unsigned int en;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &en);
+	if (ret)
+		return ret;
+
+	ret = stm32_dac_set_powerdown_mode(indio_dev, chan, en);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+static const struct iio_enum stm32_dac_powerdown_mode_en = {
+	.items = stm32_dac_powerdown_modes,
+	.num_items = ARRAY_SIZE(stm32_dac_powerdown_modes),
+	.get = stm32_dac_get_powerdown_mode,
+	.set = stm32_dac_set_powerdown_mode,
+};
+
+static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
+	{
+		.name = "powerdown",
+		.read = stm32_dac_read_powerdown,
+		.write = stm32_dac_write_powerdown,
+		.shared = IIO_SEPARATE,
+	},
+	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &stm32_dac_powerdown_mode_en),
+	IIO_ENUM_AVAILABLE("powerdown_mode", &stm32_dac_powerdown_mode_en),
+	{},
+};
+
+#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_SCALE),		\
+	/* scan_index is always 0 as num_channels is 1 */ \
+	.scan_type = {					\
+		.sign = 'u',				\
+		.realbits = 12,				\
+		.storagebits = 16,			\
+	},						\
+	.datasheet_name = name,				\
+	.ext_info = stm32_dac_ext_info			\
+}
+
+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, "reg", &channel);
+	if (ret) {
+		dev_err(&indio_dev->dev, "Failed to read reg property\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];
+	/*
+	 * Expose only one channel here, as they can be used independently,
+	 * with separate trigger. Then separate IIO devices are instantiated
+	 * to manage this.
+	 */
+	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;
+
+	return devm_iio_device_register(&pdev->dev, indio_dev);
+}
+
+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,
+	.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-qxv4g6HH51o@public.gmane.org>");
+MODULE_DESCRIPTION("STMicroelectronics STM32 DAC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

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

[Jonathan Cameron]

On 06/04/17 17:11, 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-qxv4g6HH51o@public.gmane.org>
A few little bits and pieces from the change to powerdown modes...

I have also been thinking about whether we can take this prior to sorting
out the DDS side, but worry a little that we may end up wanting to change
the way we define the channels - so that might need sorting to some degree
before we can get the basics in place.

Jonathan
> ---
> Changes in v2:
> - Define 'Hi-Z'/'enable' powerdown modes instead of using 'enable'
>   attribute normally not used for DACs.
> - use 'reg' instead of 'st,dac-channel' property
> - Use macro to differentiate channels
> - Fix typos, remove leading '&' for functions
> - Add comments on single channel per device
> - Use devm_iio_device_register variant, removes need for .remove
> ---
>  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      | 350 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 598 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-qxv4g6HH51o@public.gmane.org>.
> + *
> + * 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-qxv4g6HH51o@public.gmane.org>");
> +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..daf0993
> --- /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-qxv4g6HH51o@public.gmane.org>.
> + *
> + * 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 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..c0d993a
> --- /dev/null
> +++ b/drivers/iio/dac/stm32-dac.c
> @@ -0,0 +1,350 @@
> +/*
> + * This file is part of STM32 DAC driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Authors: Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org>
> + *	    Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
> + *
> + * 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
> +#define STM32_DAC_IS_CHAN_1(ch)		((ch) & STM32_DAC_CHANNEL_1)
> +
> +/**
> + * 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 iio_dev *indio_dev, int channel)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	u32 en, val;
> +	int ret;
> +
> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
> +	if (ret < 0)
> +		return ret;
> +	if (STM32_DAC_IS_CHAN_1(channel))
> +		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 = STM32_DAC_IS_CHAN_1(channel) ?
> +		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 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 = STM32_DAC_IS_CHAN_1(channel) ?
> +		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 (STM32_DAC_IS_CHAN_1(channel))
> +		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 (STM32_DAC_IS_CHAN_1(channel))
> +		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);
> +
> +	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;
> +	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);
> +	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,
> +};
> +
> +static const char * const stm32_dac_powerdown_modes[] = {
> +	"Hi-Z",
> +	"enable",
I wouldn't expect to see enable in here, that is handled by the power down
attribute.

Hi-Z is the currently defined threestate I think?

Documenation/ABI/testing/sysfs-bus-iio

Fine to propose additions to this but they need documenting in that general
file.
> +};
> +
> +static int stm32_dac_get_powerdown_mode(struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan)
> +{
> +	return stm32_dac_is_enabled(indio_dev, chan->channel);
> +}
> +
> +static int stm32_dac_set_powerdown_mode(struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan,
> +					unsigned int type)
> +{
> +	if (type)
> +		return stm32_dac_enable(indio_dev, chan->channel);
> +	else
> +		return stm32_dac_disable(indio_dev, chan->channel);
> +}
> +
> +static ssize_t stm32_dac_read_powerdown(struct iio_dev *indio_dev,
> +					uintptr_t private,
> +					const struct iio_chan_spec *chan,
> +					char *buf)
> +{
> +	int ret = stm32_dac_is_enabled(indio_dev, chan->channel);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", ret);
> +}
> +
> +static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 const char *buf, size_t len)
> +{
> +	unsigned int en;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 0, &en);
strtobool perhaps?
> +	if (ret)
> +		return ret;
> +
> +	ret = stm32_dac_set_powerdown_mode(indio_dev, chan, en);
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static const struct iio_enum stm32_dac_powerdown_mode_en = {
> +	.items = stm32_dac_powerdown_modes,
> +	.num_items = ARRAY_SIZE(stm32_dac_powerdown_modes),
> +	.get = stm32_dac_get_powerdown_mode,
> +	.set = stm32_dac_set_powerdown_mode,
> +};
> +
> +static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
> +	{
> +		.name = "powerdown",
> +		.read = stm32_dac_read_powerdown,
> +		.write = stm32_dac_write_powerdown,
> +		.shared = IIO_SEPARATE,
> +	},
> +	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &stm32_dac_powerdown_mode_en),
> +	IIO_ENUM_AVAILABLE("powerdown_mode", &stm32_dac_powerdown_mode_en),
> +	{},
> +};
> +
> +#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_SCALE),		\
> +	/* scan_index is always 0 as num_channels is 1 */ \
> +	.scan_type = {					\
> +		.sign = 'u',				\
> +		.realbits = 12,				\
> +		.storagebits = 16,			\
> +	},						\
> +	.datasheet_name = name,				\
> +	.ext_info = stm32_dac_ext_info			\
> +}
> +
> +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, "reg", &channel);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Failed to read reg property\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];
> +	/*
> +	 * Expose only one channel here, as they can be used independently,
> +	 * with separate trigger. Then separate IIO devices are instantiated
> +	 * to manage this.
> +	 */
> +	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;
> +
> +	return devm_iio_device_register(&pdev->dev, indio_dev);
> +}
> +
> +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,
> +	.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-qxv4g6HH51o@public.gmane.org>");
> +MODULE_DESCRIPTION("STMicroelectronics STM32 DAC driver");
> +MODULE_LICENSE("GPL v2");
> 

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

[Fabrice Gasnier]

On 04/09/2017 10:39 AM, Jonathan Cameron wrote:
> On 06/04/17 17:11, 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>
> A few little bits and pieces from the change to powerdown modes...
> 
> I have also been thinking about whether we can take this prior to sorting
> out the DDS side, but worry a little that we may end up wanting to change
> the way we define the channels - so that might need sorting to some degree
> before we can get the basics in place.

Hi Jonathan,

Just sent updated v3 with powerdown changes as per your comments.

> 
> Jonathan
>> ---
>> Changes in v2:
>> - Define 'Hi-Z'/'enable' powerdown modes instead of using 'enable'
>>   attribute normally not used for DACs.
>> - use 'reg' instead of 'st,dac-channel' property
>> - Use macro to differentiate channels
>> - Fix typos, remove leading '&' for functions
>> - Add comments on single channel per device
>> - Use devm_iio_device_register variant, removes need for .remove
>> ---
>>  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      | 350 +++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 598 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..daf0993
>> --- /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 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..c0d993a
>> --- /dev/null
>> +++ b/drivers/iio/dac/stm32-dac.c
>> @@ -0,0 +1,350 @@
>> +/*
>> + * 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
>> +#define STM32_DAC_IS_CHAN_1(ch)		((ch) & STM32_DAC_CHANNEL_1)
>> +
>> +/**
>> + * 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 iio_dev *indio_dev, int channel)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	u32 en, val;
>> +	int ret;
>> +
>> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +	if (STM32_DAC_IS_CHAN_1(channel))
>> +		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 = STM32_DAC_IS_CHAN_1(channel) ?
>> +		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 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 = STM32_DAC_IS_CHAN_1(channel) ?
>> +		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 (STM32_DAC_IS_CHAN_1(channel))
>> +		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 (STM32_DAC_IS_CHAN_1(channel))
>> +		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);
>> +
>> +	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;
>> +	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);
>> +	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,
>> +};
>> +
>> +static const char * const stm32_dac_powerdown_modes[] = {
>> +	"Hi-Z",
>> +	"enable",
> I wouldn't expect to see enable in here, that is handled by the power down
> attribute.
> 
> Hi-Z is the currently defined threestate I think?
> 
> Documenation/ABI/testing/sysfs-bus-iio

Yes, documentation states: "three_state: left floating".
Thanks for pointing that out.

> 
> Fine to propose additions to this but they need documenting in that general
> file.
>> +};
>> +
>> +static int stm32_dac_get_powerdown_mode(struct iio_dev *indio_dev,
>> +					const struct iio_chan_spec *chan)
>> +{
>> +	return stm32_dac_is_enabled(indio_dev, chan->channel);
>> +}
>> +
>> +static int stm32_dac_set_powerdown_mode(struct iio_dev *indio_dev,
>> +					const struct iio_chan_spec *chan,
>> +					unsigned int type)
>> +{
>> +	if (type)
>> +		return stm32_dac_enable(indio_dev, chan->channel);
>> +	else
>> +		return stm32_dac_disable(indio_dev, chan->channel);
>> +}
>> +
>> +static ssize_t stm32_dac_read_powerdown(struct iio_dev *indio_dev,
>> +					uintptr_t private,
>> +					const struct iio_chan_spec *chan,
>> +					char *buf)
>> +{
>> +	int ret = stm32_dac_is_enabled(indio_dev, chan->channel);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return sprintf(buf, "%d\n", ret);
>> +}
>> +
>> +static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>> +					 uintptr_t private,
>> +					 const struct iio_chan_spec *chan,
>> +					 const char *buf, size_t len)
>> +{
>> +	unsigned int en;
>> +	int ret;
>> +
>> +	ret = kstrtouint(buf, 0, &en);
> strtobool perhaps?

Done in v3. I also reworked a little all around powerdown mode.

Thanks,
Fabrice

>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = stm32_dac_set_powerdown_mode(indio_dev, chan, en);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return len;
>> +}
>> +
>> +static const struct iio_enum stm32_dac_powerdown_mode_en = {
>> +	.items = stm32_dac_powerdown_modes,
>> +	.num_items = ARRAY_SIZE(stm32_dac_powerdown_modes),
>> +	.get = stm32_dac_get_powerdown_mode,
>> +	.set = stm32_dac_set_powerdown_mode,
>> +};
>> +
>> +static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
>> +	{
>> +		.name = "powerdown",
>> +		.read = stm32_dac_read_powerdown,
>> +		.write = stm32_dac_write_powerdown,
>> +		.shared = IIO_SEPARATE,
>> +	},
>> +	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &stm32_dac_powerdown_mode_en),
>> +	IIO_ENUM_AVAILABLE("powerdown_mode", &stm32_dac_powerdown_mode_en),
>> +	{},
>> +};
>> +
>> +#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_SCALE),		\
>> +	/* scan_index is always 0 as num_channels is 1 */ \
>> +	.scan_type = {					\
>> +		.sign = 'u',				\
>> +		.realbits = 12,				\
>> +		.storagebits = 16,			\
>> +	},						\
>> +	.datasheet_name = name,				\
>> +	.ext_info = stm32_dac_ext_info			\
>> +}
>> +
>> +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, "reg", &channel);
>> +	if (ret) {
>> +		dev_err(&indio_dev->dev, "Failed to read reg property\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];
>> +	/*
>> +	 * Expose only one channel here, as they can be used independently,
>> +	 * with separate trigger. Then separate IIO devices are instantiated
>> +	 * to manage this.
>> +	 */
>> +	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;
>> +
>> +	return devm_iio_device_register(&pdev->dev, indio_dev);
>> +}
>> +
>> +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,
>> +	.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");
>>
> 

[PATCH v2 4/5] 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-qxv4g6HH51o@public.gmane.org>
---
Changes in v2:
- Fix issue with trigger, by using set_trigger callback
- trigger can now be assigned, no matters powerdown state
---
 drivers/iio/dac/Kconfig          |   3 +
 drivers/iio/dac/stm32-dac-core.h |   8 +++
 drivers/iio/dac/stm32-dac.c      | 127 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 137 insertions(+), 1 deletion(-)

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 daf0993..e51a468 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,9 +34,16 @@
 
 /* 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 STM32H7_DAC_CR_HFSEL		BIT(15)
 #define STM32_DAC_CR_EN2		BIT(16)
 
+/* 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)
  * @regmap: DAC registers shared via regmap
diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
index c0d993a..a7a078e 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>
@@ -32,15 +36,113 @@
 #define STM32_DAC_CHANNEL_1		1
 #define STM32_DAC_CHANNEL_2		2
 #define STM32_DAC_IS_CHAN_1(ch)		((ch) & STM32_DAC_CHANNEL_1)
+/* 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 (STM32_DAC_IS_CHAN_1(channel))
+			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_trigger(struct iio_dev *indio_dev,
+				 struct iio_trigger *trig)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+	int channel = indio_dev->channels[0].channel;
+	u32 shift = STM32_DAC_IS_CHAN_1(channel) ? 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 iio_dev *indio_dev, int channel)
 {
 	struct stm32_dac *dac = iio_priv(indio_dev);
@@ -167,6 +269,7 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
 static const struct iio_info stm32_dac_iio_info = {
 	.read_raw = stm32_dac_read_raw,
 	.write_raw = stm32_dac_write_raw,
+	.set_trigger = stm32_dac_set_trigger,
 	.debugfs_reg_access = stm32_dac_debugfs_reg_access,
 	.driver_module = THIS_MODULE,
 };
@@ -326,7 +429,28 @@ static int stm32_dac_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	return devm_iio_device_register(&pdev->dev, 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;
+}
+
+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;
 }
 
 static const struct of_device_id stm32_dac_of_match[] = {
@@ -337,6 +461,7 @@ static int stm32_dac_probe(struct platform_device *pdev)
 
 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,
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jonathan Cameron]

On 06/04/17 17:11, 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-qxv4g6HH51o@public.gmane.org>
Hmm. I'm really not sure on how to handle this...  The problem to my mind
is knowing when it has triggered so we can know that it makes sense to
put the next reading in.... Anyhow bear with me...

I think the same arguement holds for this as equivalent input devices.
There we have always argued that if you need multichannel synchronized
capture (which is 'kind' of what we are looking at here) then you have
to use the buffered interface.

I think we need buffered output for this to fit nicely in the subsystem.
It definitely isn't a correct use of the event triggers.

That means we really need to figure out the ABI for buffered output and
get that sorted.  To my mind it shouldn't be too tricky and should look
much like buffered input, just with us writing to a fifo from userspace
rather than reading from it.

The DMA side of that can come later, in a similar fashion to how it is
added for the ADC side of things.  We can also have 'providers'
(equivalent of 'consumers' on the ADC side), perhaps giving a neat way
of describing DDS devices (I'm not so sure on this yet).

So to my mind, if you are not in buffered mode and do a sysfs write it
should be 'instant'. If in buffered mode, then it will wait on the
trigger.

So the complex side of things is what we 'know' about the data flow.
1) Case you have here.  We want to do direct write through to the device,
but have no way of knowing (or do we?) that it has triggered and written
the data to the output.  So we have no way of knowing we can push the next
value in from a fifo yet...  In this case I guess the solution might be to
have a fifo length of 0.  That is data flows straight to hardware.

2) Simple stream case - always enough data in the fifo and we get an interrupt
to signify that the previous trigger happened.

3) Case where we are only just keeping up.  So we won't have data in the fifo
until sometime after the previous trigger.  In this case we need the fifo to
push straight through if there isn't data ready to go.

4) Case where we are not pushing data fast enough.  Just don't update?

That last case 4 is nasty as the reason we typically want to do triggered
DAC updates is to ensure we always have valid states in some control loop,
but we might get a race here where one DAC has a value ready to go on a trigger
and another one isn't quite ready.  In this case we might want to hold off
until all are ready... So there might need to be a sanity check that everyone
on a given trigger is ready to go - an extra callback.

So a bit fiddly and I'm not sure I like the representation of through flow as
a fifo of 0 length... (can't think of a neater way though atm)

Anyhow, time to sort output buffers out once and for all I think if we can
get a reasonable group of people together who have the time.

Sorry Fabrice that this has hit your driver!  Perhaps we can figure
enough out to be able to at least get the basics (i.e. patches 1,2) in as
asap.

Jonathan
> ---
> Changes in v2:
> - Fix issue with trigger, by using set_trigger callback
> - trigger can now be assigned, no matters powerdown state
> ---
>  drivers/iio/dac/Kconfig          |   3 +
>  drivers/iio/dac/stm32-dac-core.h |   8 +++
>  drivers/iio/dac/stm32-dac.c      | 127 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 137 insertions(+), 1 deletion(-)
> 
> 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 daf0993..e51a468 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,9 +34,16 @@
>  
>  /* 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 STM32H7_DAC_CR_HFSEL		BIT(15)
>  #define STM32_DAC_CR_EN2		BIT(16)
>  
> +/* 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)
>   * @regmap: DAC registers shared via regmap
> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
> index c0d993a..a7a078e 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>
> @@ -32,15 +36,113 @@
>  #define STM32_DAC_CHANNEL_1		1
>  #define STM32_DAC_CHANNEL_2		2
>  #define STM32_DAC_IS_CHAN_1(ch)		((ch) & STM32_DAC_CHANNEL_1)
> +/* 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 */
Can we get here otherwise?
If not I'd prefer to either see an error on the other case
(perhaps simply return IRQ_NONE) 
> +	if (dac->swtrig) {
> +		u32 swtrig;
> +
> +		if (STM32_DAC_IS_CHAN_1(channel))
> +			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_trigger(struct iio_dev *indio_dev,
> +				 struct iio_trigger *trig)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	int channel = indio_dev->channels[0].channel;
> +	u32 shift = STM32_DAC_IS_CHAN_1(channel) ? 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 iio_dev *indio_dev, int channel)
>  {
>  	struct stm32_dac *dac = iio_priv(indio_dev);
> @@ -167,6 +269,7 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>  static const struct iio_info stm32_dac_iio_info = {
>  	.read_raw = stm32_dac_read_raw,
>  	.write_raw = stm32_dac_write_raw,
> +	.set_trigger = stm32_dac_set_trigger,
>  	.debugfs_reg_access = stm32_dac_debugfs_reg_access,
>  	.driver_module = THIS_MODULE,
>  };
> @@ -326,7 +429,28 @@ static int stm32_dac_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> -	return devm_iio_device_register(&pdev->dev, 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;
> +}
> +
> +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;
>  }
>  
>  static const struct of_device_id stm32_dac_of_match[] = {
> @@ -337,6 +461,7 @@ static int stm32_dac_probe(struct platform_device *pdev)
>  
>  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,
> 

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

[Fabrice Gasnier]

On 04/09/2017 11:04 AM, Jonathan Cameron wrote:
> On 06/04/17 17:11, 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. I'm really not sure on how to handle this...  The problem to my mind
> is knowing when it has triggered so we can know that it makes sense to
> put the next reading in.... Anyhow bear with me...
> 
> I think the same arguement holds for this as equivalent input devices.
> There we have always argued that if you need multichannel synchronized
> capture (which is 'kind' of what we are looking at here) then you have
> to use the buffered interface.
> 
> I think we need buffered output for this to fit nicely in the subsystem.
> It definitely isn't a correct use of the event triggers.
> 
> That means we really need to figure out the ABI for buffered output and
> get that sorted.  To my mind it shouldn't be too tricky and should look
> much like buffered input, just with us writing to a fifo from userspace
> rather than reading from it.
> 
> The DMA side of that can come later, in a similar fashion to how it is
> added for the ADC side of things.  We can also have 'providers'
> (equivalent of 'consumers' on the ADC side), perhaps giving a neat way
> of describing DDS devices (I'm not so sure on this yet).
> 
> So to my mind, if you are not in buffered mode and do a sysfs write it
> should be 'instant'. If in buffered mode, then it will wait on the
> trigger.
> 
> So the complex side of things is what we 'know' about the data flow.
> 1) Case you have here.  We want to do direct write through to the device,
> but have no way of knowing (or do we?) that it has triggered and written
> the data to the output.  So we have no way of knowing we can push the next
> value in from a fifo yet...  In this case I guess the solution might be to
> have a fifo length of 0.  That is data flows straight to hardware.
> 
> 2) Simple stream case - always enough data in the fifo and we get an interrupt
> to signify that the previous trigger happened.
> 
> 3) Case where we are only just keeping up.  So we won't have data in the fifo
> until sometime after the previous trigger.  In this case we need the fifo to
> push straight through if there isn't data ready to go.
> 
> 4) Case where we are not pushing data fast enough.  Just don't update?
> 
> That last case 4 is nasty as the reason we typically want to do triggered
> DAC updates is to ensure we always have valid states in some control loop,
> but we might get a race here where one DAC has a value ready to go on a trigger
> and another one isn't quite ready.  In this case we might want to hold off
> until all are ready... So there might need to be a sanity check that everyone
> on a given trigger is ready to go - an extra callback.
> 
> So a bit fiddly and I'm not sure I like the representation of through flow as
> a fifo of 0 length... (can't think of a neater way though atm)
> 
> Anyhow, time to sort output buffers out once and for all I think if we can
> get a reasonable group of people together who have the time.
> 
> Sorry Fabrice that this has hit your driver!  Perhaps we can figure
> enough out to be able to at least get the basics (i.e. patches 1,2) in as
> asap.

Hi Jonathan,

Thanks for sharing your view on this.
I sent patches 1,2 updated with your comments. I dropped following
patches for the time being, as it obviously require additions...

I agree with your analysis and concerns above.
I hope Lars or others can give some feedback or guidelines on output
buffer? Is there something already, we may start to work on ?

Thanks all in advance.
Best Regards,
Fabrice

> 
> Jonathan
>> ---
>> Changes in v2:
>> - Fix issue with trigger, by using set_trigger callback
>> - trigger can now be assigned, no matters powerdown state
>> ---
>>  drivers/iio/dac/Kconfig          |   3 +
>>  drivers/iio/dac/stm32-dac-core.h |   8 +++
>>  drivers/iio/dac/stm32-dac.c      | 127 ++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 137 insertions(+), 1 deletion(-)
>>
>> 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 daf0993..e51a468 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,9 +34,16 @@
>>  
>>  /* 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 STM32H7_DAC_CR_HFSEL		BIT(15)
>>  #define STM32_DAC_CR_EN2		BIT(16)
>>  
>> +/* 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)
>>   * @regmap: DAC registers shared via regmap
>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>> index c0d993a..a7a078e 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>
>> @@ -32,15 +36,113 @@
>>  #define STM32_DAC_CHANNEL_1		1
>>  #define STM32_DAC_CHANNEL_2		2
>>  #define STM32_DAC_IS_CHAN_1(ch)		((ch) & STM32_DAC_CHANNEL_1)
>> +/* 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 */
> Can we get here otherwise?
> If not I'd prefer to either see an error on the other case
> (perhaps simply return IRQ_NONE) 
>> +	if (dac->swtrig) {
>> +		u32 swtrig;
>> +
>> +		if (STM32_DAC_IS_CHAN_1(channel))
>> +			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_trigger(struct iio_dev *indio_dev,
>> +				 struct iio_trigger *trig)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	int channel = indio_dev->channels[0].channel;
>> +	u32 shift = STM32_DAC_IS_CHAN_1(channel) ? 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 iio_dev *indio_dev, int channel)
>>  {
>>  	struct stm32_dac *dac = iio_priv(indio_dev);
>> @@ -167,6 +269,7 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>>  static const struct iio_info stm32_dac_iio_info = {
>>  	.read_raw = stm32_dac_read_raw,
>>  	.write_raw = stm32_dac_write_raw,
>> +	.set_trigger = stm32_dac_set_trigger,
>>  	.debugfs_reg_access = stm32_dac_debugfs_reg_access,
>>  	.driver_module = THIS_MODULE,
>>  };
>> @@ -326,7 +429,28 @@ static int stm32_dac_probe(struct platform_device *pdev)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	return devm_iio_device_register(&pdev->dev, 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;
>> +}
>> +
>> +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;
>>  }
>>  
>>  static const struct of_device_id stm32_dac_of_match[] = {
>> @@ -337,6 +461,7 @@ static int stm32_dac_probe(struct platform_device *pdev)
>>  
>>  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,
>>
> 

[PATCH v2 3/5] iio: trigger: add set_trigger callback to notify device

[Fabrice Gasnier]

Add 'set_trigger' callback to iio info structure. This allows device
to be notified when a trigger (or no trigger) has been assigned. This
maybe useful for instance in non buffered mode (e.g. event triggered).
This is called, after trigger and device side validate callbacks have
been called.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
Changes in v2:
- Added set_trigger callback used in remaining patches
---
 drivers/iio/industrialio-trigger.c | 6 ++++++
 include/linux/iio/iio.h            | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 978e1592..010bdf2 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -434,6 +434,12 @@ static ssize_t iio_trigger_write_current(struct device *dev,
 			goto out_trigger_put;
 	}
 
+	if (indio_dev->info->set_trigger) {
+		ret = indio_dev->info->set_trigger(indio_dev, trig);
+		if (ret)
+			goto out_trigger_put;
+	}
+
 	indio_dev->trig = trig;
 
 	if (oldtrig) {
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 3f5ea2e..9f51065 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -399,6 +399,7 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
  * @write_event_value:	write a configuration value for the event.
  * @validate_trigger:	function to validate the trigger when the
  *			current trigger gets changed.
+ * @set_trigger:	function to notify current trigger gets changed.
  * @update_scan_mode:	function to configure device and scan buffer when
  *			channels have changed
  * @debugfs_reg_access:	function to read or write register value of device
@@ -478,6 +479,7 @@ struct iio_info {
 
 	int (*validate_trigger)(struct iio_dev *indio_dev,
 				struct iio_trigger *trig);
+	int (*set_trigger)(struct iio_dev *indio_dev, struct iio_trigger *trig);
 	int (*update_scan_mode)(struct iio_dev *indio_dev,
 				const unsigned long *scan_mask);
 	int (*debugfs_reg_access)(struct iio_dev *indio_dev,
-- 
1.9.1

Re: [PATCH v2 3/5] iio: trigger: add set_trigger callback to notify device

[Jonathan Cameron]

On 06/04/17 17:11, Fabrice Gasnier wrote:
> Add 'set_trigger' callback to iio info structure. This allows device
> to be notified when a trigger (or no trigger) has been assigned. This
> maybe useful for instance in non buffered mode (e.g. event triggered).
> This is called, after trigger and device side validate callbacks have
> been called.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
Seems sensible to me...

J
> ---
> Changes in v2:
> - Added set_trigger callback used in remaining patches
> ---
>  drivers/iio/industrialio-trigger.c | 6 ++++++
>  include/linux/iio/iio.h            | 2 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 978e1592..010bdf2 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -434,6 +434,12 @@ static ssize_t iio_trigger_write_current(struct device *dev,
>  			goto out_trigger_put;
>  	}
>  
> +	if (indio_dev->info->set_trigger) {
> +		ret = indio_dev->info->set_trigger(indio_dev, trig);
> +		if (ret)
> +			goto out_trigger_put;
> +	}
> +
>  	indio_dev->trig = trig;
>  
>  	if (oldtrig) {
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 3f5ea2e..9f51065 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -399,6 +399,7 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
>   * @write_event_value:	write a configuration value for the event.
>   * @validate_trigger:	function to validate the trigger when the
>   *			current trigger gets changed.
> + * @set_trigger:	function to notify current trigger gets changed.
>   * @update_scan_mode:	function to configure device and scan buffer when
>   *			channels have changed
>   * @debugfs_reg_access:	function to read or write register value of device
> @@ -478,6 +479,7 @@ struct iio_info {
>  
>  	int (*validate_trigger)(struct iio_dev *indio_dev,
>  				struct iio_trigger *trig);
> +	int (*set_trigger)(struct iio_dev *indio_dev, struct iio_trigger *trig);
>  	int (*update_scan_mode)(struct iio_dev *indio_dev,
>  				const unsigned long *scan_mask);
>  	int (*debugfs_reg_access)(struct iio_dev *indio_dev,
> 

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

[Fabrice Gasnier]

STM32 DAC has built-in noise or triangle waveform generator.
- "wavetype" extended attribute selects noise or triangle.
- "amplitude" extended attribute selects amplitude for waveform generator

A DC offset can be added to waveform generator output. This can be done
using out_voltage[1/2]_offset

Waveform generator requires a trigger to be configured, to increment /
decrement internal counter in case of triangle generator. Noise
generator is a bit different,  but also requires a trigger to generate
samples.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
Changes in v2:
- use _offset parameter to add DC offset to waveform generator
- Rework ABI to better fit existing DDS ABI: use out_voltageY_wavetype,
  out_voltage_wavetype_available, out_voltageY_amplitude,
  out_voltage_amplitude_available
- Better explain trigger usage in case of waveform generator.
---
 Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 |  16 +++
 drivers/iio/dac/stm32-dac-core.h                  |   4 +
 drivers/iio/dac/stm32-dac.c                       | 158 +++++++++++++++++++++-
 3 files changed, 177 insertions(+), 1 deletion(-)
 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..8f1fa009
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
@@ -0,0 +1,16 @@
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_wavetype
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltage_wavetype_available
+KernelVersion:	4.12
+Contact:	fabrice.gasnier@st.com
+Description:
+		List and/or select waveform generation provided by STM32 DAC:
+		- "flat": waveform generator disabled (default)
+		- "noise": select noise waveform
+		- "triangle": select triangle waveform
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_amplitude
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltage_amplitude_available
+KernelVersion:	4.12
+Contact:	fabrice.gasnier@st.com
+Description:
+		List and/or select amplitude used for waveform generator
diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
index e51a468..0f02975 100644
--- a/drivers/iio/dac/stm32-dac-core.h
+++ b/drivers/iio/dac/stm32-dac-core.h
@@ -37,8 +37,12 @@
 #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)
diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
index a7a078e..2ed75db 100644
--- a/drivers/iio/dac/stm32-dac.c
+++ b/drivers/iio/dac/stm32-dac.c
@@ -42,10 +42,12 @@
 /**
  * struct stm32_dac - private data of DAC driver
  * @common:		reference to DAC common data
+ * @wavetype:		waveform generator
  * @swtrig:		Using software trigger
  */
 struct stm32_dac {
 	struct stm32_dac_common *common;
+	u32 wavetype;
 	bool swtrig;
 };
 
@@ -222,6 +224,29 @@ static int stm32_dac_set_value(struct stm32_dac *dac, int channel, int val)
 	return ret;
 }
 
+static int stm32_dac_get_offset(struct stm32_dac *dac, int channel, int *val)
+{
+	int ret;
+
+	/* Offset is only relevant in waveform generation mode. */
+	if (!dac->wavetype) {
+		*val = 0;
+		return IIO_VAL_INT;
+	}
+
+	/*
+	 * In waveform generation mode, DC offset in DHR is added to waveform
+	 * generator output, then stored to DOR (data output register).
+	 * Read offset from DHR.
+	 */
+	if (STM32_DAC_IS_CHAN_1(channel))
+		ret = regmap_read(dac->common->regmap, STM32_DAC_DHR12R1, val);
+	else
+		ret = regmap_read(dac->common->regmap, STM32_DAC_DHR12R2, val);
+
+	return ret ? ret : IIO_VAL_INT;
+}
+
 static int stm32_dac_read_raw(struct iio_dev *indio_dev,
 			      struct iio_chan_spec const *chan,
 			      int *val, int *val2, long mask)
@@ -231,6 +256,8 @@ static int stm32_dac_read_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		return stm32_dac_get_value(dac, chan->channel, val);
+	case IIO_CHAN_INFO_OFFSET:
+		return stm32_dac_get_offset(dac, chan->channel, val);
 	case IIO_CHAN_INFO_SCALE:
 		*val = dac->common->vref_mv;
 		*val2 = chan->scan_type.realbits;
@@ -247,8 +274,16 @@ static int stm32_dac_write_raw(struct iio_dev *indio_dev,
 	struct stm32_dac *dac = iio_priv(indio_dev);
 
 	switch (mask) {
+	case IIO_CHAN_INFO_OFFSET:
+		/* Offset only makes sense in waveform generation mode */
+		if (dac->wavetype)
+			return stm32_dac_set_value(dac, chan->channel, val);
+		return -EBUSY;
 	case IIO_CHAN_INFO_RAW:
-		return stm32_dac_set_value(dac, chan->channel, val);
+		if (!dac->wavetype)
+			return stm32_dac_set_value(dac, chan->channel, val);
+		/* raw value is read only in waveform generation mode */
+		return -EBUSY;
 	default:
 		return -EINVAL;
 	}
@@ -334,6 +369,122 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
 	.set = stm32_dac_set_powerdown_mode,
 };
 
+/* waveform generator wave selection */
+static const char * const stm32_dac_wavetype_desc[] = {
+	"flat",
+	"noise",
+	"triangle",
+};
+
+static int stm32_dac_set_wavetype(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan,
+				  unsigned int wavetype)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+	u32 mask, val;
+	int ret;
+
+	/*
+	 * Waveform generator requires a trigger to be configured, to increment
+	 * or decrement internal counter in case of triangle generator. Noise
+	 * generator is a bit different, but also requires a trigger to
+	 * generate samples.
+	 */
+	if (wavetype && !indio_dev->trig)
+		dev_dbg(&indio_dev->dev, "Wavegen requires a trigger\n");
+
+	if (STM32_DAC_IS_CHAN_1(chan->channel)) {
+		val = FIELD_PREP(STM32_DAC_CR_WAVE1, wavetype);
+		mask = STM32_DAC_CR_WAVE1;
+	} else {
+		val = FIELD_PREP(STM32_DAC_CR_WAVE2, wavetype);
+		mask = STM32_DAC_CR_WAVE2;
+	}
+
+	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, mask, val);
+	if (ret)
+		return ret;
+	dac->wavetype = wavetype;
+
+	return 0;
+}
+
+static int stm32_dac_get_wavetype(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+	u32 val;
+	int ret;
+
+	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
+	if (ret < 0)
+		return ret;
+
+	if (STM32_DAC_IS_CHAN_1(chan->channel))
+		return FIELD_GET(STM32_DAC_CR_WAVE1, val);
+	else
+		return FIELD_GET(STM32_DAC_CR_WAVE2, val);
+}
+
+static const struct iio_enum stm32_dac_wavetype_enum = {
+	.items = stm32_dac_wavetype_desc,
+	.num_items = ARRAY_SIZE(stm32_dac_wavetype_desc),
+	.get = stm32_dac_get_wavetype,
+	.set = stm32_dac_set_wavetype,
+};
+
+/*
+ * waveform generator mamp selection: mask/amplitude
+ * - 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_amplitude_desc[] = {
+	"1", "3", "7", "15", "31", "63", "127", "255", "511", "1023", "2047",
+	"4095",
+};
+
+static int stm32_dac_set_amplitude(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan,
+				   unsigned int amplitude)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+	u32 mask, val;
+
+	if (STM32_DAC_IS_CHAN_1(chan->channel)) {
+		val = FIELD_PREP(STM32_DAC_CR_MAMP1, amplitude);
+		mask = STM32_DAC_CR_MAMP1;
+	} else {
+		val = FIELD_PREP(STM32_DAC_CR_MAMP2, amplitude);
+		mask = STM32_DAC_CR_MAMP2;
+	}
+
+	return regmap_update_bits(dac->common->regmap, STM32_DAC_CR, mask, val);
+}
+
+static int stm32_dac_get_amplitude(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan)
+{
+	struct stm32_dac *dac = iio_priv(indio_dev);
+	u32 val;
+	int ret;
+
+	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
+	if (ret < 0)
+		return ret;
+
+	if (STM32_DAC_IS_CHAN_1(chan->channel))
+		return FIELD_GET(STM32_DAC_CR_MAMP1, val);
+	else
+		return FIELD_GET(STM32_DAC_CR_MAMP2, val);
+}
+
+static const struct iio_enum stm32_dac_amplitude_enum = {
+	.items = stm32_dac_amplitude_desc,
+	.num_items = ARRAY_SIZE(stm32_dac_amplitude_desc),
+	.get = stm32_dac_get_amplitude,
+	.set = stm32_dac_set_amplitude,
+};
+
 static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
 	{
 		.name = "powerdown",
@@ -343,6 +494,10 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
 	},
 	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &stm32_dac_powerdown_mode_en),
 	IIO_ENUM_AVAILABLE("powerdown_mode", &stm32_dac_powerdown_mode_en),
+	IIO_ENUM("wavetype", IIO_SEPARATE, &stm32_dac_wavetype_enum),
+	IIO_ENUM_AVAILABLE("wavetype", &stm32_dac_wavetype_enum),
+	IIO_ENUM("amplitude", IIO_SEPARATE, &stm32_dac_amplitude_enum),
+	IIO_ENUM_AVAILABLE("amplitude", &stm32_dac_amplitude_enum),
 	{},
 };
 
@@ -352,6 +507,7 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
 	.output = 1,					\
 	.channel = chan,				\
 	.info_mask_separate =				\
+		BIT(IIO_CHAN_INFO_OFFSET) |		\
 		BIT(IIO_CHAN_INFO_RAW) |		\
 		BIT(IIO_CHAN_INFO_SCALE),		\
 	/* scan_index is always 0 as num_channels is 1 */ \
-- 
1.9.1

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

[Jonathan Cameron]

On 06/04/17 17:11, Fabrice Gasnier wrote:
> STM32 DAC has built-in noise or triangle waveform generator.
> - "wavetype" extended attribute selects noise or triangle.
> - "amplitude" extended attribute selects amplitude for waveform generator
> 
> A DC offset can be added to waveform generator output. This can be done
> using out_voltage[1/2]_offset
> 
> Waveform generator requires a trigger to be configured, to increment /
> decrement internal counter in case of triangle generator. Noise
> generator is a bit different,  but also requires a trigger to generate
> samples.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>

Various bits inline.  Mostly I think the blockers on this will be
making sure the ABI defined is generic enough to handle the more crazy
DDS chips out there... (basically the ones doing frequency modulation).

Jonathan
> ---
> Changes in v2:
> - use _offset parameter to add DC offset to waveform generator
Conceptually this offset is just the normal DAC output value (particularly
in the flat case)  I guess we can paper over this by having the _raw
and this always have th same value, but it's a little inelegant.
Still people are going to expect _raw to control it when in DAC mode but
that makes limited sense in DDS mode.

Mind you nothing stops us defining all DDS channels as the sum of whatever
the DDS is doing and whatever is _raw is set to. Perhaps we tidy this up
purely through documentation.  Think of the DDS as a modulation on top
of the DAC...

> - Rework ABI to better fit existing DDS ABI: use out_voltageY_wavetype,
>   out_voltage_wavetype_available, out_voltageY_amplitude,
>   out_voltage_amplitude_available
Hmm. I'm thinking those amplitude values aren't nice and don't fit well
with the more general ABI.

I suggested (but didn't really expand upon) having standard defined types
for each waveform then using scale to control the amplitude.

Is that something that might work here?

So say we have our triangle standard form having an amplitude of 1V Peak to
Peak. Then we can use scale to make it whatever we actually have in this
case?  The docs for wave type will need to describe those standard forms
though.

Hmm. Whether this is worth doing is unclear however as we'll still have
to describe the 'frequency' in terms of the clock ticks (here the triggers)
So maybe amplitude is worth having.  Again, looking for input from ADI lot
on this...  There are some really fiddly cases to describe were we are doing
symbol encoding so have multiple waveforms that we are switching between based
on some external signal. Any ABI needs to encompass that sort of usage.
Parts like the AD9833 for example...

> - Better explain trigger usage in case of waveform generator.
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 |  16 +++
>  drivers/iio/dac/stm32-dac-core.h                  |   4 +
>  drivers/iio/dac/stm32-dac.c                       | 158 +++++++++++++++++++++-
>  3 files changed, 177 insertions(+), 1 deletion(-)
>  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..8f1fa009
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
Fair enough to initially introduced these for this part only, but I'd very
much like to see us agree on these sufficiently to get them into the main
docs asap so we have something to work with for getting the DDS chips out
of staging...
> @@ -0,0 +1,16 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_wavetype
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltage_wavetype_available
> +KernelVersion:	4.12
> +Contact:	fabrice.gasnier-qxv4g6HH51o@public.gmane.org
> +Description:
> +		List and/or select waveform generation provided by STM32 DAC:
> +		- "flat": waveform generator disabled (default)
> +		- "noise": select noise waveform
> +		- "triangle": select triangle waveform
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_amplitude
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltage_amplitude_available
> +KernelVersion:	4.12
> +Contact:	fabrice.gasnier-qxv4g6HH51o@public.gmane.org
> +Description:
> +		List and/or select amplitude used for waveform generator
> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
> index e51a468..0f02975 100644
> --- a/drivers/iio/dac/stm32-dac-core.h
> +++ b/drivers/iio/dac/stm32-dac-core.h
> @@ -37,8 +37,12 @@
>  #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)
> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
> index a7a078e..2ed75db 100644
> --- a/drivers/iio/dac/stm32-dac.c
> +++ b/drivers/iio/dac/stm32-dac.c
> @@ -42,10 +42,12 @@
>  /**
>   * struct stm32_dac - private data of DAC driver
>   * @common:		reference to DAC common data
> + * @wavetype:		waveform generator
>   * @swtrig:		Using software trigger
>   */
>  struct stm32_dac {
>  	struct stm32_dac_common *common;
> +	u32 wavetype;
>  	bool swtrig;
>  };
>  
> @@ -222,6 +224,29 @@ static int stm32_dac_set_value(struct stm32_dac *dac, int channel, int val)
>  	return ret;
>  }
>  
> +static int stm32_dac_get_offset(struct stm32_dac *dac, int channel, int *val)
> +{
> +	int ret;
> +
> +	/* Offset is only relevant in waveform generation mode. */
> +	if (!dac->wavetype) {
> +		*val = 0;
> +		return IIO_VAL_INT;
> +	}
> +
> +	/*
> +	 * In waveform generation mode, DC offset in DHR is added to waveform
> +	 * generator output, then stored to DOR (data output register).
> +	 * Read offset from DHR.
> +	 */
Just thinking what fun we could have if we do the fifo based output to push
this that I was suggesting for the previous patch ;) triangles on top
of fun general waveforms..

> +	if (STM32_DAC_IS_CHAN_1(channel))
> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DHR12R1, val);
> +	else
> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DHR12R2, val);
> +
> +	return ret ? ret : IIO_VAL_INT;
> +}
> +
>  static int stm32_dac_read_raw(struct iio_dev *indio_dev,
>  			      struct iio_chan_spec const *chan,
>  			      int *val, int *val2, long mask)
> @@ -231,6 +256,8 @@ static int stm32_dac_read_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		return stm32_dac_get_value(dac, chan->channel, val);
> +	case IIO_CHAN_INFO_OFFSET:
> +		return stm32_dac_get_offset(dac, chan->channel, val);
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = dac->common->vref_mv;
>  		*val2 = chan->scan_type.realbits;
> @@ -247,8 +274,16 @@ static int stm32_dac_write_raw(struct iio_dev *indio_dev,
>  	struct stm32_dac *dac = iio_priv(indio_dev);
>  
>  	switch (mask) {
> +	case IIO_CHAN_INFO_OFFSET:
> +		/* Offset only makes sense in waveform generation mode */
> +		if (dac->wavetype)
> +			return stm32_dac_set_value(dac, chan->channel, val);
> +		return -EBUSY;
Yeah, I think I sent you down a blind alley here.  If people agree, lets
just define DDS signals as always being the sum of _raw + the dds element.
Then it's easy.
>  	case IIO_CHAN_INFO_RAW:
> -		return stm32_dac_set_value(dac, chan->channel, val);
> +		if (!dac->wavetype)
> +			return stm32_dac_set_value(dac, chan->channel, val);
> +		/* raw value is read only in waveform generation mode */
> +		return -EBUSY;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -334,6 +369,122 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>  	.set = stm32_dac_set_powerdown_mode,
>  };
>  
> +/* waveform generator wave selection */
> +static const char * const stm32_dac_wavetype_desc[] = {
> +	"flat",
> +	"noise",
> +	"triangle",
> +};
> +
> +static int stm32_dac_set_wavetype(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan,
> +				  unsigned int wavetype)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	u32 mask, val;
> +	int ret;
> +
> +	/*
> +	 * Waveform generator requires a trigger to be configured, to increment
> +	 * or decrement internal counter in case of triangle generator. Noise
> +	 * generator is a bit different, but also requires a trigger to
> +	 * generate samples.
> +	 */
> +	if (wavetype && !indio_dev->trig)
> +		dev_dbg(&indio_dev->dev, "Wavegen requires a trigger\n");
> +
> +	if (STM32_DAC_IS_CHAN_1(chan->channel)) {
> +		val = FIELD_PREP(STM32_DAC_CR_WAVE1, wavetype);
> +		mask = STM32_DAC_CR_WAVE1;
> +	} else {
> +		val = FIELD_PREP(STM32_DAC_CR_WAVE2, wavetype);
> +		mask = STM32_DAC_CR_WAVE2;
> +	}
> +
> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, mask, val);
> +	if (ret)
> +		return ret;
> +	dac->wavetype = wavetype;
> +
> +	return 0;
> +}
> +
> +static int stm32_dac_get_wavetype(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (STM32_DAC_IS_CHAN_1(chan->channel))
> +		return FIELD_GET(STM32_DAC_CR_WAVE1, val);
> +	else
> +		return FIELD_GET(STM32_DAC_CR_WAVE2, val);
> +}
> +
> +static const struct iio_enum stm32_dac_wavetype_enum = {
> +	.items = stm32_dac_wavetype_desc,
> +	.num_items = ARRAY_SIZE(stm32_dac_wavetype_desc),
> +	.get = stm32_dac_get_wavetype,
> +	.set = stm32_dac_set_wavetype,
> +};
> +
> +/*
> + * waveform generator mamp selection: mask/amplitude
> + * - noise: LFSR mask (linear feedback shift register, umasks bit 0, [1:0]...)
This needs breaking out into two attributes - for noise it isn't amplitude in
any conventional sense...  Keep the result device specific for now. I'm not
even sure what type of pseudo random source that actually is...
If anyone wants to figure it out it would be great to document it in a general
form!

> + * - triangle: amplitude (equal to 1, 3, 5, 7... 4095)
> + */
> +static const char * const stm32_dac_amplitude_desc[] = {
> +	"1", "3", "7", "15", "31", "63", "127", "255", "511", "1023", "2047",
> +	"4095",
> +};
> +
> +static int stm32_dac_set_amplitude(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   unsigned int amplitude)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	u32 mask, val;
> +
> +	if (STM32_DAC_IS_CHAN_1(chan->channel)) {
> +		val = FIELD_PREP(STM32_DAC_CR_MAMP1, amplitude);
> +		mask = STM32_DAC_CR_MAMP1;
> +	} else {
> +		val = FIELD_PREP(STM32_DAC_CR_MAMP2, amplitude);
> +		mask = STM32_DAC_CR_MAMP2;
> +	}
> +
> +	return regmap_update_bits(dac->common->regmap, STM32_DAC_CR, mask, val);
> +}
> +
> +static int stm32_dac_get_amplitude(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan)
> +{
> +	struct stm32_dac *dac = iio_priv(indio_dev);
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (STM32_DAC_IS_CHAN_1(chan->channel))
> +		return FIELD_GET(STM32_DAC_CR_MAMP1, val);
> +	else
> +		return FIELD_GET(STM32_DAC_CR_MAMP2, val);
> +}
> +
> +static const struct iio_enum stm32_dac_amplitude_enum = {
> +	.items = stm32_dac_amplitude_desc,
> +	.num_items = ARRAY_SIZE(stm32_dac_amplitude_desc),
> +	.get = stm32_dac_get_amplitude,
> +	.set = stm32_dac_set_amplitude,
> +};
> +
>  static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
>  	{
>  		.name = "powerdown",
> @@ -343,6 +494,10 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>  	},
>  	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &stm32_dac_powerdown_mode_en),
>  	IIO_ENUM_AVAILABLE("powerdown_mode", &stm32_dac_powerdown_mode_en),
> +	IIO_ENUM("wavetype", IIO_SEPARATE, &stm32_dac_wavetype_enum),
> +	IIO_ENUM_AVAILABLE("wavetype", &stm32_dac_wavetype_enum),
> +	IIO_ENUM("amplitude", IIO_SEPARATE, &stm32_dac_amplitude_enum),
> +	IIO_ENUM_AVAILABLE("amplitude", &stm32_dac_amplitude_enum),
>  	{},
>  };
>  
> @@ -352,6 +507,7 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>  	.output = 1,					\
>  	.channel = chan,				\
>  	.info_mask_separate =				\
> +		BIT(IIO_CHAN_INFO_OFFSET) |		\
>  		BIT(IIO_CHAN_INFO_RAW) |		\
>  		BIT(IIO_CHAN_INFO_SCALE),		\
>  	/* scan_index is always 0 as num_channels is 1 */ \
> 

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

[Fabrice Gasnier]

On 04/09/2017 11:34 AM, Jonathan Cameron wrote:
> On 06/04/17 17:11, Fabrice Gasnier wrote:
>> STM32 DAC has built-in noise or triangle waveform generator.
>> - "wavetype" extended attribute selects noise or triangle.
>> - "amplitude" extended attribute selects amplitude for waveform generator
>>
>> A DC offset can be added to waveform generator output. This can be done
>> using out_voltage[1/2]_offset
>>
>> Waveform generator requires a trigger to be configured, to increment /
>> decrement internal counter in case of triangle generator. Noise
>> generator is a bit different,  but also requires a trigger to generate
>> samples.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> 
> Various bits inline.  Mostly I think the blockers on this will be
> making sure the ABI defined is generic enough to handle the more crazy
> DDS chips out there... (basically the ones doing frequency modulation).
> 
> Jonathan
>> ---
>> Changes in v2:
>> - use _offset parameter to add DC offset to waveform generator
> Conceptually this offset is just the normal DAC output value (particularly
yes
> in the flat case)  I guess we can paper over this by having the _raw
> and this always have th same value, but it's a little inelegant.
> Still people are going to expect _raw to control it when in DAC mode but
> that makes limited sense in DDS mode.
> 
> Mind you nothing stops us defining all DDS channels as the sum of whatever
> the DDS is doing and whatever is _raw is set to. Perhaps we tidy this up
> purely through documentation.  Think of the DDS as a modulation on top
> of the DAC...
> 
>> - Rework ABI to better fit existing DDS ABI: use out_voltageY_wavetype,
>>   out_voltage_wavetype_available, out_voltageY_amplitude,
>>   out_voltage_amplitude_available
> Hmm. I'm thinking those amplitude values aren't nice and don't fit well
> with the more general ABI.
> 
> I suggested (but didn't really expand upon) having standard defined types
> for each waveform then using scale to control the amplitude.

Do you mean _scale attribute ?
> 
> Is that something that might work here?

I probably miss the point here...
> 
> So say we have our triangle standard form having an amplitude of 1V Peak to
> Peak. Then we can use scale to make it whatever we actually have in this
> case?  The docs for wave type will need to describe those standard forms
> though.
... scale is fixed here, in line with _raw attribute. In 'amplitude'
description for STM32 DAC here (e.g. from 1...4095), same scale is used.
Can you elaborate ?

> 
> Hmm. Whether this is worth doing is unclear however as we'll still have
> to describe the 'frequency' in terms of the clock ticks (here the triggers)

Describing frequency may be an issue, not sure it makes senses in this
case: 'clock ticks', e.g. triggers here may be timers, but also an EXTI
(external...), or even software trig perhaps.

> So maybe amplitude is worth having.  Again, looking for input from ADI lot
> on this...  There are some really fiddly cases to describe were we are doing
> symbol encoding so have multiple waveforms that we are switching between based
> on some external signal. Any ABI needs to encompass that sort of usage.
> Parts like the AD9833 for example...
> 
>> - Better explain trigger usage in case of waveform generator.
>> ---
>>  Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 |  16 +++
>>  drivers/iio/dac/stm32-dac-core.h                  |   4 +
>>  drivers/iio/dac/stm32-dac.c                       | 158 +++++++++++++++++++++-
>>  3 files changed, 177 insertions(+), 1 deletion(-)
>>  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..8f1fa009
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
> Fair enough to initially introduced these for this part only, but I'd very
> much like to see us agree on these sufficiently to get them into the main
> docs asap so we have something to work with for getting the DDS chips out
> of staging...
>> @@ -0,0 +1,16 @@
>> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_wavetype
>> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltage_wavetype_available
>> +KernelVersion:	4.12
>> +Contact:	fabrice.gasnier@st.com
>> +Description:
>> +		List and/or select waveform generation provided by STM32 DAC:
>> +		- "flat": waveform generator disabled (default)
>> +		- "noise": select noise waveform
>> +		- "triangle": select triangle waveform
>> +
>> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_amplitude
>> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltage_amplitude_available
>> +KernelVersion:	4.12
>> +Contact:	fabrice.gasnier@st.com
>> +Description:
>> +		List and/or select amplitude used for waveform generator
>> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
>> index e51a468..0f02975 100644
>> --- a/drivers/iio/dac/stm32-dac-core.h
>> +++ b/drivers/iio/dac/stm32-dac-core.h
>> @@ -37,8 +37,12 @@
>>  #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)
>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>> index a7a078e..2ed75db 100644
>> --- a/drivers/iio/dac/stm32-dac.c
>> +++ b/drivers/iio/dac/stm32-dac.c
>> @@ -42,10 +42,12 @@
>>  /**
>>   * struct stm32_dac - private data of DAC driver
>>   * @common:		reference to DAC common data
>> + * @wavetype:		waveform generator
>>   * @swtrig:		Using software trigger
>>   */
>>  struct stm32_dac {
>>  	struct stm32_dac_common *common;
>> +	u32 wavetype;
>>  	bool swtrig;
>>  };
>>  
>> @@ -222,6 +224,29 @@ static int stm32_dac_set_value(struct stm32_dac *dac, int channel, int val)
>>  	return ret;
>>  }
>>  
>> +static int stm32_dac_get_offset(struct stm32_dac *dac, int channel, int *val)
>> +{
>> +	int ret;
>> +
>> +	/* Offset is only relevant in waveform generation mode. */
>> +	if (!dac->wavetype) {
>> +		*val = 0;
>> +		return IIO_VAL_INT;
>> +	}
>> +
>> +	/*
>> +	 * In waveform generation mode, DC offset in DHR is added to waveform
>> +	 * generator output, then stored to DOR (data output register).
>> +	 * Read offset from DHR.
>> +	 */
> Just thinking what fun we could have if we do the fifo based output to push
> this that I was suggesting for the previous patch ;) triangles on top
> of fun general waveforms..
> 
>> +	if (STM32_DAC_IS_CHAN_1(channel))
>> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DHR12R1, val);
>> +	else
>> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DHR12R2, val);
>> +
>> +	return ret ? ret : IIO_VAL_INT;
>> +}
>> +
>>  static int stm32_dac_read_raw(struct iio_dev *indio_dev,
>>  			      struct iio_chan_spec const *chan,
>>  			      int *val, int *val2, long mask)
>> @@ -231,6 +256,8 @@ static int stm32_dac_read_raw(struct iio_dev *indio_dev,
>>  	switch (mask) {
>>  	case IIO_CHAN_INFO_RAW:
>>  		return stm32_dac_get_value(dac, chan->channel, val);
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		return stm32_dac_get_offset(dac, chan->channel, val);
>>  	case IIO_CHAN_INFO_SCALE:
>>  		*val = dac->common->vref_mv;
>>  		*val2 = chan->scan_type.realbits;
>> @@ -247,8 +274,16 @@ static int stm32_dac_write_raw(struct iio_dev *indio_dev,
>>  	struct stm32_dac *dac = iio_priv(indio_dev);
>>  
>>  	switch (mask) {
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		/* Offset only makes sense in waveform generation mode */
>> +		if (dac->wavetype)
>> +			return stm32_dac_set_value(dac, chan->channel, val);
>> +		return -EBUSY;
> Yeah, I think I sent you down a blind alley here.  If people agree, lets
> just define DDS signals as always being the sum of _raw + the dds element.
> Then it's easy.
Ok, I can revert back to use _raw if this is fine ;-)

>>  	case IIO_CHAN_INFO_RAW:
>> -		return stm32_dac_set_value(dac, chan->channel, val);
>> +		if (!dac->wavetype)
>> +			return stm32_dac_set_value(dac, chan->channel, val);
>> +		/* raw value is read only in waveform generation mode */
>> +		return -EBUSY;
>>  	default:
>>  		return -EINVAL;
>>  	}
>> @@ -334,6 +369,122 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>>  	.set = stm32_dac_set_powerdown_mode,
>>  };
>>  
>> +/* waveform generator wave selection */
>> +static const char * const stm32_dac_wavetype_desc[] = {
>> +	"flat",
>> +	"noise",
>> +	"triangle",
>> +};
>> +
>> +static int stm32_dac_set_wavetype(struct iio_dev *indio_dev,
>> +				  const struct iio_chan_spec *chan,
>> +				  unsigned int wavetype)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	u32 mask, val;
>> +	int ret;
>> +
>> +	/*
>> +	 * Waveform generator requires a trigger to be configured, to increment
>> +	 * or decrement internal counter in case of triangle generator. Noise
>> +	 * generator is a bit different, but also requires a trigger to
>> +	 * generate samples.
>> +	 */
>> +	if (wavetype && !indio_dev->trig)
>> +		dev_dbg(&indio_dev->dev, "Wavegen requires a trigger\n");
>> +
>> +	if (STM32_DAC_IS_CHAN_1(chan->channel)) {
>> +		val = FIELD_PREP(STM32_DAC_CR_WAVE1, wavetype);
>> +		mask = STM32_DAC_CR_WAVE1;
>> +	} else {
>> +		val = FIELD_PREP(STM32_DAC_CR_WAVE2, wavetype);
>> +		mask = STM32_DAC_CR_WAVE2;
>> +	}
>> +
>> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, mask, val);
>> +	if (ret)
>> +		return ret;
>> +	dac->wavetype = wavetype;
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_dac_get_wavetype(struct iio_dev *indio_dev,
>> +				  const struct iio_chan_spec *chan)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (STM32_DAC_IS_CHAN_1(chan->channel))
>> +		return FIELD_GET(STM32_DAC_CR_WAVE1, val);
>> +	else
>> +		return FIELD_GET(STM32_DAC_CR_WAVE2, val);
>> +}
>> +
>> +static const struct iio_enum stm32_dac_wavetype_enum = {
>> +	.items = stm32_dac_wavetype_desc,
>> +	.num_items = ARRAY_SIZE(stm32_dac_wavetype_desc),
>> +	.get = stm32_dac_get_wavetype,
>> +	.set = stm32_dac_set_wavetype,
>> +};
>> +
>> +/*
>> + * waveform generator mamp selection: mask/amplitude
>> + * - noise: LFSR mask (linear feedback shift register, umasks bit 0, [1:0]...)
> This needs breaking out into two attributes - for noise it isn't amplitude in
> any conventional sense...  Keep the result device specific for now. I'm not
> even sure what type of pseudo random source that actually is...
Do you suggest to create specific attribute for this ? This will end-up
to write same register/bitfield as 'amplitude' for triangle generator.

Thanks & Regards,
Fabrice

> If anyone wants to figure it out it would be great to document it in a general
> form!
> 
>> + * - triangle: amplitude (equal to 1, 3, 5, 7... 4095)
>> + */
>> +static const char * const stm32_dac_amplitude_desc[] = {
>> +	"1", "3", "7", "15", "31", "63", "127", "255", "511", "1023", "2047",
>> +	"4095",
>> +};
>> +
>> +static int stm32_dac_set_amplitude(struct iio_dev *indio_dev,
>> +				   const struct iio_chan_spec *chan,
>> +				   unsigned int amplitude)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	u32 mask, val;
>> +
>> +	if (STM32_DAC_IS_CHAN_1(chan->channel)) {
>> +		val = FIELD_PREP(STM32_DAC_CR_MAMP1, amplitude);
>> +		mask = STM32_DAC_CR_MAMP1;
>> +	} else {
>> +		val = FIELD_PREP(STM32_DAC_CR_MAMP2, amplitude);
>> +		mask = STM32_DAC_CR_MAMP2;
>> +	}
>> +
>> +	return regmap_update_bits(dac->common->regmap, STM32_DAC_CR, mask, val);
>> +}
>> +
>> +static int stm32_dac_get_amplitude(struct iio_dev *indio_dev,
>> +				   const struct iio_chan_spec *chan)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (STM32_DAC_IS_CHAN_1(chan->channel))
>> +		return FIELD_GET(STM32_DAC_CR_MAMP1, val);
>> +	else
>> +		return FIELD_GET(STM32_DAC_CR_MAMP2, val);
>> +}
>> +
>> +static const struct iio_enum stm32_dac_amplitude_enum = {
>> +	.items = stm32_dac_amplitude_desc,
>> +	.num_items = ARRAY_SIZE(stm32_dac_amplitude_desc),
>> +	.get = stm32_dac_get_amplitude,
>> +	.set = stm32_dac_set_amplitude,
>> +};
>> +
>>  static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
>>  	{
>>  		.name = "powerdown",
>> @@ -343,6 +494,10 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>>  	},
>>  	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &stm32_dac_powerdown_mode_en),
>>  	IIO_ENUM_AVAILABLE("powerdown_mode", &stm32_dac_powerdown_mode_en),
>> +	IIO_ENUM("wavetype", IIO_SEPARATE, &stm32_dac_wavetype_enum),
>> +	IIO_ENUM_AVAILABLE("wavetype", &stm32_dac_wavetype_enum),
>> +	IIO_ENUM("amplitude", IIO_SEPARATE, &stm32_dac_amplitude_enum),
>> +	IIO_ENUM_AVAILABLE("amplitude", &stm32_dac_amplitude_enum),
>>  	{},
>>  };
>>  
>> @@ -352,6 +507,7 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>>  	.output = 1,					\
>>  	.channel = chan,				\
>>  	.info_mask_separate =				\
>> +		BIT(IIO_CHAN_INFO_OFFSET) |		\
>>  		BIT(IIO_CHAN_INFO_RAW) |		\
>>  		BIT(IIO_CHAN_INFO_SCALE),		\
>>  	/* scan_index is always 0 as num_channels is 1 */ \
>>
> 

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

[Jonathan Cameron]

On 10/04/17 17:43, Fabrice Gasnier wrote:
> On 04/09/2017 11:34 AM, Jonathan Cameron wrote:
>> On 06/04/17 17:11, Fabrice Gasnier wrote:
>>> STM32 DAC has built-in noise or triangle waveform generator.
>>> - "wavetype" extended attribute selects noise or triangle.
>>> - "amplitude" extended attribute selects amplitude for waveform generator
>>>
>>> A DC offset can be added to waveform generator output. This can be done
>>> using out_voltage[1/2]_offset
>>>
>>> Waveform generator requires a trigger to be configured, to increment /
>>> decrement internal counter in case of triangle generator. Noise
>>> generator is a bit different,  but also requires a trigger to generate
>>> samples.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
>>
>> Various bits inline.  Mostly I think the blockers on this will be
>> making sure the ABI defined is generic enough to handle the more crazy
>> DDS chips out there... (basically the ones doing frequency modulation).
>>
>> Jonathan
>>> ---
>>> Changes in v2:
>>> - use _offset parameter to add DC offset to waveform generator
>> Conceptually this offset is just the normal DAC output value (particularly
> yes
>> in the flat case)  I guess we can paper over this by having the _raw
>> and this always have th same value, but it's a little inelegant.
>> Still people are going to expect _raw to control it when in DAC mode but
>> that makes limited sense in DDS mode.
>>
>> Mind you nothing stops us defining all DDS channels as the sum of whatever
>> the DDS is doing and whatever is _raw is set to. Perhaps we tidy this up
>> purely through documentation.  Think of the DDS as a modulation on top
>> of the DAC...
>>
>>> - Rework ABI to better fit existing DDS ABI: use out_voltageY_wavetype,
>>>   out_voltage_wavetype_available, out_voltageY_amplitude,
>>>   out_voltage_amplitude_available
>> Hmm. I'm thinking those amplitude values aren't nice and don't fit well
>> with the more general ABI.
>>
>> I suggested (but didn't really expand upon) having standard defined types
>> for each waveform then using scale to control the amplitude.
> 
> Do you mean _scale attribute ?
Yes
>>
>> Is that something that might work here?
> 
> I probably miss the point here...
>>
>> So say we have our triangle standard form having an amplitude of 1V Peak to
>> Peak. Then we can use scale to make it whatever we actually have in this
>> case?  The docs for wave type will need to describe those standard forms
>> though.
> ... scale is fixed here, in line with _raw attribute. In 'amplitude'
> description for STM32 DAC here (e.g. from 1...4095), same scale is used.
> Can you elaborate ?
Good point - it is already effectively been applied to the _raw
value - I'd forgotten that.

Seems like abuse of the ABI to use calibscale, so we might need something
new here - wavescale maybe or ddsscale?  Not sure.
> 
>>
>> Hmm. Whether this is worth doing is unclear however as we'll still have
>> to describe the 'frequency' in terms of the clock ticks (here the triggers)
> 
> Describing frequency may be an issue, not sure it makes senses in this
> case: 'clock ticks', e.g. triggers here may be timers, but also an EXTI
> (external...), or even software trig perhaps.
To describe the waveform in a remotely standard way we'll have to address
this to some degree.  What's the slope?  
> 
>> So maybe amplitude is worth having.  Again, looking for input from ADI lot
>> on this...  There are some really fiddly cases to describe were we are doing
>> symbol encoding so have multiple waveforms that we are switching between based
>> on some external signal. Any ABI needs to encompass that sort of usage.
>> Parts like the AD9833 for example...
>>
>>> - Better explain trigger usage in case of waveform generator.
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 |  16 +++
>>>  drivers/iio/dac/stm32-dac-core.h                  |   4 +
>>>  drivers/iio/dac/stm32-dac.c                       | 158 +++++++++++++++++++++-
>>>  3 files changed, 177 insertions(+), 1 deletion(-)
>>>  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..8f1fa009
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
>> Fair enough to initially introduced these for this part only, but I'd very
>> much like to see us agree on these sufficiently to get them into the main
>> docs asap so we have something to work with for getting the DDS chips out
>> of staging...
>>> @@ -0,0 +1,16 @@
>>> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_wavetype
>>> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltage_wavetype_available
>>> +KernelVersion:	4.12
>>> +Contact:	fabrice.gasnier-qxv4g6HH51o@public.gmane.org
>>> +Description:
>>> +		List and/or select waveform generation provided by STM32 DAC:
>>> +		- "flat": waveform generator disabled (default)
>>> +		- "noise": select noise waveform
>>> +		- "triangle": select triangle waveform
>>> +
>>> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_amplitude
>>> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltage_amplitude_available
>>> +KernelVersion:	4.12
>>> +Contact:	fabrice.gasnier-qxv4g6HH51o@public.gmane.org
>>> +Description:
>>> +		List and/or select amplitude used for waveform generator
>>> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
>>> index e51a468..0f02975 100644
>>> --- a/drivers/iio/dac/stm32-dac-core.h
>>> +++ b/drivers/iio/dac/stm32-dac-core.h
>>> @@ -37,8 +37,12 @@
>>>  #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)
>>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>>> index a7a078e..2ed75db 100644
>>> --- a/drivers/iio/dac/stm32-dac.c
>>> +++ b/drivers/iio/dac/stm32-dac.c
>>> @@ -42,10 +42,12 @@
>>>  /**
>>>   * struct stm32_dac - private data of DAC driver
>>>   * @common:		reference to DAC common data
>>> + * @wavetype:		waveform generator
>>>   * @swtrig:		Using software trigger
>>>   */
>>>  struct stm32_dac {
>>>  	struct stm32_dac_common *common;
>>> +	u32 wavetype;
>>>  	bool swtrig;
>>>  };
>>>  
>>> @@ -222,6 +224,29 @@ static int stm32_dac_set_value(struct stm32_dac *dac, int channel, int val)
>>>  	return ret;
>>>  }
>>>  
>>> +static int stm32_dac_get_offset(struct stm32_dac *dac, int channel, int *val)
>>> +{
>>> +	int ret;
>>> +
>>> +	/* Offset is only relevant in waveform generation mode. */
>>> +	if (!dac->wavetype) {
>>> +		*val = 0;
>>> +		return IIO_VAL_INT;
>>> +	}
>>> +
>>> +	/*
>>> +	 * In waveform generation mode, DC offset in DHR is added to waveform
>>> +	 * generator output, then stored to DOR (data output register).
>>> +	 * Read offset from DHR.
>>> +	 */
>> Just thinking what fun we could have if we do the fifo based output to push
>> this that I was suggesting for the previous patch ;) triangles on top
>> of fun general waveforms..
>>
>>> +	if (STM32_DAC_IS_CHAN_1(channel))
>>> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DHR12R1, val);
>>> +	else
>>> +		ret = regmap_read(dac->common->regmap, STM32_DAC_DHR12R2, val);
>>> +
>>> +	return ret ? ret : IIO_VAL_INT;
>>> +}
>>> +
>>>  static int stm32_dac_read_raw(struct iio_dev *indio_dev,
>>>  			      struct iio_chan_spec const *chan,
>>>  			      int *val, int *val2, long mask)
>>> @@ -231,6 +256,8 @@ static int stm32_dac_read_raw(struct iio_dev *indio_dev,
>>>  	switch (mask) {
>>>  	case IIO_CHAN_INFO_RAW:
>>>  		return stm32_dac_get_value(dac, chan->channel, val);
>>> +	case IIO_CHAN_INFO_OFFSET:
>>> +		return stm32_dac_get_offset(dac, chan->channel, val);
>>>  	case IIO_CHAN_INFO_SCALE:
>>>  		*val = dac->common->vref_mv;
>>>  		*val2 = chan->scan_type.realbits;
>>> @@ -247,8 +274,16 @@ static int stm32_dac_write_raw(struct iio_dev *indio_dev,
>>>  	struct stm32_dac *dac = iio_priv(indio_dev);
>>>  
>>>  	switch (mask) {
>>> +	case IIO_CHAN_INFO_OFFSET:
>>> +		/* Offset only makes sense in waveform generation mode */
>>> +		if (dac->wavetype)
>>> +			return stm32_dac_set_value(dac, chan->channel, val);
>>> +		return -EBUSY;
>> Yeah, I think I sent you down a blind alley here.  If people agree, lets
>> just define DDS signals as always being the sum of _raw + the dds element.
>> Then it's easy.
> Ok, I can revert back to use _raw if this is fine ;-)
> 
>>>  	case IIO_CHAN_INFO_RAW:
>>> -		return stm32_dac_set_value(dac, chan->channel, val);
>>> +		if (!dac->wavetype)
>>> +			return stm32_dac_set_value(dac, chan->channel, val);
>>> +		/* raw value is read only in waveform generation mode */
>>> +		return -EBUSY;
>>>  	default:
>>>  		return -EINVAL;
>>>  	}
>>> @@ -334,6 +369,122 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>>>  	.set = stm32_dac_set_powerdown_mode,
>>>  };
>>>  
>>> +/* waveform generator wave selection */
>>> +static const char * const stm32_dac_wavetype_desc[] = {
>>> +	"flat",
>>> +	"noise",
>>> +	"triangle",
>>> +};
>>> +
>>> +static int stm32_dac_set_wavetype(struct iio_dev *indio_dev,
>>> +				  const struct iio_chan_spec *chan,
>>> +				  unsigned int wavetype)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +	u32 mask, val;
>>> +	int ret;
>>> +
>>> +	/*
>>> +	 * Waveform generator requires a trigger to be configured, to increment
>>> +	 * or decrement internal counter in case of triangle generator. Noise
>>> +	 * generator is a bit different, but also requires a trigger to
>>> +	 * generate samples.
>>> +	 */
>>> +	if (wavetype && !indio_dev->trig)
>>> +		dev_dbg(&indio_dev->dev, "Wavegen requires a trigger\n");
>>> +
>>> +	if (STM32_DAC_IS_CHAN_1(chan->channel)) {
>>> +		val = FIELD_PREP(STM32_DAC_CR_WAVE1, wavetype);
>>> +		mask = STM32_DAC_CR_WAVE1;
>>> +	} else {
>>> +		val = FIELD_PREP(STM32_DAC_CR_WAVE2, wavetype);
>>> +		mask = STM32_DAC_CR_WAVE2;
>>> +	}
>>> +
>>> +	ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, mask, val);
>>> +	if (ret)
>>> +		return ret;
>>> +	dac->wavetype = wavetype;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int stm32_dac_get_wavetype(struct iio_dev *indio_dev,
>>> +				  const struct iio_chan_spec *chan)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +	u32 val;
>>> +	int ret;
>>> +
>>> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	if (STM32_DAC_IS_CHAN_1(chan->channel))
>>> +		return FIELD_GET(STM32_DAC_CR_WAVE1, val);
>>> +	else
>>> +		return FIELD_GET(STM32_DAC_CR_WAVE2, val);
>>> +}
>>> +
>>> +static const struct iio_enum stm32_dac_wavetype_enum = {
>>> +	.items = stm32_dac_wavetype_desc,
>>> +	.num_items = ARRAY_SIZE(stm32_dac_wavetype_desc),
>>> +	.get = stm32_dac_get_wavetype,
>>> +	.set = stm32_dac_set_wavetype,
>>> +};
>>> +
>>> +/*
>>> + * waveform generator mamp selection: mask/amplitude
>>> + * - noise: LFSR mask (linear feedback shift register, umasks bit 0, [1:0]...)
>> This needs breaking out into two attributes - for noise it isn't amplitude in
>> any conventional sense...  Keep the result device specific for now. I'm not
>> even sure what type of pseudo random source that actually is...
> Do you suggest to create specific attribute for this ? This will end-up
> to write same register/bitfield as 'amplitude' for triangle generator.
I think it should definitely be a separate attribute.  Otherwise the userspace
ABI will be really confusing as this definitely isn't amplitude in any normal
sense!
> 
> Thanks & Regards,
> Fabrice
> 
>> If anyone wants to figure it out it would be great to document it in a general
>> form!
>>
>>> + * - triangle: amplitude (equal to 1, 3, 5, 7... 4095)
>>> + */
>>> +static const char * const stm32_dac_amplitude_desc[] = {
>>> +	"1", "3", "7", "15", "31", "63", "127", "255", "511", "1023", "2047",
>>> +	"4095",
>>> +};
>>> +
>>> +static int stm32_dac_set_amplitude(struct iio_dev *indio_dev,
>>> +				   const struct iio_chan_spec *chan,
>>> +				   unsigned int amplitude)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +	u32 mask, val;
>>> +
>>> +	if (STM32_DAC_IS_CHAN_1(chan->channel)) {
>>> +		val = FIELD_PREP(STM32_DAC_CR_MAMP1, amplitude);
>>> +		mask = STM32_DAC_CR_MAMP1;
>>> +	} else {
>>> +		val = FIELD_PREP(STM32_DAC_CR_MAMP2, amplitude);
>>> +		mask = STM32_DAC_CR_MAMP2;
>>> +	}
>>> +
>>> +	return regmap_update_bits(dac->common->regmap, STM32_DAC_CR, mask, val);
>>> +}
>>> +
>>> +static int stm32_dac_get_amplitude(struct iio_dev *indio_dev,
>>> +				   const struct iio_chan_spec *chan)
>>> +{
>>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>>> +	u32 val;
>>> +	int ret;
>>> +
>>> +	ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	if (STM32_DAC_IS_CHAN_1(chan->channel))
>>> +		return FIELD_GET(STM32_DAC_CR_MAMP1, val);
>>> +	else
>>> +		return FIELD_GET(STM32_DAC_CR_MAMP2, val);
>>> +}
>>> +
>>> +static const struct iio_enum stm32_dac_amplitude_enum = {
>>> +	.items = stm32_dac_amplitude_desc,
>>> +	.num_items = ARRAY_SIZE(stm32_dac_amplitude_desc),
>>> +	.get = stm32_dac_get_amplitude,
>>> +	.set = stm32_dac_set_amplitude,
>>> +};
>>> +
>>>  static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
>>>  	{
>>>  		.name = "powerdown",
>>> @@ -343,6 +494,10 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>>>  	},
>>>  	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &stm32_dac_powerdown_mode_en),
>>>  	IIO_ENUM_AVAILABLE("powerdown_mode", &stm32_dac_powerdown_mode_en),
>>> +	IIO_ENUM("wavetype", IIO_SEPARATE, &stm32_dac_wavetype_enum),
>>> +	IIO_ENUM_AVAILABLE("wavetype", &stm32_dac_wavetype_enum),
>>> +	IIO_ENUM("amplitude", IIO_SEPARATE, &stm32_dac_amplitude_enum),
>>> +	IIO_ENUM_AVAILABLE("amplitude", &stm32_dac_amplitude_enum),
>>>  	{},
>>>  };
>>>  
>>> @@ -352,6 +507,7 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>>>  	.output = 1,					\
>>>  	.channel = chan,				\
>>>  	.info_mask_separate =				\
>>> +		BIT(IIO_CHAN_INFO_OFFSET) |		\
>>>  		BIT(IIO_CHAN_INFO_RAW) |		\
>>>  		BIT(IIO_CHAN_INFO_SCALE),		\
>>>  	/* scan_index is always 0 as num_channels is 1 */ \
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>