[PATCH 0/3] Add support for LTC2688

[PATCH 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_raw1
 * out_voltageY_symbol

dither mode channels:

 * out_voltageY_dither_en
 * out_voltageY_dither_raw
 * out_voltageY_dither_raw_available
 * 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).

There are some stuff where I'm still not 100% convinced though:

1. out_voltageY_dither_raw refers to the dither amplitude. There are some
differences but in essence, the same scale as the raw attr applies. That
is not true for the offset as it's always 0. This is stated in the ABI
file and being an amplitude is more or less obvious. However, I'm not
sure if it's still valuable to have an ut_voltageY_dither_offset?

2. For now, if 'adi,toggle-dither-input' is given, a correspondent clock
as to be given as well. While this makes sense for dither channels, I'm
not so sure for toggle ones. I can easily see a toggled channel being
controlled by, for example, an host GPIO.

3. Dither capable channels are being silently "assumed" by the driver.
Not sure if an "adi,mode" dt property would make sense. Having this
explicitly could make it easier to express some dependencies in the
bindings file.

4. For now the clocks property is not part of the channels object.
The reason for this is that we only have 3 possible clocks for 16
channels so I wanted to avoid getting and enabling the same clock more
than once. But that is not really an issue and together with 3) it
could, again, make it easier to express some dependencies in the bindings
file. That said, I'm pending in doing this property a channel one (as it
truly is) unless I get feedback otherwise.

[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     |   67 +
 .../bindings/iio/dac/adi,ltc2688.yaml         |  146 +++
 MAINTAINERS                                   |    9 +
 drivers/iio/dac/Kconfig                       |   11 +
 drivers/iio/dac/Makefile                      |    1 +
 drivers/iio/dac/ltc2688.c                     | 1081 +++++++++++++++++
 6 files changed, 1315 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.17.1

[PATCH 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 | 1081 +++++++++++++++++++++++++++++++++++++
 4 files changed, 1100 insertions(+)
 create mode 100644 drivers/iio/dac/ltc2688.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 57fb0f19ee08..90960e3d778f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11162,6 +11162,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 b95619f18fa5..cf503fa93381 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -121,6 +121,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 3c17246ee89b..5904edfb8ddc 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -34,6 +34,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..895a0dbe35af
--- /dev/null
+++ b/drivers/iio/dac/ltc2688.c
@@ -0,0 +1,1081 @@
+// 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/limits.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/kernel.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_DIT_PER_MSK			GENMASK(8, 6)
+#define LTC2688_CH_DIT_PH_MSK			GENMASK(10, 9)
+#define LTC2688_CH_MODE_MSK			BIT(11)
+
+/* 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];
+	/* 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[3] ____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 = reg,
+			.bits_per_word = 8,
+			/*
+			 * This means that @val will also be part of the
+			 * transfer as there's no pad bits. That's fine as these
+			 * bits are don't care for the device and we fill
+			 * @val with the proper value afterwards. Using regmap
+			 * pad bits to get reg_size right would just make the
+			 * write part more cumbersome than this...
+			 */
+			.len = reg_size + 2,
+			.cs_change = 1,
+		}, {
+			.tx_buf = st->tx_data,
+			.rx_buf = st->rx_data,
+			.bits_per_word = 8,
+			.len = 3,
+		},
+	};
+	int ret;
+
+	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 IIO_VAL_FRACTIONAL_LOG2;
+}
+
+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 IIO_VAL_INT;
+}
+
+enum {
+	LT2688_INPUT_A,
+	LT2688_INPUT_B,
+};
+
+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 == LT2688_INPUT_B) {
+		if (code > GENMASK(13, 0))
+			return -EINVAL;
+
+		code <<= 2;
+	}
+
+	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 == LT2688_INPUT_B)
+		*code >>= 2;
+
+	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 m)
+{
+	switch (m) {
+	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 m)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		ret = ltc2688_dac_code_read(st, chan->channel, LT2688_INPUT_A,
+					    val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OFFSET:
+		return ltc2688_offset_get(st, chan->channel, val);
+	case IIO_CHAN_INFO_SCALE:
+		*val2 = 16;
+		return ltc2688_scale_get(st, chan->channel, val);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		ret = regmap_read(st->regmap,
+				  LTC2688_CMD_CH_OFFSET(chan->channel), val);
+		if (ret)
+			return ret;
+
+		/* Just 13 bits used. 2LSB ignored */
+		*val >>= 2;
+		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 mask)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val > U16_MAX || val < 0)
+			return -EINVAL;
+
+		return ltc2688_dac_code_write(st, chan->channel, LT2688_INPUT_A,
+					      val);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		if (val > GENMASK(13, 0))
+			return -EINVAL;
+
+		return regmap_write(st->regmap,
+				    LTC2688_CMD_CH_OFFSET(chan->channel),
+				    val << 2);
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return regmap_write(st->regmap,
+				    LTC2688_CMD_CH_GAIN(chan->channel), val);
+	default:
+		return -EINVAL;
+	}
+}
+
+enum {
+	LTC2688_DITHER_TOGGLE_ENABLE,
+	LTC2688_POWERDOWN,
+	LTC2688_INPUT_B,
+	LTC2688_INPUT_B_AVAIL,
+	LTC2688_SW_TOGGLE,
+	LTC2688_DITHER_FREQ,
+	LTC2688_DITHER_FREQ_AVAIL,
+};
+
+static int ltc2688_dither_toggle_set(struct ltc2688_state *st, u32 chan, bool en)
+{
+	struct ltc2688_chan *c = &st->channels[chan];
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = regmap_update_bits(st->regmap, LTC2688_CMD_TOGGLE_DITHER_EN,
+				 BIT(chan), en << chan);
+	if (ret)
+		goto unlock;
+
+	c->mode = en ? LTC2688_MODE_DITHER_TOGGLE : LTC2688_MODE_DEFAULT;
+unlock:
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static ssize_t ltc2688_reg_bool_get(const struct ltc2688_state *st, u32 chan,
+				    u32 reg, char *buf)
+{
+	int ret;
+	u32 val;
+
+	ret = regmap_read(st->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", !!(val & BIT(chan)));
+}
+
+static ssize_t ltc2688_reg_bool_set(const struct ltc2688_state *st, u32 chan,
+				    u32 reg, const char *buf)
+{
+	int ret;
+	bool en;
+
+	ret = kstrtobool(buf, &en);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(st->regmap, reg, BIT(chan), en << chan);
+}
+
+static ssize_t ltc2688_dither_freq_avail(const struct ltc2688_state *st,
+					 u32 chan, char *buf)
+{
+	const struct ltc2688_chan *c = &st->channels[chan];
+	int sz = 0;
+	u32 f;
+
+	for (f = 0; f < ARRAY_SIZE(c->dither_frequency); f++)
+		sz += sysfs_emit_at(buf, sz, "%ld ", c->dither_frequency[f]);
+
+	buf[sz - 1] = '\n';
+
+	return sz;
+}
+
+static ssize_t ltc2688_dither_freq_get(const struct ltc2688_state *st, u32 chan,
+				       char *buf)
+{
+	const struct ltc2688_chan *c = &st->channels[chan];
+	u32 reg, freq;
+	int ret;
+
+	ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(chan), &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(const struct ltc2688_state *st, u32 chan,
+				   const char *buf)
+{
+	const struct ltc2688_chan *c = &st->channels[chan];
+	long val;
+	u32 freq;
+	int ret;
+
+	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;
+
+	return regmap_update_bits(st->regmap,
+				  LTC2688_CMD_CH_SETTING(chan),
+				  LTC2688_CH_DIT_PER_MSK,
+				  FIELD_PREP(LTC2688_CH_DIT_PER_MSK, freq));
+}
+
+static ssize_t ltc2688_read_ext(struct iio_dev *indio_dev,
+				uintptr_t private,
+				const struct iio_chan_spec *chan,
+				char *buf)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+	u32 val;
+	int ret;
+
+	switch (private) {
+	case LTC2688_DITHER_TOGGLE_ENABLE:
+		return ltc2688_reg_bool_get(st, chan->channel,
+					    LTC2688_CMD_TOGGLE_DITHER_EN, buf);
+	case LTC2688_POWERDOWN:
+		return ltc2688_reg_bool_get(st, chan->channel,
+					    LTC2688_CMD_POWERDOWN, buf);
+	case LTC2688_INPUT_B:
+		ret = ltc2688_dac_code_read(st, chan->channel, LT2688_INPUT_B,
+					    &val);
+		if (ret)
+			return ret;
+
+		return sysfs_emit(buf, "%u\n", val);
+	case LTC2688_INPUT_B_AVAIL:
+		/* dither amplitude has 1/4 of the span */
+		return sysfs_emit(buf, "[%u %u %u]\n", ltc2688_raw_range[0],
+				  ltc2688_raw_range[1],
+				  ltc2688_raw_range[2] / 4);
+	case LTC2688_SW_TOGGLE:
+		return ltc2688_reg_bool_get(st, chan->channel,
+					    LTC2688_CMD_SW_TOGGLE, buf);
+	case LTC2688_DITHER_FREQ:
+		return ltc2688_dither_freq_get(st, chan->channel, buf);
+	case LTC2688_DITHER_FREQ_AVAIL:
+		return ltc2688_dither_freq_avail(st, chan->channel, buf);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t ltc2688_write_ext(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);
+	u16 val;
+	int ret;
+	bool en;
+
+	switch (private) {
+	case LTC2688_DITHER_TOGGLE_ENABLE:
+		ret = kstrtobool(buf, &en);
+		if (ret)
+			return ret;
+
+		ret = ltc2688_dither_toggle_set(st, chan->channel, en);
+		if (ret)
+			return ret;
+
+		return len;
+	case LTC2688_POWERDOWN:
+		ret = ltc2688_reg_bool_set(st, chan->channel,
+					   LTC2688_CMD_POWERDOWN, buf);
+		if (ret)
+			return ret;
+
+		return len;
+	case LTC2688_INPUT_B:
+		ret = kstrtou16(buf, 10, &val);
+		if (ret)
+			return ret;
+
+		ret = ltc2688_dac_code_write(st, chan->channel, LT2688_INPUT_B,
+					     val);
+		if (ret)
+			return ret;
+
+		return len;
+	case LTC2688_SW_TOGGLE:
+		ret = ltc2688_reg_bool_set(st, chan->channel,
+					   LTC2688_CMD_SW_TOGGLE, buf);
+		if (ret)
+			return ret;
+
+		return len;
+	case LTC2688_DITHER_FREQ:
+		ret = ltc2688_dither_freq_set(st, chan->channel, buf);
+		if (ret)
+			return ret;
+
+		return len;
+	default:
+		return -EINVAL;
+	}
+}
+
+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);
+	else
+		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) {	\
+	.name = _name,					\
+	.read = ltc2688_read_ext,			\
+	.write = ltc2688_write_ext,			\
+	.private = (_what),				\
+	.shared = (_shared),				\
+}
+
+/*
+ * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is
+ * not provided in dts.
+ */
+#define LTC2688_CHAN_TOGGLE(t, name) ({							\
+	static const struct iio_chan_spec_ext_info t ## _ext_info[] = {			\
+		LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B, IIO_SEPARATE),		\
+		LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_DITHER_TOGGLE_ENABLE,	\
+				      IIO_SEPARATE),					\
+		LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),	\
+		LTC2688_CHAN_EXT_INFO(name, LTC2688_SW_TOGGLE, IIO_SEPARATE),		\
+		{}									\
+	};										\
+	t ## _ext_info;									\
+})
+
+static struct iio_chan_spec_ext_info ltc2688_dither_ext_info[] = {
+	LTC2688_CHAN_EXT_INFO("dither_raw", LTC2688_INPUT_B, IIO_SEPARATE),
+	LTC2688_CHAN_EXT_INFO("dither_raw_available", LTC2688_INPUT_B_AVAIL,
+			      IIO_SEPARATE),
+	/*
+	 * 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", LTC2688_DITHER_FREQ,
+			      IIO_SEPARATE),
+	LTC2688_CHAN_EXT_INFO("dither_frequency_available",
+			      LTC2688_DITHER_FREQ_AVAIL, IIO_SEPARATE),
+	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_DITHER_TOGGLE_ENABLE,
+			      IIO_SEPARATE),
+	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),
+	{}
+};
+
+static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
+	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),
+	{}
+};
+
+#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					\
+}
+
+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),
+};
+
+enum {
+	LTC2688_CHAN_TD_TGP1,
+	LTC2688_CHAN_TD_TGP2,
+	LTC2688_CHAN_TD_TGP3,
+	LTC2688_CHAN_TD_MAX
+};
+
+static void ltc2688_clk_disable(void *clk)
+{
+	clk_disable_unprepare(clk);
+}
+
+/* Helper struct to deal with dither channels binded to TGPx pins */
+struct ltc2688_dither_helper {
+	u8 chan[LTC2688_DAC_CHANNELS];
+	u8 n_chans;
+};
+
+static const char * const ltc2688_clk_names[LTC2688_CHAN_TD_MAX] = {
+	"TGP1", "TGP2", "TGP3",
+};
+
+static const int ltc2688_period[LTC2688_DITHER_FREQ_AVAIL] = {
+	4, 8, 16, 32, 64,
+};
+
+static void ltc2688_dither_compute_freq_avail(struct ltc2688_state *st,
+					      const struct ltc2688_dither_helper *tgp,
+					      unsigned long rate)
+{
+	u32 e;
+
+	for (e = 0; e < tgp->n_chans; e++) {
+		int c = tgp->chan[e];
+		struct ltc2688_chan *chan = &st->channels[c];
+		u32 f;
+
+		for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++)
+			chan->dither_frequency[f] = DIV_ROUND_CLOSEST(rate, ltc2688_period[f]);
+	}
+}
+
+static int ltc2688_tgp_setup(struct ltc2688_state *st, long clk_mask,
+			     const struct ltc2688_dither_helper *tgp)
+{
+	int ret, bit;
+
+	for_each_set_bit(bit, &clk_mask, LTC2688_CHAN_TD_MAX) {
+		struct clk *clk;
+		unsigned long rate;
+
+		clk = devm_clk_get(&st->spi->dev, ltc2688_clk_names[bit]);
+		if (IS_ERR(clk))
+			return dev_err_probe(&st->spi->dev, PTR_ERR(clk),
+					     "failed to get clk: %s\n",
+					     ltc2688_clk_names[bit]);
+
+		ret = clk_prepare_enable(clk);
+		if (ret)
+			return dev_err_probe(&st->spi->dev, ret,
+					     "failed to enable clk: %s\n",
+					     ltc2688_clk_names[bit]);
+
+		ret = devm_add_action_or_reset(&st->spi->dev,
+					       ltc2688_clk_disable, clk);
+		if (ret)
+			return ret;
+
+		/* this will only be non zero for dither channels */
+		if (!tgp[bit].n_chans)
+			continue;
+
+		rate = clk_get_rate(clk);
+		ltc2688_dither_compute_freq_avail(st, &tgp[bit], rate);
+	}
+
+	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 fwnode_handle *fwnode = dev_fwnode(&st->spi->dev), *child;
+	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] = {0};
+	u32 reg, clk_input, val, mask, tmp[2];
+	unsigned long clk_msk = 0;
+	int ret, span;
+
+	fwnode_for_each_available_child_node(fwnode, child) {
+		struct ltc2688_chan *chan;
+
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret) {
+			fwnode_handle_put(child);
+			return dev_err_probe(&st->spi->dev, ret,
+					     "Failed to get reg property\n");
+		}
+
+		if (reg >= LTC2688_DAC_CHANNELS) {
+			fwnode_handle_put(child);
+			return dev_err_probe(&st->spi->dev, -EINVAL,
+					     "reg bigger than: %d\n",
+					     LTC2688_DAC_CHANNELS);
+		}
+
+		chan = &st->channels[reg];
+		if (fwnode_property_read_bool(child, "adi,toggle-mode")) {
+			chan->toggle_chan = true;
+			/* assume sw toggle ABI */
+			ltc2688_channels[reg].ext_info = LTC2688_CHAN_TOGGLE(__s, "symbol");
+		}
+
+		ret = fwnode_property_read_u32_array(child, "adi,output-range-millivolt",
+						     tmp, ARRAY_SIZE(tmp));
+		if (!ret) {
+			span = ltc2688_span_lookup(st, tmp[0], tmp[1]);
+			if (span < 0)
+				return dev_err_probe(&st->spi->dev, -EINVAL,
+						     "output range not valid:[%d %d]\n",
+						     tmp[0], tmp[1]);
+
+			mask |= LTC2688_CH_SPAN_MSK;
+			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span);
+		}
+
+		ret = fwnode_property_read_u32(child, "adi,toggle-dither-input",
+					       &clk_input);
+		if (!ret) {
+			int cur_chan = tgp[clk_input].n_chans;
+
+			if (clk_input > LTC2688_CHAN_TD_TGP3) {
+				fwnode_handle_put(child);
+				return dev_err_probe(&st->spi->dev, -EINVAL,
+						     "toggle-dither-input inv value(%d)\n",
+						     clk_input);
+			}
+
+			mask |= LTC2688_CH_TD_SEL_MSK;
+			/*
+			 * 0 means software toggle which is the default mode.
+			 * Hence the +1.
+			 */
+			val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1);
+			clk_msk |= BIT(clk_input);
+			/*
+			 * If a TGPx is given, we automatically assume a dither
+			 * capable channel (unless toggle is already enabled).
+			 * Hence, we prepar our TGPx <-> channel map to make it
+			 * easier to handle the clocks and available frequency
+			 * calculations... 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) {
+				tgp[clk_input].chan[cur_chan] = reg;
+				tgp[clk_input].n_chans++;
+				mask |= LTC2688_CH_MODE_MSK;
+				val |= FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
+				ltc2688_channels[reg].ext_info = ltc2688_dither_ext_info;
+			} else {
+				/* wait, no sw toggle after all */
+				ltc2688_channels[reg].ext_info = LTC2688_CHAN_TOGGLE(__no_s, NULL);
+			}
+		}
+
+		if (fwnode_property_read_bool(child, "adi,overrange")) {
+			chan->overrange = true;
+			val |= LTC2688_CH_OVERRANGE_MSK;
+			mask |= BIT(3);
+		}
+
+		if (!mask)
+			continue;
+
+		ret = regmap_update_bits(st->regmap,
+					 LTC2688_CMD_CH_SETTING(reg), mask,
+					 val);
+		if (ret) {
+			fwnode_handle_put(child);
+			return dev_err_probe(&st->spi->dev, -EINVAL,
+					     "failed to set chan settings\n");
+		}
+
+		mask = 0;
+		val = 0;
+	}
+
+	return ltc2688_tgp_setup(st, clk_msk, tgp);
+}
+
+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, "reset", 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);
+
+	ret = ltc2688_channel_config(st);
+	if (ret)
+		return ret;
+
+	if (!vref)
+		return 0;
+
+	return regmap_update_bits(st->regmap, LTC2688_CMD_CONFIG,
+				  LTC2688_CONFIG_EXT_REF, BIT(1));
+}
+
+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)
+{
+	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;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+	/* Just this once. No need to di it in every regmap read */
+	st->tx_data[0] = LTC2688_CMD_NOOP;
+	mutex_init(&st->lock);
+
+	st->regmap = devm_regmap_init(&spi->dev, &ltc2688_regmap_bus, st,
+				      &ltc2688_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
+				     "Failed to init regmap");
+
+	st->regulators[0].supply = "vcc";
+	st->regulators[1].supply = "iovcc";
+	ret = devm_regulator_bulk_get(&spi->dev, ARRAY_SIZE(st->regulators),
+				      st->regulators);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "Failed to get regulators\n");
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret,
+				     "Failed to enable regulators\n");
+
+	ret = devm_add_action_or_reset(&spi->dev, ltc2688_bulk_disable, st);
+	if (ret)
+		return ret;
+
+	vref_reg = devm_regulator_get_optional(&spi->dev, "vref");
+	if (!IS_ERR(vref_reg)) {
+		ret = regulator_enable(vref_reg);
+		if (ret)
+			return dev_err_probe(&spi->dev, ret,
+					     "Failed to enable vref regulators\n");
+
+		ret = devm_add_action_or_reset(&spi->dev,
+					       ltc2688_disable_regulator,
+					       vref_reg);
+		if (ret)
+			return ret;
+
+		ret = regulator_get_voltage(vref_reg);
+		if (ret < 0)
+			return dev_err_probe(&spi->dev, ret,
+					     "Failed to get vref\n");
+
+		st->vref = ret / 1000;
+	} else {
+		if (PTR_ERR(vref_reg) != -ENODEV)
+			return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
+					     "Failed to get vref regulator");
+
+		vref_reg = NULL;
+		/* internal reference */
+		st->vref = 4096;
+	}
+
+	indio_dev->name = "ltc2688";
+	indio_dev->info = &ltc2688_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ltc2688_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ltc2688_channels);
+
+	ret = ltc2688_setup(st, vref_reg);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&st->spi->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.17.1

