[PATCH v2 0/3] Add support for LTC2688

[PATCH v2 0/3] Add support for LTC2688

[Nuno Sá]

The ABI defined for this driver has some subtleties that were previously
discussed in this RFC [1]. This might not be the final state but,
hopefully, we are close to it:

toggle mode channels:

 * out_voltageY_toggle_en
 * out_voltageY_raw0
 * out_voltageY_raw1
 * out_voltageY_symbol

dither mode channels:

 * out_voltageY_dither_en
 * out_voltageY_dither_raw
 * out_voltageY_dither_raw_available
 * out_voltageY_dither_offset
 * out_voltageY_dither_frequency
 * out_voltageY_dither_frequency_available
 * out_voltageY_dither_phase
 * out_voltageY_dither_phase_available

Default channels won't have any of the above ABIs. A channel is toggle
capable if the devicetree 'adi,toggle-mode' flag is set. For dither, the
assumption is more silent. If 'adi,toggle-mode' is not given and a
channel is associated with a TGPx pin through 'adi,toggle-dither-input',
then the channel is assumed to be dither capable (there's no point in
having a dither capable channel without an input clock).

changes in v2:

 ltc2688:
  * Use local buffer for regmap read. Do not assume that reg is part of
larger buffer;
  * Renamed GPIO to "clr" so that is consistent with the datasheet;
  * Renamed 'mask' and 'm' to info. 'mask' is a thing from the past;
  * Removed 'LTC2688_CHAN_TOGGLE()' and defined to static ext_info arrays;
  * Use 'regmap_set_bits' to set external ref;
  * Use FIELD_{PREP|GET} for dither amplitude and channel calibbias where
only 13bits are used;
  * Use 'regmap_write()' instead of update_bits for channels settings;
  * Init 'val' at the beginning of the channel configuration loop
(and drop mask);
  * Comment 'ltc2688_reg_writable()' to account for the special condition;
  * Kmemdup default channels so that it can be safely changed per probed
device;
  * Replace extended info multiplexer functions by individual functions;
  * Use raw0 ABI for toggle channels;
  * Use dedicated offset ABI for dither channels;
  * Misc changes (spell fixes, blank lines...);
  * Have a clock property per channel. Note that we this I moved to OF
since we now have to use 'devm_get_clk_from_child()' which is using
device_node. Note that I could use 'to_of_node()' but mixing of.h and
property.h does not feel like a good idea.

 ABI:
  * Added out_voltageY_raw0 ABI for toggle mode;
  * Added out_voltageY_dither_offset.

 Bindings:
  * Use standard microvolt unit;
  * Added constrains for adi,output-range-microvolt and removed negative
values from the dts example;
  * Moved clocks to the channel object;
  * Dropped clock-names;
  * Add a dependency between 'adi,toggle-dither-input' and 'clocks'.
 
[1]: https://marc.info/?l=linux-iio&m=163662843603265&w=2

Nuno Sá (3):
  iio: dac: add support for ltc2688
  iio: ABI: add ABI file for the LTC2688 DAC
  dt-bindings: iio: Add ltc2688 documentation

 .../ABI/testing/sysfs-bus-iio-dac-ltc2688     |   80 ++
 .../bindings/iio/dac/adi,ltc2688.yaml         |  147 +++
 MAINTAINERS                                   |    9 +
 drivers/iio/dac/Kconfig                       |   11 +
 drivers/iio/dac/Makefile                      |    1 +
 drivers/iio/dac/ltc2688.c                     | 1070 +++++++++++++++++
 6 files changed, 1318 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
 create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
 create mode 100644 drivers/iio/dac/ltc2688.c

-- 
2.33.1

[PATCH v2 1/3] iio: dac: add support for ltc2688

[Nuno Sá]

The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
precision reference. It is guaranteed monotonic and has built in
rail-to-rail output buffers that can source or sink up to 20 mA.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 MAINTAINERS               |    7 +
 drivers/iio/dac/Kconfig   |   11 +
 drivers/iio/dac/Makefile  |    1 +
 drivers/iio/dac/ltc2688.c | 1070 +++++++++++++++++++++++++++++++++++++
 4 files changed, 1089 insertions(+)
 create mode 100644 drivers/iio/dac/ltc2688.c

diff --git a/MAINTAINERS b/MAINTAINERS
index de13836a8f7f..16e344d52b1e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11182,6 +11182,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/dac/lltc,ltc1660.yaml
 F:	drivers/iio/dac/ltc1660.c
 
+LTC2688 IIO DAC DRIVER
+M:	Nuno Sá <nuno.sa@analog.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+W:	http://ez.analog.com/community/linux-device-drivers
+F:	drivers/iio/dac/ltc2688.c
+
 LTC2947 HARDWARE MONITOR DRIVER
 M:	Nuno Sá <nuno.sa@analog.com>
 L:	linux-hwmon@vger.kernel.org
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index bfcf7568de32..c0bf0d84197f 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -131,6 +131,17 @@ config AD5624R_SPI
 	  Say yes here to build support for Analog Devices AD5624R, AD5644R and
 	  AD5664R converters (DAC). This driver uses the common SPI interface.
 
+config LTC2688
+	tristate "Analog Devices LTC2688 DAC spi driver"
+	depends on SPI
+	select REGMAP
+	help
+	  Say yes here to build support for Analog Devices
+	  LTC2688 converters (DAC).
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ltc2688.
+
 config AD5686
 	tristate
 
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 01a50131572f..ec3e42713f00 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_DS4424) += ds4424.o
 obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o
 obj-$(CONFIG_LTC1660) += ltc1660.o
 obj-$(CONFIG_LTC2632) += ltc2632.o
+obj-$(CONFIG_LTC2688) += ltc2688.o
 obj-$(CONFIG_M62332) += m62332.o
 obj-$(CONFIG_MAX517) += max517.o
 obj-$(CONFIG_MAX5821) += max5821.o
diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c
new file mode 100644
index 000000000000..ed39be7a9c12
--- /dev/null
+++ b/drivers/iio/dac/ltc2688.c
@@ -0,0 +1,1070 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * LTC2688 16 channel, 16 bit Voltage Output SoftSpan DAC driver
+ *
+ * Copyright 2021 Analog Devices Inc.
+ */
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/limits.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#define LTC2688_DAC_CHANNELS			16
+
+#define LTC2688_CMD_CH_CODE(x)			(0x00 + (x))
+#define LTC2688_CMD_CH_SETTING(x)		(0x10 + (x))
+#define LTC2688_CMD_CH_OFFSET(x)		(0X20 + (x))
+#define LTC2688_CMD_CH_GAIN(x)			(0x30 + (x))
+#define LTC2688_CMD_CH_CODE_UPDATE(x)		(0x40 + (x))
+
+#define LTC2688_CMD_CONFIG			0x70
+#define LTC2688_CMD_POWERDOWN			0x71
+#define LTC2688_CMD_A_B_SELECT			0x72
+#define LTC2688_CMD_SW_TOGGLE			0x73
+#define LTC2688_CMD_TOGGLE_DITHER_EN		0x74
+#define LTC2688_CMD_THERMAL_STAT		0x77
+#define LTC2688_CMD_UPDATE_ALL			0x7C
+#define LTC2688_CMD_NOOP			0xFF
+
+#define LTC2688_READ_OPERATION			0x80
+
+/* Channel Settings */
+#define LTC2688_CH_SPAN_MSK			GENMASK(2, 0)
+#define LTC2688_CH_OVERRANGE_MSK		BIT(3)
+#define LTC2688_CH_TD_SEL_MSK			GENMASK(5, 4)
+#define LTC2688_CH_TGP_MAX			3
+#define LTC2688_CH_DIT_PER_MSK			GENMASK(8, 6)
+#define LTC2688_CH_DIT_PH_MSK			GENMASK(10, 9)
+#define LTC2688_CH_MODE_MSK			BIT(11)
+
+#define LTC2688_DITHER_RAW_MASK			GENMASK(15, 2)
+#define LTC2688_CH_CALIBBIAS_MASK		GENMASK(15, 2)
+#define LTC2688_DITHER_RAW_MAX_VAL		(BIT(14) - 1)
+#define LTC2688_CH_CALIBBIAS_MAX_VAL		(BIT(14) - 1)
+
+/* Configuration register */
+#define LTC2688_CONFIG_RST			BIT(15)
+#define LTC2688_CONFIG_EXT_REF			BIT(1)
+
+#define LTC2688_DITHER_FREQ_AVAIL_N		5
+
+enum {
+	LTC2688_SPAN_RANGE_0V_5V,
+	LTC2688_SPAN_RANGE_0V_10V,
+	LTC2688_SPAN_RANGE_M5V_5V,
+	LTC2688_SPAN_RANGE_M10V_10V,
+	LTC2688_SPAN_RANGE_M15V_15V,
+	LTC2688_SPAN_RANGE_MAX
+};
+
+enum {
+	LTC2688_MODE_DEFAULT,
+	LTC2688_MODE_DITHER_TOGGLE,
+};
+
+struct ltc2688_chan {
+	long dither_frequency[LTC2688_DITHER_FREQ_AVAIL_N];
+	bool overrange;
+	bool toggle_chan;
+	u8 mode;
+};
+
+struct ltc2688_state {
+	struct spi_device *spi;
+	struct regmap *regmap;
+	struct regulator_bulk_data regulators[2];
+	struct ltc2688_chan channels[LTC2688_DAC_CHANNELS];
+	struct iio_chan_spec *iio_chan;
+	/* lock to protect against multiple access to the device and shared data */
+	struct mutex lock;
+	int vref;
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	u8 tx_data[6] ____cacheline_aligned;
+	u8 rx_data[3];
+};
+
+static int ltc2688_spi_read(void *context, const void *reg, size_t reg_size,
+			    void *val, size_t val_size)
+{
+	struct ltc2688_state *st = context;
+	struct spi_transfer xfers[] = {
+		{
+			.tx_buf = st->tx_data,
+			.bits_per_word = 8,
+			.len = 3,
+			.cs_change = 1,
+		}, {
+			.tx_buf = st->tx_data + 3,
+			.rx_buf = st->rx_data,
+			.bits_per_word = 8,
+			.len = 3,
+		},
+	};
+	int ret;
+
+	memcpy(st->tx_data, reg, reg_size);
+
+	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+	if (ret)
+		return ret;
+
+	memcpy(val, &st->rx_data[1], val_size);
+
+	return 0;
+}
+
+static int ltc2688_spi_write(void *context, const void *data, size_t count)
+{
+	struct ltc2688_state *st = context;
+
+	return spi_write(st->spi, data, count);
+}
+
+static int ltc2688_span_get(const struct ltc2688_state *st, int c)
+{
+	int ret, reg, span;
+
+	ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(c), &reg);
+	if (ret)
+		return ret;
+
+	span = FIELD_GET(LTC2688_CH_SPAN_MSK, reg);
+	/* sanity check to make sure we don't get any weird value from the HW */
+	if (span >= LTC2688_SPAN_RANGE_MAX)
+		return -EIO;
+
+	return span;
+}
+
+static const int ltc2688_span_helper[LTC2688_SPAN_RANGE_MAX][2] = {
+	{0, 5000}, {0, 10000}, {-5000, 5000}, {-10000, 10000}, {-15000, 15000},
+};
+
+static int ltc2688_scale_get(const struct ltc2688_state *st, int c, int *val)
+{
+	const struct ltc2688_chan *chan = &st->channels[c];
+	int span, fs;
+
+	span = ltc2688_span_get(st, c);
+	if (span < 0)
+		return span;
+
+	fs = ltc2688_span_helper[span][1] - ltc2688_span_helper[span][0];
+	if (chan->overrange)
+		fs = mult_frac(fs, 105, 100);
+
+	*val = DIV_ROUND_CLOSEST(fs * st->vref, 4096);
+
+	return 0;
+}
+
+static int ltc2688_offset_get(const struct ltc2688_state *st, int c, int *val)
+{
+	int span;
+
+	span = ltc2688_span_get(st, c);
+	if (span < 0)
+		return span;
+
+	if (ltc2688_span_helper[span][0] < 0)
+		*val = -32768;
+	else
+		*val = 0;
+
+	return 0;
+}
+
+enum {
+	LTC2688_INPUT_A,
+	LTC2688_INPUT_B,
+	LTC2688_INPUT_B_AVAIL,
+	LTC2688_DITHER_OFF,
+	LTC2688_DITHER_FREQ_AVAIL,
+};
+
+static int ltc2688_dac_code_write(struct ltc2688_state *st, u32 chan, u32 input,
+				  u16 code)
+{
+	struct ltc2688_chan *c = &st->channels[chan];
+	int ret, reg;
+
+	/* 2 LSBs set to 0 if writing dither amplitude */
+	if (!c->toggle_chan && input == LTC2688_INPUT_B) {
+		if (code > LTC2688_DITHER_RAW_MAX_VAL)
+			return -EINVAL;
+
+		code = FIELD_PREP(LTC2688_DITHER_RAW_MASK, code);
+	}
+
+	mutex_lock(&st->lock);
+	/* select the correct input register to read from */
+	ret = regmap_update_bits(st->regmap, LTC2688_CMD_A_B_SELECT, BIT(chan),
+				 input << chan);
+	if (ret)
+		goto unlock;
+
+	/*
+	 * If in dither/toggle mode the dac should be updated by an
+	 * external signal (or sw toggle) and not here.
+	 */
+	if (c->mode == LTC2688_MODE_DEFAULT)
+		reg = LTC2688_CMD_CH_CODE_UPDATE(chan);
+	else
+		reg = LTC2688_CMD_CH_CODE(chan);
+
+	ret = regmap_write(st->regmap, reg, code);
+unlock:
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
+static int ltc2688_dac_code_read(struct ltc2688_state *st, u32 chan, u32 input,
+				 u32 *code)
+{
+	struct ltc2688_chan *c = &st->channels[chan];
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = regmap_update_bits(st->regmap, LTC2688_CMD_A_B_SELECT, BIT(chan),
+				 input << chan);
+	if (ret)
+		goto unlock;
+
+	ret = regmap_read(st->regmap, LTC2688_CMD_CH_CODE(chan), code);
+unlock:
+	mutex_unlock(&st->lock);
+
+	if (!c->toggle_chan && input == LTC2688_INPUT_B)
+		*code = FIELD_GET(LTC2688_DITHER_RAW_MASK, *code);
+
+	return ret;
+}
+
+static const int ltc2688_raw_range[] = {0, 1, U16_MAX};
+
+static int ltc2688_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		*vals = ltc2688_raw_range;
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_RANGE;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltc2688_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long info)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = ltc2688_dac_code_read(st, chan->channel, LTC2688_INPUT_A,
+					    val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OFFSET:
+		ret = ltc2688_offset_get(st, chan->channel, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		ret = ltc2688_scale_get(st, chan->channel, val);
+		if (ret)
+			return ret;
+
+		*val = 16;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_CALIBBIAS:
+		ret = regmap_read(st->regmap,
+				  LTC2688_CMD_CH_OFFSET(chan->channel), val);
+		if (ret)
+			return ret;
+
+		*val = FIELD_GET(LTC2688_CH_CALIBBIAS_MASK, *val);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_CALIBSCALE:
+		ret = regmap_read(st->regmap,
+				  LTC2688_CMD_CH_GAIN(chan->channel), val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltc2688_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long info)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		if (val > U16_MAX || val < 0)
+			return -EINVAL;
+
+		return ltc2688_dac_code_write(st, chan->channel,
+					      LTC2688_INPUT_A, val);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		if (val > LTC2688_CH_CALIBBIAS_MAX_VAL)
+			return -EINVAL;
+
+		return regmap_write(st->regmap,
+				    LTC2688_CMD_CH_OFFSET(chan->channel),
+				    FIELD_PREP(LTC2688_CH_CALIBBIAS_MASK, val));
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return regmap_write(st->regmap,
+				    LTC2688_CMD_CH_GAIN(chan->channel), val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltc2688_dither_toggle_set(struct iio_dev *indio_dev,
+				     uintptr_t private,
+				     const struct iio_chan_spec *chan,
+				     const char *buf, size_t len)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+	struct ltc2688_chan *c = &st->channels[chan->channel];
+	int ret;
+	bool en;
+
+	ret = kstrtobool(buf, &en);
+	if (ret)
+		return ret;
+
+	mutex_lock(&st->lock);
+	ret = regmap_update_bits(st->regmap, LTC2688_CMD_TOGGLE_DITHER_EN,
+				 BIT(chan->channel), en << chan->channel);
+	if (ret)
+		goto unlock;
+
+	c->mode = en ? LTC2688_MODE_DITHER_TOGGLE : LTC2688_MODE_DEFAULT;
+unlock:
+	mutex_unlock(&st->lock);
+
+	return ret ?: len;
+}
+
+static ssize_t ltc2688_reg_bool_get(struct iio_dev *indio_dev,
+				    uintptr_t private,
+				    const struct iio_chan_spec *chan,
+				    char *buf)
+{
+	const struct ltc2688_state *st = iio_priv(indio_dev);
+	int ret;
+	u32 val;
+
+	ret = regmap_read(st->regmap, private, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", !!(val & BIT(chan->channel)));
+}
+
+static ssize_t ltc2688_reg_bool_set(struct iio_dev *indio_dev,
+				    uintptr_t private,
+				    const struct iio_chan_spec *chan,
+				    const char *buf, size_t len)
+{
+	const struct ltc2688_state *st = iio_priv(indio_dev);
+	int ret;
+	bool en;
+
+	ret = kstrtobool(buf, &en);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(st->regmap, private, BIT(chan->channel),
+				 en << chan->channel);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static ssize_t ltc2688_dither_freq_avail(const struct ltc2688_state *st,
+					 const struct ltc2688_chan *chan,
+					 char *buf)
+{
+	int sz = 0;
+	u32 f;
+
+	for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++)
+		sz += sysfs_emit_at(buf, sz, "%ld ", chan->dither_frequency[f]);
+
+	buf[sz - 1] = '\n';
+
+	return sz;
+}
+
+static ssize_t ltc2688_dither_freq_get(struct iio_dev *indio_dev,
+				       uintptr_t private,
+				       const struct iio_chan_spec *chan,
+				       char *buf)
+{
+	const struct ltc2688_state *st = iio_priv(indio_dev);
+	const struct ltc2688_chan *c = &st->channels[chan->channel];
+	u32 reg, freq;
+	int ret;
+
+	if (private == LTC2688_DITHER_FREQ_AVAIL)
+		return ltc2688_dither_freq_avail(st, c, buf);
+
+	ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(chan->channel),
+			  &reg);
+	if (ret)
+		return ret;
+
+	freq = FIELD_GET(LTC2688_CH_DIT_PER_MSK, reg);
+	if (freq >= ARRAY_SIZE(c->dither_frequency))
+		return -EIO;
+
+	return sysfs_emit(buf, "%ld\n", c->dither_frequency[freq]);
+}
+
+static int ltc2688_dither_freq_set(struct iio_dev *indio_dev,
+				   uintptr_t private,
+				   const struct iio_chan_spec *chan,
+				   const char *buf, size_t len)
+{
+	const struct ltc2688_state *st = iio_priv(indio_dev);
+	const struct ltc2688_chan *c = &st->channels[chan->channel];
+	long val;
+	u32 freq;
+	int ret;
+
+	if (private == LTC2688_DITHER_FREQ_AVAIL)
+		return -EINVAL;
+
+	ret = kstrtol(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	for (freq = 0; freq < ARRAY_SIZE(c->dither_frequency); freq++) {
+		if (val == c->dither_frequency[freq])
+			break;
+	}
+
+	if (freq == ARRAY_SIZE(c->dither_frequency))
+		return -EINVAL;
+
+	ret = regmap_update_bits(st->regmap,
+				 LTC2688_CMD_CH_SETTING(chan->channel),
+				 LTC2688_CH_DIT_PER_MSK,
+				 FIELD_PREP(LTC2688_CH_DIT_PER_MSK, freq));
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static ssize_t ltc2688_dac_input_read(struct iio_dev *indio_dev,
+				      uintptr_t private,
+				      const struct iio_chan_spec *chan,
+				      char *buf)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+	int ret;
+	u32 val;
+
+	if (private == LTC2688_INPUT_B_AVAIL)
+		return sysfs_emit(buf, "[%u %u %u]\n", ltc2688_raw_range[0],
+				  ltc2688_raw_range[1],
+				  ltc2688_raw_range[2] / 4);
+
+	if (private == LTC2688_DITHER_OFF)
+		return sysfs_emit(buf, "0\n");
+
+	ret = ltc2688_dac_code_read(st, chan->channel, private, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", val);
+}
+
+static ssize_t ltc2688_dac_input_write(struct iio_dev *indio_dev,
+				       uintptr_t private,
+				       const struct iio_chan_spec *chan,
+				       const char *buf, size_t len)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+	int ret;
+	u16 val;
+
+	if (private == LTC2688_INPUT_B_AVAIL || private == LTC2688_DITHER_OFF)
+		return -EINVAL;
+
+	ret = kstrtou16(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	ret = ltc2688_dac_code_write(st, chan->channel, private, val);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static int ltc2688_get_dither_phase(struct iio_dev *dev,
+				    const struct iio_chan_spec *chan)
+{
+	struct ltc2688_state *st = iio_priv(dev);
+	int ret, regval;
+
+	ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(chan->channel),
+			  &regval);
+	if (ret)
+		return ret;
+
+	return FIELD_GET(LTC2688_CH_DIT_PH_MSK, regval);
+}
+
+static int ltc2688_set_dither_phase(struct iio_dev *dev,
+				    const struct iio_chan_spec *chan,
+				    unsigned int phase)
+{
+	struct ltc2688_state *st = iio_priv(dev);
+
+	return regmap_update_bits(st->regmap,
+				  LTC2688_CMD_CH_SETTING(chan->channel),
+				  LTC2688_CH_DIT_PH_MSK,
+				  FIELD_PREP(LTC2688_CH_DIT_PH_MSK, phase));
+}
+
+static int ltc2688_reg_access(struct iio_dev *indio_dev,
+			      unsigned int reg,
+			      unsigned int writeval,
+			      unsigned int *readval)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
+}
+
+static const char * const ltc2688_dither_phase[] = {
+	"0", "90", "180", "270",
+};
+
+static const struct iio_enum ltc2688_dither_phase_enum = {
+	.items = ltc2688_dither_phase,
+	.num_items = ARRAY_SIZE(ltc2688_dither_phase),
+	.set = ltc2688_set_dither_phase,
+	.get = ltc2688_get_dither_phase,
+};
+
+#define LTC2688_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {	\
+	.name = _name,							\
+	.read = (_read),						\
+	.write = (_write),						\
+	.private = (_what),						\
+	.shared = (_shared),						\
+}
+
+/*
+ * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is
+ * not provided in dts.
+ */
+static const struct iio_chan_spec_ext_info ltc2688_toggle_sym_ext_info[] = {
+	LTC2688_CHAN_EXT_INFO("raw0", LTC2688_INPUT_A, IIO_SEPARATE,
+			      ltc2688_dac_input_read, ltc2688_dac_input_write),
+	LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B, IIO_SEPARATE,
+			      ltc2688_dac_input_read, ltc2688_dac_input_write),
+	LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_CMD_TOGGLE_DITHER_EN,
+			      IIO_SEPARATE, ltc2688_reg_bool_get,
+			      ltc2688_dither_toggle_set),
+	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_CMD_POWERDOWN, IIO_SEPARATE,
+			      ltc2688_reg_bool_get, ltc2688_reg_bool_set),
+	LTC2688_CHAN_EXT_INFO("symbol", LTC2688_CMD_SW_TOGGLE, IIO_SEPARATE,
+			      ltc2688_reg_bool_get, ltc2688_reg_bool_set),
+	{}
+};
+
+static const struct iio_chan_spec_ext_info ltc2688_toggle_ext_info[] = {
+	LTC2688_CHAN_EXT_INFO("raw0", LTC2688_INPUT_A, IIO_SEPARATE,
+			      ltc2688_dac_input_read, ltc2688_dac_input_write),
+	LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B, IIO_SEPARATE,
+			      ltc2688_dac_input_read, ltc2688_dac_input_write),
+	LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_CMD_TOGGLE_DITHER_EN,
+			      IIO_SEPARATE, ltc2688_reg_bool_get,
+			      ltc2688_dither_toggle_set),
+	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_CMD_POWERDOWN, IIO_SEPARATE,
+			      ltc2688_reg_bool_get, ltc2688_reg_bool_set),
+	{}
+};
+
+static struct iio_chan_spec_ext_info ltc2688_dither_ext_info[] = {
+	LTC2688_CHAN_EXT_INFO("dither_raw", LTC2688_INPUT_B, IIO_SEPARATE,
+			      ltc2688_dac_input_read, ltc2688_dac_input_write),
+	LTC2688_CHAN_EXT_INFO("dither_raw_available", LTC2688_INPUT_B_AVAIL,
+			      IIO_SEPARATE, ltc2688_dac_input_read,
+			      ltc2688_dac_input_write),
+	LTC2688_CHAN_EXT_INFO("dither_offset", LTC2688_DITHER_OFF, IIO_SEPARATE,
+			      ltc2688_dac_input_read, ltc2688_dac_input_write),
+	/*
+	 * Not IIO_ENUM because the available freq needs to be computed at
+	 * probe. We could still use it, but it didn't felt much right.
+	 */
+	LTC2688_CHAN_EXT_INFO("dither_frequency", 0, IIO_SEPARATE,
+			      ltc2688_dither_freq_get, ltc2688_dither_freq_set),
+	LTC2688_CHAN_EXT_INFO("dither_frequency_available",
+			      LTC2688_DITHER_FREQ_AVAIL, IIO_SEPARATE,
+			      ltc2688_dither_freq_get, ltc2688_dither_freq_set),
+	IIO_ENUM("dither_phase", IIO_SEPARATE, &ltc2688_dither_phase_enum),
+	IIO_ENUM_AVAILABLE("dither_phase", IIO_SEPARATE,
+			   &ltc2688_dither_phase_enum),
+	LTC2688_CHAN_EXT_INFO("dither_en", LTC2688_CMD_TOGGLE_DITHER_EN,
+			      IIO_SEPARATE, ltc2688_reg_bool_get,
+			      ltc2688_dither_toggle_set),
+	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_CMD_POWERDOWN, IIO_SEPARATE,
+			      ltc2688_reg_bool_get, ltc2688_reg_bool_set),
+	{}
+};
+
+static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
+	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_CMD_POWERDOWN, IIO_SEPARATE,
+			      ltc2688_reg_bool_get, ltc2688_reg_bool_set),
+	{}
+};
+
+#define LTC2688_CHANNEL(_chan) {					\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.output = 1,							\
+	.channel = (_chan),						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
+		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |	\
+		BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_RAW),	\
+	.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW),		\
+	.ext_info = ltc2688_ext_info					\
+}
+
+static const struct iio_chan_spec ltc2688_channels[] = {
+	LTC2688_CHANNEL(0),
+	LTC2688_CHANNEL(1),
+	LTC2688_CHANNEL(2),
+	LTC2688_CHANNEL(3),
+	LTC2688_CHANNEL(4),
+	LTC2688_CHANNEL(5),
+	LTC2688_CHANNEL(6),
+	LTC2688_CHANNEL(7),
+	LTC2688_CHANNEL(8),
+	LTC2688_CHANNEL(9),
+	LTC2688_CHANNEL(10),
+	LTC2688_CHANNEL(11),
+	LTC2688_CHANNEL(12),
+	LTC2688_CHANNEL(13),
+	LTC2688_CHANNEL(14),
+	LTC2688_CHANNEL(15),
+};
+
+static void ltc2688_clk_disable(void *clk)
+{
+	clk_disable_unprepare(clk);
+}
+
+static const int ltc2688_period[LTC2688_DITHER_FREQ_AVAIL_N] = {
+	4, 8, 16, 32, 64,
+};
+
+static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
+				 struct ltc2688_chan *chan,
+				 struct device_node *np, int tgp)
+{
+	unsigned long rate;
+	struct clk *clk;
+	int ret, f;
+
+	clk = devm_get_clk_from_child(&st->spi->dev, np, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(&st->spi->dev, PTR_ERR(clk),
+				     "failed to get tgp clk.\n");
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return dev_err_probe(&st->spi->dev, ret,
+				     "failed to enable tgp clk.\n");
+
+	ret = devm_add_action_or_reset(&st->spi->dev, ltc2688_clk_disable, clk);
+	if (ret)
+		return ret;
+
+	if (chan->toggle_chan)
+		return 0;
+
+	/* calculate available dither frequencies */
+	rate = clk_get_rate(clk);
+	for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++)
+		chan->dither_frequency[f] = DIV_ROUND_CLOSEST(rate, ltc2688_period[f]);
+
+	return 0;
+}
+
+static int ltc2688_span_lookup(const struct ltc2688_state *st, int min, int max)
+{
+	u32 span;
+
+	for (span = 0; span < ARRAY_SIZE(ltc2688_span_helper); span++) {
+		if (min == ltc2688_span_helper[span][0] &&
+		    max == ltc2688_span_helper[span][1])
+			return span;
+	}
+
+	return -EINVAL;
+}
+
+static int ltc2688_channel_config(struct ltc2688_state *st)
+{
+	struct device *dev = &st->spi->dev;
+	struct device_node *child;
+	u32 reg, clk_input, val, tmp[2];
+	int ret, span;
+
+	for_each_available_child_of_node(dev->of_node, child) {
+		struct ltc2688_chan *chan;
+
+		ret = of_property_read_u32(child, "reg", &reg);
+		if (ret) {
+			of_node_put(child);
+			return dev_err_probe(dev, ret,
+					     "Failed to get reg property\n");
+		}
+
+		if (reg >= LTC2688_DAC_CHANNELS) {
+			of_node_put(child);
+			return dev_err_probe(dev, -EINVAL,
+					     "reg bigger than: %d\n",
+					     LTC2688_DAC_CHANNELS);
+		}
+
+		val = 0;
+		chan = &st->channels[reg];
+		if (of_property_read_bool(child, "adi,toggle-mode")) {
+			chan->toggle_chan = true;
+			/* assume sw toggle ABI */
+			st->iio_chan[reg].ext_info = ltc2688_toggle_sym_ext_info;
+			/*
+			 * Clear IIO_CHAN_INFO_RAW bit as toggle channels expose
+			 * out_voltage_raw{0|1} files.
+			 */
+			clear_bit(IIO_CHAN_INFO_RAW,
+				  &st->iio_chan[reg].info_mask_separate);
+		}
+
+		ret = of_property_read_u32_array(child, "adi,output-range-microvolt",
+						 tmp, ARRAY_SIZE(tmp));
+		if (!ret) {
+			span = ltc2688_span_lookup(st, (int)tmp[0] / 1000,
+						   tmp[1] / 1000);
+			if (span < 0) {
+				of_node_put(child);
+				return dev_err_probe(dev, -EINVAL,
+						     "output range not valid:[%d %d]\n",
+						     tmp[0], tmp[1]);
+			}
+
+			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span);
+		}
+
+		ret = of_property_read_u32(child, "adi,toggle-dither-input",
+					   &clk_input);
+		if (!ret) {
+			if (clk_input >= LTC2688_CH_TGP_MAX) {
+				of_node_put(child);
+				return dev_err_probe(dev, -EINVAL,
+						     "toggle-dither-input inv value(%d)\n",
+						     clk_input);
+			}
+
+			ret = ltc2688_tgp_clk_setup(st, chan, child, clk_input);
+			if (ret) {
+				of_node_put(child);
+				return ret;
+			}
+
+			/*
+			 * 0 means software toggle which is the default mode.
+			 * Hence the +1.
+			 */
+			val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1);
+
+			/*
+			 * If a TGPx is given, we automatically assume a dither
+			 * capable channel (unless toggle is already enabled).
+			 * On top of this we just set here the dither bit in the
+			 * channel settings. It won't have any effect until the
+			 * global toggle/dither bit is enabled.
+			 */
+			if (!chan->toggle_chan) {
+				val |= FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
+				st->iio_chan[reg].ext_info = ltc2688_dither_ext_info;
+			} else {
+				/* wait, no sw toggle after all */
+				st->iio_chan[reg].ext_info = ltc2688_toggle_ext_info;
+			}
+		}
+
+		if (of_property_read_bool(child, "adi,overrange")) {
+			chan->overrange = true;
+			val |= LTC2688_CH_OVERRANGE_MSK;
+		}
+
+		if (!val)
+			continue;
+
+		ret = regmap_write(st->regmap, LTC2688_CMD_CH_SETTING(reg),
+				   val);
+		if (ret) {
+			of_node_put(child);
+			return dev_err_probe(dev, -EINVAL,
+					     "failed to set chan settings\n");
+		}
+	}
+
+	return 0;
+}
+
+static int ltc2688_setup(struct ltc2688_state *st, struct regulator *vref)
+{
+	struct gpio_desc *gpio;
+	int ret;
+
+	/*
+	 * If we have a reset pin, use that to reset the board, If not, use
+	 * the reset bit.
+	 */
+	gpio = devm_gpiod_get_optional(&st->spi->dev, "clr", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpio))
+		return dev_err_probe(&st->spi->dev, PTR_ERR(gpio),
+				     "Failed to get reset gpio");
+	if (gpio) {
+		usleep_range(1000, 1200);
+		/* bring device out of reset */
+		gpiod_set_value_cansleep(gpio, 0);
+	} else {
+		ret = regmap_update_bits(st->regmap, LTC2688_CMD_CONFIG,
+					 LTC2688_CONFIG_RST,
+					 LTC2688_CONFIG_RST);
+		if (ret < 0)
+			return ret;
+	}
+
+	usleep_range(10000, 12000);
+
+	/*
+	 * Duplicate the default channel configuration as it can change during
+	 * @ltc2688_channel_config()
+	 */
+	st->iio_chan = devm_kmemdup(&st->spi->dev, ltc2688_channels,
+				    sizeof(ltc2688_channels), GFP_KERNEL);
+	if (!st->iio_chan)
+		return -ENOMEM;
+
+	ret = ltc2688_channel_config(st);
+	if (ret)
+		return ret;
+
+	if (!vref)
+		return 0;
+
+	return regmap_set_bits(st->regmap, LTC2688_CMD_CONFIG,
+			       LTC2688_CONFIG_EXT_REF);
+}
+
+static void ltc2688_bulk_disable(void *data)
+{
+	struct ltc2688_state *st = data;
+
+	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
+}
+
+static void ltc2688_disable_regulator(void *regulator)
+{
+	regulator_disable(regulator);
+}
+
+static bool ltc2688_reg_readable(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LTC2688_CMD_CH_CODE(0) ... LTC2688_CMD_CH_GAIN(15):
+		return true;
+	case LTC2688_CMD_CONFIG ... LTC2688_CMD_THERMAL_STAT:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool ltc2688_reg_writable(struct device *dev, unsigned int reg)
+{
+	/*
+	 * There's a jump from 0x76 to 0x78 in the write codes and the thermal
+	 * status code is 0x77 (which is read only) so that we need to check
+	 * that special condition.
+	 */
+	if (reg <= LTC2688_CMD_UPDATE_ALL && reg != LTC2688_CMD_THERMAL_STAT)
+		return true;
+
+	return false;
+}
+
+struct regmap_bus ltc2688_regmap_bus = {
+	.read = ltc2688_spi_read,
+	.write = ltc2688_spi_write,
+	.read_flag_mask = LTC2688_READ_OPERATION,
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+	.val_format_endian_default = REGMAP_ENDIAN_BIG
+};
+
+static const struct regmap_config ltc2688_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.readable_reg = ltc2688_reg_readable,
+	.writeable_reg = ltc2688_reg_writable,
+	/* ignoring the no op command */
+	.max_register = LTC2688_CMD_UPDATE_ALL
+};
+
+static const struct iio_info ltc2688_info = {
+	.write_raw = ltc2688_write_raw,
+	.read_raw = ltc2688_read_raw,
+	.read_avail = ltc2688_read_avail,
+	.debugfs_reg_access = ltc2688_reg_access,
+};
+
+static int ltc2688_probe(struct spi_device *spi)
+{
+	struct ltc2688_state *st;
+	struct iio_dev *indio_dev;
+	struct regulator *vref_reg;
+	struct device *dev = &spi->dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+
+	/* Just write this once. No need to do it in every regmap read. */
+	st->tx_data[3] = LTC2688_CMD_NOOP;
+	mutex_init(&st->lock);
+
+	st->regmap = devm_regmap_init(dev, &ltc2688_regmap_bus, st,
+				      &ltc2688_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap),
+				     "Failed to init regmap");
+
+	st->regulators[0].supply = "vcc";
+	st->regulators[1].supply = "iovcc";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
+				      st->regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get regulators\n");
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
+
+	ret = devm_add_action_or_reset(dev, ltc2688_bulk_disable, st);
+	if (ret)
+		return ret;
+
+	vref_reg = devm_regulator_get_optional(dev, "vref");
+	if (!IS_ERR(vref_reg)) {
+		ret = regulator_enable(vref_reg);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to enable vref regulators\n");
+
+		ret = devm_add_action_or_reset(dev, ltc2688_disable_regulator,
+					       vref_reg);
+		if (ret)
+			return ret;
+
+		ret = regulator_get_voltage(vref_reg);
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Failed to get vref\n");
+
+		st->vref = ret / 1000;
+	} else {
+		if (PTR_ERR(vref_reg) != -ENODEV)
+			return dev_err_probe(dev, PTR_ERR(vref_reg),
+					     "Failed to get vref regulator");
+
+		vref_reg = NULL;
+		/* internal reference */
+		st->vref = 4096;
+	}
+
+	ret = ltc2688_setup(st, vref_reg);
+	if (ret < 0)
+		return ret;
+
+	indio_dev->name = "ltc2688";
+	indio_dev->info = &ltc2688_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = st->iio_chan;
+	indio_dev->num_channels = ARRAY_SIZE(ltc2688_channels);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id ltc2688_of_id[] = {
+	{ .compatible = "adi,ltc2688" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ltc2688_of_id);
+
+static const struct spi_device_id ltc2688_id[] = {
+	{ "ltc2688" },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ltc2688_id);
+
+static struct spi_driver ltc2688_driver = {
+	.driver = {
+		.name = "ltc2688",
+		.of_match_table = ltc2688_of_id,
+	},
+	.probe = ltc2688_probe,
+	.id_table = ltc2688_id,
+};
+module_spi_driver(ltc2688_driver);
+
+MODULE_AUTHOR("Nuno Sá <nuno.sa@analog.com>");
+MODULE_DESCRIPTION("Analog Devices LTC2688 DAC");
+MODULE_LICENSE("GPL");
-- 
2.33.1

Re: [PATCH v2 1/3] iio: dac: add support for ltc2688

[kernel test robot]

Hi "Nuno,

I love your patch! Yet something to improve:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on robh/for-next linus/master v5.16 next-20220115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nuno-S/Add-support-for-LTC2688/20220115-172930
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20220115/202201152138.RqRoWKHf-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 650fc40b6d8d9a5869b4fca525d5f237b0ee2803)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/d91bcc420e0c6077053e559f676fa4ae76114ba5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nuno-S/Add-support-for-LTC2688/20220115-172930
        git checkout d91bcc420e0c6077053e559f676fa4ae76114ba5
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/iio/dac/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/iio/dac/ltc2688.c:604:2: error: incompatible function pointer types initializing 'ssize_t (*)(struct iio_dev *, uintptr_t, const struct iio_chan_spec *, const char *, size_t)' (aka 'long (*)(struct iio_dev *, unsigned long, const struct iio_chan_spec *, const char *, unsigned long)') with an expression of type 'int (struct iio_dev *, uintptr_t, const struct iio_chan_spec *, const char *, size_t)' (aka 'int (struct iio_dev *, unsigned long, const struct iio_chan_spec *, const char *, unsigned long)') [-Werror,-Wincompatible-function-pointer-types]
           LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_CMD_TOGGLE_DITHER_EN,
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/dac/ltc2688.c:590:11: note: expanded from macro 'LTC2688_CHAN_EXT_INFO'
           .write = (_write),                                              \
                    ^~~~~~~~
   drivers/iio/dac/ltc2688.c:619:2: error: incompatible function pointer types initializing 'ssize_t (*)(struct iio_dev *, uintptr_t, const struct iio_chan_spec *, const char *, size_t)' (aka 'long (*)(struct iio_dev *, unsigned long, const struct iio_chan_spec *, const char *, unsigned long)') with an expression of type 'int (struct iio_dev *, uintptr_t, const struct iio_chan_spec *, const char *, size_t)' (aka 'int (struct iio_dev *, unsigned long, const struct iio_chan_spec *, const char *, unsigned long)') [-Werror,-Wincompatible-function-pointer-types]
           LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_CMD_TOGGLE_DITHER_EN,
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/dac/ltc2688.c:590:11: note: expanded from macro 'LTC2688_CHAN_EXT_INFO'
           .write = (_write),                                              \
                    ^~~~~~~~
   drivers/iio/dac/ltc2688.c:639:2: error: incompatible function pointer types initializing 'ssize_t (*)(struct iio_dev *, uintptr_t, const struct iio_chan_spec *, const char *, size_t)' (aka 'long (*)(struct iio_dev *, unsigned long, const struct iio_chan_spec *, const char *, unsigned long)') with an expression of type 'int (struct iio_dev *, uintptr_t, const struct iio_chan_spec *, const char *, size_t)' (aka 'int (struct iio_dev *, unsigned long, const struct iio_chan_spec *, const char *, unsigned long)') [-Werror,-Wincompatible-function-pointer-types]
           LTC2688_CHAN_EXT_INFO("dither_frequency", 0, IIO_SEPARATE,
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/dac/ltc2688.c:590:11: note: expanded from macro 'LTC2688_CHAN_EXT_INFO'
           .write = (_write),                                              \
                    ^~~~~~~~
   drivers/iio/dac/ltc2688.c:641:2: error: incompatible function pointer types initializing 'ssize_t (*)(struct iio_dev *, uintptr_t, const struct iio_chan_spec *, const char *, size_t)' (aka 'long (*)(struct iio_dev *, unsigned long, const struct iio_chan_spec *, const char *, unsigned long)') with an expression of type 'int (struct iio_dev *, uintptr_t, const struct iio_chan_spec *, const char *, size_t)' (aka 'int (struct iio_dev *, unsigned long, const struct iio_chan_spec *, const char *, unsigned long)') [-Werror,-Wincompatible-function-pointer-types]
           LTC2688_CHAN_EXT_INFO("dither_frequency_available",
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/dac/ltc2688.c:590:11: note: expanded from macro 'LTC2688_CHAN_EXT_INFO'
           .write = (_write),                                              \
                    ^~~~~~~~
   drivers/iio/dac/ltc2688.c:647:2: error: incompatible function pointer types initializing 'ssize_t (*)(struct iio_dev *, uintptr_t, const struct iio_chan_spec *, const char *, size_t)' (aka 'long (*)(struct iio_dev *, unsigned long, const struct iio_chan_spec *, const char *, unsigned long)') with an expression of type 'int (struct iio_dev *, uintptr_t, const struct iio_chan_spec *, const char *, size_t)' (aka 'int (struct iio_dev *, unsigned long, const struct iio_chan_spec *, const char *, unsigned long)') [-Werror,-Wincompatible-function-pointer-types]
           LTC2688_CHAN_EXT_INFO("dither_en", LTC2688_CMD_TOGGLE_DITHER_EN,
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/dac/ltc2688.c:590:11: note: expanded from macro 'LTC2688_CHAN_EXT_INFO'
           .write = (_write),                                              \
                    ^~~~~~~~
   5 errors generated.


vim +604 drivers/iio/dac/ltc2688.c

   594	
   595	/*
   596	 * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is
   597	 * not provided in dts.
   598	 */
   599	static const struct iio_chan_spec_ext_info ltc2688_toggle_sym_ext_info[] = {
   600		LTC2688_CHAN_EXT_INFO("raw0", LTC2688_INPUT_A, IIO_SEPARATE,
   601				      ltc2688_dac_input_read, ltc2688_dac_input_write),
   602		LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B, IIO_SEPARATE,
   603				      ltc2688_dac_input_read, ltc2688_dac_input_write),
 > 604		LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_CMD_TOGGLE_DITHER_EN,
   605				      IIO_SEPARATE, ltc2688_reg_bool_get,
   606				      ltc2688_dither_toggle_set),
   607		LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_CMD_POWERDOWN, IIO_SEPARATE,
   608				      ltc2688_reg_bool_get, ltc2688_reg_bool_set),
   609		LTC2688_CHAN_EXT_INFO("symbol", LTC2688_CMD_SW_TOGGLE, IIO_SEPARATE,
   610				      ltc2688_reg_bool_get, ltc2688_reg_bool_set),
   611		{}
   612	};
   613	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v2 1/3] iio: dac: add support for ltc2688