[PATCH 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_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_raw1
 * out_voltageY_symbol

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 .../ABI/testing/sysfs-bus-iio-dac-ltc2688     | 67 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 68 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..0c45de94dd75
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
@@ -0,0 +1,67 @@
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_en
+KernelVersion:	5.16
+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.16
+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.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Available range for dither raw amplitude values.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_frequency
+KernelVersion:	5.16
+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.16
+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.16
+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.16
+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.16
+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_raw1
+KernelVersion:	5.16
+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 the INPUT_B register while regular out_voltageY_raw
+		refers to INPUT_A. The same scale, offset, etc applies.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_symbol
+KernelVersion:	5.16
+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 90960e3d778f..61b1eaad4611 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11167,6 +11167,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.17.1

[PATCH 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         | 146 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 147 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..7919cd8ec7c9
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
@@ -0,0 +1,146 @@
+# 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.
+
+  reset-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
+
+  clocks:
+    minItems: 1
+    maxItems: 3
+
+  clock-names:
+    minItems: 1
+    maxItems: 3
+    items:
+      enum: [TGP1, TGP2, TGP3]
+
+  '#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-millivolt:
+        description:
+          Specify the channel output full scale range. Allowed values are
+            <0 5000>
+            <0 10000>
+            <-5000 5000>
+            <-10000 10000>
+            <-15000 15000>
+        $ref: /schemas/types.yaml#/definitions/int32-array
+
+      adi,overrange:
+        description: Enable 5% overrange over the selected full scale range.
+        type: boolean
+
+      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]
+
+    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>;
+
+                  clocks = <&clock_tgp2>;
+                  clock-names = "TGP2";
+
+                  #address-cells = <1>;
+                  #size-cells = <0>;
+                  channel@0 {
+                          reg = <0>;
+                          adi,toggle-mode;
+                          adi,overrange;
+                  };
+
+                  channel@1 {
+                          reg = <1>;
+                          adi,output-range-millivolt = <(-10000) 10000>;
+                          adi,toggle-dither-input = <2>;
+                  };
+          };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 61b1eaad4611..4ee2a1b6bf62 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11168,6 +11168,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.17.1

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

[Lars-Peter Clausen]

On 12/14/21 5:56 PM, Nuno Sá 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.

Looks very good!

Although I'm not sure what to make of the `raw1` API. Maybe it makes 
sense to submit an initial version of this driver without the toggle 
API. And then have a follow up discussion how to define the API for 
this. This will not be the only DAC that has this feature so it would be 
a good idea to come up with a common API.


>
> [...]
> +
> +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 = reg,
> +			.bits_per_word = 8,
> +			/*
> +			 * This means that @val will also be part of the
> +			 * transfer as there's no pad bits. That's fine as these
> +			 * bits are don't care for the device and we fill
> +			 * @val with the proper value afterwards. Using regmap
> +			 * pad bits to get reg_size right would just make the
> +			 * write part more cumbersome than this...
> +			 */
This is making assumptions about the memory layout in the regmap core. 
This could change in the future and then this driver breaks. It is 
better to not assume that reg is part of a larger buffer.
> +			.len = reg_size + 2,
> +			.cs_change = 1,
> +		}, {
> +			.tx_buf = st->tx_data,
> +			.rx_buf = st->rx_data,
> +			.bits_per_word = 8,
> +			.len = 3,
> +		},
> +	};
> +	int ret;
> +
> +	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_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
Using mask for the variable name is a relic from the days when it used 
to be a mask. For new drivers it is better to use `info`. Same for the 
other functions.
> [...]
> +
> +static const char * const ltc2688_dither_phase[] = {
> +	"0", "90", "180", "270",
> +};
Usually we use radians for phase values. Although that would make for a 
bit of an awkward API in this case.
> +
> [...]
> +/*
> + * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is
> + * not provided in dts.
> + */
> +#define LTC2688_CHAN_TOGGLE(t, name) ({							\
> +	static const struct iio_chan_spec_ext_info t ## _ext_info[] = {			\
> +		LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B, IIO_SEPARATE),		\
> +		LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_DITHER_TOGGLE_ENABLE,	\
> +				      IIO_SEPARATE),					\
> +		LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),	\
> +		LTC2688_CHAN_EXT_INFO(name, LTC2688_SW_TOGGLE, IIO_SEPARATE),		\
> +		{}									\
> +	};										\
> +	t ## _ext_info;									\
> +})

This macro is a bit strange since it declares a static, but is called in 
a function. It might be better to declare the two types of ext_infos 
statically and then reference them by name from within the function.

> [...]
> +
> +static int ltc2688_tgp_setup(struct ltc2688_state *st, long clk_mask,
> +			     const struct ltc2688_dither_helper *tgp)
> +{
> +	int ret, bit;
> +
> +	for_each_set_bit(bit, &clk_mask, LTC2688_CHAN_TD_MAX) {
clk_mask should be unsigned long
> [...]
> +
> +static int ltc2688_span_lookup(const struct ltc2688_state *st, int min, int max)
> +{
> +	u32 span;
Nit: Why u32 and not unsigned int? The size doesn't seem to be important 
for the loop variable.
> +
> +	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 fwnode_handle *fwnode = dev_fwnode(&st->spi->dev), *child;
> +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] = {0};
> +	u32 reg, clk_input, val, mask, tmp[2];
> +	unsigned long clk_msk = 0;
> +	int ret, span;
> +
> +	fwnode_for_each_available_child_node(fwnode, child) {
> [...]
> +		chan = &st->channels[reg];
> +		if (fwnode_property_read_bool(child, "adi,toggle-mode")) {
> +			chan->toggle_chan = true;
> +			/* assume sw toggle ABI */
> +			ltc2688_channels[reg].ext_info = LTC2688_CHAN_TOGGLE(__s, "symbol");
Updating ltc2688_channels at runtime will break if you have multiple 
instances of the device with a different configuration. You need to 
kmemdup() the channel array.
> +		}
> +[...]
> +	return ltc2688_tgp_setup(st, clk_msk, tgp);
> +}
> +
> +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.
> +	 */
Looking at the datasheet I do not see a reset pin on the chip.
> +	gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", GPIOD_OUT_HIGH);
Usually when we have a reset which is active low we define it in the DT 
as active low rather than doing the inversion in the driver.
> +	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);
> +
> +	ret = ltc2688_channel_config(st);
> +	if (ret)
> +		return ret;
> +
> +	if (!vref)
> +		return 0;
> +
> +	return regmap_update_bits(st->regmap, LTC2688_CMD_CONFIG,
> +				  LTC2688_CONFIG_EXT_REF, BIT(1));

This is a bit confusing since you are using LTC2688_CONFIG_EXT_REF for 
the mask and BIT(1) for the value, even though both are the same.

There is a new API regmap_set_bits()/regmap_clear_bits() that allows you 
to write this in a more compact way. There are a few other places in the 
driver where they can be used as well.

> +}
> [...]
> +

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

[Sa, Nuno]

Hi Lars,

Thanks for the review!

> From: Lars-Peter Clausen <lars@metafoo.de>
> Sent: Wednesday, December 15, 2021 11:24 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org
> Cc: Jonathan Cameron <jic23@kernel.org>; Rob Herring
> <robh+dt@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
> > 
> On 12/14/21 5:56 PM, Nuno Sá 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.
> 
> Looks very good!
> 
> Although I'm not sure what to make of the `raw1` API. Maybe it makes
> sense to submit an initial version of this driver without the toggle
> API. And then have a follow up discussion how to define the API for
> this. This will not be the only DAC that has this feature so it would be
> a good idea to come up with a common API.

This was discussed in the RFC. The raw1 refers to the second code
on the DAC output path so we can toggle between raw and raw1. The
feeling I got from HW guys internally is that this is an important feature
to support. That said, I understand that this being ABI is always sensitive
stuff but ultimately, I honestly think that raw1 fits this feature.

The thing that is making more reluctant about toggle mode is not so
much the ABI but forcing a clock to be used when a toggle channel is
mapped to a TGPx pin...
 
> 
> >
> > [...]
> > +
> > +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 = reg,
> > +			.bits_per_word = 8,
> > +			/*
> > +			 * This means that @val will also be part of the
> > +			 * transfer as there's no pad bits. That's fine as
> these
> > +			 * bits are don't care for the device and we fill
> > +			 * @val with the proper value afterwards. Using
> regmap
> > +			 * pad bits to get reg_size right would just make
> the
> > +			 * write part more cumbersome than this...
> > +			 */
> This is making assumptions about the memory layout in the regmap
> core.
> This could change in the future and then this driver breaks. It is
> better to not assume that reg is part of a larger buffer.

True, but I think reg_size should not really change as we define it in
our regmap_config. Are you suggesting to just use plain 3 here?

> > +			.len = reg_size + 2,
> > +			.cs_change = 1,
> > +		}, {
> > +			.tx_buf = st->tx_data,
> > +			.rx_buf = st->rx_data,
> > +			.bits_per_word = 8,
> > +			.len = 3,
> > +		},
> > +	};
> > +	int ret;
> > +
> > +	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_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan, int val,
> > +			     int val2, long mask)
> Using mask for the variable name is a relic from the days when it used
> to be a mask. For new drivers it is better to use `info`. Same for the
> other functions.

Got it. Probably copy paste...

> > [...]
> > +
> > +static const char * const ltc2688_dither_phase[] = {
> > +	"0", "90", "180", "270",
> > +};
> Usually we use radians for phase values. Although that would make for
> a
> bit of an awkward API in this case.
> > +
> > [...]
> > +/*
> > + * For toggle mode we only expose the symbol attr (sw_toggle) in
> case a TGPx is
> > + * not provided in dts.
> > + */
> > +#define LTC2688_CHAN_TOGGLE(t, name) ({
> 				\
> > +	static const struct iio_chan_spec_ext_info t ## _ext_info[] = {
> 			\
> > +		LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B,
> IIO_SEPARATE),		\
> > +		LTC2688_CHAN_EXT_INFO("toggle_en",
> LTC2688_DITHER_TOGGLE_ENABLE,	\
> > +				      IIO_SEPARATE),
> 		\
> > +		LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE),	\
> > +		LTC2688_CHAN_EXT_INFO(name,
> LTC2688_SW_TOGGLE, IIO_SEPARATE),		\
> > +		{}
> 		\
> > +	};
> 		\
> > +	t ## _ext_info;
> 		\
> > +})
> 
> This macro is a bit strange since it declares a static, but is called in
> a function. It might be better to declare the two types of ext_infos
> statically and then reference them by name from within the function.

Yeah, I also though about that. In the end, the result is the same. I ended
up with this just to save some lines but I'm totally fine with your approach.
It actually makes the code easier to read.

> > [...]
> > +
> > +static int ltc2688_tgp_setup(struct ltc2688_state *st, long clk_mask,
> > +			     const struct ltc2688_dither_helper *tgp)
> > +{
> > +	int ret, bit;
> > +
> > +	for_each_set_bit(bit, &clk_mask, LTC2688_CHAN_TD_MAX) {
> clk_mask should be unsigned long
> > [...]
> > +
> > +static int ltc2688_span_lookup(const struct ltc2688_state *st, int
> min, int max)
> > +{
> > +	u32 span;
> Nit: Why u32 and not unsigned int? The size doesn't seem to be
> important
> for the loop variable.

Shorter to type :)

> > +
> > +	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 fwnode_handle *fwnode = dev_fwnode(&st->spi-
> >dev), *child;
> > +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] =
> {0};
> > +	u32 reg, clk_input, val, mask, tmp[2];
> > +	unsigned long clk_msk = 0;
> > +	int ret, span;
> > +
> > +	fwnode_for_each_available_child_node(fwnode, child) {
> > [...]
> > +		chan = &st->channels[reg];
> > +		if (fwnode_property_read_bool(child, "adi,toggle-
> mode")) {
> > +			chan->toggle_chan = true;
> > +			/* assume sw toggle ABI */
> > +			ltc2688_channels[reg].ext_info =
> LTC2688_CHAN_TOGGLE(__s, "symbol");
> Updating ltc2688_channels at runtime will break if you have multiple
> instances of the device with a different configuration. You need to
> kmemdup() the channel array.

Arghh, yeah (dummy mistake)! You're right! Will fix it in v2...

> > +		}
> > +[...]
> > +	return ltc2688_tgp_setup(st, clk_msk, tgp);
> > +}
> > +
> > +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.
> > +	 */
> Looking at the datasheet I do not see a reset pin on the chip.

IIRC, it's called CLR... But looking at it again if feels like a reset pin but
without directly saying so in the datasheet.

> > +	gpio = devm_gpiod_get_optional(&st->spi->dev, "reset",
> GPIOD_OUT_HIGH);
> Usually when we have a reset which is active low we define it in the DT
> as active low rather than doing the inversion in the driver.

And that's how I tested it in dts. The ' GPIOD_OUT_HIGH' is to request
it in the asserted state and then we just have to de-assert it to take it
out of reset. It's actually the same pattern used in the adis lib. IIRC,
you were actually the one to suggest this :)

> > +	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);
> > +
> > +	ret = ltc2688_channel_config(st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!vref)
> > +		return 0;
> > +
> > +	return regmap_update_bits(st->regmap,
> LTC2688_CMD_CONFIG,
> > +				  LTC2688_CONFIG_EXT_REF, BIT(1));
> 
> This is a bit confusing since you are using LTC2688_CONFIG_EXT_REF
> for
> the mask and BIT(1) for the value, even though both are the same.

I tried to be more or less consistent. So, for masks I used a define and
for the actually value I used the "raw" BIT, FIELD_PREP, FIELD_GET as
I think Jonathan prefers that way. If that's also the preferred way for masks,
I'm happy to update it.

> There is a new API regmap_set_bits()/regmap_clear_bits() that allows
> you
> to write this in a more compact way. There are a few other places in
> the
> driver where they can be used as well.

Hmm, will look at the new API...

- Nuno Sá

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

[Sa, Nuno]

> -----Original Message-----
> From: Sa, Nuno <Nuno.Sa@analog.com>
> Sent: Wednesday, December 15, 2021 2:41 PM
> To: Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org
> Cc: Jonathan Cameron <jic23@kernel.org>; Rob Herring
> <robh+dt@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: RE: [PATCH 1/3] iio: dac: add support for ltc2688
> 
> [External]
> 
> Hi Lars,
> 
> Thanks for the review!
> 
> > From: Lars-Peter Clausen <lars@metafoo.de>
> > Sent: Wednesday, December 15, 2021 11:24 AM
> > To: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org
> > Cc: Jonathan Cameron <jic23@kernel.org>; Rob Herring
> > <robh+dt@kernel.org>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>
> > Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
> > >
> > On 12/14/21 5:56 PM, Nuno Sá 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.
> >
> > Looks very good!
> >
> > Although I'm not sure what to make of the `raw1` API. Maybe it
> makes
> > sense to submit an initial version of this driver without the toggle
> > API. And then have a follow up discussion how to define the API for
> > this. This will not be the only DAC that has this feature so it would be
> > a good idea to come up with a common API.
> 
> This was discussed in the RFC. The raw1 refers to the second code
> on the DAC output path so we can toggle between raw and raw1. The
> feeling I got from HW guys internally is that this is an important feature
> to support. That said, I understand that this being ABI is always
> sensitive
> stuff but ultimately, I honestly think that raw1 fits this feature.
> 
> The thing that is making more reluctant about toggle mode is not so
> much the ABI but forcing a clock to be used when a toggle channel is
> mapped to a TGPx pin...
> 
> >
> > >
> > > [...]
> > > +
> > > +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 = reg,
> > > +			.bits_per_word = 8,
> > > +			/*
> > > +			 * This means that @val will also be part of the
> > > +			 * transfer as there's no pad bits. That's fine as
> > these
> > > +			 * bits are don't care for the device and we fill
> > > +			 * @val with the proper value afterwards. Using
> > regmap
> > > +			 * pad bits to get reg_size right would just make
> > the
> > > +			 * write part more cumbersome than this...
> > > +			 */
> > This is making assumptions about the memory layout in the regmap
> > core.
> > This could change in the future and then this driver breaks. It is
> > better to not assume that reg is part of a larger buffer.
> 
> True, but I think reg_size should not really change as we define it in
> our regmap_config. Are you suggesting to just use plain 3 here?

Ahhh I get what you mean. So you are saying to just use a bounce buffer
(that actually holds space for 3 bytes) in our state struct and just copy reg
onto it? That makes sense...

- Nuno Sá

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

[Lars-Peter Clausen]

On 12/15/21 2:40 PM, Sa, Nuno wrote:
>
>>> +		}
>>> +[...]
>>> +	return ltc2688_tgp_setup(st, clk_msk, tgp);
>>> +}
>>> +
>>> +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.
>>> +	 */
>> Looking at the datasheet I do not see a reset pin on the chip.
> IIRC, it's called CLR... But looking at it again if feels like a reset pin but
> without directly saying so in the datasheet.
ok, but then the gpio should be called "clr" and not "reset".
>
>>> +	gpio = devm_gpiod_get_optional(&st->spi->dev, "reset",
>> GPIOD_OUT_HIGH);
>> Usually when we have a reset which is active low we define it in the DT
>> as active low rather than doing the inversion in the driver.
> And that's how I tested it in dts. The ' GPIOD_OUT_HIGH' is to request
> it in the asserted state and then we just have to de-assert it to take it
> out of reset. It's actually the same pattern used in the adis lib. IIRC,
> you were actually the one to suggest this :)
I'm stupid... just read it the wrong way, code is correct the way it is
>>> +	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);
>>> +
>>> +	ret = ltc2688_channel_config(st);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (!vref)
>>> +		return 0;
>>> +
>>> +	return regmap_update_bits(st->regmap,
>> LTC2688_CMD_CONFIG,
>>> +				  LTC2688_CONFIG_EXT_REF, BIT(1));
>> This is a bit confusing since you are using LTC2688_CONFIG_EXT_REF
>> for
>> the mask and BIT(1) for the value, even though both are the same.
> I tried to be more or less consistent. So, for masks I used a define and
> for the actually value I used the "raw" BIT, FIELD_PREP, FIELD_GET as
> I think Jonathan prefers that way. If that's also the preferred way for masks,
> I'm happy to update it.

Just 5 lines above you use the define for both the mask and the value :)

I don't think it is a good idea to use raw BIT(x) in the code. They are 
just as magic of a value as writing 0x8. There is no way for a reviewer 
to quickly see whether that BIT(x) actually is the right value for the mask.

If you wanted to go the FIELD_PREP route you could write this as

..., LTC2688_CONFIG_EXT_REF, FIELD_PREP(LTC2688_CONFIG_EXT_REF, 1)

But my personal preference is just to pass the mask as the value when 
changing a single bit value. Makes it clear that it is a single bit 
field and you are setting it. Or just use regmap_set_bits().

>> There is a new API regmap_set_bits()/regmap_clear_bits() that allows
>> you
>> to write this in a more compact way. There are a few other places in
>> the
>> driver where they can be used as well.
> Hmm, will look at the new API...
>
> - Nuno Sá

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

[Sa, Nuno]

> From: Lars-Peter Clausen <lars@metafoo.de>
> Sent: Wednesday, December 15, 2021 4:58 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org
> Cc: Jonathan Cameron <jic23@kernel.org>; Rob Herring
> <robh+dt@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
> 
> [External]
> 
> On 12/15/21 2:40 PM, Sa, Nuno wrote:
> >
> >>> +		}
> >>> +[...]
> >>> +	return ltc2688_tgp_setup(st, clk_msk, tgp);
> >>> +}
> >>> +
> >>> +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.
> >>> +	 */
> >> Looking at the datasheet I do not see a reset pin on the chip.
> > IIRC, it's called CLR... But looking at it again if feels like a reset pin but
> > without directly saying so in the datasheet.
> ok, but then the gpio should be called "clr" and not "reset".

ok...

> >
> >>> +	gpio = devm_gpiod_get_optional(&st->spi->dev, "reset",
> >> GPIOD_OUT_HIGH);
> >> Usually when we have a reset which is active low we define it in the
> DT
> >> as active low rather than doing the inversion in the driver.
> > And that's how I tested it in dts. The ' GPIOD_OUT_HIGH' is to
> request
> > it in the asserted state and then we just have to de-assert it to take it
> > out of reset. It's actually the same pattern used in the adis lib. IIRC,
> > you were actually the one to suggest this :)
> I'm stupid... just read it the wrong way, code is correct the way it is
> >>> +	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);
> >>> +
> >>> +	ret = ltc2688_channel_config(st);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	if (!vref)
> >>> +		return 0;
> >>> +
> >>> +	return regmap_update_bits(st->regmap,
> >> LTC2688_CMD_CONFIG,
> >>> +				  LTC2688_CONFIG_EXT_REF, BIT(1));
> >> This is a bit confusing since you are using
> LTC2688_CONFIG_EXT_REF
> >> for
> >> the mask and BIT(1) for the value, even though both are the same.
> > I tried to be more or less consistent. So, for masks I used a define
> and
> > for the actually value I used the "raw" BIT, FIELD_PREP, FIELD_GET as
> > I think Jonathan prefers that way. If that's also the preferred way for
> masks,
> > I'm happy to update it.
> 
> Just 5 lines above you use the define for both the mask and the value
> :)

:facepalm:! At least in my mind I tried to do it...

> I don't think it is a good idea to use raw BIT(x) in the code. They are
> just as magic of a value as writing 0x8. There is no way for a reviewer

Well BIT(3) is still better than 0x8 but I get your point.

> to quickly see whether that BIT(x) actually is the right value for the
> mask.
> 
> If you wanted to go the FIELD_PREP route you could write this as
> 
> ..., LTC2688_CONFIG_EXT_REF,
> FIELD_PREP(LTC2688_CONFIG_EXT_REF, 1)
> 
> But my personal preference is just to pass the mask as the value when
> changing a single bit value. Makes it clear that it is a single bit
> field and you are setting it. Or just use regmap_set_bits().

I think 'regmap_set_bits()' is ideal for these cases and probably introduced
for things like this. And I suspect you prefer that I use
'LTC2688_CONFIG_EXT_REF' as the bit argument :)

- Nuno Sá

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

[Rob Herring]