[kernel test robot]

Hi "Nuno,

I love your patch! Yet something to improve:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on robh/for-next linus/master v5.16 next-20220115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nuno-S/Add-support-for-LTC2688/20220115-172930
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20220116/202201160237.Mxs5GYw3-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d91bcc420e0c6077053e559f676fa4ae76114ba5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nuno-S/Add-support-for-LTC2688/20220115-172930
        git checkout d91bcc420e0c6077053e559f676fa4ae76114ba5
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/iio/dac/ltc2688.c:590:18: error: initialization of 'ssize_t (*)(struct iio_dev *, uintptr_t,  const struct iio_chan_spec *, const char *, size_t)' {aka 'long int (*)(struct iio_dev *, long unsigned int,  const struct iio_chan_spec *, const char *, long unsigned int)'} from incompatible pointer type 'int (*)(struct iio_dev *, uintptr_t,  const struct iio_chan_spec *, const char *, size_t)' {aka 'int (*)(struct iio_dev *, long unsigned int,  const struct iio_chan_spec *, const char *, long unsigned int)'} [-Werror=incompatible-pointer-types]
     590 |         .write = (_write),                                              \
         |                  ^
   drivers/iio/dac/ltc2688.c:604:9: note: in expansion of macro 'LTC2688_CHAN_EXT_INFO'
     604 |         LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_CMD_TOGGLE_DITHER_EN,
         |         ^~~~~~~~~~~~~~~~~~~~~
   drivers/iio/dac/ltc2688.c:590:18: note: (near initialization for 'ltc2688_toggle_sym_ext_info[2].write')
     590 |         .write = (_write),                                              \
         |                  ^
   drivers/iio/dac/ltc2688.c:604:9: note: in expansion of macro 'LTC2688_CHAN_EXT_INFO'
     604 |         LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_CMD_TOGGLE_DITHER_EN,
         |         ^~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/dac/ltc2688.c:590:18: error: initialization of 'ssize_t (*)(struct iio_dev *, uintptr_t,  const struct iio_chan_spec *, const char *, size_t)' {aka 'long int (*)(struct iio_dev *, long unsigned int,  const struct iio_chan_spec *, const char *, long unsigned int)'} from incompatible pointer type 'int (*)(struct iio_dev *, uintptr_t,  const struct iio_chan_spec *, const char *, size_t)' {aka 'int (*)(struct iio_dev *, long unsigned int,  const struct iio_chan_spec *, const char *, long unsigned int)'} [-Werror=incompatible-pointer-types]
     590 |         .write = (_write),                                              \
         |                  ^
   drivers/iio/dac/ltc2688.c:619:9: note: in expansion of macro 'LTC2688_CHAN_EXT_INFO'
     619 |         LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_CMD_TOGGLE_DITHER_EN,
         |         ^~~~~~~~~~~~~~~~~~~~~
   drivers/iio/dac/ltc2688.c:590:18: note: (near initialization for 'ltc2688_toggle_ext_info[2].write')
     590 |         .write = (_write),                                              \
         |                  ^
   drivers/iio/dac/ltc2688.c:619:9: note: in expansion of macro 'LTC2688_CHAN_EXT_INFO'
     619 |         LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_CMD_TOGGLE_DITHER_EN,
         |         ^~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/dac/ltc2688.c:590:18: error: initialization of 'ssize_t (*)(struct iio_dev *, uintptr_t,  const struct iio_chan_spec *, const char *, size_t)' {aka 'long int (*)(struct iio_dev *, long unsigned int,  const struct iio_chan_spec *, const char *, long unsigned int)'} from incompatible pointer type 'int (*)(struct iio_dev *, uintptr_t,  const struct iio_chan_spec *, const char *, size_t)' {aka 'int (*)(struct iio_dev *, long unsigned int,  const struct iio_chan_spec *, const char *, long unsigned int)'} [-Werror=incompatible-pointer-types]
     590 |         .write = (_write),                                              \
         |                  ^
   drivers/iio/dac/ltc2688.c:639:9: note: in expansion of macro 'LTC2688_CHAN_EXT_INFO'
     639 |         LTC2688_CHAN_EXT_INFO("dither_frequency", 0, IIO_SEPARATE,
         |         ^~~~~~~~~~~~~~~~~~~~~
   drivers/iio/dac/ltc2688.c:590:18: note: (near initialization for 'ltc2688_dither_ext_info[3].write')
     590 |         .write = (_write),                                              \
         |                  ^
   drivers/iio/dac/ltc2688.c:639:9: note: in expansion of macro 'LTC2688_CHAN_EXT_INFO'
     639 |         LTC2688_CHAN_EXT_INFO("dither_frequency", 0, IIO_SEPARATE,
         |         ^~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/dac/ltc2688.c:590:18: error: initialization of 'ssize_t (*)(struct iio_dev *, uintptr_t,  const struct iio_chan_spec *, const char *, size_t)' {aka 'long int (*)(struct iio_dev *, long unsigned int,  const struct iio_chan_spec *, const char *, long unsigned int)'} from incompatible pointer type 'int (*)(struct iio_dev *, uintptr_t,  const struct iio_chan_spec *, const char *, size_t)' {aka 'int (*)(struct iio_dev *, long unsigned int,  const struct iio_chan_spec *, const char *, long unsigned int)'} [-Werror=incompatible-pointer-types]
     590 |         .write = (_write),                                              \
         |                  ^
   drivers/iio/dac/ltc2688.c:641:9: note: in expansion of macro 'LTC2688_CHAN_EXT_INFO'
     641 |         LTC2688_CHAN_EXT_INFO("dither_frequency_available",
         |         ^~~~~~~~~~~~~~~~~~~~~
   drivers/iio/dac/ltc2688.c:590:18: note: (near initialization for 'ltc2688_dither_ext_info[4].write')
     590 |         .write = (_write),                                              \
         |                  ^
   drivers/iio/dac/ltc2688.c:641:9: note: in expansion of macro 'LTC2688_CHAN_EXT_INFO'
     641 |         LTC2688_CHAN_EXT_INFO("dither_frequency_available",
         |         ^~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/dac/ltc2688.c:590:18: error: initialization of 'ssize_t (*)(struct iio_dev *, uintptr_t,  const struct iio_chan_spec *, const char *, size_t)' {aka 'long int (*)(struct iio_dev *, long unsigned int,  const struct iio_chan_spec *, const char *, long unsigned int)'} from incompatible pointer type 'int (*)(struct iio_dev *, uintptr_t,  const struct iio_chan_spec *, const char *, size_t)' {aka 'int (*)(struct iio_dev *, long unsigned int,  const struct iio_chan_spec *, const char *, long unsigned int)'} [-Werror=incompatible-pointer-types]
     590 |         .write = (_write),                                              \
         |                  ^
   drivers/iio/dac/ltc2688.c:647:9: note: in expansion of macro 'LTC2688_CHAN_EXT_INFO'
     647 |         LTC2688_CHAN_EXT_INFO("dither_en", LTC2688_CMD_TOGGLE_DITHER_EN,
         |         ^~~~~~~~~~~~~~~~~~~~~
   drivers/iio/dac/ltc2688.c:590:18: note: (near initialization for 'ltc2688_dither_ext_info[7].write')
     590 |         .write = (_write),                                              \
         |                  ^
   drivers/iio/dac/ltc2688.c:647:9: note: in expansion of macro 'LTC2688_CHAN_EXT_INFO'
     647 |         LTC2688_CHAN_EXT_INFO("dither_en", LTC2688_CMD_TOGGLE_DITHER_EN,
         |         ^~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +590 drivers/iio/dac/ltc2688.c

   586	
   587	#define LTC2688_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {	\
   588		.name = _name,							\
   589		.read = (_read),						\
 > 590		.write = (_write),						\
   591		.private = (_what),						\
   592		.shared = (_shared),						\
   593	}
   594	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v2 1/3] iio: dac: add support for ltc2688

[Jonathan Cameron]

On Sat, 15 Jan 2022 10:27:03 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> precision reference. It is guaranteed monotonic and has built in
> rail-to-rail output buffers that can source or sink up to 20 mA.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

A few minor additions inline.

In particular I think we can work around that lack of
device_for_each_available_child_node() issue and use generic fw propreties.
rather than of ones.  That way we can separate things from the question of
how to 'fix' that issue.

One thing I'm not sure on is the phase units. Right now I think you are
exposing just the raw register value whereas I think that needs converting
to radians.

Jonathan



...

> +static int ltc2688_channel_config(struct ltc2688_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct device_node *child;
> +	u32 reg, clk_input, val, tmp[2];
> +	int ret, span;
> +
> +	for_each_available_child_of_node(dev->of_node, child) {

Gah. This still going on with there not being a generic _available_
specific form.  We need to kick that again because I'm not keen to
merge another driver we'll need to tidy up later to use generic
properties.

Best bet is probably to just define
device_for_each_available_child_node() and see if anyone shoots
it down (even if it does the same as device_for_each_child_node()
in at least some cases).

Or thinking about it.. Here you could use device_for_each_child_node()
and then call fwnode_device_is_available() on the result and continue
if not true.

Will always return true (I think) but will make the intent clear.

We can tidy up to a new for_* if / when it becomes available.



> +		struct ltc2688_chan *chan;
> +
> +		ret = of_property_read_u32(child, "reg", &reg);
> +		if (ret) {
> +			of_node_put(child);
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get reg property\n");
> +		}
> +
> +		if (reg >= LTC2688_DAC_CHANNELS) {
> +			of_node_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "reg bigger than: %d\n",
> +					     LTC2688_DAC_CHANNELS);
> +		}
> +
> +		val = 0;
> +		chan = &st->channels[reg];
> +		if (of_property_read_bool(child, "adi,toggle-mode")) {
> +			chan->toggle_chan = true;
> +			/* assume sw toggle ABI */
> +			st->iio_chan[reg].ext_info = ltc2688_toggle_sym_ext_info;
> +			/*
> +			 * Clear IIO_CHAN_INFO_RAW bit as toggle channels expose
> +			 * out_voltage_raw{0|1} files.
> +			 */
> +			clear_bit(IIO_CHAN_INFO_RAW,
> +				  &st->iio_chan[reg].info_mask_separate);
> +		}
> +
> +		ret = of_property_read_u32_array(child, "adi,output-range-microvolt",
> +						 tmp, ARRAY_SIZE(tmp));
> +		if (!ret) {
> +			span = ltc2688_span_lookup(st, (int)tmp[0] / 1000,
> +						   tmp[1] / 1000);
> +			if (span < 0) {
> +				of_node_put(child);
> +				return dev_err_probe(dev, -EINVAL,
> +						     "output range not valid:[%d %d]\n",
> +						     tmp[0], tmp[1]);
> +			}
> +
> +			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span);
> +		}
> +
> +		ret = of_property_read_u32(child, "adi,toggle-dither-input",
> +					   &clk_input);
> +		if (!ret) {
> +			if (clk_input >= LTC2688_CH_TGP_MAX) {
> +				of_node_put(child);
> +				return dev_err_probe(dev, -EINVAL,
> +						     "toggle-dither-input inv value(%d)\n",
> +						     clk_input);
> +			}
> +
> +			ret = ltc2688_tgp_clk_setup(st, chan, child, clk_input);
> +			if (ret) {
> +				of_node_put(child);
> +				return ret;
> +			}
> +
> +			/*
> +			 * 0 means software toggle which is the default mode.
> +			 * Hence the +1.
> +			 */
> +			val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1);
> +
> +			/*
> +			 * If a TGPx is given, we automatically assume a dither
> +			 * capable channel (unless toggle is already enabled).
> +			 * On top of this we just set here the dither bit in the
> +			 * channel settings. It won't have any effect until the
> +			 * global toggle/dither bit is enabled.
> +			 */
> +			if (!chan->toggle_chan) {
> +				val |= FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
> +				st->iio_chan[reg].ext_info = ltc2688_dither_ext_info;
> +			} else {
> +				/* wait, no sw toggle after all */
> +				st->iio_chan[reg].ext_info = ltc2688_toggle_ext_info;
> +			}
> +		}
> +
> +		if (of_property_read_bool(child, "adi,overrange")) {
> +			chan->overrange = true;
> +			val |= LTC2688_CH_OVERRANGE_MSK;
> +		}
> +
> +		if (!val)
> +			continue;
> +
> +		ret = regmap_write(st->regmap, LTC2688_CMD_CH_SETTING(reg),
> +				   val);
> +		if (ret) {
> +			of_node_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "failed to set chan settings\n");
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int ltc2688_setup(struct ltc2688_state *st, struct regulator *vref)
> +{
> +	struct gpio_desc *gpio;
> +	int ret;
> +
> +	/*
> +	 * If we have a reset pin, use that to reset the board, If not, use
> +	 * the reset bit.
> +	 */
> +	gpio = devm_gpiod_get_optional(&st->spi->dev, "clr", GPIOD_OUT_HIGH);
> +	if (IS_ERR(gpio))
> +		return dev_err_probe(&st->spi->dev, PTR_ERR(gpio),
> +				     "Failed to get reset gpio");
> +	if (gpio) {
> +		usleep_range(1000, 1200);
> +		/* bring device out of reset */
> +		gpiod_set_value_cansleep(gpio, 0);
> +	} else {
> +		ret = regmap_update_bits(st->regmap, LTC2688_CMD_CONFIG,
> +					 LTC2688_CONFIG_RST,
> +					 LTC2688_CONFIG_RST);
> +		if (ret < 0)
A bit of a mixture in here on whether you prefer if (ret) or if (ret < 0)
for error returns you know can't be postive.

I'd go with if (ret) for all of them.  It makes things consistent with the
cases where you directly return the value without checking it for less
than 0 like below.


> +			return ret;
> +	}
> +
> +	usleep_range(10000, 12000);
> +
> +	/*
> +	 * Duplicate the default channel configuration as it can change during
> +	 * @ltc2688_channel_config()
> +	 */
> +	st->iio_chan = devm_kmemdup(&st->spi->dev, ltc2688_channels,
> +				    sizeof(ltc2688_channels), GFP_KERNEL);
> +	if (!st->iio_chan)
> +		return -ENOMEM;
> +
> +	ret = ltc2688_channel_config(st);
> +	if (ret)
> +		return ret;
> +
> +	if (!vref)
> +		return 0;
> +
> +	return regmap_set_bits(st->regmap, LTC2688_CMD_CONFIG,
> +			       LTC2688_CONFIG_EXT_REF);
> +}
> +
> +static void ltc2688_bulk_disable(void *data)

rename to mention regulators.  Could be bulk disabling something else...

> +{
> +	struct ltc2688_state *st = data;
> +
> +	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
> +}
> +

...

Thanks,

Jonathan

RE: [PATCH v2 1/3] iio: dac: add support for ltc2688

[Sa, Nuno]

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, January 16, 2022 1:44 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH v2 1/3] iio: dac: add support for ltc2688
> 
> [External]
> 
> On Sat, 15 Jan 2022 10:27:03 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> > precision reference. It is guaranteed monotonic and has built in
> > rail-to-rail output buffers that can source or sink up to 20 mA.
> >
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> A few minor additions inline.
> 
> In particular I think we can work around that lack of
> device_for_each_available_child_node() issue and use generic fw
> propreties.
> rather than of ones.  That way we can separate things from the
> question of
> how to 'fix' that issue.
> 
> One thing I'm not sure on is the phase units. Right now I think you are
> exposing just the raw register value whereas I think that needs
> converting
> to radians.

It's returning degrees which I think is fairly ok. But I know that in general
we report radians, so I'm more than fine in changing this if you prefer it.

> Jonathan
> 
> 
> 
> ...
> 
> > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	struct device_node *child;
> > +	u32 reg, clk_input, val, tmp[2];
> > +	int ret, span;
> > +
> > +	for_each_available_child_of_node(dev->of_node, child) {
> 
> Gah. This still going on with there not being a generic _available_
> specific form.  We need to kick that again because I'm not keen to
> merge another driver we'll need to tidy up later to use generic
> properties.
> 
> Best bet is probably to just define
> device_for_each_available_child_node() and see if anyone shoots
> it down (even if it does the same as device_for_each_child_node()
> in at least some cases).
> 
> Or thinking about it.. Here you could use
> device_for_each_child_node()
> and then call fwnode_device_is_available() on the result and continue
> if not true.
> 
> Will always return true (I think) but will make the intent clear.
> 
> We can tidy up to a new for_* if / when it becomes available.
> 

Hmm, not sure I'm following you... I mean, I understand what you're
saying here but there is a reason for why I changed the whole thing to
use OF. Please take a look at the cover... I explain why I've done it.

> 
> > +		struct ltc2688_chan *chan;
> > +
> > +		ret = of_property_read_u32(child, "reg", &reg);
> > +		if (ret) {
> > +			of_node_put(child);
> > +			return dev_err_probe(dev, ret,
> > +					     "Failed to get reg
> property\n");
> > +		}
> > +
> > +		if (reg >= LTC2688_DAC_CHANNELS) {
> > +			of_node_put(child);
> > +			return dev_err_probe(dev, -EINVAL,
> > +					     "reg bigger than: %d\n",
> > +					     LTC2688_DAC_CHANNELS);
> > +		}
> > +
> > +		val = 0;
> > +		chan = &st->channels[reg];
> > +		if (of_property_read_bool(child, "adi,toggle-mode")) {
> > +			chan->toggle_chan = true;
> > +			/* assume sw toggle ABI */
> > +			st->iio_chan[reg].ext_info =
> ltc2688_toggle_sym_ext_info;
> > +			/*
> > +			 * Clear IIO_CHAN_INFO_RAW bit as toggle
> channels expose
> > +			 * out_voltage_raw{0|1} files.
> > +			 */
> > +			clear_bit(IIO_CHAN_INFO_RAW,
> > +				  &st-
> >iio_chan[reg].info_mask_separate);
> > +		}
> > +
> > +		ret = of_property_read_u32_array(child, "adi,output-
> range-microvolt",
> > +						 tmp,
> ARRAY_SIZE(tmp));
> > +		if (!ret) {
> > +			span = ltc2688_span_lookup(st, (int)tmp[0] /
> 1000,
> > +						   tmp[1] / 1000);
> > +			if (span < 0) {
> > +				of_node_put(child);
> > +				return dev_err_probe(dev, -EINVAL,
> > +						     "output range not
> valid:[%d %d]\n",
> > +						     tmp[0], tmp[1]);
> > +			}
> > +
> > +			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK,
> span);
> > +		}
> > +
> > +		ret = of_property_read_u32(child, "adi,toggle-dither-
> input",
> > +					   &clk_input);
> > +		if (!ret) {
> > +			if (clk_input >= LTC2688_CH_TGP_MAX) {
> > +				of_node_put(child);
> > +				return dev_err_probe(dev, -EINVAL,
> > +						     "toggle-dither-input
> inv value(%d)\n",
> > +						     clk_input);
> > +			}
> > +
> > +			ret = ltc2688_tgp_clk_setup(st, chan, child,
> clk_input);
> > +			if (ret) {
> > +				of_node_put(child);
> > +				return ret;
> > +			}
> > +
> > +			/*
> > +			 * 0 means software toggle which is the default
> mode.
> > +			 * Hence the +1.
> > +			 */
> > +			val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK,
> clk_input + 1);
> > +
> > +			/*
> > +			 * If a TGPx is given, we automatically assume a
> dither
> > +			 * capable channel (unless toggle is already
> enabled).
> > +			 * On top of this we just set here the dither bit
> in the
> > +			 * channel settings. It won't have any effect
> until the
> > +			 * global toggle/dither bit is enabled.
> > +			 */
> > +			if (!chan->toggle_chan) {
> > +				val |=
> FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
> > +				st->iio_chan[reg].ext_info =
> ltc2688_dither_ext_info;
> > +			} else {
> > +				/* wait, no sw toggle after all */
> > +				st->iio_chan[reg].ext_info =
> ltc2688_toggle_ext_info;
> > +			}
> > +		}
> > +
> > +		if (of_property_read_bool(child, "adi,overrange")) {
> > +			chan->overrange = true;
> > +			val |= LTC2688_CH_OVERRANGE_MSK;
> > +		}
> > +
> > +		if (!val)
> > +			continue;
> > +
> > +		ret = regmap_write(st->regmap,
> LTC2688_CMD_CH_SETTING(reg),
> > +				   val);
> > +		if (ret) {
> > +			of_node_put(child);
> > +			return dev_err_probe(dev, -EINVAL,
> > +					     "failed to set chan
> settings\n");
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ltc2688_setup(struct ltc2688_state *st, struct regulator
> *vref)
> > +{
> > +	struct gpio_desc *gpio;
> > +	int ret;
> > +
> > +	/*
> > +	 * If we have a reset pin, use that to reset the board, If not, use
> > +	 * the reset bit.
> > +	 */
> > +	gpio = devm_gpiod_get_optional(&st->spi->dev, "clr",
> GPIOD_OUT_HIGH);
> > +	if (IS_ERR(gpio))
> > +		return dev_err_probe(&st->spi->dev, PTR_ERR(gpio),
> > +				     "Failed to get reset gpio");
> > +	if (gpio) {
> > +		usleep_range(1000, 1200);
> > +		/* bring device out of reset */
> > +		gpiod_set_value_cansleep(gpio, 0);
> > +	} else {
> > +		ret = regmap_update_bits(st->regmap,
> LTC2688_CMD_CONFIG,
> > +					 LTC2688_CONFIG_RST,
> > +					 LTC2688_CONFIG_RST);
> > +		if (ret < 0)
> A bit of a mixture in here on whether you prefer if (ret) or if (ret < 0)
> for error returns you know can't be postive.
> 
> I'd go with if (ret) for all of them.  It makes things consistent with the
> cases where you directly return the value without checking it for less
> than 0 like below.

Yeah, sometimes my hands get confused :). Or probably, just copy pasting.
But I agree with you, when ret > 0 has no meaning I always want to do if (ret)
but you know...

- Nuno Sá

Re: [PATCH v2 1/3] iio: dac: add support for ltc2688

[Jonathan Cameron]

On Sun, 16 Jan 2022 16:28:08 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, January 16, 2022 1:44 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; Rob
> > Herring <robh+dt@kernel.org>; Lars-Peter Clausen
> > <lars@metafoo.de>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>
> > Subject: Re: [PATCH v2 1/3] iio: dac: add support for ltc2688
> > 
> > [External]
> > 
> > On Sat, 15 Jan 2022 10:27:03 +0100
> > Nuno Sá <nuno.sa@analog.com> wrote:
> >   
> > > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> > > precision reference. It is guaranteed monotonic and has built in
> > > rail-to-rail output buffers that can source or sink up to 20 mA.
> > >
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>  
> > 
> > A few minor additions inline.
> > 
> > In particular I think we can work around that lack of
> > device_for_each_available_child_node() issue and use generic fw
> > propreties.
> > rather than of ones.  That way we can separate things from the
> > question of
> > how to 'fix' that issue.
> > 
> > One thing I'm not sure on is the phase units. Right now I think you are
> > exposing just the raw register value whereas I think that needs
> > converting
> > to radians.  
> 
> It's returning degrees which I think is fairly ok. But I know that in general
> we report radians, so I'm more than fine in changing this if you prefer it.

Radians for consistency is a must as users reading the docs may see the main
_phase descriptions and have no reason to think this one might be different.
 

> 
> > Jonathan
> > 
> > 
> > 
> > ...
> >   
> > > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > > +{
> > > +	struct device *dev = &st->spi->dev;
> > > +	struct device_node *child;
> > > +	u32 reg, clk_input, val, tmp[2];
> > > +	int ret, span;
> > > +
> > > +	for_each_available_child_of_node(dev->of_node, child) {  
> > 
> > Gah. This still going on with there not being a generic _available_
> > specific form.  We need to kick that again because I'm not keen to
> > merge another driver we'll need to tidy up later to use generic
> > properties.
> > 
> > Best bet is probably to just define
> > device_for_each_available_child_node() and see if anyone shoots
> > it down (even if it does the same as device_for_each_child_node()
> > in at least some cases).
> > 
> > Or thinking about it.. Here you could use
> > device_for_each_child_node()
> > and then call fwnode_device_is_available() on the result and continue
> > if not true.
> > 
> > Will always return true (I think) but will make the intent clear.
> > 
> > We can tidy up to a new for_* if / when it becomes available.
> >   
> 
> Hmm, not sure I'm following you... I mean, I understand what you're
> saying here but there is a reason for why I changed the whole thing to
> use OF. Please take a look at the cover... I explain why I've done it.

Hohum. Reading the cover letter? :)  Next you'll be suggesting
I read manuals of new hardware!  I'll take a look.

Jonathan

[PATCH v2 2/3] iio: ABI: add ABI file for the LTC2688 DAC

[Nuno Sá]

Define the sysfs interface for toggle or dither capable channels. Dither
capable channels will have the extended interface:

 * out_voltageY_dither_en
 * out_voltageY_dither_raw
 * out_voltageY_dither_offset
 * out_voltageY_dither_raw_available
 * out_voltageY_dither_frequency
 * out_voltageY_dither_frequency_available
 * out_voltageY_dither_phase
 * out_voltageY_dither_phase_available

Toggle enabled channels will have:

 * out_voltageY_toggle_en
 * out_voltageY_raw0
 * out_voltageY_raw1
 * out_voltageY_symbol

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 .../ABI/testing/sysfs-bus-iio-dac-ltc2688     | 80 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 81 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688 b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
new file mode 100644
index 000000000000..38d1df81c6dc
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
@@ -0,0 +1,80 @@
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_en
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Dither enable. Write 1 to enable dither or 0 to disable it.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_raw
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This raw, unscaled value refers to the dither signal amplitude.
+		The same scale as in out_voltageY_raw applies. However, the
+		offset might be different as it's always 0 for this attribute.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_raw_available
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Available range for dither raw amplitude values.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_offset
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Offset applied to out_voltageY_dither_raw. Read only attribute
+		always set to 0.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_frequency
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Sets the dither signal frequency.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_frequency_available
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Returns the available values for the dither frequency.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_phase
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Sets the dither signal phase.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_phase_available
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Returns the available values for the dither phase.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_toggle_en
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Toggle enable. Write 1 to enable toggle or 0 to disable it.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_raw0
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		It has the same meaning as out_voltageY_raw. This attribute is
+		specific to toggle enabled channels and refers to the DAC output
+		code in INPUT_A. The same scale, offset, etc applies.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_raw1
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Same as out_voltageY_raw0 but referring to the DAC output code
+		in INPUT_B.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_symbol
+KernelVersion:	5.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Performs a SW toggle. This attribute is specific to toggle
+		enabled channels and allows to toggle between out_voltageY_raw
+		and out_voltageY_raw1 through software. Writing 0 will select
+		out_voltageY_raw while 1 selects out_voltageY_raw1.
diff --git a/MAINTAINERS b/MAINTAINERS
index 16e344d52b1e..16a7fd7f98ee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11187,6 +11187,7 @@ M:	Nuno Sá <nuno.sa@analog.com>
 L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	http://ez.analog.com/community/linux-device-drivers
+F:	Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
 F:	drivers/iio/dac/ltc2688.c
 
 LTC2947 HARDWARE MONITOR DRIVER
-- 
2.33.1

Re: [PATCH v2 2/3] iio: ABI: add ABI file for the LTC2688 DAC

[Jonathan Cameron]

On Sat, 15 Jan 2022 10:27:04 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> Define the sysfs interface for toggle or dither capable channels. Dither
> capable channels will have the extended interface:
> 
>  * out_voltageY_dither_en
>  * out_voltageY_dither_raw
>  * out_voltageY_dither_offset
>  * out_voltageY_dither_raw_available
>  * out_voltageY_dither_frequency
>  * out_voltageY_dither_frequency_available
>  * out_voltageY_dither_phase
>  * out_voltageY_dither_phase_available
> 
> Toggle enabled channels will have:
> 
>  * out_voltageY_toggle_en
>  * out_voltageY_raw0
>  * out_voltageY_raw1
>  * out_voltageY_symbol

Maybe worth just stating the normal interface here as well because
it's not clear from these examples if
out_voltageY_raw still exists for toggle enabled channels (I'm assuming not?)

> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
ABI seems good to me, just a few comments on details of the documentation.

Thanks,

Jonathan
> ---
>  .../ABI/testing/sysfs-bus-iio-dac-ltc2688     | 80 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 81 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688 b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
> new file mode 100644
> index 000000000000..38d1df81c6dc
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
> @@ -0,0 +1,80 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_en
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Dither enable. Write 1 to enable dither or 0 to disable it.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_raw
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This raw, unscaled value refers to the dither signal amplitude.
> +		The same scale as in out_voltageY_raw applies. However, the
> +		offset might be different as it's always 0 for this attribute.

We'll have to be careful if we ever generalize these docs but what you have here
is fine whilst it applies to just this device.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_raw_available
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Available range for dither raw amplitude values.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_offset
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Offset applied to out_voltageY_dither_raw. Read only attribute
> +		always set to 0.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_frequency
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Sets the dither signal frequency.
Units.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_frequency_available
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Returns the available values for the dither frequency.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_phase
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Sets the dither signal phase.

Units.  Radians, or refer to the main _phase docs and say it's the same.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_phase_available
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Returns the available values for the dither phase.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_toggle_en
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Toggle enable. Write 1 to enable toggle or 0 to disable it.

Say why this is useful (presumably toggle with a clock rather than via _symbol)

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_raw0
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		It has the same meaning as out_voltageY_raw. This attribute is
> +		specific to toggle enabled channels and refers to the DAC output
> +		code in INPUT_A. The same scale, offset, etc applies.

Same as what?

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_raw1
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Same as out_voltageY_raw0 but referring to the DAC output code
> +		in INPUT_B.

You could combine this with previous and have two what lines.  Might allow
a slightly more compact clear description.


> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_symbol

Ah. That answers one of my binding related questions :)  You have kept
software control as an option for toggle.

> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Performs a SW toggle. This attribute is specific to toggle
> +		enabled channels and allows to toggle between out_voltageY_raw

_raw0

> +		and out_voltageY_raw1 through software. Writing 0 will select
> +		out_voltageY_raw while 1 selects out_voltageY_raw1.
_raw0

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 16e344d52b1e..16a7fd7f98ee 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11187,6 +11187,7 @@ M:	Nuno Sá <nuno.sa@analog.com>
>  L:	linux-iio@vger.kernel.org
>  S:	Supported
>  W:	http://ez.analog.com/community/linux-device-drivers
> +F:	Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
>  F:	drivers/iio/dac/ltc2688.c
>  
>  LTC2947 HARDWARE MONITOR DRIVER

RE: [PATCH v2 2/3] iio: ABI: add ABI file for the LTC2688 DAC

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, January 16, 2022 1:21 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH v2 2/3] iio: ABI: add ABI file for the LTC2688 DAC
> 
> [External]
> 
> On Sat, 15 Jan 2022 10:27:04 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > Define the sysfs interface for toggle or dither capable channels.
> Dither
> > capable channels will have the extended interface:
> >
> >  * out_voltageY_dither_en
> >  * out_voltageY_dither_raw
> >  * out_voltageY_dither_offset
> >  * out_voltageY_dither_raw_available
> >  * out_voltageY_dither_frequency
> >  * out_voltageY_dither_frequency_available
> >  * out_voltageY_dither_phase
> >  * out_voltageY_dither_phase_available
> >
> > Toggle enabled channels will have:
> >
> >  * out_voltageY_toggle_en
> >  * out_voltageY_raw0
> >  * out_voltageY_raw1
> >  * out_voltageY_symbol
> 
> Maybe worth just stating the normal interface here as well because
> it's not clear from these examples if
> out_voltageY_raw still exists for toggle enabled channels (I'm assuming
> not?)

Yes, it does not exist as It would not make sense :)...

> >
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ABI seems good to me, just a few comments on details of the
> documentation.
> Thanks,
> 
> Jonathan
> > ---
> >  .../ABI/testing/sysfs-bus-iio-dac-ltc2688     | 80
> +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 81 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-
> ltc2688
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
> b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
> > new file mode 100644
> > index 000000000000..38d1df81c6dc
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
> > @@ -0,0 +1,80 @@
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_en
> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Dither enable. Write 1 to enable dither or 0 to disable it.
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_raw
> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		This raw, unscaled value refers to the dither signal
> amplitude.
> > +		The same scale as in out_voltageY_raw applies.
> However, the
> > +		offset might be different as it's always 0 for this
> attribute.
> 
> We'll have to be careful if we ever generalize these docs but what you
> have here
> is fine whilst it applies to just this device.
> 
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_raw_av
> ailable
> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Available range for dither raw amplitude values.
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_offset
> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Offset applied to out_voltageY_dither_raw. Read only
> attribute
> > +		always set to 0.
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_freque
> ncy
> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Sets the dither signal frequency.
> Units.
> 
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_freque
> ncy_available
> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Returns the available values for the dither frequency.
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_phase
> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Sets the dither signal phase.
> 
> Units.  Radians, or refer to the main _phase docs and say it's the same.
> 
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_phase_
> available
> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Returns the available values for the dither phase.
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_toggle_en
> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Toggle enable. Write 1 to enable toggle or 0 to disable
> it.
> 
> Say why this is useful (presumably toggle with a clock rather than via
> _symbol)
> 
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_raw0
> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		It has the same meaning as out_voltageY_raw. This
> attribute is
> > +		specific to toggle enabled channels and refers to the
> DAC output
> > +		code in INPUT_A. The same scale, offset, etc applies.
> 
> Same as what?
> 
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_raw1
> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Same as out_voltageY_raw0 but referring to the DAC
> output code
> > +		in INPUT_B.
> 
> You could combine this with previous and have two what lines.  Might
> allow
> a slightly more compact clear description.

Did not remembered I could do that...

> 
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_symbol
> 
> Ah. That answers one of my binding related questions :)  You have
> kept
> software control as an option for toggle.

Yeah... It was your suggestion to expose this one only when no pin is
given in dts 😉.

> > +KernelVersion:	5.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Performs a SW toggle. This attribute is specific to toggle
> > +		enabled channels and allows to toggle between
> out_voltageY_raw
> 
> _raw0
> 
> > +		and out_voltageY_raw1 through software. Writing 0
> will select
> > +		out_voltageY_raw while 1 selects out_voltageY_raw1.
> _raw0

Forgot to change it here and above :/

- Nuno Sá

[PATCH v2 3/3] dt-bindings: iio: Add ltc2688 documentation

[Nuno Sá]

Document the LTC2688 devicetree properties.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 .../bindings/iio/dac/adi,ltc2688.yaml         | 147 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 148 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
new file mode 100644
index 000000000000..ecd0943fb813
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
@@ -0,0 +1,147 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/dac/adi,ltc2688.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices LTC2688 DAC
+
+maintainers:
+  - Nuno Sá <nuno.sa@analog.com>
+
+description: |
+  Analog Devices LTC2688 16 channel, 16 bit, +-15V DAC
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2688.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ltc2688
+
+  reg:
+    maxItems: 1
+
+  vcc-supply:
+    description: Analog Supply Voltage Input.
+
+  iovcc-supply:
+    description: Digital Input/Output Supply Voltage.
+
+  vref-supply:
+    description:
+      Reference Input/Output. The voltage at the REF pin sets the full-scale
+      range of all channels. By default, the internal reference is routed to
+      this pin.
+
+  clr-gpios:
+    description:
+      If specified, it will be asserted during driver probe. As the line is
+      active low, it should be marked GPIO_ACTIVE_LOW.
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  "^channel@([0-9]|1[0-5])$":
+    type: object
+
+    properties:
+      reg:
+        description: The channel number representing the DAC output channel.
+        maximum: 15
+
+      adi,toggle-mode:
+        description:
+          Set the channel as a toggle enabled channel. Toggle operation enables
+          fast switching of a DAC output between two different DAC codes without
+          any SPI transaction. It will result in a different ABI for the
+          channel.
+        type: boolean
+
+      adi,output-range-microvolt:
+        description: Specify the channel output full scale range.
+        oneOf:
+          - items:
+              - const: 0
+              - enum: [5000000, 10000000]
+          - items:
+              - const: -5000000
+              - const: 5000000
+          - items:
+              - const: -10000000
+              - const: 10000000
+          - items:
+              - const: -15000000
+              - const: 15000000
+
+      adi,overrange:
+        description: Enable 5% overrange over the selected full scale range.
+        type: boolean
+
+      clocks:
+        maxItems: 1
+
+      adi,toggle-dither-input:
+        description:
+          Selects the TGPx pin to be associated with this channel. This setting
+          only makes sense for toggle or dither enabled channels. If
+          @adi,toggle-mode is not set and this property is given, the channel is
+          assumed to be a dither capable channel. Note that multiple channels
+          can be mapped to the same pin. If this setting is given, the
+          respective @clock must also be provided. Mappings between this and
+          clocks
+            0 - TGP1
+            1 - TGP2
+            2 - TGP3
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 1, 2]
+
+    dependencies:
+      adi,toggle-dither-input: [ clocks ]
+
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+
+    spi {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          ltc2688: ltc2688@0 {
+                  compatible = "adi,ltc2688";
+                  reg = <0>;
+
+                  vcc-supply = <&vcc>;
+                  iovcc-supply = <&vcc>;
+                  vref-supply = <&vref>;
+
+                  #address-cells = <1>;
+                  #size-cells = <0>;
+                  channel@0 {
+                          reg = <0>;
+                          adi,toggle-mode;
+                          adi,overrange;
+                  };
+
+                  channel@1 {
+                          reg = <1>;
+                          adi,output-range-microvolt = <0 10000000>;
+
+                          clocks = <&clock_tgp3>;
+                          adi,toggle-dither-input = <2>;
+                  };
+          };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 16a7fd7f98ee..137d4ed992cf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11188,6 +11188,7 @@ L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	http://ez.analog.com/community/linux-device-drivers
 F:	Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
+F:	Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
 F:	drivers/iio/dac/ltc2688.c
 
 LTC2947 HARDWARE MONITOR DRIVER
-- 
2.33.1

Re: [PATCH v2 3/3] dt-bindings: iio: Add ltc2688 documentation

[Jonathan Cameron]

On Sat, 15 Jan 2022 10:27:05 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> Document the LTC2688 devicetree properties.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Hi Nuno,

A few minor comments inline.  I've reviewed this first so might
find answers to some of them when reading the driver code.

Thanks,

Jonathan

> ---
>  .../bindings/iio/dac/adi,ltc2688.yaml         | 147 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 148 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> new file mode 100644
> index 000000000000..ecd0943fb813
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> @@ -0,0 +1,147 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/dac/adi,ltc2688.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices LTC2688 DAC
> +
> +maintainers:
> +  - Nuno Sá <nuno.sa@analog.com>
> +
> +description: |
> +  Analog Devices LTC2688 16 channel, 16 bit, +-15V DAC
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2688.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ltc2688
> +
> +  reg:
> +    maxItems: 1
> +
> +  vcc-supply:
> +    description: Analog Supply Voltage Input.
> +
> +  iovcc-supply:
> +    description: Digital Input/Output Supply Voltage.
> +
> +  vref-supply:
> +    description:
> +      Reference Input/Output. The voltage at the REF pin sets the full-scale
> +      range of all channels. By default, the internal reference is routed to
> +      this pin.

Perhaps "If not provided the internal reference is used and also provided on the
VREF pin".

> +
> +  clr-gpios:
> +    description:
> +      If specified, it will be asserted during driver probe. As the line is
> +      active low, it should be marked GPIO_ACTIVE_LOW.
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  "^channel@([0-9]|1[0-5])$":
> +    type: object
> +
> +    properties:
> +      reg:
> +        description: The channel number representing the DAC output channel.
> +        maximum: 15
> +
> +      adi,toggle-mode:
> +        description:
> +          Set the channel as a toggle enabled channel. Toggle operation enables
> +          fast switching of a DAC output between two different DAC codes without
> +          any SPI transaction. It will result in a different ABI for the
> +          channel.

The ABI bit is a linux specific part so shouldn't be in the binding.  Just leave
it unsaid.

> +        type: boolean
> +
> +      adi,output-range-microvolt:
> +        description: Specify the channel output full scale range.
> +        oneOf:
> +          - items:
> +              - const: 0
> +              - enum: [5000000, 10000000]
> +          - items:
> +              - const: -5000000
> +              - const: 5000000
> +          - items:
> +              - const: -10000000
> +              - const: 10000000
> +          - items:
> +              - const: -15000000
> +              - const: 15000000

Not necessarily a thing to do now, but this is common enough we should
make it a standard DAC channel property.

> +
> +      adi,overrange:
> +        description: Enable 5% overrange over the selected full scale range.
> +        type: boolean
> +
> +      clocks:
> +        maxItems: 1
> +
> +      adi,toggle-dither-input:
> +        description:
> +          Selects the TGPx pin to be associated with this channel. This setting
> +          only makes sense for toggle or dither enabled channels. If
> +          @adi,toggle-mode is not set and this property is given, the channel is
> +          assumed to be a dither capable channel. Note that multiple channels
> +          can be mapped to the same pin. If this setting is given, the
> +          respective @clock must also be provided. Mappings between this and
> +          clocks

That's not clear, mapping between this an input pin is probably more accurate?

> +            0 - TGP1
> +            1 - TGP2
> +            2 - TGP3
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2]
> +
> +    dependencies:
> +      adi,toggle-dither-input: [ clocks ]
> +
> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    spi {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +          ltc2688: ltc2688@0 {
> +                  compatible = "adi,ltc2688";
> +                  reg = <0>;
> +
> +                  vcc-supply = <&vcc>;
> +                  iovcc-supply = <&vcc>;
> +                  vref-supply = <&vref>;
> +
> +                  #address-cells = <1>;
> +                  #size-cells = <0>;
> +                  channel@0 {
> +                          reg = <0>;
> +                          adi,toggle-mode;

Can do this without adi,toggle-dither-input?

> +                          adi,overrange;
> +                  };
> +
> +                  channel@1 {
> +                          reg = <1>;
> +                          adi,output-range-microvolt = <0 10000000>;
> +
> +                          clocks = <&clock_tgp3>;
> +                          adi,toggle-dither-input = <2>;
> +                  };
> +          };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 16a7fd7f98ee..137d4ed992cf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11188,6 +11188,7 @@ L:	linux-iio@vger.kernel.org
>  S:	Supported
>  W:	http://ez.analog.com/community/linux-device-drivers
>  F:	Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
> +F:	Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
>  F:	drivers/iio/dac/ltc2688.c
>  
>  LTC2947 HARDWARE MONITOR DRIVER

RE: [PATCH v2 3/3] dt-bindings: iio: Add ltc2688 documentation

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, January 16, 2022 1:02 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH v2 3/3] dt-bindings: iio: Add ltc2688
> documentation
> 
> [External]
> 
> On Sat, 15 Jan 2022 10:27:05 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > Document the LTC2688 devicetree properties.
> >
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> Hi Nuno,
> 
> A few minor comments inline.  I've reviewed this first so might
> find answers to some of them when reading the driver code.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  .../bindings/iio/dac/adi,ltc2688.yaml         | 147
> ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 148 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> >
> > diff --git
> a/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> > new file mode 100644
> > index 000000000000..ecd0943fb813
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> > @@ -0,0 +1,147 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> https://urldefense.com/v3/__http://devicetree.org/schemas/iio/dac/
> adi,ltc2688.yaml*__;Iw!!A3Ni8CS0y2Y!qhcjFFDY939hv3Lo2iT1fAIbSObt
> YscKoXjAnLgWZRNG6_WE2WXgZ8AdE4FUwg$
> > +$schema:
> https://urldefense.com/v3/__http://devicetree.org/meta-
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!qhcjFFDY939hv3Lo2iT1fAIbS
> ObtYscKoXjAnLgWZRNG6_WE2WXgZ8BOzIvN9g$
> > +
> > +title: Analog Devices LTC2688 DAC
> > +
> > +maintainers:
> > +  - Nuno Sá <nuno.sa@analog.com>
> > +
> > +description: |
> > +  Analog Devices LTC2688 16 channel, 16 bit, +-15V DAC
> > +  https://www.analog.com/media/en/technical-
> documentation/data-sheets/ltc2688.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,ltc2688
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vcc-supply:
> > +    description: Analog Supply Voltage Input.
> > +
> > +  iovcc-supply:
> > +    description: Digital Input/Output Supply Voltage.
> > +
> > +  vref-supply:
> > +    description:
> > +      Reference Input/Output. The voltage at the REF pin sets the full-
> scale
> > +      range of all channels. By default, the internal reference is routed
> to
> > +      this pin.
> 
> Perhaps "If not provided the internal reference is used and also
> provided on the
> VREF pin".

Sure...

> > +
> > +  clr-gpios:
> > +    description:
> > +      If specified, it will be asserted during driver probe. As the line is
> > +      active low, it should be marked GPIO_ACTIVE_LOW.
> > +    maxItems: 1
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^channel@([0-9]|1[0-5])$":
> > +    type: object
> > +
> > +    properties:
> > +      reg:
> > +        description: The channel number representing the DAC output
> channel.
> > +        maximum: 15
> > +
> > +      adi,toggle-mode:
> > +        description:
> > +          Set the channel as a toggle enabled channel. Toggle operation
> enables
> > +          fast switching of a DAC output between two different DAC
> codes without
> > +          any SPI transaction. It will result in a different ABI for the
> > +          channel.
> 
> The ABI bit is a linux specific part so shouldn't be in the binding.  Just
> leave
> it unsaid.

Makes sense...
> > +        type: boolean
> > +
> > +      adi,output-range-microvolt:
> > +        description: Specify the channel output full scale range.
> > +        oneOf:
> > +          - items:
> > +              - const: 0
> > +              - enum: [5000000, 10000000]
> > +          - items:
> > +              - const: -5000000
> > +              - const: 5000000
> > +          - items:
> > +              - const: -10000000
> > +              - const: 10000000
> > +          - items:
> > +              - const: -15000000
> > +              - const: 15000000
> 
> Not necessarily a thing to do now, but this is common enough we
> should
> make it a standard DAC channel property.
> 
> > +
> > +      adi,overrange:
> > +        description: Enable 5% overrange over the selected full scale
> range.
> > +        type: boolean
> > +
> > +      clocks:
> > +        maxItems: 1
> > +
> > +      adi,toggle-dither-input:
> > +        description:
> > +          Selects the TGPx pin to be associated with this channel. This
> setting
> > +          only makes sense for toggle or dither enabled channels. If
> > +          @adi,toggle-mode is not set and this property is given, the
> channel is
> > +          assumed to be a dither capable channel. Note that multiple
> channels
> > +          can be mapped to the same pin. If this setting is given, the
> > +          respective @clock must also be provided. Mappings between
> this and
> > +          clocks
> 
> That's not clear, mapping between this an input pin is probably more
> accurate?

Yeah, you're right. When I added this, I had the clock-names properties with 
these exact names. So, in the first version it was more acceptable but since
I removed clock-names, this is not clear anymore...

> > +            0 - TGP1
> > +            1 - TGP2
> > +            2 - TGP3
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        enum: [0, 1, 2]
> > +
> > +    dependencies:
> > +      adi,toggle-dither-input: [ clocks ]
> > +
> > +    required:
> > +      - reg
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    spi {
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +          ltc2688: ltc2688@0 {
> > +                  compatible = "adi,ltc2688";
> > +                  reg = <0>;
> > +
> > +                  vcc-supply = <&vcc>;
> > +                  iovcc-supply = <&vcc>;
> > +                  vref-supply = <&vref>;
> > +
> > +                  #address-cells = <1>;
> > +                  #size-cells = <0>;
> > +                  channel@0 {
> > +                          reg = <0>;
> > +                          adi,toggle-mode;
> 
> Can do this without adi,toggle-dither-input?

Yes, In this case it will assume SW toggle and expose the _symbol
interface...

- Nuno Sá

Re: [PATCH v2 0/3] Add support for LTC2688

[Jonathan Cameron]

>   * Have a clock property per channel. Note that we this I moved to OF
> since we now have to use 'devm_get_clk_from_child()' which is using
> device_node. Note that I could use 'to_of_node()' but mixing of.h and
> property.h does not feel like a good idea.

Ah, that's unfortunate given the clk is only needed in certain modes...

Andy/Rafael/Rob, any thoughts on how we should handle this?  Obviously
ACPI and clocks is generally a no go, but in this particular case we
aren't looking at a power management related use of clocks, but rather
using the clk framework to provide a way to control one of our inputs
used to generate the output dithered signal...  If the device just its
own clock then we'd just control it directly with no problems, but it uses
and external source.

We don't know of anyone actually looking at this device in conjunction with
ACPI so maybe just using dt specific calls for now rather than generic
firmware properties is the best we can do.

Thanks,

Jonathan

> 
>  ABI:
>   * Added out_voltageY_raw0 ABI for toggle mode;
>   * Added out_voltageY_dither_offset.
> 
>  Bindings:
>   * Use standard microvolt unit;
>   * Added constrains for adi,output-range-microvolt and removed negative
> values from the dts example;
>   * Moved clocks to the channel object;
>   * Dropped clock-names;
>   * Add a dependency between 'adi,toggle-dither-input' and 'clocks'.
>  
> [1]: https://marc.info/?l=linux-iio&m=163662843603265&w=2
> 
> Nuno Sá (3):
>   iio: dac: add support for ltc2688
>   iio: ABI: add ABI file for the LTC2688 DAC
>   dt-bindings: iio: Add ltc2688 documentation
> 
>  .../ABI/testing/sysfs-bus-iio-dac-ltc2688     |   80 ++
>  .../bindings/iio/dac/adi,ltc2688.yaml         |  147 +++
>  MAINTAINERS                                   |    9 +
>  drivers/iio/dac/Kconfig                       |   11 +
>  drivers/iio/dac/Makefile                      |    1 +
>  drivers/iio/dac/ltc2688.c                     | 1070 +++++++++++++++++
>  6 files changed, 1318 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
>  create mode 100644 drivers/iio/dac/ltc2688.c
> 

Re: [PATCH v2 0/3] Add support for LTC2688

[Andy Shevchenko]

On Sun, Jan 16, 2022 at 7:28 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
>
>
> >   * Have a clock property per channel. Note that we this I moved to OF
> > since we now have to use 'devm_get_clk_from_child()' which is using
> > device_node. Note that I could use 'to_of_node()' but mixing of.h and
> > property.h does not feel like a good idea.
>
> Ah, that's unfortunate given the clk is only needed in certain modes...
>
> Andy/Rafael/Rob, any thoughts on how we should handle this?  Obviously
> ACPI and clocks is generally a no go, but in this particular case we
> aren't looking at a power management related use of clocks, but rather
> using the clk framework to provide a way to control one of our inputs
> used to generate the output dithered signal...  If the device just its
> own clock then we'd just control it directly with no problems, but it uses
> and external source.
>
> We don't know of anyone actually looking at this device in conjunction with
> ACPI so maybe just using dt specific calls for now rather than generic
> firmware properties is the best we can do.

The problem is the CCF is so OF-centric and maintainer(s) seems
skeptical about switching it to use fwnode APIs (they wanted, last
time I tried, to show how exactly it will be used, so here is your
chance!), but OTOH switching to fwnode API is a half made road,
because for ACPI we might need a glue layer which may be way too
tricky to be considered as a part of this series.

-- 
With Best Regards,
Andy Shevchenko