On Tue, Dec 14, 2021 at 05:56:08PM +0100, Nuno Sá wrote:
> Document the LTC2688 devicetree properties.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  .../bindings/iio/dac/adi,ltc2688.yaml         | 146 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 147 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..7919cd8ec7c9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> @@ -0,0 +1,146 @@
> +# 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.
> +
> +  reset-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
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 3
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 3
> +    items:
> +      enum: [TGP1, TGP2, TGP3]

pattern: '^TGP[1-3]$'

> +
> +  '#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-millivolt:

Not one of the defined units. Use '-microvolt'
> +        description:
> +          Specify the channel output full scale range. Allowed values are
> +            <0 5000>
> +            <0 10000>
> +            <-5000 5000>
> +            <-10000 10000>
> +            <-15000 15000>

Looks like constraints.

items:
  - enum: [ -15000, -10000, -5000, 0 ]
  - enum: [ 5000, 10000, 15000 ]

though that will need to all be x1000.

> +        $ref: /schemas/types.yaml#/definitions/int32-array

And drop the type.

> +
> +      adi,overrange:
> +        description: Enable 5% overrange over the selected full scale range.
> +        type: boolean
> +
> +      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]
> +
> +    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>;
> +
> +                  clocks = <&clock_tgp2>;
> +                  clock-names = "TGP2";
> +
> +                  #address-cells = <1>;
> +                  #size-cells = <0>;
> +                  channel@0 {
> +                          reg = <0>;
> +                          adi,toggle-mode;
> +                          adi,overrange;
> +                  };
> +
> +                  channel@1 {
> +                          reg = <1>;
> +                          adi,output-range-millivolt = <(-10000) 10000>;
> +                          adi,toggle-dither-input = <2>;
> +                  };
> +          };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 61b1eaad4611..4ee2a1b6bf62 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11168,6 +11168,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.17.1
> 
> 

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

[Jonathan Cameron]

On Wed, 15 Dec 2021 15:30:37 -0600
Rob Herring <robh@kernel.org> wrote:

> On Tue, Dec 14, 2021 at 05:56:08PM +0100, Nuno Sá wrote:
> > Document the LTC2688 devicetree properties.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> >  .../bindings/iio/dac/adi,ltc2688.yaml         | 146 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 147 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..7919cd8ec7c9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> > @@ -0,0 +1,146 @@
> > +# 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.
> > +
> > +  reset-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
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 3
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 3
> > +    items:
> > +      enum: [TGP1, TGP2, TGP3]  
> 
> pattern: '^TGP[1-3]$'
> 
> > +
> > +  '#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-millivolt:  
> 
> Not one of the defined units. Use '-microvolt'

> > +        description:
> > +          Specify the channel output full scale range. Allowed values are
> > +            <0 5000>
> > +            <0 10000>
> > +            <-5000 5000>
> > +            <-10000 10000>
> > +            <-15000 15000>  
> 
> Looks like constraints.
> 
> items:
>   - enum: [ -15000, -10000, -5000, 0 ]
>   - enum: [ 5000, 10000, 15000 ]
> 
> though that will need to all be x1000.

also should be constrained to allowed combinations which probably
means a oneOf construct.

> 
> > +        $ref: /schemas/types.yaml#/definitions/int32-array  
> 
> And drop the type.
> 
> > +
> > +      adi,overrange:
> > +        description: Enable 5% overrange over the selected full scale range.
> > +        type: boolean
> > +
> > +      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]
> > +
> > +    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>;
> > +
> > +                  clocks = <&clock_tgp2>;
> > +                  clock-names = "TGP2";
> > +
> > +                  #address-cells = <1>;
> > +                  #size-cells = <0>;
> > +                  channel@0 {
> > +                          reg = <0>;
> > +                          adi,toggle-mode;
> > +                          adi,overrange;
> > +                  };
> > +
> > +                  channel@1 {
> > +                          reg = <1>;
> > +                          adi,output-range-millivolt = <(-10000) 10000>;
> > +                          adi,toggle-dither-input = <2>;
> > +                  };
> > +          };
> > +    };
> > +
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 61b1eaad4611..4ee2a1b6bf62 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11168,6 +11168,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.17.1
> > 
> >   

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

[Jonathan Cameron]

On Tue, 14 Dec 2021 17:56:07 +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_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_raw1
>  * out_voltageY_symbol
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---

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

I was thinking more raw0 and raw1 rather than not having the 0 for the
first one.   If someone has the device in a circuit where they want to use
the toggle mode then I'd assume they know about the special ABI and can
cope with this.

> +KernelVersion:	5.16
> +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 the INPUT_B register while regular out_voltageY_raw
> +		refers to INPUT_A. The same scale, offset, etc applies.

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

[Jonathan Cameron]

On Tue, 14 Dec 2021 17:56:06 +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>

I'm not that keen on toggle having to be clock driven, but I guess we can
always change that later when usecases come along.

Some comments inline.

Jonathan

> ---

> +
> +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 = reg,
> +			.bits_per_word = 8,
> +			/*
> +			 * This means that @val will also be part of the
> +			 * transfer as there's no pad bits. That's fine as these
> +			 * bits are don't care for the device and we fill
> +			 * @val with the proper value afterwards. Using regmap
> +			 * pad bits to get reg_size right would just make the
> +			 * write part more cumbersome than this...
> +			 */
> +			.len = reg_size + 2,

what Lars said :)

> +			.cs_change = 1,
> +		}, {
> +			.tx_buf = st->tx_data,
> +			.rx_buf = st->rx_data,
> +			.bits_per_word = 8,
> +			.len = 3,
> +		},
> +	};
> +	int ret;
> +
> +	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_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 == LT2688_INPUT_B) {
> +		if (code > GENMASK(13, 0))
> +			return -EINVAL;
> +
> +		code <<= 2;

FIELD_PREP preferred.

> +	}
> +
> +	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_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long m)
> +{
> +	struct ltc2688_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ltc2688_dac_code_read(st, chan->channel, LT2688_INPUT_A,
> +					    val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_OFFSET:
> +		return ltc2688_offset_get(st, chan->channel, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		*val2 = 16;
> +		return ltc2688_scale_get(st, chan->channel, val);

I'm not against functions returning the IIO_VAL_* like this, but if they
are I expect the function to set val2 as well.

I'd suggest return 0 on success and then do similar to what you have done for code_read above.

> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		ret = regmap_read(st->regmap,
> +				  LTC2688_CMD_CH_OFFSET(chan->channel), val);
> +		if (ret)
> +			return ret;
> +
> +		/* Just 13 bits used. 2LSB ignored */
> +		*val >>= 2;
FIELD_GET() would get rid of need for the comment.

> +		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 ssize_t ltc2688_read_ext(struct iio_dev *indio_dev,
> +				uintptr_t private,
> +				const struct iio_chan_spec *chan,
> +				char *buf)
> +{
> +	struct ltc2688_state *st = iio_priv(indio_dev);
> +	u32 val;
> +	int ret;
> +
> +	switch (private) {
> +	case LTC2688_DITHER_TOGGLE_ENABLE:

As below. I'd have separate functions rather than multiplexing this
to little benefit.

> +		return ltc2688_reg_bool_get(st, chan->channel,
> +					    LTC2688_CMD_TOGGLE_DITHER_EN, buf);
> +	case LTC2688_POWERDOWN:
> +		return ltc2688_reg_bool_get(st, chan->channel,
> +					    LTC2688_CMD_POWERDOWN, buf);
> +	case LTC2688_INPUT_B:
> +		ret = ltc2688_dac_code_read(st, chan->channel, LT2688_INPUT_B,
> +					    &val);
> +		if (ret)
> +			return ret;
> +
> +		return sysfs_emit(buf, "%u\n", val);
> +	case LTC2688_INPUT_B_AVAIL:
> +		/* dither amplitude has 1/4 of the span */
> +		return sysfs_emit(buf, "[%u %u %u]\n", ltc2688_raw_range[0],
> +				  ltc2688_raw_range[1],
> +				  ltc2688_raw_range[2] / 4);
> +	case LTC2688_SW_TOGGLE:
> +		return ltc2688_reg_bool_get(st, chan->channel,
> +					    LTC2688_CMD_SW_TOGGLE, buf);
> +	case LTC2688_DITHER_FREQ:
> +		return ltc2688_dither_freq_get(st, chan->channel, buf);
> +	case LTC2688_DITHER_FREQ_AVAIL:
> +		return ltc2688_dither_freq_avail(st, chan->channel, buf);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t ltc2688_write_ext(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);
> +	u16 val;
> +	int ret;
> +	bool en;
> +
> +	switch (private) {
> +	case LTC2688_DITHER_TOGGLE_ENABLE:

As commented on below, I'd just have a function for each case block you have
here.

> +		ret = kstrtobool(buf, &en);
> +		if (ret)
> +			return ret;
> +
> +		ret = ltc2688_dither_toggle_set(st, chan->channel, en);
> +		if (ret)
> +			return ret;
> +
> +		return len;
> +	case LTC2688_POWERDOWN:
> +		ret = ltc2688_reg_bool_set(st, chan->channel,
> +					   LTC2688_CMD_POWERDOWN, buf);
> +		if (ret)
> +			return ret;
> +
> +		return len;
> +	case LTC2688_INPUT_B:
> +		ret = kstrtou16(buf, 10, &val);
> +		if (ret)
> +			return ret;
> +
> +		ret = ltc2688_dac_code_write(st, chan->channel, LT2688_INPUT_B,
> +					     val);
> +		if (ret)
> +			return ret;
> +
> +		return len;
> +	case LTC2688_SW_TOGGLE:
> +		ret = ltc2688_reg_bool_set(st, chan->channel,
> +					   LTC2688_CMD_SW_TOGGLE, buf);
> +		if (ret)
> +			return ret;
> +
> +		return len;
> +	case LTC2688_DITHER_FREQ:
> +		ret = ltc2688_dither_freq_set(st, chan->channel, buf);
> +		if (ret)
> +			return ret;
> +
> +		return len;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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);
> +	else
> +		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) {	\
> +	.name = _name,					\
> +	.read = ltc2688_read_ext,			\
> +	.write = ltc2688_write_ext,			\

I'm not really convinced big multiplexer functions are a good idea here.
They seem to save little code and hurt readability a bit.

> +	.private = (_what),				\
> +	.shared = (_shared),				\
> +}
> +
> +/*
> + * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is
> + * not provided in dts.
> + */
> +#define LTC2688_CHAN_TOGGLE(t, name) ({							\
> +	static const struct iio_chan_spec_ext_info t ## _ext_info[] = {			\
> +		LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B, IIO_SEPARATE),		\
> +		LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_DITHER_TOGGLE_ENABLE,	\
> +				      IIO_SEPARATE),					\
> +		LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),	\
> +		LTC2688_CHAN_EXT_INFO(name, LTC2688_SW_TOGGLE, IIO_SEPARATE),		\
> +		{}									\
> +	};										\
> +	t ## _ext_info;									\
> +})
> +
> +static struct iio_chan_spec_ext_info ltc2688_dither_ext_info[] = {
> +	LTC2688_CHAN_EXT_INFO("dither_raw", LTC2688_INPUT_B, IIO_SEPARATE),
> +	LTC2688_CHAN_EXT_INFO("dither_raw_available", LTC2688_INPUT_B_AVAIL,
> +			      IIO_SEPARATE),
> +	/*
> +	 * 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.
> +	 *

no extra blank line.

> +	 */
> +	LTC2688_CHAN_EXT_INFO("dither_frequency", LTC2688_DITHER_FREQ,
> +			      IIO_SEPARATE),
> +	LTC2688_CHAN_EXT_INFO("dither_frequency_available",
> +			      LTC2688_DITHER_FREQ_AVAIL, IIO_SEPARATE),
> +	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_DITHER_TOGGLE_ENABLE,
> +			      IIO_SEPARATE),
> +	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),
> +	{}
> +};
> +
> +static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
> +	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),
> +	{}
> +};
> +

> +
> +enum {
> +	LTC2688_CHAN_TD_TGP1,
> +	LTC2688_CHAN_TD_TGP2,
> +	LTC2688_CHAN_TD_TGP3,
> +	LTC2688_CHAN_TD_MAX
> +};

> +/* Helper struct to deal with dither channels binded to TGPx pins */
> +struct ltc2688_dither_helper {
> +	u8 chan[LTC2688_DAC_CHANNELS];
> +	u8 n_chans;
> +};
> +
bitmap perhaps given ordering doesn't matter (I think)



...

> +
> +static int ltc2688_channel_config(struct ltc2688_state *st)
> +{
> +	struct fwnode_handle *fwnode = dev_fwnode(&st->spi->dev), *child;
> +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] = {0};
> +	u32 reg, clk_input, val, mask, tmp[2];
> +	unsigned long clk_msk = 0;
> +	int ret, span;
> +

I think you need to sanity check you have a fwnode

> +	fwnode_for_each_available_child_node(fwnode, child) {

I guess this is because of the whole device_for_each_available_child_node() not
existing discussion that isn't resolved. 

> +		struct ltc2688_chan *chan;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(&st->spi->dev, ret,
> +					     "Failed to get reg property\n");
> +		}
> +
> +		if (reg >= LTC2688_DAC_CHANNELS) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(&st->spi->dev, -EINVAL,
> +					     "reg bigger than: %d\n",
> +					     LTC2688_DAC_CHANNELS);
> +		}
> +
> +		chan = &st->channels[reg];
> +		if (fwnode_property_read_bool(child, "adi,toggle-mode")) {
> +			chan->toggle_chan = true;
> +			/* assume sw toggle ABI */
> +			ltc2688_channels[reg].ext_info = LTC2688_CHAN_TOGGLE(__s, "symbol");

That's a little nasty.  Pick it up from a static const array defined outside
this function.

> +		}
> +
> +		ret = fwnode_property_read_u32_array(child, "adi,output-range-millivolt",
> +						     tmp, ARRAY_SIZE(tmp));
> +		if (!ret) {
> +			span = ltc2688_span_lookup(st, tmp[0], tmp[1]);
> +			if (span < 0)
> +				return dev_err_probe(&st->spi->dev, -EINVAL,
> +						     "output range not valid:[%d %d]\n",
> +						     tmp[0], tmp[1]);
> +
> +			mask |= LTC2688_CH_SPAN_MSK;
> +			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span);
> +		}
> +
> +		ret = fwnode_property_read_u32(child, "adi,toggle-dither-input",
> +					       &clk_input);
> +		if (!ret) {
> +			int cur_chan = tgp[clk_input].n_chans;
> +
> +			if (clk_input > LTC2688_CHAN_TD_TGP3) {
> +				fwnode_handle_put(child);
> +				return dev_err_probe(&st->spi->dev, -EINVAL,
> +						     "toggle-dither-input inv value(%d)\n",
> +						     clk_input);
> +			}
> +
> +			mask |= LTC2688_CH_TD_SEL_MSK;
> +			/*
> +			 * 0 means software toggle which is the default mode.
> +			 * Hence the +1.
> +			 */
> +			val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1);
> +			clk_msk |= BIT(clk_input);
> +			/*
> +			 * If a TGPx is given, we automatically assume a dither
> +			 * capable channel (unless toggle is already enabled).
> +			 * Hence, we prepar our TGPx <-> channel map to make it

prepare

> +			 * easier to handle the clocks and available frequency
> +			 * calculations... 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) {
> +				tgp[clk_input].chan[cur_chan] = reg;
> +				tgp[clk_input].n_chans++;
> +				mask |= LTC2688_CH_MODE_MSK;
> +				val |= FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
> +				ltc2688_channels[reg].ext_info = ltc2688_dither_ext_info;
> +			} else {
> +				/* wait, no sw toggle after all */
> +				ltc2688_channels[reg].ext_info = LTC2688_CHAN_TOGGLE(__no_s, NULL);
> +			}
> +		}
> +
> +		if (fwnode_property_read_bool(child, "adi,overrange")) {
> +			chan->overrange = true;
> +			val |= LTC2688_CH_OVERRANGE_MSK;
> +			mask |= BIT(3);
> +		}
> +
> +		if (!mask)
> +			continue;
> +
> +		ret = regmap_update_bits(st->regmap,
> +					 LTC2688_CMD_CH_SETTING(reg), mask,
> +					 val);

Maybe I'm missing something, but why not just write the whole register?
Certainly most if not all of the value is controlled by this function or known
at this point to be in the reset state.


> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(&st->spi->dev, -EINVAL,
> +					     "failed to set chan settings\n");
> +		}
> +
> +		mask = 0;
> +		val = 0;

Why not at the start of the loop?  Particularly as I can't see them being
initialised for the first loop.


> +	}
> +
> +	return ltc2688_tgp_setup(st, clk_msk, tgp);
> +}


...

> +static bool ltc2688_reg_writable(struct device *dev, unsigned int reg)
> +{
> +	if (reg <= LTC2688_CMD_UPDATE_ALL && reg != LTC2688_CMD_THERMAL_STAT)

Isn't UPDATE_ALL the last register?  So how do you get higher than that?
Definitely needs a comment if there is a reason that check is necessary.

> +		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;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;

blank line so it's clear the comment refers to the next block.

> +	/* Just this once. No need to di it in every regmap read */

do

> +	st->tx_data[0] = LTC2688_CMD_NOOP;
> +	mutex_init(&st->lock);
> +
> +	st->regmap = devm_regmap_init(&spi->dev, &ltc2688_regmap_bus, st,

A lot of references to &spi->dev so probably worth a local
struct device *dev = &spi->dev; Might let a few things fit
on fewer lines, but either way it'll be a little tidier.


> +				      &ltc2688_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> +				     "Failed to init regmap");
> +
> +	st->regulators[0].supply = "vcc";
> +	st->regulators[1].supply = "iovcc";
> +	ret = devm_regulator_bulk_get(&spi->dev, ARRAY_SIZE(st->regulators),
> +				      st->regulators);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to get regulators\n");
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "Failed to enable regulators\n");
> +
> +	ret = devm_add_action_or_reset(&spi->dev, ltc2688_bulk_disable, st);
> +	if (ret)
> +		return ret;
> +
> +	vref_reg = devm_regulator_get_optional(&spi->dev, "vref");
> +	if (!IS_ERR(vref_reg)) {
> +		ret = regulator_enable(vref_reg);
> +		if (ret)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to enable vref regulators\n");
> +
> +		ret = devm_add_action_or_reset(&spi->dev,
> +					       ltc2688_disable_regulator,
> +					       vref_reg);
> +		if (ret)
> +			return ret;
> +
> +		ret = regulator_get_voltage(vref_reg);
> +		if (ret < 0)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to get vref\n");
> +
> +		st->vref = ret / 1000;
> +	} else {
> +		if (PTR_ERR(vref_reg) != -ENODEV)
> +			return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
> +					     "Failed to get vref regulator");
> +
> +		vref_reg = NULL;
> +		/* internal reference */
> +		st->vref = 4096;
> +	}
> +
> +	indio_dev->name = "ltc2688";
> +	indio_dev->info = &ltc2688_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ltc2688_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ltc2688_channels);
> +
> +	ret = ltc2688_setup(st, vref_reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_iio_device_register(&st->spi->dev, indio_dev);
> +}
> +

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

[Sa, Nuno]

> From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
> Sent: Thursday, December 16, 2021 2:33 PM
> To: Rob Herring <robh@kernel.org>
> Cc: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>;
> Hennerich, Michael <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH 3/3] dt-bindings: iio: Add ltc2688 documentation
> 
> 
> On Wed, 15 Dec 2021 15:30:37 -0600
> Rob Herring <robh@kernel.org> wrote:
> 
> > On Tue, Dec 14, 2021 at 05:56:08PM +0100, Nuno Sá wrote:
> > > Document the LTC2688 devicetree properties.
> > >
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > ---
> > >  .../bindings/iio/dac/adi,ltc2688.yaml         | 146
> ++++++++++++++++++
> > >  MAINTAINERS                                   |   1 +
> > >  2 files changed, 147 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..7919cd8ec7c9
> > > --- /dev/null
> > > +++
> b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> > > @@ -0,0 +1,146 @@
> > > +# 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!rHThZYvGYZfm2zOTRFsr1xH61Bf
> mq371ojtDKEdpTSeC7lCU_dS7CnRBJvPcEQ$
> > > +$schema:
> https://urldefense.com/v3/__http://devicetree.org/meta-
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!rHThZYvGYZfm2zOTRFsr1xH
> 61Bfmq371ojtDKEdpTSeC7lCU_dS7CnSFhKxW0w$
> > > +
> > > +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.
> > > +
> > > +  reset-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
> > > +
> > > +  clocks:
> > > +    minItems: 1
> > > +    maxItems: 3
> > > +
> > > +  clock-names:
> > > +    minItems: 1
> > > +    maxItems: 3
> > > +    items:
> > > +      enum: [TGP1, TGP2, TGP3]
> >
> > pattern: '^TGP[1-3]$'
> >
> > > +
> > > +  '#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-millivolt:
> >
> > Not one of the defined units. Use '-microvolt'
> 
> > > +        description:
> > > +          Specify the channel output full scale range. Allowed values
> are
> > > +            <0 5000>
> > > +            <0 10000>
> > > +            <-5000 5000>
> > > +            <-10000 10000>
> > > +            <-15000 15000>
> >
> > Looks like constraints.
> >
> > items:
> >   - enum: [ -15000, -10000, -5000, 0 ]
> >   - enum: [ 5000, 10000, 15000 ]
> >
> > though that will need to all be x1000.
> 
> also should be constrained to allowed combinations which probably
> means a oneOf construct.
> 

Exactly. AFICT, with Rob's suggestion things like <-15000 5000> would
be validated but not really possible and the driver does not allow it. I did
tried some stuff before sending this simplified form (constrains in description):

...
oneOf:
  - items:
      - const: 0
      - enum: [5000, 10000]
  - items:
      - const: -5000
      - const: 5000
...

Whiles things worked for <0 5000> and <0 10000>, they failed for <(-5000) 5000>:

"
next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.example.dt.y
aml: ltc2688@0: channel@1:adi,output-range-millivolt: 'oneOf'
conditional failed, one must be fixed:
	0 was expected
	-5000 was expected
	From schema: /home/nsa/work/linux-adi-
next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yam"

Trying this combination <0 (-5000)> led to:

"
next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.example.dt.y
aml: ltc2688@0: channel@1:adi,output-range-microvolt: 'oneOf'
conditional failed, one must be fixed:
	-5000 was expected
	4294962296 is not one of [5000, 10000]
	From schema: /home/nsa/work/linux-adi-
next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
"

it makes me feel that something is going on with signed/unsigned
comparisons. But I might be completely off with this approach :)

- Nuno Sá

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

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
> Sent: Thursday, December 16, 2021 2:35 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 2/3] iio: ABI: add ABI file for the LTC2688 DAC
> 
> [External]
> 
> On Tue, 14 Dec 2021 17:56:07 +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_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_raw1
> >  * out_voltageY_symbol
> >
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> 
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_voltageY_raw1
> 
> I was thinking more raw0 and raw1 rather than not having the 0 for the
> first one.   If someone has the device in a circuit where they want to
> use
> the toggle mode then I'd assume they know about the special ABI and
> can
> cope with this.
> 

Hmm I see. That means more probe time handling but well, we are doing
a lot anyways that a bit more won't hurt :).

- Nuno Sá

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

[Sa, Nuno]

> From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
> Sent: Thursday, December 16, 2021 3:11 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 1/3] iio: dac: add support for ltc2688
> 
> [External]
> 
> On Tue, 14 Dec 2021 17:56:06 +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>
> 
> I'm not that keen on toggle having to be clock driven, but I guess we
> can
> always change that later when usecases come along.
> 

I did wrote about some concerns with toggle (among others) in the cover.
When you have the time, some feedback in there would be very welcome :).

Anyways, for toggle mode, I do agree that "has to be clock driven" is likely to harsh.
Right now if a toggle channel is associated with a TGPx pin, then a clock is 
mandatory and that's the condition that probably should be made optional.
Someone can very well want to drive the outputs with a GPIO even though
in that case we could argue to use the SW_TOGGLE.

> 
> > +
> > +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 = reg,
> > +			.bits_per_word = 8,
> > +			/*
> > +			 * This means that @val will also be part of the
> > +			 * transfer as there's no pad bits. That's fine as
> these
> > +			 * bits are don't care for the device and we fill
> > +			 * @val with the proper value afterwards. Using
> regmap
> > +			 * pad bits to get reg_size right would just make
> the
> > +			 * write part more cumbersome than this...
> > +			 */
> > +			.len = reg_size + 2,
> 
> what Lars said :)

Agree...

> > +			.cs_change = 1,
> > +		}, {
> > +			.tx_buf = st->tx_data,
> > +			.rx_buf = st->rx_data,
> > +			.bits_per_word = 8,
> > +			.len = 3,
> > +		},
> > +	};
> > +	int ret;
> > +
> > +	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_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 == LT2688_INPUT_B) {
> > +		if (code > GENMASK(13, 0))
> > +			return -EINVAL;
> > +
> > +		code <<= 2;
> 
> FIELD_PREP preferred.

Will do...

> > +	}
> > +
> > +	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_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int *val,
> > +			    int *val2, long m)
> > +{
> > +	struct ltc2688_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (m) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = ltc2688_dac_code_read(st, chan->channel,
> LT2688_INPUT_A,
> > +					    val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_OFFSET:
> > +		return ltc2688_offset_get(st, chan->channel, val);
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val2 = 16;
> > +		return ltc2688_scale_get(st, chan->channel, val);
> 
> I'm not against functions returning the IIO_VAL_* like this, but if they
> are I expect the function to set val2 as well.
> 
> I'd suggest return 0 on success and then do similar to what you have
> done for code_read above.

Typically I do like to save lines of code when doable and readability is
not hurt which is the case. I'm not doing the same for the code_read
because that one is also used from the extended_info interface. That
said, I don't have strong feeling about this so I can do as you suggest.

> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		ret = regmap_read(st->regmap,
> > +				  LTC2688_CMD_CH_OFFSET(chan-
> >channel), val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Just 13 bits used. 2LSB ignored */
> > +		*val >>= 2;
> FIELD_GET() would get rid of need for the comment.
> 
> > +		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 ssize_t ltc2688_read_ext(struct iio_dev *indio_dev,
> > +				uintptr_t private,
> > +				const struct iio_chan_spec *chan,
> > +				char *buf)
> > +{
> > +	struct ltc2688_state *st = iio_priv(indio_dev);
> > +	u32 val;
> > +	int ret;
> > +
> > +	switch (private) {
> > +	case LTC2688_DITHER_TOGGLE_ENABLE:
> 
> As below. I'd have separate functions rather than multiplexing this
> to little benefit.
> 
> > +		return ltc2688_reg_bool_get(st, chan->channel,
> > +
> LTC2688_CMD_TOGGLE_DITHER_EN, buf);
> > +	case LTC2688_POWERDOWN:
> > +		return ltc2688_reg_bool_get(st, chan->channel,
> > +
> LTC2688_CMD_POWERDOWN, buf);
> > +	case LTC2688_INPUT_B:
> > +		ret = ltc2688_dac_code_read(st, chan->channel,
> LT2688_INPUT_B,
> > +					    &val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return sysfs_emit(buf, "%u\n", val);
> > +	case LTC2688_INPUT_B_AVAIL:
> > +		/* dither amplitude has 1/4 of the span */
> > +		return sysfs_emit(buf, "[%u %u %u]\n",
> ltc2688_raw_range[0],
> > +				  ltc2688_raw_range[1],
> > +				  ltc2688_raw_range[2] / 4);
> > +	case LTC2688_SW_TOGGLE:
> > +		return ltc2688_reg_bool_get(st, chan->channel,
> > +					    LTC2688_CMD_SW_TOGGLE,
> buf);
> > +	case LTC2688_DITHER_FREQ:
> > +		return ltc2688_dither_freq_get(st, chan->channel,
> buf);
> > +	case LTC2688_DITHER_FREQ_AVAIL:
> > +		return ltc2688_dither_freq_avail(st, chan->channel,
> buf);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static ssize_t ltc2688_write_ext(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);
> > +	u16 val;
> > +	int ret;
> > +	bool en;
> > +
> > +	switch (private) {
> > +	case LTC2688_DITHER_TOGGLE_ENABLE:
> 
> As commented on below, I'd just have a function for each case block
> you have
> here.
> 
> > +		ret = kstrtobool(buf, &en);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = ltc2688_dither_toggle_set(st, chan->channel, en);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return len;
> > +	case LTC2688_POWERDOWN:
> > +		ret = ltc2688_reg_bool_set(st, chan->channel,
> > +					   LTC2688_CMD_POWERDOWN,
> buf);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return len;
> > +	case LTC2688_INPUT_B:
> > +		ret = kstrtou16(buf, 10, &val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = ltc2688_dac_code_write(st, chan->channel,
> LT2688_INPUT_B,
> > +					     val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return len;
> > +	case LTC2688_SW_TOGGLE:
> > +		ret = ltc2688_reg_bool_set(st, chan->channel,
> > +					   LTC2688_CMD_SW_TOGGLE,
> buf);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return len;
> > +	case LTC2688_DITHER_FREQ:
> > +		ret = ltc2688_dither_freq_set(st, chan->channel, buf);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return len;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +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);
> > +	else
> > +		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) {	\
> > +	.name = _name,					\
> > +	.read = ltc2688_read_ext,			\
> > +	.write = ltc2688_write_ext,			\
> 
> I'm not really convinced big multiplexer functions are a good idea here.
> They seem to save little code and hurt readability a bit.

I think this is a very common pattern seen in IIO and probably HWMON no?
Anyways, I'm ok with either way so I can just extend the macro to accept
the individual functions. I have to admit that in some cases (when locking is
required in some case blocks) I'm also not a big fan of these multiplexes
functions. And I think I'm calling individual functions in all the case blocks
anyways...
  
> > +	.private = (_what),				\
> > +	.shared = (_shared),				\
> > +}
> > +
> > +/*
> > + * For toggle mode we only expose the symbol attr (sw_toggle) in
> case a TGPx is
> > + * not provided in dts.
> > + */
> > +#define LTC2688_CHAN_TOGGLE(t, name) ({
> 				\
> > +	static const struct iio_chan_spec_ext_info t ## _ext_info[] = {
> 			\
> > +		LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B,
> IIO_SEPARATE),		\
> > +		LTC2688_CHAN_EXT_INFO("toggle_en",
> LTC2688_DITHER_TOGGLE_ENABLE,	\
> > +				      IIO_SEPARATE),
> 		\
> > +		LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE),	\
> > +		LTC2688_CHAN_EXT_INFO(name,
> LTC2688_SW_TOGGLE, IIO_SEPARATE),		\
> > +		{}
> 		\
> > +	};
> 		\
> > +	t ## _ext_info;
> 		\
> > +})
> > +
> > +static struct iio_chan_spec_ext_info ltc2688_dither_ext_info[] = {
> > +	LTC2688_CHAN_EXT_INFO("dither_raw", LTC2688_INPUT_B,
> IIO_SEPARATE),
> > +	LTC2688_CHAN_EXT_INFO("dither_raw_available",
> LTC2688_INPUT_B_AVAIL,
> > +			      IIO_SEPARATE),
> > +	/*
> > +	 * 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.
> > +	 *
> 
> no extra blank line.
> 
> > +	 */
> > +	LTC2688_CHAN_EXT_INFO("dither_frequency",
> LTC2688_DITHER_FREQ,
> > +			      IIO_SEPARATE),
> > +	LTC2688_CHAN_EXT_INFO("dither_frequency_available",
> > +			      LTC2688_DITHER_FREQ_AVAIL,
> IIO_SEPARATE),
> > +	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_DITHER_TOGGLE_ENABLE,
> > +			      IIO_SEPARATE),
> > +	LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE),
> > +	{}
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
> > +	LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE),
> > +	{}
> > +};
> > +
> 
> > +
> > +enum {
> > +	LTC2688_CHAN_TD_TGP1,
> > +	LTC2688_CHAN_TD_TGP2,
> > +	LTC2688_CHAN_TD_TGP3,
> > +	LTC2688_CHAN_TD_MAX
> > +};
> 
> > +/* Helper struct to deal with dither channels binded to TGPx pins */
> > +struct ltc2688_dither_helper {
> > +	u8 chan[LTC2688_DAC_CHANNELS];
> > +	u8 n_chans;
> > +};
> > +
> bitmap perhaps given ordering doesn't matter (I think)
> 

Yeah, did not thought about it but I think it will look better with a bitmap yes.
Although I'm not sure if I will continue with this approach or make the clocks
property a per channel one (more on this in the cover letter).
> 
> ...
> 
> > +
> > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > +{
> > +	struct fwnode_handle *fwnode = dev_fwnode(&st->spi-
> >dev), *child;
> > +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] =
> {0};
> > +	u32 reg, clk_input, val, mask, tmp[2];
> > +	unsigned long clk_msk = 0;
> > +	int ret, span;
> > +
> 
> I think you need to sanity check you have a fwnode

AFAICT, it's done by us already :)

https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L741

> > +	fwnode_for_each_available_child_node(fwnode, child) {
> 
> I guess this is because of the whole
> device_for_each_available_child_node() not
> existing discussion that isn't resolved.

exactly... I wanted the available option and this was the only way I
could find...

> > +		struct ltc2688_chan *chan;
> > +
> > +		ret = fwnode_property_read_u32(child, "reg", &reg);
> > +		if (ret) {
> > +			fwnode_handle_put(child);
> > +			return dev_err_probe(&st->spi->dev, ret,
> > +					     "Failed to get reg
> property\n");
> > +		}
> > +
> > +		if (reg >= LTC2688_DAC_CHANNELS) {
> > +			fwnode_handle_put(child);
> > +			return dev_err_probe(&st->spi->dev, -EINVAL,
> > +					     "reg bigger than: %d\n",
> > +					     LTC2688_DAC_CHANNELS);
> > +		}
> > +
> > +		chan = &st->channels[reg];
> > +		if (fwnode_property_read_bool(child, "adi,toggle-
> mode")) {
> > +			chan->toggle_chan = true;
> > +			/* assume sw toggle ABI */
> > +			ltc2688_channels[reg].ext_info =
> LTC2688_CHAN_TOGGLE(__s, "symbol");
> 
> That's a little nasty.  Pick it up from a static const array defined outside
> this function.

Ehehe, personally I do not think is that bad but ok, Lars was also complaining
about it so better listen to you :)

> > +		}
> > +
> > +		ret = fwnode_property_read_u32_array(child,
> "adi,output-range-millivolt",
> > +						     tmp,
> ARRAY_SIZE(tmp));
> > +		if (!ret) {
> > +			span = ltc2688_span_lookup(st, tmp[0],
> tmp[1]);
> > +			if (span < 0)
> > +				return dev_err_probe(&st->spi->dev, -
> EINVAL,
> > +						     "output range not
> valid:[%d %d]\n",
> > +						     tmp[0], tmp[1]);
> > +
> > +			mask |= LTC2688_CH_SPAN_MSK;
> > +			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK,
> span);
> > +		}
> > +
> > +		ret = fwnode_property_read_u32(child, "adi,toggle-
> dither-input",
> > +					       &clk_input);
> > +		if (!ret) {
> > +			int cur_chan = tgp[clk_input].n_chans;
> > +
> > +			if (clk_input > LTC2688_CHAN_TD_TGP3) {
> > +				fwnode_handle_put(child);
> > +				return dev_err_probe(&st->spi->dev, -
> EINVAL,
> > +						     "toggle-dither-input
> inv value(%d)\n",
> > +						     clk_input);
> > +			}
> > +
> > +			mask |= LTC2688_CH_TD_SEL_MSK;
> > +			/*
> > +			 * 0 means software toggle which is the default
> mode.
> > +			 * Hence the +1.
> > +			 */
> > +			val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK,
> clk_input + 1);
> > +			clk_msk |= BIT(clk_input);
> > +			/*
> > +			 * If a TGPx is given, we automatically assume a
> dither
> > +			 * capable channel (unless toggle is already
> enabled).
> > +			 * Hence, we prepar our TGPx <-> channel map
> to make it
> 
> prepare
> 
> > +			 * easier to handle the clocks and available
> frequency
> > +			 * calculations... 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) {
> > +				tgp[clk_input].chan[cur_chan] = reg;
> > +				tgp[clk_input].n_chans++;
> > +				mask |= LTC2688_CH_MODE_MSK;
> > +				val |=
> FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
> > +				ltc2688_channels[reg].ext_info =
> ltc2688_dither_ext_info;
> > +			} else {
> > +				/* wait, no sw toggle after all */
> > +				ltc2688_channels[reg].ext_info =
> LTC2688_CHAN_TOGGLE(__no_s, NULL);
> > +			}
> > +		}
> > +
> > +		if (fwnode_property_read_bool(child,
> "adi,overrange")) {
> > +			chan->overrange = true;
> > +			val |= LTC2688_CH_OVERRANGE_MSK;
> > +			mask |= BIT(3);
> > +		}
> > +
> > +		if (!mask)
> > +			continue;
> > +
> > +		ret = regmap_update_bits(st->regmap,
> > +
> LTC2688_CMD_CH_SETTING(reg), mask,
> > +					 val);
> 
> Maybe I'm missing something, but why not just write the whole
> register?
> Certainly most if not all of the value is controlled by this function or
> known
> at this point to be in the reset state.

Yeah, that's true. The register will be 0 in reset so just writing will
be enough. Nice catch.

> 
> > +		if (ret) {
> > +			fwnode_handle_put(child);
> > +			return dev_err_probe(&st->spi->dev, -EINVAL,
> > +					     "failed to set chan
> settings\n");
> > +		}
> > +
> > +		mask = 0;
> > +		val = 0;
> 
> Why not at the start of the loop?  Particularly as I can't see them being
> initialised for the first loop.

Hmm I'm actually wondering how did this worked in my tests. I will just
move val to the beginning and mask can be dropped.
 
> 
> > +	}
> > +
> > +	return ltc2688_tgp_setup(st, clk_msk, tgp);
> > +}
> 
> 
> ...
> 
> > +static bool ltc2688_reg_writable(struct device *dev, unsigned int
> reg)
> > +{
> > +	if (reg <= LTC2688_CMD_UPDATE_ALL && reg !=
> LTC2688_CMD_THERMAL_STAT)
> 
> Isn't UPDATE_ALL the last register?  So how do you get higher than
> that?
> Definitely needs a comment if there is a reason that check is
> necessary.

If you look at the commands table you see that on the write side
we jump from 0x76 to 0x78 (UPDATE_ALL=0x7c). 0x77 refers to
reading the thermal status reg which is not writable. Actually in the
end, as it's a read the command for reading the thermal status will
be 0xf7.

> > +		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;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +	st->spi = spi;
> 
> blank line so it's clear the comment refers to the next block.
> 
> > +	/* Just this once. No need to di it in every regmap read */
> 
> do
> 
> > +	st->tx_data[0] = LTC2688_CMD_NOOP;
> > +	mutex_init(&st->lock);
> > +
> > +	st->regmap = devm_regmap_init(&spi->dev,
> &ltc2688_regmap_bus, st,
> 
> A lot of references to &spi->dev so probably worth a local
> struct device *dev = &spi->dev; Might let a few things fit
> on fewer lines, but either way it'll be a little tidier.

can do that...

- Nuno Sá

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

[Jonathan Cameron]

On Tue, 14 Dec 2021 17:56:05 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> 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_raw1
>  * out_voltageY_symbol
> 
> dither mode channels:
> 
>  * out_voltageY_dither_en
>  * out_voltageY_dither_raw
>  * out_voltageY_dither_raw_available
>  * 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).
> 
> There are some stuff where I'm still not 100% convinced though:
> 
> 1. out_voltageY_dither_raw refers to the dither amplitude. There are some
> differences but in essence, the same scale as the raw attr applies. That
> is not true for the offset as it's always 0. This is stated in the ABI
> file and being an amplitude is more or less obvious. However, I'm not
> sure if it's still valuable to have an ut_voltageY_dither_offset?

I think if we have out_voltageY_offset then we should have
out_voltageY_dither_offset to avoid any potential confusion + appropriate
ABI docs text to make it clear that that the more specific _offset takes
precedence.  I have some vague recollection we had a debate about a similar
case in the past where we had
in_voltageX_offset that covered lots of channels and in_voltage99_offset
(number made up) that just happened to be different.  Not sure any
driver takes advantage of ABI perhaps allowing (not sure it's written down)
a more specific attribute to override a generic one...

> 
> 2. For now, if 'adi,toggle-dither-input' is given, a correspondent clock
> as to be given as well. While this makes sense for dither channels, I'm
> not so sure for toggle ones. I can easily see a toggled channel being
> controlled by, for example, an host GPIO.

I agree.  But I think we can relax this when needed rather than it being
necessary to allow for more complex toggle conditions from the start.
Generating a clock driven set of voltages is probably a common usecase
for toggle mode so fine to just support that one until another usecase
comes along.

> 
> 3. Dither capable channels are being silently "assumed" by the driver.
> Not sure if an "adi,mode" dt property would make sense. Having this
> explicitly could make it easier to express some dependencies in the
> bindings file.

I kind of like the assumed default of toggle if the pin is wired up, but
if you prefer an explicit control it becomes a question of whether
Rob (and maybe others) think the binding is sane or not.

> 
> 4. For now the clocks property is not part of the channels object.
> The reason for this is that we only have 3 possible clocks for 16
> channels so I wanted to avoid getting and enabling the same clock more
> than once. But that is not really an issue and together with 3) it
> could, again, make it easier to express some dependencies in the bindings
> file. That said, I'm pending in doing this property a channel one (as it
> truly is) unless I get feedback otherwise.

Interesting question on how to do this.  Maybe a questions where Rob's
input would be particularly useful.  Likely to be similar cases somewhere
else from a dt-binding point of view.

Jonathan

> 
> [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     |   67 +
>  .../bindings/iio/dac/adi,ltc2688.yaml         |  146 +++
>  MAINTAINERS                                   |    9 +
>  drivers/iio/dac/Kconfig                       |   11 +
>  drivers/iio/dac/Makefile                      |    1 +
>  drivers/iio/dac/ltc2688.c                     | 1081 +++++++++++++++++
>  6 files changed, 1315 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 1/3] iio: dac: add support for ltc2688

[Jonathan Cameron]

On Fri, 17 Dec 2021 12:31:57 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
> > Sent: Thursday, December 16, 2021 3:11 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 1/3] iio: dac: add support for ltc2688
> > 
> > [External]
> > 
> > On Tue, 14 Dec 2021 17:56:06 +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>  
> > 
> > I'm not that keen on toggle having to be clock driven, but I guess we
> > can
> > always change that later when usecases come along.
> >   
> 
> I did wrote about some concerns with toggle (among others) in the cover.
> When you have the time, some feedback in there would be very welcome :).

Doh.  Guess I didn't look at the cover letter. Now replied to that as well.

> 
> Anyways, for toggle mode, I do agree that "has to be clock driven" is likely to harsh.
> Right now if a toggle channel is associated with a TGPx pin, then a clock is 
> mandatory and that's the condition that probably should be made optional.
> Someone can very well want to drive the outputs with a GPIO even though
> in that case we could argue to use the SW_TOGGLE.

I wonder if we also need the case where the toggle source is invisible to us
as it's the output of some other hardware.  Obviously would be nice to model
that hardware in DT but that might not always be possible.


> > > +
> > > +static int ltc2688_read_raw(struct iio_dev *indio_dev,
> > > +			    struct iio_chan_spec const *chan, int *val,
> > > +			    int *val2, long m)
> > > +{
> > > +	struct ltc2688_state *st = iio_priv(indio_dev);
> > > +	int ret;
> > > +
> > > +	switch (m) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		ret = ltc2688_dac_code_read(st, chan->channel,  
> > LT2688_INPUT_A,  
> > > +					    val);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		return IIO_VAL_INT;
> > > +	case IIO_CHAN_INFO_OFFSET:
> > > +		return ltc2688_offset_get(st, chan->channel, val);
> > > +	case IIO_CHAN_INFO_SCALE:
> > > +		*val2 = 16;
> > > +		return ltc2688_scale_get(st, chan->channel, val);  
> > 
> > I'm not against functions returning the IIO_VAL_* like this, but if they
> > are I expect the function to set val2 as well.
> > 
> > I'd suggest return 0 on success and then do similar to what you have
> > done for code_read above.  
> 
> Typically I do like to save lines of code when doable and readability is
> not hurt which is the case. I'm not doing the same for the code_read
> because that one is also used from the extended_info interface. That
> said, I don't have strong feeling about this so I can do as you suggest.

Either option is fine for me.  Set val2 inside _scale_get() or return 0
from that and then do a return IIO_VAL_INT_PLUS_MICRO here.

The particular combination at the moment is rather inconsistent as
val, val2 and the return value should all come from the same 'source'
whether it's here, or in _scale_get()

> 
> > > +	case IIO_CHAN_INFO_CALIBBIAS:
> > > +		ret = regmap_read(st->regmap,
> > > +				  LTC2688_CMD_CH_OFFSET(chan-
> > >channel), val);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		/* Just 13 bits used. 2LSB ignored */
> > > +		*val >>= 2;  
> > FIELD_GET() would get rid of need for the comment.
> >   
> > > +		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 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) {	\
> > > +	.name = _name,					\
> > > +	.read = ltc2688_read_ext,			\
> > > +	.write = ltc2688_write_ext,			\  
> > 
> > I'm not really convinced big multiplexer functions are a good idea here.
> > They seem to save little code and hurt readability a bit.  
> 
> I think this is a very common pattern seen in IIO and probably HWMON no?
> Anyways, I'm ok with either way so I can just extend the macro to accept
> the individual functions. I have to admit that in some cases (when locking is
> required in some case blocks) I'm also not a big fan of these multiplexes
> functions. And I think I'm calling individual functions in all the case blocks
> anyways...

Common pattern, but not always a good idea.  All depends on how much
common code there is.  In this case I don't think there is enough for it
to make sense.

>   
> > > +	.private = (_what),				\
> > > +	.shared = (_shared),				\
> > > +}
> > > +

...

> > > +	 */
> > > +	LTC2688_CHAN_EXT_INFO("dither_frequency",  
> > LTC2688_DITHER_FREQ,  
> > > +			      IIO_SEPARATE),
> > > +	LTC2688_CHAN_EXT_INFO("dither_frequency_available",
> > > +			      LTC2688_DITHER_FREQ_AVAIL,  
> > IIO_SEPARATE),  
> > > +	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_DITHER_TOGGLE_ENABLE,  
> > > +			      IIO_SEPARATE),
> > > +	LTC2688_CHAN_EXT_INFO("powerdown",  
> > LTC2688_POWERDOWN, IIO_SEPARATE),  
> > > +	{}
> > > +};
> > > +
> > > +static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
> > > +	LTC2688_CHAN_EXT_INFO("powerdown",  
> > LTC2688_POWERDOWN, IIO_SEPARATE),  
> > > +	{}
> > > +};
> > > +  
> >   
> > > +
> > > +enum {
> > > +	LTC2688_CHAN_TD_TGP1,
> > > +	LTC2688_CHAN_TD_TGP2,
> > > +	LTC2688_CHAN_TD_TGP3,
> > > +	LTC2688_CHAN_TD_MAX
> > > +};  
> >   
> > > +/* Helper struct to deal with dither channels binded to TGPx pins */
> > > +struct ltc2688_dither_helper {
> > > +	u8 chan[LTC2688_DAC_CHANNELS];
> > > +	u8 n_chans;
> > > +};
> > > +  
> > bitmap perhaps given ordering doesn't matter (I think)
> >   
> 
> Yeah, did not thought about it but I think it will look better with a bitmap yes.
> Although I'm not sure if I will continue with this approach or make the clocks
> property a per channel one (more on this in the cover letter).

I'm not sure how the per channel version will look so leaving this entirely
up to you!

> > 
> > ...
> >   
> > > +
> > > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > > +{
> > > +	struct fwnode_handle *fwnode = dev_fwnode(&st->spi-
> > >dev), *child;
> > > +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] =  
> > {0};  
> > > +	u32 reg, clk_input, val, mask, tmp[2];
> > > +	unsigned long clk_msk = 0;
> > > +	int ret, span;
> > > +  
> > 
> > I think you need to sanity check you have a fwnode  
> 
> AFAICT, it's done by us already :)
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L741

Ah.  Good point. Ignore that one then.

> 
> > > +	fwnode_for_each_available_child_node(fwnode, child) {  
> > 
> > I guess this is because of the whole
> > device_for_each_available_child_node() not
> > existing discussion that isn't resolved.  
> 
> exactly... I wanted the available option and this was the only way I
> could find...
> 

Hmm. I need to revisit that discussion and see where we got to.

> >   
> > > +static bool ltc2688_reg_writable(struct device *dev, unsigned int  
> > reg)  
> > > +{
> > > +	if (reg <= LTC2688_CMD_UPDATE_ALL && reg !=  
> > LTC2688_CMD_THERMAL_STAT)
> > 
> > Isn't UPDATE_ALL the last register?  So how do you get higher than
> > that?
> > Definitely needs a comment if there is a reason that check is
> > necessary.  
> 
> If you look at the commands table you see that on the write side
> we jump from 0x76 to 0x78 (UPDATE_ALL=0x7c). 0x77 refers to
> reading the thermal status reg which is not writable. Actually in the
> end, as it's a read the command for reading the thermal status will
> be 0xf7.

I'm lost on this.   My confusion is how you get > LTC2688_CMD_UPDATE_ALL
Possibly that's what you are referring to with teh read command being 0xf...
Maybe try to distil this info down to a brief comment for next
version?

> 
> > > +		return true;
> > > +
> > > +	return false;
> > > +}

Thanks,

Jonathan

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

[Jonathan Cameron]

On Fri, 17 Dec 2021 09:09:30 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
> > Sent: Thursday, December 16, 2021 2:33 PM
> > To: Rob Herring <robh@kernel.org>
> > Cc: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>;
> > Hennerich, Michael <Michael.Hennerich@analog.com>
> > Subject: Re: [PATCH 3/3] dt-bindings: iio: Add ltc2688 documentation
> > 
> > 
> > On Wed, 15 Dec 2021 15:30:37 -0600
> > Rob Herring <robh@kernel.org> wrote:
> >   
> > > On Tue, Dec 14, 2021 at 05:56:08PM +0100, Nuno Sá wrote:  
> > > > Document the LTC2688 devicetree properties.
> > > >
> > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > > ---
> > > >  .../bindings/iio/dac/adi,ltc2688.yaml         | 146  
> > ++++++++++++++++++  
> > > >  MAINTAINERS                                   |   1 +
> > > >  2 files changed, 147 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..7919cd8ec7c9
> > > > --- /dev/null
> > > > +++  
> > b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml  
> > > > @@ -0,0 +1,146 @@
> > > > +# 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!rHThZYvGYZfm2zOTRFsr1xH61Bf
> > mq371ojtDKEdpTSeC7lCU_dS7CnRBJvPcEQ$  
> > > > +$schema:  
> > https://urldefense.com/v3/__http://devicetree.org/meta-
> > schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!rHThZYvGYZfm2zOTRFsr1xH
> > 61Bfmq371ojtDKEdpTSeC7lCU_dS7CnSFhKxW0w$  
> > > > +
> > > > +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.
> > > > +
> > > > +  reset-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
> > > > +
> > > > +  clocks:
> > > > +    minItems: 1
> > > > +    maxItems: 3
> > > > +
> > > > +  clock-names:
> > > > +    minItems: 1
> > > > +    maxItems: 3
> > > > +    items:
> > > > +      enum: [TGP1, TGP2, TGP3]  
> > >
> > > pattern: '^TGP[1-3]$'
> > >  
> > > > +
> > > > +  '#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-millivolt:  
> > >
> > > Not one of the defined units. Use '-microvolt'  
> >   
> > > > +        description:
> > > > +          Specify the channel output full scale range. Allowed values  
> > are  
> > > > +            <0 5000>
> > > > +            <0 10000>
> > > > +            <-5000 5000>
> > > > +            <-10000 10000>
> > > > +            <-15000 15000>  
> > >
> > > Looks like constraints.
> > >
> > > items:
> > >   - enum: [ -15000, -10000, -5000, 0 ]
> > >   - enum: [ 5000, 10000, 15000 ]
> > >
> > > though that will need to all be x1000.  
> > 
> > also should be constrained to allowed combinations which probably
> > means a oneOf construct.
> >   
> 
> Exactly. AFICT, with Rob's suggestion things like <-15000 5000> would
> be validated but not really possible and the driver does not allow it. I did
> tried some stuff before sending this simplified form (constrains in description):
> 
> ...
> oneOf:
>   - items:
>       - const: 0
>       - enum: [5000, 10000]
>   - items:
>       - const: -5000
>       - const: 5000
> ...
> 
> Whiles things worked for <0 5000> and <0 10000>, they failed for <(-5000) 5000>:
> 
> "
> next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.example.dt.y
> aml: ltc2688@0: channel@1:adi,output-range-millivolt: 'oneOf'
> conditional failed, one must be fixed:
> 	0 was expected
> 	-5000 was expected
> 	From schema: /home/nsa/work/linux-adi-
> next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yam"
> 
> Trying this combination <0 (-5000)> led to:
> 
> "
> next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.example.dt.y
> aml: ltc2688@0: channel@1:adi,output-range-microvolt: 'oneOf'
> conditional failed, one must be fixed:
> 	-5000 was expected
> 	4294962296 is not one of [5000, 10000]
> 	From schema: /home/nsa/work/linux-adi-
> next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> "
> 
> it makes me feel that something is going on with signed/unsigned
> comparisons. But I might be completely off with this approach :)

I'll go with huh...

Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml

and other places have pretty much the same construct though examples
don't actually use it.

I have some vague recollection of a previous discussion about negatives
and that Rob had some plan to make them work long term. In the meantime
we just avoid them in the examples.

Rob, is my memory deceiving me?

Thanks,

Jonathan
 
> 
> - Nuno Sá
> 

RE: [PATCH 0/3] Add support for LTC2688

[Sa, Nuno]

Hi Jonathan,

Somehow I failed to see these replies and only when I was about to send a v2,
I remembered to double check :). So let's settle some remaining points and I'll
send the v2 next week.

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Thursday, December 30, 2021 4:14 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 0/3] Add support for LTC2688
> 
> [External]
> 
> On Tue, 14 Dec 2021 17:56:05 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > 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_raw1
> >  * out_voltageY_symbol
> >
> > dither mode channels:
> >
> >  * out_voltageY_dither_en
> >  * out_voltageY_dither_raw
> >  * out_voltageY_dither_raw_available
> >  * 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).
> >
> > There are some stuff where I'm still not 100% convinced though:
> >
> > 1. out_voltageY_dither_raw refers to the dither amplitude. There
> are some
> > differences but in essence, the same scale as the raw attr applies.
> That
> > is not true for the offset as it's always 0. This is stated in the ABI
> > file and being an amplitude is more or less obvious. However, I'm not
> > sure if it's still valuable to have an ut_voltageY_dither_offset?
> 
> I think if we have out_voltageY_offset then we should have
> out_voltageY_dither_offset to avoid any potential confusion +
> appropriate
> ABI docs text to make it clear that that the more specific _offset takes
> precedence.  I have some vague recollection we had a debate about a
> similar
> case in the past where we had
> in_voltageX_offset that covered lots of channels and
> in_voltage99_offset
> (number made up) that just happened to be different.  Not sure any
> driver takes advantage of ABI perhaps allowing (not sure it's written
> down)
> a more specific attribute to override a generic one...

Ok. out_voltageY_dither_offset works for me. I will add it to the dither
ABI and will just return 0 (which might or might *not* be equal to out_voltageX_offset).

> >
> > 2. For now, if 'adi,toggle-dither-input' is given, a correspondent clock
> > as to be given as well. While this makes sense for dither channels, I'm
> > not so sure for toggle ones. I can easily see a toggled channel being
> > controlled by, for example, an host GPIO.
> 
> I agree.  But I think we can relax this when needed rather than it being
> necessary to allow for more complex toggle conditions from the start.
> Generating a clock driven set of voltages is probably a common
> usecase
> for toggle mode so fine to just support that one until another usecase
> comes along.

Fair enough. Let's then let it as-is. Even though not the most neat thing to do,
we can always use a fixed clock to overcome the constrain anyways :) .

> >
> > 3. Dither capable channels are being silently "assumed" by the driver.
> > Not sure if an "adi,mode" dt property would make sense. Having this
> > explicitly could make it easier to express some dependencies in the
> > bindings file.
> 
> I kind of like the assumed default of toggle if the pin is wired up, but
> if you prefer an explicit control it becomes a question of whether
> Rob (and maybe others) think the binding is sane or not.

We can leave it as is for now. Honestly the main motivation for it was
to express some dependencies in the bindings. Like the following pseudo code:

if adi,mode == dither; then
   adi,toggle-dither-input depends on clocks

This is naturally only possible if we make clocks a property of the channels
object...

> >
> > 4. For now the clocks property is not part of the channels object.
> > The reason for this is that we only have 3 possible clocks for 16
> > channels so I wanted to avoid getting and enabling the same clock
> more
> > than once. But that is not really an issue and together with 3) it
> > could, again, make it easier to express some dependencies in the
> bindings
> > file. That said, I'm pending in doing this property a channel one (as it
> > truly is) unless I get feedback otherwise.
> 
> Interesting question on how to do this.  Maybe a questions where
> Rob's
> input would be particularly useful.  Likely to be similar cases
> somewhere
> else from a dt-binding point of view.
> 

I do think it might make sense to have clocks as part of the channel object as
it is indeed a property of the channel.

And as we are leaving the constrain for toggle mode channels (of
providing a clock if a tgpx pin is provided) one neat thing to have in the
bindings would be:

dependencies: adi,toggle-dither-input ["clocks"]

Rob, any input on the above?

- Nuno Sá

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

[Sa, Nuno]

> From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
> Sent: Thursday, December 30, 2021 4:28 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 1/3] iio: dac: add support for ltc2688
> 
> [External]
> 
> On Fri, 17 Dec 2021 12:31:57 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
> > > Sent: Thursday, December 16, 2021 3:11 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 1/3] iio: dac: add support for ltc2688
> > >
> > > [External]
> > >
> > > On Tue, 14 Dec 2021 17:56:06 +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>
> > >
> > > I'm not that keen on toggle having to be clock driven, but I guess
> we
> > > can
> > > always change that later when usecases come along.
> > >
> >
> > I did wrote about some concerns with toggle (among others) in the
> cover.
> > When you have the time, some feedback in there would be very
> welcome :).
> 
> Doh.  Guess I didn't look at the cover letter. Now replied to that as
> well.
> 
> >
> > Anyways, for toggle mode, I do agree that "has to be clock driven" is
> likely to harsh.
> > Right now if a toggle channel is associated with a TGPx pin, then a
> clock is
> > mandatory and that's the condition that probably should be made
> optional.
> > Someone can very well want to drive the outputs with a GPIO even
> though
> > in that case we could argue to use the SW_TOGGLE.
> 
> I wonder if we also need the case where the toggle source is invisible
> to us
> as it's the output of some other hardware.  Obviously would be nice to
> model
> that hardware in DT but that might not always be possible.

Yeah, could fall into the host GPIO usecase. We don't really control it
in the driver but the user can still use it to drive the dac output. Anyways,
as stated in the cover, let's leave things as is for now...

> 
> > > > +
> > > > +static int ltc2688_read_raw(struct iio_dev *indio_dev,
> > > > +			    struct iio_chan_spec const *chan, int *val,
> > > > +			    int *val2, long m)
> > > > +{
> > > > +	struct ltc2688_state *st = iio_priv(indio_dev);
> > > > +	int ret;
> > > > +
> > > > +	switch (m) {
> > > > +	case IIO_CHAN_INFO_RAW:
> > > > +		ret = ltc2688_dac_code_read(st, chan->channel,
> > > LT2688_INPUT_A,
> > > > +					    val);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		return IIO_VAL_INT;
> > > > +	case IIO_CHAN_INFO_OFFSET:
> > > > +		return ltc2688_offset_get(st, chan->channel, val);
> > > > +	case IIO_CHAN_INFO_SCALE:
> > > > +		*val2 = 16;
> > > > +		return ltc2688_scale_get(st, chan->channel, val);
> > >
> > > I'm not against functions returning the IIO_VAL_* like this, but if
> they
> > > are I expect the function to set val2 as well.
> > >
> > > I'd suggest return 0 on success and then do similar to what you
> have
> > > done for code_read above.
> >
> > Typically I do like to save lines of code when doable and readability is
> > not hurt which is the case. I'm not doing the same for the code_read
> > because that one is also used from the extended_info interface.
> That
> > said, I don't have strong feeling about this so I can do as you suggest.
> 
> Either option is fine for me.  Set val2 inside _scale_get() or return 0
> from that and then do a return IIO_VAL_INT_PLUS_MICRO here.
> 
> The particular combination at the moment is rather inconsistent as
> val, val2 and the return value should all come from the same 'source'
> whether it's here, or in _scale_get()

Already did as you first advised...

> >
> > > > +	case IIO_CHAN_INFO_CALIBBIAS:
> > > > +		ret = regmap_read(st->regmap,
> > > > +				  LTC2688_CMD_CH_OFFSET(chan-
> > > >channel), val);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		/* Just 13 bits used. 2LSB ignored */
> > > > +		*val >>= 2;
> > > FIELD_GET() would get rid of need for the comment.
> > >
> > > > +		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 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) {	\
> > > > +	.name = _name,					\
> > > > +	.read = ltc2688_read_ext,			\
> > > > +	.write = ltc2688_write_ext,			\
> > >
> > > I'm not really convinced big multiplexer functions are a good idea
> here.
> > > They seem to save little code and hurt readability a bit.
> >
> > I think this is a very common pattern seen in IIO and probably
> HWMON no?
> > Anyways, I'm ok with either way so I can just extend the macro to
> accept
> > the individual functions. I have to admit that in some cases (when
> locking is
> > required in some case blocks) I'm also not a big fan of these
> multiplexes
> > functions. And I think I'm calling individual functions in all the case
> blocks
> > anyways...
> 
> Common pattern, but not always a good idea.  All depends on how
> much
> common code there is.  In this case I don't think there is enough for it
> to make sense.

Agreed. 

> >
> > > > +	.private = (_what),				\
> > > > +	.shared = (_shared),				\
> > > > +}
> > > > +
> 
> ...
> 
> > > > +	 */
> > > > +	LTC2688_CHAN_EXT_INFO("dither_frequency",
> > > LTC2688_DITHER_FREQ,
> > > > +			      IIO_SEPARATE),
> > > > +	LTC2688_CHAN_EXT_INFO("dither_frequency_available",
> > > > +			      LTC2688_DITHER_FREQ_AVAIL,
> > > IIO_SEPARATE),
> > > > +	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_DITHER_TOGGLE_ENABLE,
> > > > +			      IIO_SEPARATE),
> > > > +	LTC2688_CHAN_EXT_INFO("powerdown",
> > > LTC2688_POWERDOWN, IIO_SEPARATE),
> > > > +	{}
> > > > +};
> > > > +
> > > > +static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
> > > > +	LTC2688_CHAN_EXT_INFO("powerdown",
> > > LTC2688_POWERDOWN, IIO_SEPARATE),
> > > > +	{}
> > > > +};
> > > > +
> > >
> > > > +
> > > > +enum {
> > > > +	LTC2688_CHAN_TD_TGP1,
> > > > +	LTC2688_CHAN_TD_TGP2,
> > > > +	LTC2688_CHAN_TD_TGP3,
> > > > +	LTC2688_CHAN_TD_MAX
> > > > +};
> > >
> > > > +/* Helper struct to deal with dither channels binded to TGPx
> pins */
> > > > +struct ltc2688_dither_helper {
> > > > +	u8 chan[LTC2688_DAC_CHANNELS];
> > > > +	u8 n_chans;
> > > > +};
> > > > +
> > > bitmap perhaps given ordering doesn't matter (I think)
> > >
> >
> > Yeah, did not thought about it but I think it will look better with a
> bitmap yes.
> > Although I'm not sure if I will continue with this approach or make
> the clocks
> > property a per channel one (more on this in the cover letter).
> 
> I'm not sure how the per channel version will look so leaving this
> entirely
> up to you!
> 
> > >
> > > ...
> > >
> > > > +
> > > > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > > > +{
> > > > +	struct fwnode_handle *fwnode = dev_fwnode(&st->spi-
> > > >dev), *child;
> > > > +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] =
> > > {0};
> > > > +	u32 reg, clk_input, val, mask, tmp[2];
> > > > +	unsigned long clk_msk = 0;
> > > > +	int ret, span;
> > > > +
> > >
> > > I think you need to sanity check you have a fwnode
> >
> > AFAICT, it's done by us already :)
> >
> >
> https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/s
> ource/drivers/base/property.c*L741__;Iw!!A3Ni8CS0y2Y!sxVAp8P8XS
> 0R5sR447hKmVu7dK01fdsMfL6_c4woz7kDsbsI2fKLKfWopK4mOQ$
> 
> Ah.  Good point. Ignore that one then.
> 
> >
> > > > +	fwnode_for_each_available_child_node(fwnode, child) {
> > >
> > > I guess this is because of the whole
> > > device_for_each_available_child_node() not
> > > existing discussion that isn't resolved.
> >
> > exactly... I wanted the available option and this was the only way I
> > could find...
> >
> 
> Hmm. I need to revisit that discussion and see where we got to.
> 
> > >
> > > > +static bool ltc2688_reg_writable(struct device *dev, unsigned
> int
> > > reg)
> > > > +{
> > > > +	if (reg <= LTC2688_CMD_UPDATE_ALL && reg !=
> > > LTC2688_CMD_THERMAL_STAT)
> > >
> > > Isn't UPDATE_ALL the last register?  So how do you get higher than
> > > that?
> > > Definitely needs a comment if there is a reason that check is
> > > necessary.
> >
> > If you look at the commands table you see that on the write side
> > we jump from 0x76 to 0x78 (UPDATE_ALL=0x7c). 0x77 refers to
> > reading the thermal status reg which is not writable. Actually in the
> > end, as it's a read the command for reading the thermal status will
> > be 0xf7.
> 
> I'm lost on this.   My confusion is how you get >
> LTC2688_CMD_UPDATE_ALL
> Possibly that's what you are referring to with teh read command being
> 0xf...

Exactly... So I'm basically using the regmap read bit to get the read command
as in the datasheet. And that bit is still not set when we get into these callbacks
which means you can get 0x77 here which is < LTC2688_CMD_UPDATE_ALL but
still not writable...

I did had a comment for v2. Let's see if it's god enough :)

- Nuno Sá

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

[Sa, Nuno]

> From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
> Sent: Thursday, December 30, 2021 4:44 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>; devicetree@vger.kernel.org
> Cc: Rob Herring <robh@kernel.org>; linux-iio@vger.kernel.org; Lars-
> Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH 3/3] dt-bindings: iio: Add ltc2688 documentation
> 
> [External]
> 
> On Fri, 17 Dec 2021 09:09:30 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
> > > Sent: Thursday, December 16, 2021 2:33 PM
> > > To: Rob Herring <robh@kernel.org>
> > > Cc: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio@vger.kernel.org;
> > > devicetree@vger.kernel.org; Lars-Peter Clausen
> <lars@metafoo.de>;
> > > Hennerich, Michael <Michael.Hennerich@analog.com>
> > > Subject: Re: [PATCH 3/3] dt-bindings: iio: Add ltc2688
> documentation
> > >
> > >
> > > On Wed, 15 Dec 2021 15:30:37 -0600
> > > Rob Herring <robh@kernel.org> wrote:
> > >
> > > > On Tue, Dec 14, 2021 at 05:56:08PM +0100, Nuno Sá wrote:
> > > > > Document the LTC2688 devicetree properties.
> > > > >
> > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > > > ---
> > > > >  .../bindings/iio/dac/adi,ltc2688.yaml         | 146
> > > ++++++++++++++++++
> > > > >  MAINTAINERS                                   |   1 +
> > > > >  2 files changed, 147 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..7919cd8ec7c9
> > > > > --- /dev/null
> > > > > +++
> > > b/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> > > > > @@ -0,0 +1,146 @@
> > > > > +# 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!rHThZYvGYZfm2zOTRFsr1xH61Bf
> > > mq371ojtDKEdpTSeC7lCU_dS7CnRBJvPcEQ$
> > > > > +$schema:
> > > https://urldefense.com/v3/__http://devicetree.org/meta-
> > >
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!rHThZYvGYZfm2zOTRFsr1xH
> > > 61Bfmq371ojtDKEdpTSeC7lCU_dS7CnSFhKxW0w$
> > > > > +
> > > > > +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.
> > > > > +
> > > > > +  reset-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
> > > > > +
> > > > > +  clocks:
> > > > > +    minItems: 1
> > > > > +    maxItems: 3
> > > > > +
> > > > > +  clock-names:
> > > > > +    minItems: 1
> > > > > +    maxItems: 3
> > > > > +    items:
> > > > > +      enum: [TGP1, TGP2, TGP3]
> > > >
> > > > pattern: '^TGP[1-3]$'
> > > >
> > > > > +
> > > > > +  '#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-millivolt:
> > > >
> > > > Not one of the defined units. Use '-microvolt'
> > >
> > > > > +        description:
> > > > > +          Specify the channel output full scale range. Allowed
> values
> > > are
> > > > > +            <0 5000>
> > > > > +            <0 10000>
> > > > > +            <-5000 5000>
> > > > > +            <-10000 10000>
> > > > > +            <-15000 15000>
> > > >
> > > > Looks like constraints.
> > > >
> > > > items:
> > > >   - enum: [ -15000, -10000, -5000, 0 ]
> > > >   - enum: [ 5000, 10000, 15000 ]
> > > >
> > > > though that will need to all be x1000.
> > >
> > > also should be constrained to allowed combinations which probably
> > > means a oneOf construct.
> > >
> >
> > Exactly. AFICT, with Rob's suggestion things like <-15000 5000> would
> > be validated but not really possible and the driver does not allow it. I
> did
> > tried some stuff before sending this simplified form (constrains in
> description):
> >
> > ...
> > oneOf:
> >   - items:
> >       - const: 0
> >       - enum: [5000, 10000]
> >   - items:
> >       - const: -5000
> >       - const: 5000
> > ...
> >
> > Whiles things worked for <0 5000> and <0 10000>, they failed for <(-
> 5000) 5000>:
> >
> > "
> >
> next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.example
> .dt.y
> > aml: ltc2688@0: channel@1:adi,output-range-millivolt: 'oneOf'
> > conditional failed, one must be fixed:
> > 	0 was expected
> > 	-5000 was expected
> > 	From schema: /home/nsa/work/linux-adi-
> > next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yam"
> >
> > Trying this combination <0 (-5000)> led to:
> >
> > "
> >
> next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.example
> .dt.y
> > aml: ltc2688@0: channel@1:adi,output-range-microvolt: 'oneOf'
> > conditional failed, one must be fixed:
> > 	-5000 was expected
> > 	4294962296 is not one of [5000, 10000]
> > 	From schema: /home/nsa/work/linux-adi-
> > next/Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
> > "
> >
> > it makes me feel that something is going on with signed/unsigned
> > comparisons. But I might be completely off with this approach :)
> 
> I'll go with huh...
> 
> Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> 
> and other places have pretty much the same construct though
> examples
> don't actually use it.
> 
> I have some vague recollection of a previous discussion about
> negatives
> and that Rob had some plan to make them work long term. In the
> meantime
> we just avoid them in the examples.
> 
> Rob, is my memory deceiving me?
> 

Rob, any input on this? Should I just not add the property to the examples
for now?

- Nuno Sá