[RFC PATCH 0/1] LTC2688 support

[RFC PATCH 0/1] LTC2688 support

[Nuno Sá]

The reason why this is a RFC is because I'm still waiting for proper HW
to test this thing. The reason why I'm sending this already is because
there's some non usual features which might require extra ABI. Hence,
while waiting I thought we could already speed up the process in regards
with the ABI.

I still pushed the driver but the intent here is not really to review it.
Naturally, if someone already wants to give some feedback, that's very
much appreciated :)

Now, there are three main interfaces depending on the channel mode:
 1) default (no new ABI)
 2) toggle mode
 3) dither mode

The channel mode will be a devicetree property as it does not really
make much sense to change between modes at runtime even more because the
piece of HW we might want to control with a channel might be different
depending on the selected mode (I'm fairly sure on this between toggle
and other modes but not so sure between dither and default mode).

toggle mode special ABI:

 * Toggle operation enables fast switching of a DAC output between two
different DAC codes without any SPI transaction. Two codes are set in
input_a and input_b and then the output switches according to an input
signal (input_a -> clk high; input_b -> clk low).

out_voltageY_input_register
 - selects between input_a and input_b. After selecting one register, we
   can write the dac code in out_voltageY_raw.
out_voltageY_toggle_en
 - Disable/Enable toggle mode. The reason why I think this one is
   important is because if we wanna change the 2 codes, we should first
   disable this, set the codes and only then enable the mode back...
   As I'm writing this, It kind of came to me that we can probably
   achieve this with out_voltageY_powerdown attr (maybe takes a bit more
   time to see the outputs but...) 

dither mode special ABI:

 * Dither operation adds a small sinusoidal wave to the digital DAC
signal path. Dithering is a signal processing technique that involves
the injection of ac noise to the signal path to reduce system
nonlinearities.

out_voltage0_dither_en
 - Same as in toggle mode.
out_voltage0_dither_period
out_voltage0_dither_phase
 - Period and phase of the signal. Only some values are valid so there's
   also *_available files for these. I'm not sure if we can use the more
   generic IIO_CHAN_INFO_PHASE and IIO_CHAN_INFO_FREQUENCY here as these
   parameters don't really apply to the channel output signal... 
out_voltage0_input_register
 - Same as in toggle mode. However in this mode the code set in the
   input_b register has a special meaning. It's the amplitude of the
   dither signal.

One special mention to the dac scale. In this part this is something
that can be purely controlled by SW so that I'm allowing userspace to
change it rather then having it in dts. The available scales are:

* [0 5V] -> offset 0
* [0 10V] -> offset 0
* [-5 5V] -> offset -32678
* [-10 10V] -> offset -32768
* [-15 15V] -> offset -32768

With the above, we also need to have the offset configurable and right
now I'm going to some trouble to make sure the scale + offset is
something valid. Honestly, I think I'm overdoing it because things can
still go wrong with [0 10V] and [-5 5V] as the scales are the same
here. Furthermore, there's no real arm that can be done to the HW. Is
just that the readings won't match with what someone might be expecting.
My plan is to just remove those checks and assume is up to userspace to
make it right and not have [-10 10V] scale with 0 offset for example.

I know that I'm taking a shortcut here :) so if you prefer to only
discuss this in the __real__ series, I totally get it.

https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2688.pdf

- Nuno Sá

Nuno Sá (1):
  iio: dac: add support for ltc2688

 drivers/iio/dac/ltc2688.c | 995 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 995 insertions(+)
 create mode 100644 drivers/iio/dac/ltc2688.c

-- 
2.33.1

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

[Nuno Sá]

The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
precission 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>
---
 drivers/iio/dac/ltc2688.c | 995 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 995 insertions(+)
 create mode 100644 drivers/iio/dac/ltc2688.c

diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c
new file mode 100644
index 000000000000..b394701b044e
--- /dev/null
+++ b/drivers/iio/dac/ltc2688.c
@@ -0,0 +1,995 @@
+// 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/kernel.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/unaligned/be_byteshift.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_REG			0x70
+#define LTC2688_CMD_POWERDOWN_REG		0x71
+#define LTC2688_CMD_A_B_SELECT_REG		0x72
+#define LTC2688_CMD_TOGGLE_DITHER_EN_REG	0x74
+#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)
+
+enum {
+	LTC2688_SPAN_RANGE_0V_5V,
+	LTC2688_SPAN_RANGE_M10V_10V,
+	LTC2688_SPAN_RANGE_M15V_15V,
+	/*
+	 * These 2 were deliberately re-arranged to be the last items as they
+	 * have the same fs and we will just return 1 in read_available.
+	 */
+	LTC2688_SPAN_RANGE_0V_10V,
+	LTC2688_SPAN_RANGE_M5V_5V,
+	LTC2688_SPAN_RANGE_MAX
+};
+
+struct ltc2688_chan {
+	int span_tbl[LTC2688_SPAN_RANGE_MAX][2];
+	bool overrange;
+	bool dither_toggle;
+	u8 offset_idx;
+};
+
+/*
+ * Helper tables to make sure the selected span is valid for the current offset
+ * and to translate the reg value to set for a specific span...
+ */
+static const int ltc2688_off_tbl[LTC2688_SPAN_RANGE_MAX] = {
+	[LTC2688_SPAN_RANGE_0V_5V] = 0,
+	[LTC2688_SPAN_RANGE_M10V_10V] = -32768,
+	[LTC2688_SPAN_RANGE_M15V_15V] = -32768,
+	[LTC2688_SPAN_RANGE_0V_10V] = 0,
+	[LTC2688_SPAN_RANGE_M5V_5V] = -32768
+};
+
+static const int ltc2688_reg_val[LTC2688_SPAN_RANGE_MAX] = {
+	[LTC2688_SPAN_RANGE_0V_5V] = 0,
+	[LTC2688_SPAN_RANGE_M10V_10V] = 3,
+	[LTC2688_SPAN_RANGE_M15V_15V] = 4,
+	[LTC2688_SPAN_RANGE_0V_10V] = 1,
+	[LTC2688_SPAN_RANGE_M5V_5V] = 2
+};
+
+struct ltc2688_state {
+	struct spi_device *spi;
+	struct regmap *regmap;
+	struct ltc2688_chan channels[LTC2688_DAC_CHANNELS];
+	int vref;
+	u8 tx_data[6] ____cacheline_aligned;
+	u8 rx_data[3];
+};
+
+static int ltc2688_spi_read(void *context, unsigned int cmd, unsigned int *val)
+{
+	struct ltc2688_state *st = context;
+	struct spi_transfer xfers[] = {
+		{
+			.tx_buf = st->tx_data,
+			.bits_per_word = 8,
+			.len = 3,
+			.cs_change = 1,
+		}, {
+			.tx_buf = st->tx_data + 3,
+			.rx_buf = st->rx_data,
+			.bits_per_word = 8,
+			.len = 3,
+		},
+	};
+	int ret;
+
+	st->tx_data[0] = cmd | LTC2688_READ_OPERATION;
+	st->tx_data[3] = LTC2688_CMD_NOOP;
+
+	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+	if (ret)
+		return ret;
+
+	/* first byte is to be ignored */
+	*val = get_unaligned_be16(&st->rx_data[1]);
+
+	return 0;
+}
+
+static int ltc2688_spi_write(void *context, unsigned int cmd, unsigned int val)
+{
+	struct ltc2688_state *st = context;
+
+	st->tx_data[0] = cmd;
+	put_unaligned_be16(val, &st->tx_data[1]);
+
+	return spi_write(st->spi, st->tx_data, 3);
+}
+
+static int ltc2688_span_get(const struct ltc2688_state *st, int c, int *val2)
+{
+	const struct ltc2688_chan *chan = &st->channels[c];
+	int ret, reg, span;
+	u32 i;
+
+	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;
+
+	/* look for correct span */
+	for (i = 0; i < ARRAY_SIZE(ltc2688_reg_val); i++)
+		if (span == ltc2688_reg_val[i])
+			break;
+
+	*val2 = chan->span_tbl[i][1];
+
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static const int ltc2688_offset_avail[] = { -32768, 0 };
+
+static int ltc2688_span_set(const struct ltc2688_state *st, int c, int span)
+{
+	const struct ltc2688_chan *chan = &st->channels[c];
+	u32 i = ARRAY_SIZE(chan->span_tbl);
+	int off = ltc2688_offset_avail[chan->offset_idx];
+
+	while (i--) {
+		if (span == chan->span_tbl[i][1]) {
+			/*
+			 * The selected scale needs to fit  the current offset.
+			 * Note that for [0V 10V] and [-5V 5V], as the scale is
+			 * the same, we will always succeed here. Hence if
+			 * someone really wants [0 10V] and does not set the
+			 * offset to 0, then :man-shrugging:
+			 */
+			if (off == ltc2688_off_tbl[i]) {
+				break;
+			} else if (i < LTC2688_SPAN_RANGE_0V_10V) {
+				/*
+				 * At this point, only one offset is valid so we
+				 * already can assume error.
+				 */
+				dev_err(&st->spi->dev, "Offset(%d) not valid for current scale(0.%09d)\n",
+					off, span);
+				return -EPERM;
+			}
+
+			continue;
+		}
+
+		if (!i) {
+			dev_err(&st->spi->dev, "span(%d) not available\n", span);
+			return -EINVAL;
+		}
+	}
+
+	return regmap_update_bits(st->regmap, LTC2688_CMD_CH_SETTING(c),
+				  LTC2688_CH_SPAN_MSK,
+				  FIELD_PREP(LTC2688_CH_SPAN_MSK, ltc2688_reg_val[i]));
+};
+
+static int ltc2688_offset_set(struct ltc2688_state *st, int c, int off)
+{
+	struct ltc2688_chan *chan = &st->channels[c];
+
+	if (off != ltc2688_offset_avail[0] && off != ltc2688_offset_avail[1])
+		return -EINVAL;
+
+	if (off == ltc2688_offset_avail[0])
+		chan->offset_idx = 0;
+	else
+		chan->offset_idx = 1;
+
+	return 0;
+}
+
+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)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+	int c = chan->channel;
+
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (const int *)st->channels[c].span_tbl;
+		*length = ARRAY_SIZE(st->channels[c].span_tbl) * 2 - 2;
+		*type = IIO_VAL_INT_PLUS_NANO;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_OFFSET:
+		*vals = ltc2688_offset_avail;
+		*length = ARRAY_SIZE(ltc2688_offset_avail);
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+	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;
+	u8 offset;
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_read(st->regmap,
+				  LTC2688_CMD_CH_CODE(chan->address), val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OFFSET:
+		offset = st->channels[chan->channel].offset_idx;
+		*val = ltc2688_offset_avail[offset];
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 0;
+		return ltc2688_span_get(st, chan->channel, val2);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		ret = regmap_read(st->regmap,
+				  LTC2688_CMD_CH_OFFSET(chan->channel), val);
+		if (ret)
+			return ret;
+
+		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_get_fmt(struct iio_dev *indio_dev,
+				     struct iio_chan_spec const *chan,
+				     long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+	case IIO_CHAN_INFO_CALIBSCALE:
+	case IIO_CHAN_INFO_CALIBBIAS:
+	case IIO_CHAN_INFO_OFFSET:
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	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);
+	int cmd;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val > U16_MAX || val < 0)
+			return -EINVAL;
+		/*
+		 * If in dither/toggle mode the dac should be updated by an
+		 * external signal and not here
+		 */
+		if (st->channels[chan->channel].dither_toggle)
+			cmd = LTC2688_CMD_CH_CODE(chan->channel);
+		else
+			cmd = LTC2688_CMD_CH_CODE_UPDATE(chan->channel);
+
+		return regmap_write(st->regmap, cmd, val);
+	case IIO_CHAN_INFO_SCALE:
+		if (val)
+			return -EINVAL;
+
+		return ltc2688_span_set(st, chan->channel, val2);
+	case IIO_CHAN_INFO_OFFSET:
+		return ltc2688_offset_set(st, chan->channel, val);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		return regmap_write(st->regmap,
+				    LTC2688_CMD_CH_OFFSET(chan->channel), val);
+	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,
+};
+
+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);
+	int ret, regval;
+	u32 reg;
+
+	switch (private) {
+	case LTC2688_DITHER_TOGGLE_ENABLE:
+		reg = LTC2688_CMD_TOGGLE_DITHER_EN_REG;
+		break;
+	case LTC2688_POWERDOWN:
+		reg = LTC2688_CMD_POWERDOWN_REG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_read(st->regmap, reg, &regval);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%u\n", !!(regval & BIT(chan->channel)));
+}
+
+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);
+	bool en;
+	int ret;
+	u32 val = 0, reg;
+
+	ret = strtobool(buf, &en);
+	if (ret)
+		return ret;
+
+	switch (private) {
+	case LTC2688_DITHER_TOGGLE_ENABLE:
+		reg = LTC2688_CMD_TOGGLE_DITHER_EN_REG;
+		break;
+	case LTC2688_POWERDOWN:
+		reg = LTC2688_CMD_POWERDOWN_REG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (en)
+		val = BIT(chan->channel);
+
+	ret = regmap_update_bits(st->regmap, reg, BIT(chan->channel), val);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static int ltc2688_get_dither_period(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_PER_MSK, regval);
+}
+
+static int ltc2688_set_dither_period(struct iio_dev *dev,
+				     const struct iio_chan_spec *chan,
+				     unsigned int period)
+{
+	struct ltc2688_state *st = iio_priv(dev);
+
+	return regmap_update_bits(st->regmap,
+				  LTC2688_CMD_CH_SETTING(chan->channel),
+				  LTC2688_CH_DIT_PER_MSK,
+				  FIELD_PREP(LTC2688_CH_DIT_PER_MSK, period));
+}
+
+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_get_reg_select(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_A_B_SELECT_REG, &regval);
+	if (ret < 0)
+		return ret;
+
+	return !!(regval & BIT(chan->channel));
+}
+
+static int ltc2688_set_reg_select(struct iio_dev *dev,
+				  const struct iio_chan_spec *chan,
+				  unsigned int reg)
+{
+	struct ltc2688_state *st = iio_priv(dev);
+
+	return regmap_update_bits(st->regmap, LTC2688_CMD_A_B_SELECT_REG,
+				  BIT(chan->channel), reg << chan->channel);
+}
+
+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_input_register[] = {
+	"input_a",
+	"input_b",
+};
+
+static const struct iio_enum ltc2688_input_reg_enum = {
+	.items = ltc2688_input_register,
+	.num_items = ARRAY_SIZE(ltc2688_input_register),
+	.set = ltc2688_set_reg_select,
+	.get = ltc2688_get_reg_select,
+};
+
+static const char * const ltc2688_dither_period[] = {
+	"4",
+	"8",
+	"16",
+	"32",
+	"64",
+};
+
+static const struct iio_enum ltc2688_dither_period_enum = {
+	.items = ltc2688_dither_period,
+	.num_items = ARRAY_SIZE(ltc2688_dither_period),
+	.set = ltc2688_set_dither_period,
+	.get = ltc2688_get_dither_period,
+};
+
+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,				\
+}
+
+#define IIO_ENUM_AVAILABLE_SHARED(_name, _shared, _e) {	\
+	.name = (_name "_available"),			\
+	.shared = _shared,				\
+	.read = iio_enum_available_read,		\
+	.private = (uintptr_t)(_e),			\
+}
+
+static const struct iio_chan_spec_ext_info ltc2688_dither_ext_info[] = {
+	IIO_ENUM("input_register", IIO_SEPARATE, &ltc2688_input_reg_enum),
+	IIO_ENUM_AVAILABLE_SHARED("input_register",
+				  IIO_SHARED_BY_TYPE, &ltc2688_input_reg_enum),
+	IIO_ENUM("dither_period", IIO_SEPARATE, &ltc2688_dither_period_enum),
+	IIO_ENUM_AVAILABLE_SHARED("dither_period",
+				  IIO_SHARED_BY_TYPE, &ltc2688_dither_period_enum),
+	IIO_ENUM("dither_phase", IIO_SEPARATE, &ltc2688_dither_phase_enum),
+	IIO_ENUM_AVAILABLE_SHARED("dither_phase",
+				  IIO_SHARED_BY_TYPE, &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_toggle_ext_info[] = {
+	IIO_ENUM("input_register", IIO_SEPARATE, &ltc2688_input_reg_enum),
+	IIO_ENUM_AVAILABLE_SHARED("input_register",
+				  IIO_SHARED_BY_TYPE, &ltc2688_input_reg_enum),
+	LTC2688_CHAN_EXT_INFO("toggle_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_default_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_SCALE),	\
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OFFSET),\
+	.ext_info = ltc2688_default_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_MODE_TOGGLE,
+	LTC2688_CHAN_MODE_DITHER
+};
+
+enum {
+	LTC2688_CHAN_TD_TGP1,
+	LTC2688_CHAN_TD_TGP2,
+	LTC2688_CHAN_TD_TGP3,
+	LTC2688_CHAN_TD_MAX
+};
+
+static const char * const ltc2688_clk_names[LTC2688_CHAN_TD_MAX] = {
+	"TGP1", "TGP2", "TGP3"
+};
+
+static void ltc2688_clk_disable(void *clk)
+{
+	clk_disable_unprepare(clk);
+}
+
+static int ltc2688_clk_setup(const struct ltc2688_state *st, unsigned long mask)
+{
+	u32 bit;
+	int ret;
+
+	for_each_set_bit(bit, &mask, ARRAY_SIZE(ltc2688_clk_names)) {
+		struct clk *clk;
+		/*
+		 * If a TGP pin is set, then we need to provide a valid clock at
+		 * the pin.
+		 */
+		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;
+	}
+
+	return 0;
+}
+
+static int ltc2688_channel_config(struct ltc2688_state *st)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(&st->spi->dev), *child;
+	u32 reg, clk_input, mode, val, mask;
+	unsigned long clk_msk = 0;
+	int ret;
+
+	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");
+		} else if (reg >= LTC2688_DAC_CHANNELS) {
+			fwnode_handle_put(child);
+			return dev_err_probe(&st->spi->dev, -EINVAL,
+					     "reg >= %d\n", LTC2688_DAC_CHANNELS);
+		}
+
+		chan = &st->channels[reg];
+		ret = fwnode_property_read_u32(child, "adi,mode", &mode);
+		if (!ret) {
+			if (mode > LTC2688_CHAN_MODE_DITHER) {
+				fwnode_handle_put(child);
+				return dev_err_probe(&st->spi->dev, -EINVAL,
+						     "chan mode inv value(%d)\n",
+						     mode);
+			}
+
+			chan->dither_toggle = true;
+			if (mode == LTC2688_CHAN_MODE_TOGGLE) {
+				ltc2688_channels[reg].ext_info = ltc2688_toggle_ext_info;
+			} else {
+				ltc2688_channels[reg].ext_info = ltc2688_dither_ext_info;
+				/* enable dither mode */
+				mask = LTC2688_CH_MODE_MSK;
+				val = BIT(11);
+			}
+		}
+
+		ret = fwnode_property_read_u32(child, "adi,toggle-dither-input",
+					       &clk_input);
+		if (!ret) {
+			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);
+			}
+
+			clk_msk |= BIT(clk_input);
+			mask |= LTC2688_CH_TD_SEL_MSK;
+			/*
+			 * 0 means software toggle which we are not supporting
+			 * for now. Hence the +1
+			 */
+			val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1);
+		} else if (chan->dither_toggle) {
+			/*
+			 * As sw_toggle is not supported, we need to make sure
+			 * a valid input is selected if toggle/dither mode is
+			 * requested
+			 */
+			return dev_err_probe(&st->spi->dev, -EINVAL,
+					     "toggle-dither set but no toggle-dither-input\n");
+		}
+
+		chan->overrange = fwnode_property_read_bool(child,
+							    "adi,overrange");
+		if (chan->overrange) {
+			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_clk_setup(st, clk_msk);
+}
+
+static const int ltc2688_span_helper[LTC2688_SPAN_RANGE_MAX][2] = {
+	{0, 5000}, {-10000, 10000}, {-15000, 15000}, {0, 10000}, {-5000, 5000},
+};
+
+static int ltc2688_get_full_scale(const struct ltc2688_chan *chan, int idx)
+{
+	int fs = ltc2688_span_helper[idx][1] - ltc2688_span_helper[idx][0];
+
+	if (chan->overrange)
+		fs = mult_frac(fs, 105, 100);
+
+	return fs;
+}
+
+static void ltc2688_span_avail_compute(struct ltc2688_state *st)
+{
+	u32 c, i;
+
+	for (c = 0; c < ARRAY_SIZE(st->channels); c++) {
+		struct ltc2688_chan *chan = &st->channels[c];
+
+		for (i = 0; i < ARRAY_SIZE(chan->span_tbl); i++) {
+			u32 fs, s;
+
+			fs = ltc2688_get_full_scale(chan, i);
+			/* we will return IIO_VAL_INT_PLUS_NANO */
+			s = DIV_ROUND_CLOSEST_ULL(st->vref * fs * 1000000000ULL,
+						  4096 * 1 << 16);
+
+			chan->span_tbl[i][1] = s;
+			/* default offset is 0 */
+			chan->offset_idx = 1;
+		}
+	}
+}
+
+static int ltc2688_setup(struct ltc2688_state *st, struct regulator *vref)
+{
+	struct gpio_desc *gpio;
+	int ret;
+
+	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_REG,
+					 LTC2688_CONFIG_RST,
+					 LTC2688_CONFIG_RST);
+		if (ret < 0)
+			return ret;
+	}
+
+	usleep_range(10000, 12000);
+	ret = ltc2688_channel_config(st);
+	if (ret < 0)
+		return ret;
+
+	if (vref) {
+		ret = regmap_update_bits(st->regmap, LTC2688_CMD_CONFIG_REG,
+					 LTC2688_CONFIG_EXT_REF, BIT(1));
+		if (ret < 0)
+			return ret;
+	}
+
+	ltc2688_span_avail_compute(st);
+	return 0;
+}
+
+static void ltc2688_disable_regulator(void *regulator)
+{
+	regulator_disable(regulator);
+}
+
+static int ltc2688_regulator_supply_get(const struct ltc2688_state *st,
+					const char *name)
+{
+	struct regulator *reg;
+	int ret;
+
+	reg = devm_regulator_get(&st->spi->dev, name);
+	if (IS_ERR(reg))
+		return dev_err_probe(&st->spi->dev, PTR_ERR(reg),
+				     "Failed to get %s regulator\n", name);
+
+	ret = regulator_enable(reg);
+	if (ret)
+		return dev_err_probe(&st->spi->dev, ret,
+				     "Failed to enable %s regulator\n", name);
+
+	return devm_add_action_or_reset(&st->spi->dev,
+					ltc2688_disable_regulator, reg);
+}
+
+static bool ltc2688_reg_readable(struct device *dev, unsigned int cmd)
+{
+	switch (cmd | LTC2688_READ_OPERATION) {
+	case 0x80 ... 0xBF:
+		return true;
+	case 0xF0 ... 0xFE:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool ltc2688_reg_writable(struct device *dev, unsigned int cmd)
+{
+	if (cmd <= LTC2688_CMD_UPDATE_ALL)
+		return true;
+
+	return false;
+}
+
+static const struct regmap_config ltc2688_regmap_config = {
+	.reg_read = ltc2688_spi_read,
+	.reg_write = ltc2688_spi_write,
+	.readable_reg = ltc2688_reg_readable,
+	.writeable_reg = ltc2688_reg_writable,
+	/* ignoring the no op command */
+	.max_register = U16_MAX - 1
+};
+
+static const struct iio_info ltc2688_info = {
+	.write_raw = ltc2688_write_raw,
+	.read_raw = ltc2688_read_raw,
+	.write_raw_get_fmt = ltc2688_write_raw_get_fmt,
+	.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;
+
+	st->regmap = devm_regmap_init(&spi->dev, NULL, st, &ltc2688_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
+				     "Failed to init regmap");
+
+	ret = ltc2688_regulator_supply_get(st, "vcc");
+	if (ret)
+		return ret;
+
+	ret = ltc2688_regulator_supply_get(st, "iovcc");
+	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.33.1

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

[Andy Shevchenko]

On Thu, Nov 11, 2021 at 1:01 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> precission reference. It is guaranteed monotonic and has built in

precision

> rail-to-rail output buffers that can source or sink up to 20 mA.

It's in good shape, several comments below, though.

...

> +enum {
> +       LTC2688_SPAN_RANGE_0V_5V,
> +       LTC2688_SPAN_RANGE_M10V_10V,
> +       LTC2688_SPAN_RANGE_M15V_15V,
> +       /*
> +        * These 2 were deliberately re-arranged to be the last items as they
> +        * have the same fs and we will just return 1 in read_available.

WTH "fs" mean?

> +        */
> +       LTC2688_SPAN_RANGE_0V_10V,
> +       LTC2688_SPAN_RANGE_M5V_5V,
> +       LTC2688_SPAN_RANGE_MAX
> +};

...

> +static const int ltc2688_off_tbl[LTC2688_SPAN_RANGE_MAX] = {
> +       [LTC2688_SPAN_RANGE_0V_5V] = 0,
> +       [LTC2688_SPAN_RANGE_M10V_10V] = -32768,
> +       [LTC2688_SPAN_RANGE_M15V_15V] = -32768,

> +       [LTC2688_SPAN_RANGE_0V_10V] = 0,
> +       [LTC2688_SPAN_RANGE_M5V_5V] = -32768

+ Comma

Isn't it more natural to move them up by two lines?

> +};
> +
> +static const int ltc2688_reg_val[LTC2688_SPAN_RANGE_MAX] = {
> +       [LTC2688_SPAN_RANGE_0V_5V] = 0,
> +       [LTC2688_SPAN_RANGE_M10V_10V] = 3,
> +       [LTC2688_SPAN_RANGE_M15V_15V] = 4,
> +       [LTC2688_SPAN_RANGE_0V_10V] = 1,
> +       [LTC2688_SPAN_RANGE_M5V_5V] = 2

As per above.

> +};

...

> +static int ltc2688_span_set(const struct ltc2688_state *st, int c, int span)
> +{
> +       const struct ltc2688_chan *chan = &st->channels[c];
> +       u32 i = ARRAY_SIZE(chan->span_tbl);
> +       int off = ltc2688_offset_avail[chan->offset_idx];

> +       while (i--) {

Can ARRAY_SIZE() give 0?
If not, do {} while (--i); looks more natural.

> +               if (span == chan->span_tbl[i][1]) {
> +                       /*
> +                        * The selected scale needs to fit  the current offset.
> +                        * Note that for [0V 10V] and [-5V 5V], as the scale is
> +                        * the same, we will always succeed here. Hence if
> +                        * someone really wants [0 10V] and does not set the
> +                        * offset to 0, then :man-shrugging:
> +                        */
> +                       if (off == ltc2688_off_tbl[i]) {
> +                               break;

> +                       } else if (i < LTC2688_SPAN_RANGE_0V_10V) {

Redundant 'else'.

> +                               /*
> +                                * At this point, only one offset is valid so we
> +                                * already can assume error.
> +                                */
> +                               dev_err(&st->spi->dev, "Offset(%d) not valid for current scale(0.%09d)\n",
> +                                       off, span);
> +                               return -EPERM;
> +                       }

> +                       continue;

Redundant, see below.

> +               }

> +               if (!i) {
> +                       dev_err(&st->spi->dev, "span(%d) not available\n", span);
> +                       return -EINVAL;
> +               }

This is invariant to the loop.

> +       }
> +
> +       return regmap_update_bits(st->regmap, LTC2688_CMD_CH_SETTING(c),
> +                                 LTC2688_CH_SPAN_MSK,
> +                                 FIELD_PREP(LTC2688_CH_SPAN_MSK, ltc2688_reg_val[i]));
> +};

...

> +       if (off != ltc2688_offset_avail[0] && off != ltc2688_offset_avail[1])
> +               return -EINVAL;

Extra condition, See below.

> +       if (off == ltc2688_offset_avail[0])
> +               chan->offset_idx = 0;
> +       else

else if

> +               chan->offset_idx = 1;

else
  return -EINVAL;

...

> +       ret = regmap_read(st->regmap, reg, &regval);
> +       if (ret < 0)

Is it the first time you tested explicitly for a negative result?

> +               return ret;

...

> +       return sprintf(buf, "%u\n", !!(regval & BIT(chan->channel)));

sysfs_emit() ?

> +}

...

> +       struct ltc2688_state *st = iio_priv(indio_dev);
> +       bool en;
> +       int ret;
> +       u32 val = 0, reg;

Reversed xmas tree order ?

> +       ret = strtobool(buf, &en);

kstrtobool()

...

> +       if (ret)
> +               return ret;
> +               break;

What does this mean?

...

> +       "4",
> +       "8",
> +       "16",
> +       "32",
> +       "64",

One line?

...

> +       "0",
> +       "90",
> +       "180",
> +       "270",

Ditto.

...

> +enum {
> +       LTC2688_CHAN_MODE_TOGGLE,
> +       LTC2688_CHAN_MODE_DITHER

+ Comma.

> +};

...

> +static const char * const ltc2688_clk_names[LTC2688_CHAN_TD_MAX] = {
> +       "TGP1", "TGP2", "TGP3"

In this notation, + comma.

> +};

...

> +       for_each_set_bit(bit, &mask, ARRAY_SIZE(ltc2688_clk_names)) {
> +               struct clk *clk;

+ blank line

> +               /*
> +                * If a TGP pin is set, then we need to provide a valid clock at
> +                * the pin.
> +                */

> +       }

...

> +       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");

One line.

> +               } else if (reg >= LTC2688_DAC_CHANNELS) {

Redundant 'else'.

> +                       fwnode_handle_put(child);
> +                       return dev_err_probe(&st->spi->dev, -EINVAL,

> +                                            "reg >= %d\n", LTC2688_DAC_CHANNELS);

The message is cryptic. Decrypt it for the user.

> +               }
> +
> +               chan = &st->channels[reg];
> +               ret = fwnode_property_read_u32(child, "adi,mode", &mode);
> +               if (!ret) {
> +                       if (mode > LTC2688_CHAN_MODE_DITHER) {
> +                               fwnode_handle_put(child);
> +                               return dev_err_probe(&st->spi->dev, -EINVAL,
> +                                                    "chan mode inv value(%d)\n",
> +                                                    mode);
> +                       }
> +
> +                       chan->dither_toggle = true;
> +                       if (mode == LTC2688_CHAN_MODE_TOGGLE) {
> +                               ltc2688_channels[reg].ext_info = ltc2688_toggle_ext_info;
> +                       } else {
> +                               ltc2688_channels[reg].ext_info = ltc2688_dither_ext_info;
> +                               /* enable dither mode */
> +                               mask = LTC2688_CH_MODE_MSK;
> +                               val = BIT(11);
> +                       }
> +               }
> +
> +               ret = fwnode_property_read_u32(child, "adi,toggle-dither-input",
> +                                              &clk_input);

> +               if (!ret) {

if (ret) {
  if ...
} else {
 ...
}

> +                       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);
> +                       }
> +
> +                       clk_msk |= BIT(clk_input);
> +                       mask |= LTC2688_CH_TD_SEL_MSK;
> +                       /*
> +                        * 0 means software toggle which we are not supporting
> +                        * for now. Hence the +1

In all multi-line comments miseed (grammatical) period.

> +                        */
> +                       val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1);
> +               } else if (chan->dither_toggle) {
> +                       /*
> +                        * As sw_toggle is not supported, we need to make sure
> +                        * a valid input is selected if toggle/dither mode is
> +                        * requested
> +                        */
> +                       return dev_err_probe(&st->spi->dev, -EINVAL,
> +                                            "toggle-dither set but no toggle-dither-input\n");
> +               }

> +               chan->overrange = fwnode_property_read_bool(child,
> +                                                           "adi,overrange");

One line?

> +               if (chan->overrange) {
> +                       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;
> +       }

...

> +       int fs = ltc2688_span_helper[idx][1] - ltc2688_span_helper[idx][0];
> +
> +       if (chan->overrange)
> +               fs = mult_frac(fs, 105, 100);
> +
> +       return fs;

if (...)
 return ...
?

...

> +       u32 c, i;

In one case you use int or unsigned int if I'm not mistaken, here out
of a sudden u32. Why? Please, be consistent over the code.

...

> +                       /* we will return IIO_VAL_INT_PLUS_NANO */
> +                       s = DIV_ROUND_CLOSEST_ULL(st->vref * fs * 1000000000ULL,
> +                                                 4096 * 1 << 16);

Can you make use of one of the SI prefixes here? (See unit.h)

...

> +       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");

> +

Redundant blank line.

> +       if (gpio) {
> +               usleep_range(1000, 1200);
> +               /* bring device out of reset */
> +               gpiod_set_value_cansleep(gpio, 0);

> +       } else {

I'm wondering why 'else'? Can't it be both? Why not?

> +               ret = regmap_update_bits(st->regmap, LTC2688_CMD_CONFIG_REG,
> +                                        LTC2688_CONFIG_RST,
> +                                        LTC2688_CONFIG_RST);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
> +       usleep_range(10000, 12000);

+ blank line, so the reader won't be confused about the function of this sleep.

> +       ret = ltc2688_channel_config(st);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (vref) {
> +               ret = regmap_update_bits(st->regmap, LTC2688_CMD_CONFIG_REG,
> +                                        LTC2688_CONFIG_EXT_REF, BIT(1));
> +               if (ret < 0)

These ' < 0' parts are inconsistent over the code.

> +                       return ret;
> +       }
> +
> +       ltc2688_span_avail_compute(st);
> +       return 0;
> +}

...

> +       /* ignoring the no op command */
> +       .max_register = U16_MAX - 1

Really?! Have you ever considered what burden you bring with this to
one who accidentally or on purpose tries to dump registers via
debugfs?

...

> +       st->regmap = devm_regmap_init(&spi->dev, NULL, st, &ltc2688_regmap_config);

I'm wondering why it's not a regmap SPI?

> +       if (IS_ERR(st->regmap))
> +               return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> +                                    "Failed to init regmap");

...

> +       ret = ltc2688_regulator_supply_get(st, "vcc");
> +       if (ret)
> +               return ret;
> +
> +       ret = ltc2688_regulator_supply_get(st, "iovcc");
> +       if (ret)
> +               return ret;

Can bulk regulators be used?

...

> +               st->vref = ret / 1000;

Make use of unit.h

--
With Best Regards,
Andy Shevchenko

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

[Sa, Nuno]

Hi Andy,

Thanks for the preliminary review...

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Thursday, November 11, 2021 2:49 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>
> Subject: Re: [RFC PATCH 1/1] iio: dac: add support for ltc2688
> 
> [External]
> 
> On Thu, Nov 11, 2021 at 1:01 PM Nuno Sá <nuno.sa@analog.com>
> wrote:
> >
> > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> > precission reference. It is guaranteed monotonic and has built in
> 
> precision
> 
> > rail-to-rail output buffers that can source or sink up to 20 mA.
> 
> It's in good shape, several comments below, though.
> 
> ...
> 
> > +enum {
> > +       LTC2688_SPAN_RANGE_0V_5V,
> > +       LTC2688_SPAN_RANGE_M10V_10V,
> > +       LTC2688_SPAN_RANGE_M15V_15V,
> > +       /*
> > +        * These 2 were deliberately re-arranged to be the last items as
> they
> > +        * have the same fs and we will just return 1 in read_available.
> 
> WTH "fs" mean?

full scale... will spell it out

> > +        */
> > +       LTC2688_SPAN_RANGE_0V_10V,
> > +       LTC2688_SPAN_RANGE_M5V_5V,
> > +       LTC2688_SPAN_RANGE_MAX
> > +};
> 
> ...
> 
> > +static const int ltc2688_off_tbl[LTC2688_SPAN_RANGE_MAX] = {
> > +       [LTC2688_SPAN_RANGE_0V_5V] = 0,
> > +       [LTC2688_SPAN_RANGE_M10V_10V] = -32768,
> > +       [LTC2688_SPAN_RANGE_M15V_15V] = -32768,
> 
> > +       [LTC2688_SPAN_RANGE_0V_10V] = 0,
> > +       [LTC2688_SPAN_RANGE_M5V_5V] = -32768
> 
> + Comma
> 
> Isn't it more natural to move them up by two lines?

There's a reason for this to be ordered like this. Anyways,
as I said in the cover letter I will probably remove all of these things
used to validate scale + offset pairs. I think is a bit overdone...

> > +};
> > +
> > +static const int ltc2688_reg_val[LTC2688_SPAN_RANGE_MAX] = {
> > +       [LTC2688_SPAN_RANGE_0V_5V] = 0,
> > +       [LTC2688_SPAN_RANGE_M10V_10V] = 3,
> > +       [LTC2688_SPAN_RANGE_M15V_15V] = 4,
> > +       [LTC2688_SPAN_RANGE_0V_10V] = 1,
> > +       [LTC2688_SPAN_RANGE_M5V_5V] = 2
> 
> As per above.
> 
> > +};
> 
> ...
> 
> > +static int ltc2688_span_set(const struct ltc2688_state *st, int c, int
> span)
> > +{
> > +       const struct ltc2688_chan *chan = &st->channels[c];
> > +       u32 i = ARRAY_SIZE(chan->span_tbl);
> > +       int off = ltc2688_offset_avail[chan->offset_idx];
> 
> > +       while (i--) {
> 
> Can ARRAY_SIZE() give 0?

nope 

> If not, do {} while (--i); looks more natural.

can do like that

> > +               if (span == chan->span_tbl[i][1]) {
> > +                       /*
> > +                        * The selected scale needs to fit  the current offset.
> > +                        * Note that for [0V 10V] and [-5V 5V], as the scale is
> > +                        * the same, we will always succeed here. Hence if
> > +                        * someone really wants [0 10V] and does not set the
> > +                        * offset to 0, then :man-shrugging:
> > +                        */
> > +                       if (off == ltc2688_off_tbl[i]) {
> > +                               break;
> 
> > +                       } else if (i < LTC2688_SPAN_RANGE_0V_10V) {
> 
> Redundant 'else'.

Why? Note that I really (where possible) want to make a distinction
between the errors... Again, I will probably remove this part and
only match 'if (span == chan->span_tbl[i][1])'

> > +                               /*
> > +                                * At this point, only one offset is valid so we
> > +                                * already can assume error.
> > +                                */
> > +                               dev_err(&st->spi->dev, "Offset(%d) not valid for
> current scale(0.%09d)\n",
> > +                                       off, span);
> > +                               return -EPERM;
> > +                       }
> 
> > +                       continue;
> 
> Redundant, see below.

true, but at this point I already know that doing the next if (!i) is not really
needed. But yeah, less lines of code 
> 
> > +               }
> 
> > +               if (!i) {
> > +                       dev_err(&st->spi->dev, "span(%d) not available\n",
> span);
> > +                       return -EINVAL;
> > +               }
> 
> This is invariant to the loop.
> 
> > +       }
> > +
> > +       return regmap_update_bits(st->regmap,
> LTC2688_CMD_CH_SETTING(c),
> > +                                 LTC2688_CH_SPAN_MSK,
> > +                                 FIELD_PREP(LTC2688_CH_SPAN_MSK,
> ltc2688_reg_val[i]));
> > +};
> 
> ...
> 
> > +       if (off != ltc2688_offset_avail[0] && off !=
> ltc2688_offset_avail[1])
> > +               return -EINVAL;
> 
> Extra condition, See below.
> 
> > +       if (off == ltc2688_offset_avail[0])
> > +               chan->offset_idx = 0;
> > +       else
> 
> else if
> 
> > +               chan->offset_idx = 1;
> 
> else
>   return -EINVAL;

sure...

> ...
> 
> > +       ret = regmap_read(st->regmap, reg, &regval);
> > +       if (ret < 0)
> 
> Is it the first time you tested explicitly for a negative result?

should not be like this...

> > +               return ret;
> 
> ...
> 
> > +       return sprintf(buf, "%u\n", !!(regval & BIT(chan->channel)));
> 
> sysfs_emit() ?

definetly...

> > +}
> 
> ...
> 
> > +       struct ltc2688_state *st = iio_priv(indio_dev);
> > +       bool en;
> > +       int ret;
> > +       u32 val = 0, reg;
> 
> Reversed xmas tree order ?

sure

> 
> > +       ret = strtobool(buf, &en);
> 
> kstrtobool()

sure

> 
> ...
> 
> > +       if (ret)
> > +               return ret;
> > +               break;
> 
> What does this mean?

means I was sleeping...

> ...
> 
> > +       "4",
> > +       "8",
> > +       "16",
> > +       "32",
> > +       "64",
> 
> One line?

yes

> 
> ...
> 
> > +       "0",
> > +       "90",
> > +       "180",
> > +       "270",
> 
> Ditto.
> 
> ...
> 
> > +enum {
> > +       LTC2688_CHAN_MODE_TOGGLE,
> > +       LTC2688_CHAN_MODE_DITHER
> 
> + Comma.
> 
> > +};
> 
> ...
> 
> > +static const char * const
> ltc2688_clk_names[LTC2688_CHAN_TD_MAX] = {
> > +       "TGP1", "TGP2", "TGP3"
> 
> In this notation, + comma.
> 
> > +};
> 
> ...
> 
> > +       for_each_set_bit(bit, &mask, ARRAY_SIZE(ltc2688_clk_names))
> {
> > +               struct clk *clk;
> 
> + blank line
> 
> > +               /*
> > +                * If a TGP pin is set, then we need to provide a valid clock at
> > +                * the pin.
> > +                */
> 
> > +       }
> 
> ...
> 
> > +       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");
> 
> One line.
> 
> > +               } else if (reg >= LTC2688_DAC_CHANNELS) {
> 
> Redundant 'else'.

Do you mean?

if (ret)
 ...

if (reg >= LTC2688_DAC_CHANNELS)
 ....

> > +                       fwnode_handle_put(child);
> > +                       return dev_err_probe(&st->spi->dev, -EINVAL,
> 
> > +                                            "reg >= %d\n", LTC2688_DAC_CHANNELS);
> 
> The message is cryptic. Decrypt it for the user.
> 
> > +               }
> > +
> > +               chan = &st->channels[reg];
> > +               ret = fwnode_property_read_u32(child, "adi,mode",
> &mode);
> > +               if (!ret) {
> > +                       if (mode > LTC2688_CHAN_MODE_DITHER) {
> > +                               fwnode_handle_put(child);
> > +                               return dev_err_probe(&st->spi->dev, -EINVAL,
> > +                                                    "chan mode inv value(%d)\n",
> > +                                                    mode);
> > +                       }
> > +
> > +                       chan->dither_toggle = true;
> > +                       if (mode == LTC2688_CHAN_MODE_TOGGLE) {
> > +                               ltc2688_channels[reg].ext_info =
> ltc2688_toggle_ext_info;
> > +                       } else {
> > +                               ltc2688_channels[reg].ext_info =
> ltc2688_dither_ext_info;
> > +                               /* enable dither mode */
> > +                               mask = LTC2688_CH_MODE_MSK;
> > +                               val = BIT(11);
> > +                       }
> > +               }
> > +
> > +               ret = fwnode_property_read_u32(child, "adi,toggle-dither-
> input",
> > +                                              &clk_input);
> 
> > +               if (!ret) {
> 
> if (ret) {
>   if ...
> } else {
>  ...
> }

sure

> > +                       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);
> > +                       }
> > +
> > +                       clk_msk |= BIT(clk_input);
> > +                       mask |= LTC2688_CH_TD_SEL_MSK;
> > +                       /*
> > +                        * 0 means software toggle which we are not supporting
> > +                        * for now. Hence the +1
> 
> In all multi-line comments miseed (grammatical) period.
> 
> > +                        */
> > +                       val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input
> + 1);
> > +               } else if (chan->dither_toggle) {
> > +                       /*
> > +                        * As sw_toggle is not supported, we need to make sure
> > +                        * a valid input is selected if toggle/dither mode is
> > +                        * requested
> > +                        */
> > +                       return dev_err_probe(&st->spi->dev, -EINVAL,
> > +                                            "toggle-dither set but no toggle-dither-
> input\n");
> > +               }
> 
> > +               chan->overrange = fwnode_property_read_bool(child,
> > +                                                           "adi,overrange");
> 
> One line?

It will pass the 80 col limit. AFAIR, Jonathan prefers to keep it when it
does not hurt readability...

> > +               if (chan->overrange) {
> > +                       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;
> > +       }
> 
> ...
> 
> > +       int fs = ltc2688_span_helper[idx][1] -
> ltc2688_span_helper[idx][0];
> > +
> > +       if (chan->overrange)
> > +               fs = mult_frac(fs, 105, 100);
> > +
> > +       return fs;
> 
> if (...)
>  return ...
> ?

Yes...

> ...
> 
> > +       u32 c, i;
> 
> In one case you use int or unsigned int if I'm not mistaken, here out
> of a sudden u32. Why? Please, be consistent over the code.

I will check that...

> ...
> 
> > +                       /* we will return IIO_VAL_INT_PLUS_NANO */
> > +                       s = DIV_ROUND_CLOSEST_ULL(st->vref * fs *
> 1000000000ULL,
> > +                                                 4096 * 1 << 16);
> 
> Can you make use of one of the SI prefixes here? (See unit.h)

Sure

> ...
> 
> > +       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");
> 
> > +
> 
> Redundant blank line.
> 
> > +       if (gpio) {
> > +               usleep_range(1000, 1200);
> > +               /* bring device out of reset */
> > +               gpiod_set_value_cansleep(gpio, 0);
> 
> > +       } else {
> 
> I'm wondering why 'else'? Can't it be both? Why not?

Well, if we have the reset pin, then we reset using it. If there's
no pin, we use the SW reset. It's a common pattern that we already use
for example in the ADIS devices.

> > +               ret = regmap_update_bits(st->regmap,
> LTC2688_CMD_CONFIG_REG,
> > +                                        LTC2688_CONFIG_RST,
> > +                                        LTC2688_CONFIG_RST);
> > +               if (ret < 0)
> > +                       return ret;
> > +       }
> > +
> > +       usleep_range(10000, 12000);
> 
> + blank line, so the reader won't be confused about the function of
> this sleep.
> 
> > +       ret = ltc2688_channel_config(st);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (vref) {
> > +               ret = regmap_update_bits(st->regmap,
> LTC2688_CMD_CONFIG_REG,
> > +                                        LTC2688_CONFIG_EXT_REF, BIT(1));
> > +               if (ret < 0)
> 
> These ' < 0' parts are inconsistent over the code.

Yes, I will fix that...

> > +                       return ret;
> > +       }
> > +
> > +       ltc2688_span_avail_compute(st);
> > +       return 0;
> > +}
> 
> ...
> 
> > +       /* ignoring the no op command */
> > +       .max_register = U16_MAX - 1
> 
> Really?! Have you ever considered what burden you bring with this to
> one who accidentally or on purpose tries to dump registers via
> debugfs?

Ups, this is completely wrong!! It should be U8_MAX...

> ...
> 
> > +       st->regmap = devm_regmap_init(&spi->dev, NULL, st,
> &ltc2688_regmap_config);
> 
> I'm wondering why it's not a regmap SPI?

The problem is on the read side... In the first transfer we write the command/register
to read, then we need to release the CS pin so that the device executes the command,
and only then we read the data. AFAIK, the regmap spi implementation won't work like
this. I think CS is kept asserted the whole time...

> > +       if (IS_ERR(st->regmap))
> > +               return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> > +                                    "Failed to init regmap");
> 
> ...
> 
> > +       ret = ltc2688_regulator_supply_get(st, "vcc");
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = ltc2688_regulator_supply_get(st, "iovcc");
> > +       if (ret)
> > +               return ret;
> 
> Can bulk regulators be used?

Hmm, I need to check but I think so... There's no ordering requirement here

> ...
> 
> > +               st->vref = ret / 1000;
> 
> Make use of unit.h
> 

- Nuno Sá

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

[Andy Shevchenko]

On Thu, Nov 11, 2021 at 4:30 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Thursday, November 11, 2021 2:49 PM
> > On Thu, Nov 11, 2021 at 1:01 PM Nuno Sá <nuno.sa@analog.com>
> > wrote:

...

> > > +static const int ltc2688_off_tbl[LTC2688_SPAN_RANGE_MAX] = {
> > > +       [LTC2688_SPAN_RANGE_0V_5V] = 0,
> > > +       [LTC2688_SPAN_RANGE_M10V_10V] = -32768,
> > > +       [LTC2688_SPAN_RANGE_M15V_15V] = -32768,
> >
> > > +       [LTC2688_SPAN_RANGE_0V_10V] = 0,
> > > +       [LTC2688_SPAN_RANGE_M5V_5V] = -32768
> >
> > + Comma
> >
> > Isn't it more natural to move them up by two lines?
>
> There's a reason for this to be ordered like this.

I understand that for enum, but here it doesn't make any sense to me
since you are addressing them by indices.

>  Anyways,
> as I said in the cover letter I will probably remove all of these things
> used to validate scale + offset pairs. I think is a bit overdone...

> > > +};

...

> > > +                       if (off == ltc2688_off_tbl[i]) {
> > > +                               break;
> >
> > > +                       } else if (i < LTC2688_SPAN_RANGE_0V_10V) {
> >
> > Redundant 'else'.
>
> Why? Note that I really (where possible) want to make a distinction
> between the errors...

while () {
  if ()
    ...
    break;
  } else {
    ...
  }
}

Isn't it obvious?

Same with

if ()
  return
else
  ...

> > > +                       }

...

> > > +               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");
> >
> > One line.
> >
> > > +               } else if (reg >= LTC2688_DAC_CHANNELS) {
> >
> > Redundant 'else'.
>
> Do you mean?
>
> if (ret)
>  ...
>
> if (reg >= LTC2688_DAC_CHANNELS)
>  ....

Yes.

> > > +               }

...

> > > +               chan->overrange = fwnode_property_read_bool(child,
> > > +                                                           "adi,overrange");
> >
> > One line?
>
> It will pass the 80 col limit. AFAIR, Jonathan prefers to keep it when it
> does not hurt readability...

I believe it will increase readability being located on one line.

...

> > > +       if (gpio) {
> > > +               usleep_range(1000, 1200);
> > > +               /* bring device out of reset */
> > > +               gpiod_set_value_cansleep(gpio, 0);
> >
> > > +       } else {
> >
> > I'm wondering why 'else'? Can't it be both? Why not?
>
> Well, if we have the reset pin, then we reset using it. If there's
> no pin, we use the SW reset. It's a common pattern that we already use
> for example in the ADIS devices.

Perhaps a comment in the code is needed.

> > > +               ret = regmap_update_bits(st->regmap,
> > LTC2688_CMD_CONFIG_REG,
> > > +                                        LTC2688_CONFIG_RST,
> > > +                                        LTC2688_CONFIG_RST);
> > > +               if (ret < 0)
> > > +                       return ret;
> > > +       }

...

> > > +       /* ignoring the no op command */
> > > +       .max_register = U16_MAX - 1
> >
> > Really?! Have you ever considered what burden you bring with this to
> > one who accidentally or on purpose tries to dump registers via
> > debugfs?
>
> Ups, this is completely wrong!! It should be U8_MAX...

Ah, that explains!

...

> > > +       st->regmap = devm_regmap_init(&spi->dev, NULL, st,
> > &ltc2688_regmap_config);
> >
> > I'm wondering why it's not a regmap SPI?
>
> The problem is on the read side... In the first transfer we write the command/register
> to read, then we need to release the CS pin so that the device executes the command,
> and only then we read the data. AFAIK, the regmap spi implementation won't work like
> this. I think CS is kept asserted the whole time...

I believe it's configurable, no? Like the cs_change flag somewhere.
Can you double check?

-- 
With Best Regards,
Andy Shevchenko

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

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Thursday, November 11, 2021 3:42 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>
> Subject: Re: [RFC PATCH 1/1] iio: dac: add support for ltc2688
> 
> [External]
> 
> On Thu, Nov 11, 2021 at 4:30 PM Sa, Nuno <Nuno.Sa@analog.com>
> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Thursday, November 11, 2021 2:49 PM
> > > On Thu, Nov 11, 2021 at 1:01 PM Nuno Sá <nuno.sa@analog.com>
> > > wrote:
> 
> ...
> 
> > > > +static const int ltc2688_off_tbl[LTC2688_SPAN_RANGE_MAX] =
> {
> > > > +       [LTC2688_SPAN_RANGE_0V_5V] = 0,
> > > > +       [LTC2688_SPAN_RANGE_M10V_10V] = -32768,
> > > > +       [LTC2688_SPAN_RANGE_M15V_15V] = -32768,
> > >
> > > > +       [LTC2688_SPAN_RANGE_0V_10V] = 0,
> > > > +       [LTC2688_SPAN_RANGE_M5V_5V] = -32768
> > >
> > > + Comma
> > >
> > > Isn't it more natural to move them up by two lines?
> >
> > There's a reason for this to be ordered like this.
> 
> I understand that for enum, but here it doesn't make any sense to me
> since you are addressing them by indices.

Yeah, right... If we keep this, I will do as you suggested

> >  Anyways,
> > as I said in the cover letter I will probably remove all of these things
> > used to validate scale + offset pairs. I think is a bit overdone...
> 
> > > > +};
> 
> ...
> 
> > > > +                       if (off == ltc2688_off_tbl[i]) {
> > > > +                               break;
> > >
> > > > +                       } else if (i < LTC2688_SPAN_RANGE_0V_10V) {
> > >
> > > Redundant 'else'.
> >
> > Why? Note that I really (where possible) want to make a distinction
> > between the errors...
> 
> while () {
>   if ()
>     ...
>     break;
>   } else {
>     ...
>   }
> }
> 
> Isn't it obvious?

Ah ok, I see know what you mean (and yes, it is obvious; for my shame)...

if ()
   break

if ()
   return -EPERM

As we do the break, the else becomes irrelevant 
> Same with
> 
> if ()
>   return
> else
>   ...

yeah yeah... 

> > > > +                       }
> 
> ...
> 
> > > > +               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");
> > >
> > > One line.
> > >
> > > > +               } else if (reg >= LTC2688_DAC_CHANNELS) {
> > >
> > > Redundant 'else'.
> >
> > Do you mean?
> >
> > if (ret)
> >  ...
> >
> > if (reg >= LTC2688_DAC_CHANNELS)
> >  ....
> 
> Yes.
> 
> > > > +               }
> 
> ...
> 
> > > > +               chan->overrange = fwnode_property_read_bool(child,
> > > > +                                                           "adi,overrange");
> > >
> > > One line?
> >
> > It will pass the 80 col limit. AFAIR, Jonathan prefers to keep it when it
> > does not hurt readability...
> 
> I believe it will increase readability being located on one line.

I mean, this is perfectly aligned with the open "(", so it's a pretty
normal pattern. Anyways, I'm more than happy to move this into a one
liner and just use the 100 limit. But let's see what Jonathan has to say
because I do not want to move back and forward...

> ...
> 
> > > > +       if (gpio) {
> > > > +               usleep_range(1000, 1200);
> > > > +               /* bring device out of reset */
> > > > +               gpiod_set_value_cansleep(gpio, 0);
> > >
> > > > +       } else {
> > >
> > > I'm wondering why 'else'? Can't it be both? Why not?
> >
> > Well, if we have the reset pin, then we reset using it. If there's
> > no pin, we use the SW reset. It's a common pattern that we already
> use
> > for example in the ADIS devices.
> 
> Perhaps a comment in the code is needed.

can do that...

> > > > +               ret = regmap_update_bits(st->regmap,
> > > LTC2688_CMD_CONFIG_REG,
> > > > +                                        LTC2688_CONFIG_RST,
> > > > +                                        LTC2688_CONFIG_RST);
> > > > +               if (ret < 0)
> > > > +                       return ret;
> > > > +       }
> 
> ...
> 
> > > > +       /* ignoring the no op command */
> > > > +       .max_register = U16_MAX - 1
> > >
> > > Really?! Have you ever considered what burden you bring with this
> to
> > > one who accidentally or on purpose tries to dump registers via
> > > debugfs?
> >
> > Ups, this is completely wrong!! It should be U8_MAX...
> 
> Ah, that explains!
> 
> ...
> 
> > > > +       st->regmap = devm_regmap_init(&spi->dev, NULL, st,
> > > &ltc2688_regmap_config);
> > >
> > > I'm wondering why it's not a regmap SPI?
> >
> > The problem is on the read side... In the first transfer we write the
> command/register
> > to read, then we need to release the CS pin so that the device
> executes the command,
> > and only then we read the data. AFAIK, the regmap spi
> implementation won't work like
> > this. I think CS is kept asserted the whole time...
> 
> I believe it's configurable, no? Like the cs_change flag somewhere.
> Can you double check?

Don't think we can... The read part just calls:
https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-spi.c#L98
and has no control over the spi transfer bits...

- Nuno Sá

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

[Jonathan Cameron]

> >   
> > > > > +               chan->overrange = fwnode_property_read_bool(child,
> > > > > +                                                           "adi,overrange");  
> > > >
> > > > One line?  
> > >
> > > It will pass the 80 col limit. AFAIR, Jonathan prefers to keep it when it
> > > does not hurt readability...  
> > 
> > I believe it will increase readability being located on one line.  
> 
> I mean, this is perfectly aligned with the open "(", so it's a pretty
> normal pattern. Anyways, I'm more than happy to move this into a one
> liner and just use the 100 limit. But let's see what Jonathan has to say
> because I do not want to move back and forward...

Here it happens to be particularly ugly because of the short first parameter,
so I'm fine with a longer line for this one.


> > > > > +       st->regmap = devm_regmap_init(&spi->dev, NULL, st,  
> > > > &ltc2688_regmap_config);
> > > >
> > > > I'm wondering why it's not a regmap SPI?  
> > >
> > > The problem is on the read side... In the first transfer we write the  
> > command/register  
> > > to read, then we need to release the CS pin so that the device  
> > executes the command,  
> > > and only then we read the data. AFAIK, the regmap spi  
> > implementation won't work like  
> > > this. I think CS is kept asserted the whole time...  
> > 
> > I believe it's configurable, no? Like the cs_change flag somewhere.
> > Can you double check?  
> 
> Don't think we can... The read part just calls:
> https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-spi.c#L98
> and has no control over the spi transfer bits...

Feature to add then or a custom regmap_bus if you want to keep it in the driver.

J
> 
> - Nuno Sá

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

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Friday, November 12, 2021 4:23 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; linux-iio
> <linux-iio@vger.kernel.org>
> Subject: Re: [RFC PATCH 1/1] iio: dac: add support for ltc2688
> 
> [External]
> 
> 
> > >
> > > > > > +               chan->overrange =
> fwnode_property_read_bool(child,
> > > > > > +                                                           "adi,overrange");
> > > > >
> > > > > One line?
> > > >
> > > > It will pass the 80 col limit. AFAIR, Jonathan prefers to keep it
> when it
> > > > does not hurt readability...
> > >
> > > I believe it will increase readability being located on one line.
> >
> > I mean, this is perfectly aligned with the open "(", so it's a pretty
> > normal pattern. Anyways, I'm more than happy to move this into a
> one
> > liner and just use the 100 limit. But let's see what Jonathan has to say
> > because I do not want to move back and forward...
> 
> Here it happens to be particularly ugly because of the short first
> parameter,
> so I'm fine with a longer line for this one.
> 

Ok then...

> 
> > > > > > +       st->regmap = devm_regmap_init(&spi->dev, NULL, st,
> > > > > &ltc2688_regmap_config);
> > > > >
> > > > > I'm wondering why it's not a regmap SPI?
> > > >
> > > > The problem is on the read side... In the first transfer we write
> the
> > > command/register
> > > > to read, then we need to release the CS pin so that the device
> > > executes the command,
> > > > and only then we read the data. AFAIK, the regmap spi
> > > implementation won't work like
> > > > this. I think CS is kept asserted the whole time...
> > >
> > > I believe it's configurable, no? Like the cs_change flag somewhere.
> > > Can you double check?
> >
> > Don't think we can... The read part just calls:
> >
> https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/s
> ource/drivers/base/regmap/regmap-
> spi.c*L98__;Iw!!A3Ni8CS0y2Y!orMauLgKQzcdnc3Mh7DBQY9nGq9sgf8jl
> KRBR8IOHhKMT2tdkLj9AyvIHDPYmA$
> > and has no control over the spi transfer bits...
> 
> Feature to add then or a custom regmap_bus if you want to keep it in
> the driver.

Hmm I see what you mean with the custom bus. We can actually make
use of more regmap infrastructure if I just define my regmap_bus in 
the driver. Thanks for the tip!

- Nuno Sá

Re: [RFC PATCH 0/1] LTC2688 support

[Jonathan Cameron]

On Thu, 11 Nov 2021 12:00:42 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

Hi Nuno,

> The reason why this is a RFC is because I'm still waiting for proper HW
> to test this thing. The reason why I'm sending this already is because
> there's some non usual features which might require extra ABI. Hence,
> while waiting I thought we could already speed up the process in regards
> with the ABI.

Wise move as this is an unusual beast :)

> 
> I still pushed the driver but the intent here is not really to review it.
> Naturally, if someone already wants to give some feedback, that's very
> much appreciated :)

> 
> Now, there are three main interfaces depending on the channel mode:
>  1) default (no new ABI)
>  2) toggle mode
>  3) dither mode
> 
> The channel mode will be a devicetree property as it does not really
> make much sense to change between modes at runtime even more because the
> piece of HW we might want to control with a channel might be different
> depending on the selected mode (I'm fairly sure on this between toggle
> and other modes but not so sure between dither and default mode).

I agree on toggle vs dither definitely being different, but normal you
could implement either as software toggle, or dither with a 0 magnitude
sine wave.  So might not be worth implementing default mode at all.
No harm in doing so though if there are advantages to having it.

> 
> toggle mode special ABI:
> 
>  * Toggle operation enables fast switching of a DAC output between two
> different DAC codes without any SPI transaction. Two codes are set in
> input_a and input_b and then the output switches according to an input
> signal (input_a -> clk high; input_b -> clk low).
> 
> out_voltageY_input_register
>  - selects between input_a and input_b. After selecting one register, we
>    can write the dac code in out_voltageY_raw.
> out_voltageY_toggle_en
>  - Disable/Enable toggle mode. The reason why I think this one is
>    important is because if we wanna change the 2 codes, we should first
>    disable this, set the codes and only then enable the mode back...
>    As I'm writing this, It kind of came to me that we can probably
>    achieve this with out_voltageY_powerdown attr (maybe takes a bit more
>    time to see the outputs but...)

Hmm. These corner cases always take a bit of figuring out.  What you have
here is a bit 'device specific' for my liking.

So there is precedence for ABI in this area, on the frequency devices but only
for devices we still haven't moved out of staging.  For those we needed a means
to define selectable phases for PSK - where the selection was either software or,
much like here, a selection pin.
 
out_altvotage0_phase0 etc

so I guess the equivalent here would be
out_voltageY_raw0
out_voltageY_raw1
and the selection would be via something like
out_voltageY_symbol (0 or 1 in this case). - note this is only
relevant if you have the software toggle. Any enable needed for setting
can be done as part of the write sequence for the  raw0 / raw1 and should
ideally not be exposed to userspace (controls that require manual sequencing
tend to be hard to use / document).

However, I'm not 100% sure it really maps to this case.  What do you think?

I'm not sure if whether a channel is in toggle mode is a circuit thing or not..
(and hence DT or userspace control?)
Can see that even in a case where you did want to use external controls to
pick the values, you might also want to override from software...
Given there is a software toggle I guess we can use that as override.
Actually that raises the question of what the point in having normal mode is?
Can we just implement that as a software toggle toggle mode? One less thing to
worry about if we can.

There is also the question of whether selection of which toggle pin is used
should be a dt thing or a userspace control...

> 
> dither mode special ABI:
> 
>  * Dither operation adds a small sinusoidal wave to the digital DAC
> signal path. Dithering is a signal processing technique that involves
> the injection of ac noise to the signal path to reduce system
> nonlinearities.
> 

This is a complex feature to describe as (if I read it correctly) we have
a dither clocked from an external pin, or in theory from software. That clock
frequency must match the dither.  Realistically that means it is a clock
in our control or we have to match the period below to the frequency of that
clock.

> out_voltage0_dither_en
>  - Same as in toggle mode.
> out_voltage0_dither_period
> out_voltage0_dither_phase
>  - Period and phase of the signal. Only some values are valid so there's
>    also *_available files for these. I'm not sure if we can use the more
>    generic IIO_CHAN_INFO_PHASE and IIO_CHAN_INFO_FREQUENCY here as these
>    parameters don't really apply to the channel output signal..

Possibly not helpful to do so, but you could describe the channel as an
out_altvoltage channel that happens to have a significant offset (the DC
level) and phase, frequency etc as for a normal altvoltage channel.

That would hide the intention here though so perhaps not a good plan
even if it ensures we end up with standard ABI.

> out_voltage0_input_register
>  - Same as in toggle mode. However in this mode the code set in the
>    input_b register has a special meaning. It's the amplitude of the
>    dither signal.
Don't do that - provide a direct attribute representing the value of
register_b and when it is written implicitly switch to the right register.
Any ABI that requires a sequence of events is hard to use.


> 
> One special mention to the dac scale. In this part this is something
> that can be purely controlled by SW so that I'm allowing userspace to
> change it rather then having it in dts. The available scales are:
> 
> * [0 5V] -> offset 0
> * [0 10V] -> offset 0
> * [-5 5V] -> offset -32678
> * [-10 10V] -> offset -32768
> * [-15 15V] -> offset -32768
> 
> With the above, we also need to have the offset configurable and right
> now I'm going to some trouble to make sure the scale + offset is
> something valid. Honestly, I think I'm overdoing it because things can
> still go wrong with [0 10V] and [-5 5V] as the scales are the same
> here. Furthermore, there's no real arm that can be done to the HW. Is
> just that the readings won't match with what someone might be expecting.
> My plan is to just remove those checks and assume is up to userspace to
> make it right and not have [-10 10V] scale with 0 offset for example.

So this is something we've debated a few times in the past.
There is a fairly strong argument for output devices that the range is
a characteristic of the circuit.  At the very least it makes sense to
restrict it in DT even if we allow safe forms of tweaking in the driver.
For an initial driver, I'd just have it in DT.

Jonathan


> 
> I know that I'm taking a shortcut here :) so if you prefer to only
> discuss this in the __real__ series, I totally get it.

It's a fine short cut to take - I send out RFCs all the time for similar
open questions!

Jonathan

> 
> https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2688.pdf
> 
> - Nuno Sá
> 
> Nuno Sá (1):
>   iio: dac: add support for ltc2688
> 
>  drivers/iio/dac/ltc2688.c | 995 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 995 insertions(+)
>  create mode 100644 drivers/iio/dac/ltc2688.c
> 

RE: [RFC PATCH 0/1] LTC2688 support

[Sa, Nuno]

Hi Jonathan,

Thanks for your inputs...

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Friday, November 12, 2021 5:15 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org
> Subject: Re: [RFC PATCH 0/1] LTC2688 support
> 
> [External]
> 
> On Thu, 11 Nov 2021 12:00:42 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> Hi Nuno,
> 
> > The reason why this is a RFC is because I'm still waiting for proper HW
> > to test this thing. The reason why I'm sending this already is because
> > there's some non usual features which might require extra ABI.
> Hence,
> > while waiting I thought we could already speed up the process in
> regards
> > with the ABI.
> 
> Wise move as this is an unusual beast :)
> 
> >
> > I still pushed the driver but the intent here is not really to review it.
> > Naturally, if someone already wants to give some feedback, that's
> very
> > much appreciated :)
> 
> >
> > Now, there are three main interfaces depending on the channel
> mode:
> >  1) default (no new ABI)
> >  2) toggle mode
> >  3) dither mode
> >
> > The channel mode will be a devicetree property as it does not really
> > make much sense to change between modes at runtime even more
> because the
> > piece of HW we might want to control with a channel might be
> different
> > depending on the selected mode (I'm fairly sure on this between
> toggle
> > and other modes but not so sure between dither and default mode).
> 
> I agree on toggle vs dither definitely being different, but normal you
> could implement either as software toggle, or dither with a 0
> magnitude
> sine wave.  So might not be worth implementing default mode at all.
> No harm in doing so though if there are advantages to having it.

My feeling is that we could probably have dither as the "default mode".
More on this below...

> >
> > toggle mode special ABI:
> >
> >  * Toggle operation enables fast switching of a DAC output between
> two
> > different DAC codes without any SPI transaction. Two codes are set
> in
> > input_a and input_b and then the output switches according to an
> input
> > signal (input_a -> clk high; input_b -> clk low).
> >
> > out_voltageY_input_register
> >  - selects between input_a and input_b. After selecting one register,
> we
> >    can write the dac code in out_voltageY_raw.
> > out_voltageY_toggle_en
> >  - Disable/Enable toggle mode. The reason why I think this one is
> >    important is because if we wanna change the 2 codes, we should
> first
> >    disable this, set the codes and only then enable the mode back...
> >    As I'm writing this, It kind of came to me that we can probably
> >    achieve this with out_voltageY_powerdown attr (maybe takes a bit
> more
> >    time to see the outputs but...)
> 
> Hmm. These corner cases always take a bit of figuring out.  What you
> have
> here is a bit 'device specific' for my liking.
> 
> So there is precedence for ABI in this area, on the frequency devices
> but only
> for devices we still haven't moved out of staging.  For those we
> needed a means
> to define selectable phases for PSK - where the selection was either
> software or,
> much like here, a selection pin.
> 
> out_altvotage0_phase0 etc
> 
> so I guess the equivalent here would be
> out_voltageY_raw0
> out_voltageY_raw1
> and the selection would be via something like
> out_voltageY_symbol (0 or 1 in this case). - note this is only
> relevant if you have the software toggle. Any enable needed for
> setting
> can be done as part of the write sequence for the  raw0 / raw1 and
> should
> ideally not be exposed to userspace (controls that require manual
> sequencing
> tend to be hard to use / document).

Yeah, I thought about that. I was even thinking in having something like
*_amplitude for dither mode. In some cases, where we might be left
in some non obvious state (eg: moved the select register to input b and
then we failed to set the code;), I prefer to leave things as flexible as
possible for userspace. But I agree it adds up more complexity and in
this case, I can just always go to 'input_a' when writing 'raw0'...

> However, I'm not 100% sure it really maps to this case.  What do you
> think?

I think it can work. Though out_voltageY_symbol probably needs to be
shared by type to be coherent with what we might have with TGPx.
Note the sw_toggle register has a bit mask of the channels we want to
toggle which means we can toggle multiple channels at the same time.
It works the same with TGPx if you map multiple channels to the same
pin.

There's also the question on how to handle this if a TGPx is provided?
I guess it will just override the pin... But most likely having them both
at the same time will lead to non desired results (unless we have a
way to gate/ungate the clocks)... 
 
> I'm not sure if whether a channel is in toggle mode is a circuit thing or
> not..
> (and hence DT or userspace control?)

The only reason I can think off to have it as DT is that toggle mode seems
to be for more specific use cases so I guess the HW we want to control (
and connect to a toggle enabled channel) will be different.

I'm also not seeing an use case for ping ponging between the modes mostly
because of the above...

> Can see that even in a case where you did want to use external
> controls to
> pick the values, you might also want to override from software...
> Given there is a software toggle I guess we can use that as override.
> Actually that raises the question of what the point in having normal
> mode is?
> Can we just implement that as a software toggle toggle mode? One
> less thing to
> worry about if we can.

I did thought about the sw_toggle thing (it's something that is only valid
for channels in dither/toggle mode). My reasoning was that either

1) I did not supported it and made the TGPx selection mandatory (in case
dither/toggle mode enabled) or
2) I did support it  and hence the pins  are not really mandatory.

I went with 1) because, honestly, I'm not seeing the point of having these
modes and use sw toggle (at least on a production system). However, if we
want to get rid of the default mode and have it as the dither mode, I agree
we need sw_toggle because If someone just wants to use the channel
without any dithering, we can't have an hard requirement to provide a
external TGPx. Moreover, if the default channel will be a dither capable
one, we need to provide full functionality and hence, sw_toggle.

As I stated before, I'm just not sure on how to handle things if a TGPx is
also provided. Maybe they should be mutual exclusive? I mean, if someone
tries to toggle a channel with a mapped TGPx we return some error code?
 
> There is also the question of whether selection of which toggle pin is
> used
> should be a dt thing or a userspace control...

Well, this definitely means some HW wiring to have the external signals and
I'm not sure if there's any added valuable in being able to change the
external signal at runtime?

> >
> > dither mode special ABI:
> >
> >  * Dither operation adds a small sinusoidal wave to the digital DAC
> > signal path. Dithering is a signal processing technique that involves
> > the injection of ac noise to the signal path to reduce system
> > nonlinearities.
> >
> 
> This is a complex feature to describe as (if I read it correctly) we have
> a dither clocked from an external pin, or in theory from software. That
> clock
> frequency must match the dither.  Realistically that means it is a clock
> in our control or we have to match the period below to the frequency
> of that
> clock.

Yeah, the frequency  of the dither signal is fsig = fclk / N, where N can only
be: [4 8 16 32 64]. So, we kind of just have these available options for the
signal frequency and fclk is something we can control and know (assuming
we have TGPx mapping which I'm bundling with a clk).

The only quirk with having this with frequency rather than raw N is
to handle the sw_toggle where we have no idea about fclk? We could also 
think of this attr as some kind of decimation...

> > out_voltage0_dither_en
> >  - Same as in toggle mode.
> > out_voltage0_dither_period
> > out_voltage0_dither_phase
> >  - Period and phase of the signal. Only some values are valid so
> there's
> >    also *_available files for these. I'm not sure if we can use the more
> >    generic IIO_CHAN_INFO_PHASE and IIO_CHAN_INFO_FREQUENCY
> here as these
> >    parameters don't really apply to the channel output signal..
> 
> Possibly not helpful to do so, but you could describe the channel as an
> out_altvoltage channel that happens to have a significant offset (the
> DC
> level) and phase, frequency etc as for a normal altvoltage channel.
> That would hide the intention here though so perhaps not a good plan
> even if it ensures we end up with standard ABI.

I think altvoltage might not be optimal here  because the phase and frequency
are really not characteristics of the output signal of the channel.

> > out_voltage0_input_register
> >  - Same as in toggle mode. However in this mode the code set in the
> >    input_b register has a special meaning. It's the amplitude of the
> >    dither signal.
> Don't do that - provide a direct attribute representing the value of
> register_b and when it is written implicitly switch to the right register.
> Any ABI that requires a sequence of events is hard to use.

I guess we can just use the same raw1 attr here? Even though, in dither
mode this has special meaning (it is the amplitude)...

> 
> >
> > One special mention to the dac scale. In this part this is something
> > that can be purely controlled by SW so that I'm allowing userspace to
> > change it rather then having it in dts. The available scales are:
> >
> > * [0 5V] -> offset 0
> > * [0 10V] -> offset 0
> > * [-5 5V] -> offset -32678
> > * [-10 10V] -> offset -32768
> > * [-15 15V] -> offset -32768
> >
> > With the above, we also need to have the offset configurable and
> right
> > now I'm going to some trouble to make sure the scale + offset is
> > something valid. Honestly, I think I'm overdoing it because things can
> > still go wrong with [0 10V] and [-5 5V] as the scales are the same
> > here. Furthermore, there's no real arm that can be done to the HW.
> Is
> > just that the readings won't match with what someone might be
> expecting.
> > My plan is to just remove those checks and assume is up to
> userspace to
> > make it right and not have [-10 10V] scale with 0 offset for example.
> 
> So this is something we've debated a few times in the past.
> There is a fairly strong argument for output devices that the range is
> a characteristic of the circuit.  At the very least it makes sense to
> restrict it in DT even if we allow safe forms of tweaking in the driver.
> For an initial driver, I'd just have it in DT.
> 

No complaints against that and makes things way simpler to handle.

- Nuno Sá

Re: [RFC PATCH 0/1] LTC2688 support

[Jonathan Cameron]

On Mon, 15 Nov 2021 10:28:51 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> Hi Jonathan,
> 
> Thanks for your inputs...
> 
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Friday, November 12, 2021 5:15 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio@vger.kernel.org
> > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > 
> > [External]
> > 
> > On Thu, 11 Nov 2021 12:00:42 +0100
> > Nuno Sá <nuno.sa@analog.com> wrote:
> > 
> > Hi Nuno,
> >   
> > > The reason why this is a RFC is because I'm still waiting for proper HW
> > > to test this thing. The reason why I'm sending this already is because
> > > there's some non usual features which might require extra ABI.  
> > Hence,  
> > > while waiting I thought we could already speed up the process in  
> > regards  
> > > with the ABI.  
> > 
> > Wise move as this is an unusual beast :)
> >   
> > >
> > > I still pushed the driver but the intent here is not really to review it.
> > > Naturally, if someone already wants to give some feedback, that's  
> > very  
> > > much appreciated :)  
> >   
> > >
> > > Now, there are three main interfaces depending on the channel  
> > mode:  
> > >  1) default (no new ABI)
> > >  2) toggle mode
> > >  3) dither mode
> > >
> > > The channel mode will be a devicetree property as it does not really
> > > make much sense to change between modes at runtime even more  
> > because the  
> > > piece of HW we might want to control with a channel might be  
> > different  
> > > depending on the selected mode (I'm fairly sure on this between  
> > toggle  
> > > and other modes but not so sure between dither and default mode).  
> > 
> > I agree on toggle vs dither definitely being different, but normal you
> > could implement either as software toggle, or dither with a 0
> > magnitude
> > sine wave.  So might not be worth implementing default mode at all.
> > No harm in doing so though if there are advantages to having it.  
> 
> My feeling is that we could probably have dither as the "default mode".
> More on this below...
> 
> > >
> > > toggle mode special ABI:
> > >
> > >  * Toggle operation enables fast switching of a DAC output between  
> > two  
> > > different DAC codes without any SPI transaction. Two codes are set  
> > in  
> > > input_a and input_b and then the output switches according to an  
> > input  
> > > signal (input_a -> clk high; input_b -> clk low).
> > >
> > > out_voltageY_input_register
> > >  - selects between input_a and input_b. After selecting one register,  
> > we  
> > >    can write the dac code in out_voltageY_raw.
> > > out_voltageY_toggle_en
> > >  - Disable/Enable toggle mode. The reason why I think this one is
> > >    important is because if we wanna change the 2 codes, we should  
> > first  
> > >    disable this, set the codes and only then enable the mode back...
> > >    As I'm writing this, It kind of came to me that we can probably
> > >    achieve this with out_voltageY_powerdown attr (maybe takes a bit  
> > more  
> > >    time to see the outputs but...)  
> > 
> > Hmm. These corner cases always take a bit of figuring out.  What you
> > have
> > here is a bit 'device specific' for my liking.
> > 
> > So there is precedence for ABI in this area, on the frequency devices
> > but only
> > for devices we still haven't moved out of staging.  For those we
> > needed a means
> > to define selectable phases for PSK - where the selection was either
> > software or,
> > much like here, a selection pin.
> > 
> > out_altvotage0_phase0 etc
> > 
> > so I guess the equivalent here would be
> > out_voltageY_raw0
> > out_voltageY_raw1
> > and the selection would be via something like
> > out_voltageY_symbol (0 or 1 in this case). - note this is only
> > relevant if you have the software toggle. Any enable needed for
> > setting
> > can be done as part of the write sequence for the  raw0 / raw1 and
> > should
> > ideally not be exposed to userspace (controls that require manual
> > sequencing
> > tend to be hard to use / document).  
> 
> Yeah, I thought about that. I was even thinking in having something like
> *_amplitude for dither mode. In some cases, where we might be left
> in some non obvious state (eg: moved the select register to input b and
> then we failed to set the code;), I prefer to leave things as flexible as
> possible for userspace. But I agree it adds up more complexity and in
> this case, I can just always go to 'input_a' when writing 'raw0'...
> 
> > However, I'm not 100% sure it really maps to this case.  What do you
> > think?  
> 
> I think it can work. Though out_voltageY_symbol probably needs to be
> shared by type to be coherent with what we might have with TGPx.

That's fine.

> Note the sw_toggle register has a bit mask of the channels we want to
> toggle which means we can toggle multiple channels at the same time.

Using that wired up to buffer mode might make sense.  You'd provide multiple
buffers and allow channels to be selected into one of them at a time. Each
buffer is then tied to a different toggle (TGP0, TGP1, etc)

The same could be true for the software toggle.  It'll get a bit fiddly though.

Perhaps this is an advanced feature to think about later...

> It works the same with TGPx if you map multiple channels to the same
> pin.
> 
> There's also the question on how to handle this if a TGPx is provided?
> I guess it will just override the pin... But most likely having them both
> at the same time will lead to non desired results (unless we have a
> way to gate/ungate the clocks)... 

I don't follow this bit.  You mean TGPx and software toggle. As far as I can
tell it's an either/or thing per channel.

>  
> > I'm not sure if whether a channel is in toggle mode is a circuit thing or
> > not..
> > (and hence DT or userspace control?)  
> 
> The only reason I can think off to have it as DT is that toggle mode seems
> to be for more specific use cases so I guess the HW we want to control (
> and connect to a toggle enabled channel) will be different.
> 
> I'm also not seeing an use case for ping ponging between the modes mostly
> because of the above...

Only use I can see is to reduce traffic if you happen to be switching between
two sets of DAC outputs repeatedly. If there wasn't an LDAC pin I'd suspect
this was there to enable simultaneous updates but we have that anyway.
Maybe if the LDAC isn't wired this could be used to provide similar
functionality?  If that's the case, we could just leave it as a possible
TODO if anyone wants it in future...
I think you could use the TGPx to provide controlled switching of
sets of channels.  Maybe that's something useful?

> 
> > Can see that even in a case where you did want to use external
> > controls to
> > pick the values, you might also want to override from software...
> > Given there is a software toggle I guess we can use that as override.
> > Actually that raises the question of what the point in having normal
> > mode is?
> > Can we just implement that as a software toggle toggle mode? One
> > less thing to
> > worry about if we can.  
> 
> I did thought about the sw_toggle thing (it's something that is only valid
> for channels in dither/toggle mode). My reasoning was that either
> 
> 1) I did not supported it and made the TGPx selection mandatory (in case
> dither/toggle mode enabled) or
> 2) I did support it  and hence the pins  are not really mandatory.
> 
> I went with 1) because, honestly, I'm not seeing the point of having these
> modes and use sw toggle (at least on a production system). However, if we
> want to get rid of the default mode and have it as the dither mode, I agree
> we need sw_toggle because If someone just wants to use the channel
> without any dithering, we can't have an hard requirement to provide a
> external TGPx. Moreover, if the default channel will be a dither capable
> one, we need to provide full functionality and hence, sw_toggle.
> 
> As I stated before, I'm just not sure on how to handle things if a TGPx is
> also provided. Maybe they should be mutual exclusive? I mean, if someone
> tries to toggle a channel with a mapped TGPx we return some error code?

Given the mapping of TGPx to channel is a software control I think ultimately
you'd want to expose that - one way I can think of doing that is via
the buffer interface.

4x buffers.  One of each TGP0,1,2 and SW toggle.  Enable the channels you
want for a given 'buffer' and then they will switch together based on the
data in the buffer.  If the buffer has a series of toggling values then
it's simple - if not then after each toggle the buffer would need to preload
the next value.  The snag there is that you'd need to know a toggle occurred
and if the toggle pins are wired to somewhere other than our host I'm not sure
how you would know that in general? (could wire the same TGPx signal to an
interrupt on the host controller but seems unlikely).

Whether software toggle is worth bothering when we have LDAC to control
simultaneous DAC updates isn't clear to me.  I guess it's fewer writes
if we happen to be cycling between values.  Perhaps you are right and that
feature is just for debug.

>  
> > There is also the question of whether selection of which toggle pin is
> > used
> > should be a dt thing or a userspace control...  
> 
> Well, this definitely means some HW wiring to have the external signals and
> I'm not sure if there's any added valuable in being able to change the
> external signal at runtime?

Whilst I can conjecture reasons to do this, you may well be right - real
usecases will know which signal groups they want to control together.

> 
> > >
> > > dither mode special ABI:
> > >
> > >  * Dither operation adds a small sinusoidal wave to the digital DAC
> > > signal path. Dithering is a signal processing technique that involves
> > > the injection of ac noise to the signal path to reduce system
> > > nonlinearities.
> > >  
> > 
> > This is a complex feature to describe as (if I read it correctly) we have
> > a dither clocked from an external pin, or in theory from software. That
> > clock
> > frequency must match the dither.  Realistically that means it is a clock
> > in our control or we have to match the period below to the frequency
> > of that
> > clock.  
> 
> Yeah, the frequency  of the dither signal is fsig = fclk / N, where N can only
> be: [4 8 16 32 64]. So, we kind of just have these available options for the
> signal frequency and fclk is something we can control and know (assuming
> we have TGPx mapping which I'm bundling with a clk).
> 
> The only quirk with having this with frequency rather than raw N is
> to handle the sw_toggle where we have no idea about fclk? We could also 
> think of this attr as some kind of decimation...

Does seems unlikely anyone would use dither with a sw toggle.
Perhaps best plan here is to not support that combination.

As to the clock, these are about controlling a sine wave frequency. I'm not sure
decimation fits as a model. (figure 19)

Given you know the input clock, perhaps present this as something like
out_voltage0_dither_frequency

I don't thing dithers are always this simple, so probably want to be specific its
a sine wave so maybe we need
out_voltage0_dither_type 'sine' 

We used to have some DDS chips in staging but looks like they all got dropped due
to end of time Those had various waveforms and IIRC all we came up with was
descriptive terms + frequencies and magnitudes.


> 
> > > out_voltage0_dither_en
> > >  - Same as in toggle mode.
> > > out_voltage0_dither_period
> > > out_voltage0_dither_phase
> > >  - Period and phase of the signal. Only some values are valid so  
> > there's  
> > >    also *_available files for these. I'm not sure if we can use the more
> > >    generic IIO_CHAN_INFO_PHASE and IIO_CHAN_INFO_FREQUENCY  
> > here as these  
> > >    parameters don't really apply to the channel output signal..  
> > 
> > Possibly not helpful to do so, but you could describe the channel as an
> > out_altvoltage channel that happens to have a significant offset (the
> > DC
> > level) and phase, frequency etc as for a normal altvoltage channel.
> > That would hide the intention here though so perhaps not a good plan
> > even if it ensures we end up with standard ABI.  
> 
> I think altvoltage might not be optimal here  because the phase and frequency
> are really not characteristics of the output signal of the channel.

Well they kind of are if you set the magnitude high enough  - but I get your
point.  That's not how they are intended to be used.

> 
> > > out_voltage0_input_register
> > >  - Same as in toggle mode. However in this mode the code set in the
> > >    input_b register has a special meaning. It's the amplitude of the
> > >    dither signal.  
> > Don't do that - provide a direct attribute representing the value of
> > register_b and when it is written implicitly switch to the right register.
> > Any ABI that requires a sequence of events is hard to use.  
> 
> I guess we can just use the same raw1 attr here? Even though, in dither
> mode this has special meaning (it is the amplitude)...

I was thinking toggle mode here. This interface doesn't work for dither.
in that case there is just 
out_voltage0_raw for the DC part and
out_voltage0_dither_raw for the dither amplitude.  Assumption being same scaling
as I don't really want to support multiple scale factors if we can avoid it.



> 
> >   
> > >
> > > One special mention to the dac scale. In this part this is something
> > > that can be purely controlled by SW so that I'm allowing userspace to
> > > change it rather then having it in dts. The available scales are:
> > >
> > > * [0 5V] -> offset 0
> > > * [0 10V] -> offset 0
> > > * [-5 5V] -> offset -32678
> > > * [-10 10V] -> offset -32768
> > > * [-15 15V] -> offset -32768
> > >
> > > With the above, we also need to have the offset configurable and  
> > right  
> > > now I'm going to some trouble to make sure the scale + offset is
> > > something valid. Honestly, I think I'm overdoing it because things can
> > > still go wrong with [0 10V] and [-5 5V] as the scales are the same
> > > here. Furthermore, there's no real arm that can be done to the HW.  
> > Is  
> > > just that the readings won't match with what someone might be  
> > expecting.  
> > > My plan is to just remove those checks and assume is up to  
> > userspace to  
> > > make it right and not have [-10 10V] scale with 0 offset for example.  
> > 
> > So this is something we've debated a few times in the past.
> > There is a fairly strong argument for output devices that the range is
> > a characteristic of the circuit.  At the very least it makes sense to
> > restrict it in DT even if we allow safe forms of tweaking in the driver.
> > For an initial driver, I'd just have it in DT.
> >   
> 
> No complaints against that and makes things way simpler to handle.
Great.
> 
> - Nuno Sá
> 
So conclusions.. Hmm. Not strong ones yet, but for dither mode at least
I think you want to link particular channels to a TGPx choice

out_voltage0_raw
out_voltage0_raw_available ( nice to have on DACs)
out_voltage0_scale
out_voltage0_dither_raw
out_voltage0_dither_raw_available
out_voltage0_dither_frequency
out_voltage0_dither_frequency_available
out_voltage0_dither_phase
out_voltage0_dither_phase_available

Toggle mode is less clear to me but symbol approach plus TGPx in DT maybe works
You could allow for software override to set the symbol.  Interface to unset
it being to write an empty string to _symbol. Maybe leave that for now.

out_voltage0_raw0
out_voltage0_raw1
out_voltage0_scale
out_voltage0_symbol

Perhaps that's enough for an initial driver and we can think more or complex
corner cases after that is in place?

Jonathan

RE: [RFC PATCH 0/1] LTC2688 support

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, November 21, 2021 1:18 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org
> Subject: Re: [RFC PATCH 0/1] LTC2688 support
> 
> [External]
> 
> On Mon, 15 Nov 2021 10:28:51 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > Hi Jonathan,
> >
> > Thanks for your inputs...
> >
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Friday, November 12, 2021 5:15 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio@vger.kernel.org
> > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > >
> > > [External]
> > >
> > > On Thu, 11 Nov 2021 12:00:42 +0100
> > > Nuno Sá <nuno.sa@analog.com> wrote:
> > >
> > > Hi Nuno,
> > >
> > > > The reason why this is a RFC is because I'm still waiting for proper
> HW
> > > > to test this thing. The reason why I'm sending this already is
> because
> > > > there's some non usual features which might require extra ABI.
> > > Hence,
> > > > while waiting I thought we could already speed up the process in
> > > regards
> > > > with the ABI.
> > >
> > > Wise move as this is an unusual beast :)
> > >
> > > >
> > > > I still pushed the driver but the intent here is not really to review
> it.
> > > > Naturally, if someone already wants to give some feedback,
> that's
> > > very
> > > > much appreciated :)
> > >
> > > >
> > > > Now, there are three main interfaces depending on the channel
> > > mode:
> > > >  1) default (no new ABI)
> > > >  2) toggle mode
> > > >  3) dither mode
> > > >
> > > > The channel mode will be a devicetree property as it does not
> really
> > > > make much sense to change between modes at runtime even
> more
> > > because the
> > > > piece of HW we might want to control with a channel might be
> > > different
> > > > depending on the selected mode (I'm fairly sure on this between
> > > toggle
> > > > and other modes but not so sure between dither and default
> mode).
> > >
> > > I agree on toggle vs dither definitely being different, but normal
> you
> > > could implement either as software toggle, or dither with a 0
> > > magnitude
> > > sine wave.  So might not be worth implementing default mode at
> all.
> > > No harm in doing so though if there are advantages to having it.
> >
> > My feeling is that we could probably have dither as the "default
> mode".
> > More on this below...
> >
> > > >
> > > > toggle mode special ABI:
> > > >
> > > >  * Toggle operation enables fast switching of a DAC output
> between
> > > two
> > > > different DAC codes without any SPI transaction. Two codes are
> set
> > > in
> > > > input_a and input_b and then the output switches according to
> an
> > > input
> > > > signal (input_a -> clk high; input_b -> clk low).
> > > >
> > > > out_voltageY_input_register
> > > >  - selects between input_a and input_b. After selecting one
> register,
> > > we
> > > >    can write the dac code in out_voltageY_raw.
> > > > out_voltageY_toggle_en
> > > >  - Disable/Enable toggle mode. The reason why I think this one is
> > > >    important is because if we wanna change the 2 codes, we
> should
> > > first
> > > >    disable this, set the codes and only then enable the mode
> back...
> > > >    As I'm writing this, It kind of came to me that we can probably
> > > >    achieve this with out_voltageY_powerdown attr (maybe takes a
> bit
> > > more
> > > >    time to see the outputs but...)
> > >
> > > Hmm. These corner cases always take a bit of figuring out.  What
> you
> > > have
> > > here is a bit 'device specific' for my liking.
> > >
> > > So there is precedence for ABI in this area, on the frequency
> devices
> > > but only
> > > for devices we still haven't moved out of staging.  For those we
> > > needed a means
> > > to define selectable phases for PSK - where the selection was
> either
> > > software or,
> > > much like here, a selection pin.
> > >
> > > out_altvotage0_phase0 etc
> > >
> > > so I guess the equivalent here would be
> > > out_voltageY_raw0
> > > out_voltageY_raw1
> > > and the selection would be via something like
> > > out_voltageY_symbol (0 or 1 in this case). - note this is only
> > > relevant if you have the software toggle. Any enable needed for
> > > setting
> > > can be done as part of the write sequence for the  raw0 / raw1 and
> > > should
> > > ideally not be exposed to userspace (controls that require manual
> > > sequencing
> > > tend to be hard to use / document).
> >
> > Yeah, I thought about that. I was even thinking in having something
> like
> > *_amplitude for dither mode. In some cases, where we might be left
> > in some non obvious state (eg: moved the select register to input b
> and
> > then we failed to set the code;), I prefer to leave things as flexible as
> > possible for userspace. But I agree it adds up more complexity and in
> > this case, I can just always go to 'input_a' when writing 'raw0'...
> >
> > > However, I'm not 100% sure it really maps to this case.  What do
> you
> > > think?
> >
> > I think it can work. Though out_voltageY_symbol probably needs to
> be
> > shared by type to be coherent with what we might have with TGPx.
> 
> That's fine.
> 
> > Note the sw_toggle register has a bit mask of the channels we want
> to
> > toggle which means we can toggle multiple channels at the same
> time.
> 
> Using that wired up to buffer mode might make sense.  You'd provide
> multiple
> buffers and allow channels to be selected into one of them at a time.
> Each
> buffer is then tied to a different toggle (TGP0, TGP1, etc)
> 
> The same could be true for the software toggle.  It'll get a bit fiddly
> though.
> 
> Perhaps this is an advanced feature to think about later...
> 
> > It works the same with TGPx if you map multiple channels to the
> same
> > pin.
> >
> > There's also the question on how to handle this if a TGPx is provided?
> > I guess it will just override the pin... But most likely having them both
> > at the same time will lead to non desired results (unless we have a
> > way to gate/ungate the clocks)...
> I don't follow this bit.  You mean TGPx and software toggle. As far as I
> can
> tell it's an either/or thing per channel.
> 

Here I meant that if we have a TGPx pin bundled to some channel(s) we
do not want to also dance with the sw_toggle bit of that channel. Ultimately,
that's on the user responsibility but we could also add some guards I guess.
I'm not sure if it's an either/or thing per channel... IIUC, we spoke about
making dither and default mode the same. That might complicate things a bit
as:

1) We should not force a user to specify a TGPx pin for those channels (since
it can also work with dithering disabled).
2) Because of 1), we should also support sw_toggle for these channels since
someone might want to enable dither mode (at runtime) and the TGPx pin was
not given. Hence, we need to have a way to update the DAC using the sw_toggle.

Did I understood things wrong? One thing that comes to my mind, is to return
error (eg: EPERM or ENOTSUPP) if someone tries to enable dither mode and
no TGPx pin was selected for that channel? Hence, we do not need to add
the sw_toggle ABI  (out_voltage_symbol) for the default/dither mode. Or
maybe even better, we just expose the dither ABI if a TGPx pin is given over
dt (I try to explain the toggle/dither modes below; that might help you in
understanding my reasoning here)?

Alternatively, we just keep the approach I have in this RFC and we keep the
3 different modes (being mode a dt property; in the current state I'm using
a boolean to say a channel Is in toggle mode)... Maybe with the difference
that we allow sw_toggle for toggle enabled channels.

> >
> > > I'm not sure if whether a channel is in toggle mode is a circuit thing
> or
> > > not..
> > > (and hence DT or userspace control?)
> >
> > The only reason I can think off to have it as DT is that toggle mode
> seems
> > to be for more specific use cases so I guess the HW we want to
> control (
> > and connect to a toggle enabled channel) will be different.
> >
> > I'm also not seeing an use case for ping ponging between the modes
> mostly
> > because of the above...
> 
> Only use I can see is to reduce traffic if you happen to be switching
> between
> two sets of DAC outputs repeatedly. If there wasn't an LDAC pin I'd
> suspect
> this was there to enable simultaneous updates but we have that
> anyway.
> Maybe if the LDAC isn't wired this could be used to provide similar
> functionality?  If that's the case, we could just leave it as a possible
> TODO if anyone wants it in future...
> I think you could use the TGPx to provide controlled switching of
> sets of channels.  Maybe that's something useful?

Hmm, I think LDAC is something else. The LDAC pin updates all 16 channels
with the value of the input register (which I assume is INPUT_A) independent
of them being in dither/toggle mode. Honestly, I'm not even sure of how
thing would work if someone plays with LDAC + channels in dither/toggle
mode. I think extra care would be needed...

So, TGPx pins are indeed used to control set of channels. But note that these
pins (as sw_toggle) are only meaningful if the channel is in dither or toggle
mode (the channel bit in the toggle/dither enable register has to be set):

* In toggle mode we have two set of input registers: INPUT_A and INPUT_B
and we toggle between these two values. If we wire a TGPx pin and bundle
it to a toggle enabled channel, when the input signal is high, we output INPUT_A
and INPUT_B when the signal is low. We can mimic the same behavior by writing
1/0 to the sw_toggle register.
* In dither mode, things are slightly different as INPUT_B is the value for the
sinusoidal amplitude. And if we have an external signal on a TGPx pin, the dac
channel is updated on the rising edge of the signal. Again, we can mimic the same
behavior with the sw_toggle register.  

So, for dither mode, I'm not really seeing why someone would want to use the
sw_toggle. Even for toggle mode, I have my doubts but I can more easily see it.

> >
> > > Can see that even in a case where you did want to use external
> > > controls to
> > > pick the values, you might also want to override from software...
> > > Given there is a software toggle I guess we can use that as
> override.
> > > Actually that raises the question of what the point in having normal
> > > mode is?
> > > Can we just implement that as a software toggle toggle mode? One
> > > less thing to
> > > worry about if we can.
> >
> > I did thought about the sw_toggle thing (it's something that is only
> valid
> > for channels in dither/toggle mode). My reasoning was that either
> >
> > 1) I did not supported it and made the TGPx selection mandatory (in
> case
> > dither/toggle mode enabled) or
> > 2) I did support it  and hence the pins  are not really mandatory.
> >
> > I went with 1) because, honestly, I'm not seeing the point of having
> these
> > modes and use sw toggle (at least on a production system).
> However, if we
> > want to get rid of the default mode and have it as the dither mode, I
> agree
> > we need sw_toggle because If someone just wants to use the
> channel
> > without any dithering, we can't have an hard requirement to provide
> a
> > external TGPx. Moreover, if the default channel will be a dither
> capable
> > one, we need to provide full functionality and hence, sw_toggle.
> >
> > As I stated before, I'm just not sure on how to handle things if a TGPx
> is
> > also provided. Maybe they should be mutual exclusive? I mean, if
> someone
> > tries to toggle a channel with a mapped TGPx we return some error
> code?
> 
> Given the mapping of TGPx to channel is a software control I think
> ultimately
> you'd want to expose that - one way I can think of doing that is via
> the buffer interface.
> 
> 4x buffers.  One of each TGP0,1,2 and SW toggle.  Enable the channels
> you
> want for a given 'buffer' and then they will switch together based on
> the
> data in the buffer.  If the buffer has a series of toggling values then
> it's simple - if not then after each toggle the buffer would need to
> preload
> the next value.  The snag there is that you'd need to know a toggle
> occurred
> and if the toggle pins are wired to somewhere other than our host I'm
> not sure
> how you would know that in general? (could wire the same TGPx signal
> to an
> interrupt on the host controller but seems unlikely).

Honestly I think that we would be probably over engineering things with
this even though I get your point. OTOH, as you said, the triggering would
be very hacky to handle and that might already tell us something :)
 
> Whether software toggle is worth bothering when we have LDAC to
> control
> simultaneous DAC updates isn't clear to me.  I guess it's fewer writes

Hopefully I could make it clear that sw_toggle is something else than LDAC :)

> if we happen to be cycling between values.  Perhaps you are right and
> that
> feature is just for debug.
> 
> >
> > > There is also the question of whether selection of which toggle pin
> is
> > > used
> > > should be a dt thing or a userspace control...
> >
> > Well, this definitely means some HW wiring to have the external
> signals and
> > I'm not sure if there's any added valuable in being able to change the
> > external signal at runtime?
> 
> Whilst I can conjecture reasons to do this, you may well be right - real
> usecases will know which signal groups they want to control together.
> 
> >
> > > >
> > > > dither mode special ABI:
> > > >
> > > >  * Dither operation adds a small sinusoidal wave to the digital DAC
> > > > signal path. Dithering is a signal processing technique that
> involves
> > > > the injection of ac noise to the signal path to reduce system
> > > > nonlinearities.
> > > >
> > >
> > > This is a complex feature to describe as (if I read it correctly) we
> have
> > > a dither clocked from an external pin, or in theory from software.
> That
> > > clock
> > > frequency must match the dither.  Realistically that means it is a
> clock
> > > in our control or we have to match the period below to the
> frequency
> > > of that
> > > clock.
> >
> > Yeah, the frequency  of the dither signal is fsig = fclk / N, where N can
> only
> > be: [4 8 16 32 64]. So, we kind of just have these available options for
> the
> > signal frequency and fclk is something we can control and know
> (assuming
> > we have TGPx mapping which I'm bundling with a clk).
> >
> > The only quirk with having this with frequency rather than raw N is
> > to handle the sw_toggle where we have no idea about fclk? We
> could also
> > think of this attr as some kind of decimation...
> 
> Does seems unlikely anyone would use dither with a sw toggle.
> Perhaps best plan here is to not support that combination.

Agree... But there are some subtleties that we need to settle 
about the channel default mode as I raised in my previous comments.

> As to the clock, these are about controlling a sine wave frequency. I'm
> not sure
> decimation fits as a model. (figure 19)
> 
> Given you know the input clock, perhaps present this as something like
> out_voltage0_dither_frequency
> 
> I don't thing dithers are always this simple, so probably want to be
> specific its
> a sine wave so maybe we need
> out_voltage0_dither_type 'sine'
> 
> We used to have some DDS chips in staging but looks like they all got
> dropped due
> to end of time Those had various waveforms and IIRC all we came up
> with was
> descriptive terms + frequencies and magnitudes.
> 
> 
> >
> > > > out_voltage0_dither_en
> > > >  - Same as in toggle mode.
> > > > out_voltage0_dither_period
> > > > out_voltage0_dither_phase
> > > >  - Period and phase of the signal. Only some values are valid so
> > > there's
> > > >    also *_available files for these. I'm not sure if we can use the
> more
> > > >    generic IIO_CHAN_INFO_PHASE and
> IIO_CHAN_INFO_FREQUENCY
> > > here as these
> > > >    parameters don't really apply to the channel output signal..
> > >
> > > Possibly not helpful to do so, but you could describe the channel as
> an
> > > out_altvoltage channel that happens to have a significant offset
> (the
> > > DC
> > > level) and phase, frequency etc as for a normal altvoltage channel.
> > > That would hide the intention here though so perhaps not a good
> plan
> > > even if it ensures we end up with standard ABI.
> >
> > I think altvoltage might not be optimal here  because the phase and
> frequency
> > are really not characteristics of the output signal of the channel.
> 
> Well they kind of are if you set the magnitude high enough  - but I get
> your
> point.  That's not how they are intended to be used.
> 
> >
> > > > out_voltage0_input_register
> > > >  - Same as in toggle mode. However in this mode the code set in
> the
> > > >    input_b register has a special meaning. It's the amplitude of the
> > > >    dither signal.
> > > Don't do that - provide a direct attribute representing the value of
> > > register_b and when it is written implicitly switch to the right
> register.
> > > Any ABI that requires a sequence of events is hard to use.
> >
> > I guess we can just use the same raw1 attr here? Even though, in
> dither
> > mode this has special meaning (it is the amplitude)...
> 
> I was thinking toggle mode here. This interface doesn't work for dither.
> in that case there is just
> out_voltage0_raw for the DC part and
> out_voltage0_dither_raw for the dither amplitude.  Assumption being
> same scaling
> as I don't really want to support multiple scale factors if we can avoid it.

I'm fairly sure it is the same scaling....

> 
> 
> >
> > >
> > > >
> > > > One special mention to the dac scale. In this part this is something
> > > > that can be purely controlled by SW so that I'm allowing
> userspace to
> > > > change it rather then having it in dts. The available scales are:
> > > >
> > > > * [0 5V] -> offset 0
> > > > * [0 10V] -> offset 0
> > > > * [-5 5V] -> offset -32678
> > > > * [-10 10V] -> offset -32768
> > > > * [-15 15V] -> offset -32768
> > > >
> > > > With the above, we also need to have the offset configurable
> and
> > > right
> > > > now I'm going to some trouble to make sure the scale + offset is
> > > > something valid. Honestly, I think I'm overdoing it because things
> can
> > > > still go wrong with [0 10V] and [-5 5V] as the scales are the same
> > > > here. Furthermore, there's no real arm that can be done to the
> HW.
> > > Is
> > > > just that the readings won't match with what someone might be
> > > expecting.
> > > > My plan is to just remove those checks and assume is up to
> > > userspace to
> > > > make it right and not have [-10 10V] scale with 0 offset for
> example.
> > >
> > > So this is something we've debated a few times in the past.
> > > There is a fairly strong argument for output devices that the range
> is
> > > a characteristic of the circuit.  At the very least it makes sense to
> > > restrict it in DT even if we allow safe forms of tweaking in the
> driver.
> > > For an initial driver, I'd just have it in DT.
> > >
> >
> > No complaints against that and makes things way simpler to handle.
> Great.
> >
> > - Nuno Sá
> >
> So conclusions.. Hmm. Not strong ones yet, but for dither mode at
> least
> I think you want to link particular channels to a TGPx choice
> 
> out_voltage0_raw
> out_voltage0_raw_available ( nice to have on DACs)

I guess here you mean 'IIO_AVAIL_RANGE'?

> out_voltage0_scale
> out_voltage0_dither_raw
> out_voltage0_dither_raw_available
> out_voltage0_dither_frequency
> out_voltage0_dither_frequency_available
> out_voltage0_dither_phase
> out_voltage0_dither_phase_available

> Toggle mode is less clear to me but symbol approach plus TGPx in DT
> maybe works
> You could allow for software override to set the symbol.  Interface to
> unset
> it being to write an empty string to _symbol. Maybe leave that for
> now.
> 
> out_voltage0_raw0
> out_voltage0_raw1
> out_voltage0_scale
> out_voltage0_symbol

Well, in short, I do agree with this ABI. And actually, for toggle mode, I think
this is more or less what we will have. For dither/default mode, there's still
the questions I raised above... Maybe, at the end, we will end up with 3 different
ABI's...

I would only add this to the ABI:
 * out_voltage0_dither_en
 * out_voltage0_toggle_en

Because if someone wants to change, let's say the dither frequency, the best way
to update things is to first disable dithering, update all the stuff, and then enable
it again...

- Nuno Sá

RE: [RFC PATCH 0/1] LTC2688 support

[Sa, Nuno]

> -----Original Message-----
> From: Sa, Nuno <Nuno.Sa@analog.com>
> Sent: Tuesday, November 30, 2021 3:43 PM
> To: Jonathan Cameron <jic23@kernel.org>
> Cc: linux-iio@vger.kernel.org
> Subject: RE: [RFC PATCH 0/1] LTC2688 support
> 
> [External]
> 
> 
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, November 21, 2021 1:18 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio@vger.kernel.org
> > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> >
> > [External]
> >
> > On Mon, 15 Nov 2021 10:28:51 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >
> > > Hi Jonathan,
> > >
> > > Thanks for your inputs...
> > >
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Friday, November 12, 2021 5:15 PM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: linux-iio@vger.kernel.org
> > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > >
> > > > [External]
> > > >
> > > > On Thu, 11 Nov 2021 12:00:42 +0100
> > > > Nuno Sá <nuno.sa@analog.com> wrote:
> > > >
> > > > Hi Nuno,
> > > >
> > > > > The reason why this is a RFC is because I'm still waiting for
> proper
> > HW
> > > > > to test this thing. The reason why I'm sending this already is
> > because
> > > > > there's some non usual features which might require extra ABI.
> > > > Hence,
> > > > > while waiting I thought we could already speed up the process
> in
> > > > regards
> > > > > with the ABI.
> > > >
> > > > Wise move as this is an unusual beast :)
> > > >
> > > > >
> > > > > I still pushed the driver but the intent here is not really to
> review
> > it.
> > > > > Naturally, if someone already wants to give some feedback,
> > that's
> > > > very
> > > > > much appreciated :)
> > > >
> > > > >
> > > > > Now, there are three main interfaces depending on the
> channel
> > > > mode:
> > > > >  1) default (no new ABI)
> > > > >  2) toggle mode
> > > > >  3) dither mode
> > > > >
> > > > > The channel mode will be a devicetree property as it does not
> > really
> > > > > make much sense to change between modes at runtime even
> > more
> > > > because the
> > > > > piece of HW we might want to control with a channel might be
> > > > different
> > > > > depending on the selected mode (I'm fairly sure on this
> between
> > > > toggle
> > > > > and other modes but not so sure between dither and default
> > mode).
> > > >
> > > > I agree on toggle vs dither definitely being different, but normal
> > you
> > > > could implement either as software toggle, or dither with a 0
> > > > magnitude
> > > > sine wave.  So might not be worth implementing default mode at
> > all.
> > > > No harm in doing so though if there are advantages to having it.
> > >
> > > My feeling is that we could probably have dither as the "default
> > mode".
> > > More on this below...
> > >
> > > > >
> > > > > toggle mode special ABI:
> > > > >
> > > > >  * Toggle operation enables fast switching of a DAC output
> > between
> > > > two
> > > > > different DAC codes without any SPI transaction. Two codes are
> > set
> > > > in
> > > > > input_a and input_b and then the output switches according to
> > an
> > > > input
> > > > > signal (input_a -> clk high; input_b -> clk low).
> > > > >
> > > > > out_voltageY_input_register
> > > > >  - selects between input_a and input_b. After selecting one
> > register,
> > > > we
> > > > >    can write the dac code in out_voltageY_raw.
> > > > > out_voltageY_toggle_en
> > > > >  - Disable/Enable toggle mode. The reason why I think this one
> is
> > > > >    important is because if we wanna change the 2 codes, we
> > should
> > > > first
> > > > >    disable this, set the codes and only then enable the mode
> > back...
> > > > >    As I'm writing this, It kind of came to me that we can probably
> > > > >    achieve this with out_voltageY_powerdown attr (maybe
> takes a
> > bit
> > > > more
> > > > >    time to see the outputs but...)
> > > >
> > > > Hmm. These corner cases always take a bit of figuring out.  What
> > you
> > > > have
> > > > here is a bit 'device specific' for my liking.
> > > >
> > > > So there is precedence for ABI in this area, on the frequency
> > devices
> > > > but only
> > > > for devices we still haven't moved out of staging.  For those we
> > > > needed a means
> > > > to define selectable phases for PSK - where the selection was
> > either
> > > > software or,
> > > > much like here, a selection pin.
> > > >
> > > > out_altvotage0_phase0 etc
> > > >
> > > > so I guess the equivalent here would be
> > > > out_voltageY_raw0
> > > > out_voltageY_raw1
> > > > and the selection would be via something like
> > > > out_voltageY_symbol (0 or 1 in this case). - note this is only
> > > > relevant if you have the software toggle. Any enable needed for
> > > > setting
> > > > can be done as part of the write sequence for the  raw0 / raw1
> and
> > > > should
> > > > ideally not be exposed to userspace (controls that require
> manual
> > > > sequencing
> > > > tend to be hard to use / document).
> > >
> > > Yeah, I thought about that. I was even thinking in having something
> > like
> > > *_amplitude for dither mode. In some cases, where we might be
> left
> > > in some non obvious state (eg: moved the select register to input b
> > and
> > > then we failed to set the code;), I prefer to leave things as flexible
> as
> > > possible for userspace. But I agree it adds up more complexity and
> in
> > > this case, I can just always go to 'input_a' when writing 'raw0'...
> > >
> > > > However, I'm not 100% sure it really maps to this case.  What do
> > you
> > > > think?
> > >
> > > I think it can work. Though out_voltageY_symbol probably needs to
> > be
> > > shared by type to be coherent with what we might have with
> TGPx.
> >
> > That's fine.
> >
> > > Note the sw_toggle register has a bit mask of the channels we
> want
> > to
> > > toggle which means we can toggle multiple channels at the same
> > time.
> >
> > Using that wired up to buffer mode might make sense.  You'd
> provide
> > multiple
> > buffers and allow channels to be selected into one of them at a time.
> > Each
> > buffer is then tied to a different toggle (TGP0, TGP1, etc)
> >
> > The same could be true for the software toggle.  It'll get a bit fiddly
> > though.
> >
> > Perhaps this is an advanced feature to think about later...
> >
> > > It works the same with TGPx if you map multiple channels to the
> > same
> > > pin.
> > >
> > > There's also the question on how to handle this if a TGPx is
> provided?
> > > I guess it will just override the pin... But most likely having them
> both
> > > at the same time will lead to non desired results (unless we have a
> > > way to gate/ungate the clocks)...
> > I don't follow this bit.  You mean TGPx and software toggle. As far as I
> > can
> > tell it's an either/or thing per channel.
> >
> 
> Here I meant that if we have a TGPx pin bundled to some channel(s)
> we
> do not want to also dance with the sw_toggle bit of that channel.

Just a note on this. After starting my tests with the device, I can actually
say that if we have a TGPx set in the channel settings register, the device
will pretty much ignore the sw_toggle settings for that channel. I could
see that the output voltage was not toggling at all. As soon as I removed
the TGPx setting, then dancing with the sw_toggle started to work. So, for
the HW this is not really an issue as it just ignores the sw_toggle. On a SW
perspective, I'm still not sure if I just ignore this and write whatever the
user sets or if I return some error code in the case a user tries to toggle
a channel with a binded TGPx. The first one is appealing as it makes the
code much simpler while OTHO it might make sense to be verbose here
otherwise the user might think something is happening when it isn't...

Anyways, I would argue that if someone has a pin wired, it's highly unlikely
that he cares about sw_toggling...

- Nuno Sá

Re: [RFC PATCH 0/1] LTC2688 support

[Jonathan Cameron]

On Thu, 2 Dec 2021 15:37:40 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Sa, Nuno <Nuno.Sa@analog.com>
> > Sent: Tuesday, November 30, 2021 3:43 PM
> > To: Jonathan Cameron <jic23@kernel.org>
> > Cc: linux-iio@vger.kernel.org
> > Subject: RE: [RFC PATCH 0/1] LTC2688 support
> > 
> > [External]
> > 
> > 
> >   
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Sunday, November 21, 2021 1:18 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio@vger.kernel.org
> > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > >
> > > [External]
> > >
> > > On Mon, 15 Nov 2021 10:28:51 +0000
> > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > >  
> > > > Hi Jonathan,
> > > >
> > > > Thanks for your inputs...
> > > >  
> > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > Sent: Friday, November 12, 2021 5:15 PM
> > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Cc: linux-iio@vger.kernel.org
> > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > > >
> > > > > [External]
> > > > >
> > > > > On Thu, 11 Nov 2021 12:00:42 +0100
> > > > > Nuno Sá <nuno.sa@analog.com> wrote:
> > > > >
> > > > > Hi Nuno,
> > > > >  
> > > > > > The reason why this is a RFC is because I'm still waiting for  
> > proper  
> > > HW  
> > > > > > to test this thing. The reason why I'm sending this already is  
> > > because  
> > > > > > there's some non usual features which might require extra ABI.  
> > > > > Hence,  
> > > > > > while waiting I thought we could already speed up the process  
> > in  
> > > > > regards  
> > > > > > with the ABI.  
> > > > >
> > > > > Wise move as this is an unusual beast :)
> > > > >  
> > > > > >
> > > > > > I still pushed the driver but the intent here is not really to  
> > review  
> > > it.  
> > > > > > Naturally, if someone already wants to give some feedback,  
> > > that's  
> > > > > very  
> > > > > > much appreciated :)  
> > > > >  
> > > > > >
> > > > > > Now, there are three main interfaces depending on the  
> > channel  
> > > > > mode:  
> > > > > >  1) default (no new ABI)
> > > > > >  2) toggle mode
> > > > > >  3) dither mode
> > > > > >
> > > > > > The channel mode will be a devicetree property as it does not  
> > > really  
> > > > > > make much sense to change between modes at runtime even  
> > > more  
> > > > > because the  
> > > > > > piece of HW we might want to control with a channel might be  
> > > > > different  
> > > > > > depending on the selected mode (I'm fairly sure on this  
> > between  
> > > > > toggle  
> > > > > > and other modes but not so sure between dither and default  
> > > mode).  
> > > > >
> > > > > I agree on toggle vs dither definitely being different, but normal  
> > > you  
> > > > > could implement either as software toggle, or dither with a 0
> > > > > magnitude
> > > > > sine wave.  So might not be worth implementing default mode at  
> > > all.  
> > > > > No harm in doing so though if there are advantages to having it.  
> > > >
> > > > My feeling is that we could probably have dither as the "default  
> > > mode".  
> > > > More on this below...
> > > >  
> > > > > >
> > > > > > toggle mode special ABI:
> > > > > >
> > > > > >  * Toggle operation enables fast switching of a DAC output  
> > > between  
> > > > > two  
> > > > > > different DAC codes without any SPI transaction. Two codes are  
> > > set  
> > > > > in  
> > > > > > input_a and input_b and then the output switches according to  
> > > an  
> > > > > input  
> > > > > > signal (input_a -> clk high; input_b -> clk low).
> > > > > >
> > > > > > out_voltageY_input_register
> > > > > >  - selects between input_a and input_b. After selecting one  
> > > register,  
> > > > > we  
> > > > > >    can write the dac code in out_voltageY_raw.
> > > > > > out_voltageY_toggle_en
> > > > > >  - Disable/Enable toggle mode. The reason why I think this one  
> > is  
> > > > > >    important is because if we wanna change the 2 codes, we  
> > > should  
> > > > > first  
> > > > > >    disable this, set the codes and only then enable the mode  
> > > back...  
> > > > > >    As I'm writing this, It kind of came to me that we can probably
> > > > > >    achieve this with out_voltageY_powerdown attr (maybe  
> > takes a  
> > > bit  
> > > > > more  
> > > > > >    time to see the outputs but...)  
> > > > >
> > > > > Hmm. These corner cases always take a bit of figuring out.  What  
> > > you  
> > > > > have
> > > > > here is a bit 'device specific' for my liking.
> > > > >
> > > > > So there is precedence for ABI in this area, on the frequency  
> > > devices  
> > > > > but only
> > > > > for devices we still haven't moved out of staging.  For those we
> > > > > needed a means
> > > > > to define selectable phases for PSK - where the selection was  
> > > either  
> > > > > software or,
> > > > > much like here, a selection pin.
> > > > >
> > > > > out_altvotage0_phase0 etc
> > > > >
> > > > > so I guess the equivalent here would be
> > > > > out_voltageY_raw0
> > > > > out_voltageY_raw1
> > > > > and the selection would be via something like
> > > > > out_voltageY_symbol (0 or 1 in this case). - note this is only
> > > > > relevant if you have the software toggle. Any enable needed for
> > > > > setting
> > > > > can be done as part of the write sequence for the  raw0 / raw1  
> > and  
> > > > > should
> > > > > ideally not be exposed to userspace (controls that require  
> > manual  
> > > > > sequencing
> > > > > tend to be hard to use / document).  
> > > >
> > > > Yeah, I thought about that. I was even thinking in having something  
> > > like  
> > > > *_amplitude for dither mode. In some cases, where we might be  
> > left  
> > > > in some non obvious state (eg: moved the select register to input b  
> > > and  
> > > > then we failed to set the code;), I prefer to leave things as flexible  
> > as  
> > > > possible for userspace. But I agree it adds up more complexity and  
> > in  
> > > > this case, I can just always go to 'input_a' when writing 'raw0'...
> > > >  
> > > > > However, I'm not 100% sure it really maps to this case.  What do  
> > > you  
> > > > > think?  
> > > >
> > > > I think it can work. Though out_voltageY_symbol probably needs to  
> > > be  
> > > > shared by type to be coherent with what we might have with  
> > TGPx.  
> > >
> > > That's fine.
> > >  
> > > > Note the sw_toggle register has a bit mask of the channels we  
> > want  
> > > to  
> > > > toggle which means we can toggle multiple channels at the same  
> > > time.
> > >
> > > Using that wired up to buffer mode might make sense.  You'd  
> > provide  
> > > multiple
> > > buffers and allow channels to be selected into one of them at a time.
> > > Each
> > > buffer is then tied to a different toggle (TGP0, TGP1, etc)
> > >
> > > The same could be true for the software toggle.  It'll get a bit fiddly
> > > though.
> > >
> > > Perhaps this is an advanced feature to think about later...
> > >  
> > > > It works the same with TGPx if you map multiple channels to the  
> > > same  
> > > > pin.
> > > >
> > > > There's also the question on how to handle this if a TGPx is  
> > provided?  
> > > > I guess it will just override the pin... But most likely having them  
> > both  
> > > > at the same time will lead to non desired results (unless we have a
> > > > way to gate/ungate the clocks)...  
> > > I don't follow this bit.  You mean TGPx and software toggle. As far as I
> > > can
> > > tell it's an either/or thing per channel.
> > >  
> > 
> > Here I meant that if we have a TGPx pin bundled to some channel(s)
> > we
> > do not want to also dance with the sw_toggle bit of that channel.  
> 
> Just a note on this. After starting my tests with the device, I can actually
> say that if we have a TGPx set in the channel settings register, the device
> will pretty much ignore the sw_toggle settings for that channel. I could
> see that the output voltage was not toggling at all. As soon as I removed
> the TGPx setting, then dancing with the sw_toggle started to work. So, for
> the HW this is not really an issue as it just ignores the sw_toggle. On a SW
> perspective, I'm still not sure if I just ignore this and write whatever the
> user sets or if I return some error code in the case a user tries to toggle
> a channel with a binded TGPx. The first one is appealing as it makes the
> code much simpler while OTHO it might make sense to be verbose here
> otherwise the user might think something is happening when it isn't...

If we are in a static configuration (see below) then just don't expose the
software toggle control.  Not having a big red button to press is the best way to
tell userspace to not press the big red button...
> 
> Anyways, I would argue that if someone has a pin wired, it's highly unlikely
> that he cares about sw_toggling...

I'd agree if there was one to one mapping from TGPx to channel.  Given
it's highly configurable, they 'might' want to set the mapping differently
at runtime.  I'm fine if we don't support that option until someone asks
though.

Jonathan


> 
> - Nuno Sá
> 

RE: [RFC PATCH 0/1] LTC2688 support

[Sa, Nuno]

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, December 5, 2021 7:04 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org
> Subject: Re: [RFC PATCH 0/1] LTC2688 support
> 
> [External]
> 
> On Thu, 2 Dec 2021 15:37:40 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Sa, Nuno <Nuno.Sa@analog.com>
> > > Sent: Tuesday, November 30, 2021 3:43 PM
> > > To: Jonathan Cameron <jic23@kernel.org>
> > > Cc: linux-iio@vger.kernel.org
> > > Subject: RE: [RFC PATCH 0/1] LTC2688 support
> > >
> > > [External]
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Sunday, November 21, 2021 1:18 PM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: linux-iio@vger.kernel.org
> > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > >
> > > > [External]
> > > >
> > > > On Mon, 15 Nov 2021 10:28:51 +0000
> > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > > >
> > > > > Hi Jonathan,
> > > > >
> > > > > Thanks for your inputs...
> > > > >
> > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > Sent: Friday, November 12, 2021 5:15 PM
> > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > Cc: linux-iio@vger.kernel.org
> > > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > > > >
> > > > > > [External]
> > > > > >
> > > > > > On Thu, 11 Nov 2021 12:00:42 +0100
> > > > > > Nuno Sá <nuno.sa@analog.com> wrote:
> > > > > >
> > > > > > Hi Nuno,
> > > > > >
> > > > > > > The reason why this is a RFC is because I'm still waiting for
> > > proper
> > > > HW
> > > > > > > to test this thing. The reason why I'm sending this already is
> > > > because
> > > > > > > there's some non usual features which might require extra
> ABI.
> > > > > > Hence,
> > > > > > > while waiting I thought we could already speed up the
> process
> > > in
> > > > > > regards
> > > > > > > with the ABI.
> > > > > >
> > > > > > Wise move as this is an unusual beast :)
> > > > > >
> > > > > > >
> > > > > > > I still pushed the driver but the intent here is not really to
> > > review
> > > > it.
> > > > > > > Naturally, if someone already wants to give some feedback,
> > > > that's
> > > > > > very
> > > > > > > much appreciated :)
> > > > > >
> > > > > > >
> > > > > > > Now, there are three main interfaces depending on the
> > > channel
> > > > > > mode:
> > > > > > >  1) default (no new ABI)
> > > > > > >  2) toggle mode
> > > > > > >  3) dither mode
> > > > > > >
> > > > > > > The channel mode will be a devicetree property as it does
> not
> > > > really
> > > > > > > make much sense to change between modes at runtime
> even
> > > > more
> > > > > > because the
> > > > > > > piece of HW we might want to control with a channel might
> be
> > > > > > different
> > > > > > > depending on the selected mode (I'm fairly sure on this
> > > between
> > > > > > toggle
> > > > > > > and other modes but not so sure between dither and
> default
> > > > mode).
> > > > > >
> > > > > > I agree on toggle vs dither definitely being different, but
> normal
> > > > you
> > > > > > could implement either as software toggle, or dither with a 0
> > > > > > magnitude
> > > > > > sine wave.  So might not be worth implementing default
> mode at
> > > > all.
> > > > > > No harm in doing so though if there are advantages to having
> it.
> > > > >
> > > > > My feeling is that we could probably have dither as the "default
> > > > mode".
> > > > > More on this below...
> > > > >
> > > > > > >
> > > > > > > toggle mode special ABI:
> > > > > > >
> > > > > > >  * Toggle operation enables fast switching of a DAC output
> > > > between
> > > > > > two
> > > > > > > different DAC codes without any SPI transaction. Two codes
> are
> > > > set
> > > > > > in
> > > > > > > input_a and input_b and then the output switches
> according to
> > > > an
> > > > > > input
> > > > > > > signal (input_a -> clk high; input_b -> clk low).
> > > > > > >
> > > > > > > out_voltageY_input_register
> > > > > > >  - selects between input_a and input_b. After selecting one
> > > > register,
> > > > > > we
> > > > > > >    can write the dac code in out_voltageY_raw.
> > > > > > > out_voltageY_toggle_en
> > > > > > >  - Disable/Enable toggle mode. The reason why I think this
> one
> > > is
> > > > > > >    important is because if we wanna change the 2 codes, we
> > > > should
> > > > > > first
> > > > > > >    disable this, set the codes and only then enable the mode
> > > > back...
> > > > > > >    As I'm writing this, It kind of came to me that we can
> probably
> > > > > > >    achieve this with out_voltageY_powerdown attr (maybe
> > > takes a
> > > > bit
> > > > > > more
> > > > > > >    time to see the outputs but...)
> > > > > >
> > > > > > Hmm. These corner cases always take a bit of figuring out.
> What
> > > > you
> > > > > > have
> > > > > > here is a bit 'device specific' for my liking.
> > > > > >
> > > > > > So there is precedence for ABI in this area, on the frequency
> > > > devices
> > > > > > but only
> > > > > > for devices we still haven't moved out of staging.  For those
> we
> > > > > > needed a means
> > > > > > to define selectable phases for PSK - where the selection was
> > > > either
> > > > > > software or,
> > > > > > much like here, a selection pin.
> > > > > >
> > > > > > out_altvotage0_phase0 etc
> > > > > >
> > > > > > so I guess the equivalent here would be
> > > > > > out_voltageY_raw0
> > > > > > out_voltageY_raw1
> > > > > > and the selection would be via something like
> > > > > > out_voltageY_symbol (0 or 1 in this case). - note this is only
> > > > > > relevant if you have the software toggle. Any enable needed
> for
> > > > > > setting
> > > > > > can be done as part of the write sequence for the  raw0 /
> raw1
> > > and
> > > > > > should
> > > > > > ideally not be exposed to userspace (controls that require
> > > manual
> > > > > > sequencing
> > > > > > tend to be hard to use / document).
> > > > >
> > > > > Yeah, I thought about that. I was even thinking in having
> something
> > > > like
> > > > > *_amplitude for dither mode. In some cases, where we might
> be
> > > left
> > > > > in some non obvious state (eg: moved the select register to
> input b
> > > > and
> > > > > then we failed to set the code;), I prefer to leave things as
> flexible
> > > as
> > > > > possible for userspace. But I agree it adds up more complexity
> and
> > > in
> > > > > this case, I can just always go to 'input_a' when writing 'raw0'...
> > > > >
> > > > > > However, I'm not 100% sure it really maps to this case.  What
> do
> > > > you
> > > > > > think?
> > > > >
> > > > > I think it can work. Though out_voltageY_symbol probably
> needs to
> > > > be
> > > > > shared by type to be coherent with what we might have with
> > > TGPx.
> > > >
> > > > That's fine.
> > > >
> > > > > Note the sw_toggle register has a bit mask of the channels we
> > > want
> > > > to
> > > > > toggle which means we can toggle multiple channels at the
> same
> > > > time.
> > > >
> > > > Using that wired up to buffer mode might make sense.  You'd
> > > provide
> > > > multiple
> > > > buffers and allow channels to be selected into one of them at a
> time.
> > > > Each
> > > > buffer is then tied to a different toggle (TGP0, TGP1, etc)
> > > >
> > > > The same could be true for the software toggle.  It'll get a bit
> fiddly
> > > > though.
> > > >
> > > > Perhaps this is an advanced feature to think about later...
> > > >
> > > > > It works the same with TGPx if you map multiple channels to
> the
> > > > same
> > > > > pin.
> > > > >
> > > > > There's also the question on how to handle this if a TGPx is
> > > provided?
> > > > > I guess it will just override the pin... But most likely having them
> > > both
> > > > > at the same time will lead to non desired results (unless we
> have a
> > > > > way to gate/ungate the clocks)...
> > > > I don't follow this bit.  You mean TGPx and software toggle. As far
> as I
> > > > can
> > > > tell it's an either/or thing per channel.
> > > >
> > >
> > > Here I meant that if we have a TGPx pin bundled to some
> channel(s)
> > > we
> > > do not want to also dance with the sw_toggle bit of that channel.
> >
> > Just a note on this. After starting my tests with the device, I can
> actually
> > say that if we have a TGPx set in the channel settings register, the
> device
> > will pretty much ignore the sw_toggle settings for that channel. I
> could
> > see that the output voltage was not toggling at all. As soon as I
> removed
> > the TGPx setting, then dancing with the sw_toggle started to work.
> So, for
> > the HW this is not really an issue as it just ignores the sw_toggle. On a
> SW
> > perspective, I'm still not sure if I just ignore this and write whatever
> the
> > user sets or if I return some error code in the case a user tries to
> toggle
> > a channel with a binded TGPx. The first one is appealing as it makes
> the
> > code much simpler while OTHO it might make sense to be verbose
> here
> > otherwise the user might think something is happening when it
> isn't...
> 
> If we are in a static configuration (see below) then just don't expose
> the
> software toggle control.  Not having a big red button to press is the
> best way to
> tell userspace to not press the big red button...


Hmm, I get your point and that's valid if I have the sw_toggle as a per
channel attribute. Right now, I'm doing it as shared_by_type. The reason is
the sw_toggling is done by writing 1/0 in the toggle register and that register
is a bitmask being the mask 16bits wide. This allows you to toggle channels
at the same time in the same way you can do it if, say, you map 2,3 or more
channels to the same TGPx pin.

However, I'm also not happy for having this as shared_by_type attr. One of
my complains is that it makes it look like a dither capable channel will also
support this (and we already agreed that sw_toggle does not make sense
for dither mode; so do not expose it). For instance the output of 
'iio_attr'  on a dither enabled channel is:

```
root@analog:~# iio_attr -c ltc2688 voltage0
dev 'ltc2688', channel 'voltage0' (output), attr 'calibbias', value '0'
dev 'ltc2688', channel 'voltage0' (output), attr 'calibscale', value '0'
dev 'ltc2688', channel 'voltage0' (output), attr 'dither_en', value '0'
dev 'ltc2688', channel 'voltage0' (output), attr 'dither_frequency', value '32768'
dev 'ltc2688', channel 'voltage0' (output), attr 'dither_frequency_available', value '32768 16384 8192 4096 2048'
dev 'ltc2688', channel 'voltage0' (output), attr 'dither_phase', value '0'
dev 'ltc2688', channel 'voltage0' (output), attr 'dither_phase_available', value '0 90 180 270'
dev 'ltc2688', channel 'voltage0' (output), attr 'dither_raw', value '0'
dev 'ltc2688', channel 'voltage0' (output), attr 'dither_raw_available', value '[0 1 65535]'
dev 'ltc2688', channel 'voltage0' (output), attr 'offset', value '0'
dev 'ltc2688', channel 'voltage0' (output), attr 'powerdown', value '0'
dev 'ltc2688', channel 'voltage0' (output), attr 'raw', value '0'
dev 'ltc2688', channel 'voltage0' (output), attr 'raw_available', value '[0 1 65535]'
dev 'ltc2688', channel 'voltage0' (output), attr 'scale', value '0.076293945'
dev 'ltc2688', channel 'voltage0' (output), attr 'symbol', value '0'
```

So you can see that symbol attr which does not make sense to be there. And it's
definitely not something wrong in the iio_attr app as the attr is shared by type.

Also, as you suggested, not having the symbol attr when it does not make sense
to have it also makes a lot of sense to me and that is one more plus point to have
this as a per channel thing.

Anyways, I will probably send the patch with things as I have now so you can
have a felling of how it looks like. Unless you already tell me to just not have it
as a shared_by_type attr (which I'm getting more and more convinced on my own;
I guess I just need an extra push :D).

- Nuno Sá

Re: [RFC PATCH 0/1] LTC2688 support

[Jonathan Cameron]

On Mon, 6 Dec 2021 11:07:55 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, December 5, 2021 7:04 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio@vger.kernel.org
> > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > 
> > [External]
> > 
> > On Thu, 2 Dec 2021 15:37:40 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Sent: Tuesday, November 30, 2021 3:43 PM
> > > > To: Jonathan Cameron <jic23@kernel.org>
> > > > Cc: linux-iio@vger.kernel.org
> > > > Subject: RE: [RFC PATCH 0/1] LTC2688 support
> > > >
> > > > [External]
> > > >
> > > >
> > > >  
> > > > > -----Original Message-----
> > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > Sent: Sunday, November 21, 2021 1:18 PM
> > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Cc: linux-iio@vger.kernel.org
> > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > > >
> > > > > [External]
> > > > >
> > > > > On Mon, 15 Nov 2021 10:28:51 +0000
> > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > > > >  
> > > > > > Hi Jonathan,
> > > > > >
> > > > > > Thanks for your inputs...
> > > > > >  
> > > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > > Sent: Friday, November 12, 2021 5:15 PM
> > > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > > Cc: linux-iio@vger.kernel.org
> > > > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > > > > >
> > > > > > > [External]
> > > > > > >
> > > > > > > On Thu, 11 Nov 2021 12:00:42 +0100
> > > > > > > Nuno Sá <nuno.sa@analog.com> wrote:
> > > > > > >
> > > > > > > Hi Nuno,
> > > > > > >  
> > > > > > > > The reason why this is a RFC is because I'm still waiting for  
> > > > proper  
> > > > > HW  
> > > > > > > > to test this thing. The reason why I'm sending this already is  
> > > > > because  
> > > > > > > > there's some non usual features which might require extra  
> > ABI.  
> > > > > > > Hence,  
> > > > > > > > while waiting I thought we could already speed up the  
> > process  
> > > > in  
> > > > > > > regards  
> > > > > > > > with the ABI.  
> > > > > > >
> > > > > > > Wise move as this is an unusual beast :)
> > > > > > >  
> > > > > > > >
> > > > > > > > I still pushed the driver but the intent here is not really to  
> > > > review  
> > > > > it.  
> > > > > > > > Naturally, if someone already wants to give some feedback,  
> > > > > that's  
> > > > > > > very  
> > > > > > > > much appreciated :)  
> > > > > > >  
> > > > > > > >
> > > > > > > > Now, there are three main interfaces depending on the  
> > > > channel  
> > > > > > > mode:  
> > > > > > > >  1) default (no new ABI)
> > > > > > > >  2) toggle mode
> > > > > > > >  3) dither mode
> > > > > > > >
> > > > > > > > The channel mode will be a devicetree property as it does  
> > not  
> > > > > really  
> > > > > > > > make much sense to change between modes at runtime  
> > even  
> > > > > more  
> > > > > > > because the  
> > > > > > > > piece of HW we might want to control with a channel might  
> > be  
> > > > > > > different  
> > > > > > > > depending on the selected mode (I'm fairly sure on this  
> > > > between  
> > > > > > > toggle  
> > > > > > > > and other modes but not so sure between dither and  
> > default  
> > > > > mode).  
> > > > > > >
> > > > > > > I agree on toggle vs dither definitely being different, but  
> > normal  
> > > > > you  
> > > > > > > could implement either as software toggle, or dither with a 0
> > > > > > > magnitude
> > > > > > > sine wave.  So might not be worth implementing default  
> > mode at  
> > > > > all.  
> > > > > > > No harm in doing so though if there are advantages to having  
> > it.  
> > > > > >
> > > > > > My feeling is that we could probably have dither as the "default  
> > > > > mode".  
> > > > > > More on this below...
> > > > > >  
> > > > > > > >
> > > > > > > > toggle mode special ABI:
> > > > > > > >
> > > > > > > >  * Toggle operation enables fast switching of a DAC output  
> > > > > between  
> > > > > > > two  
> > > > > > > > different DAC codes without any SPI transaction. Two codes  
> > are  
> > > > > set  
> > > > > > > in  
> > > > > > > > input_a and input_b and then the output switches  
> > according to  
> > > > > an  
> > > > > > > input  
> > > > > > > > signal (input_a -> clk high; input_b -> clk low).
> > > > > > > >
> > > > > > > > out_voltageY_input_register
> > > > > > > >  - selects between input_a and input_b. After selecting one  
> > > > > register,  
> > > > > > > we  
> > > > > > > >    can write the dac code in out_voltageY_raw.
> > > > > > > > out_voltageY_toggle_en
> > > > > > > >  - Disable/Enable toggle mode. The reason why I think this  
> > one  
> > > > is  
> > > > > > > >    important is because if we wanna change the 2 codes, we  
> > > > > should  
> > > > > > > first  
> > > > > > > >    disable this, set the codes and only then enable the mode  
> > > > > back...  
> > > > > > > >    As I'm writing this, It kind of came to me that we can  
> > probably  
> > > > > > > >    achieve this with out_voltageY_powerdown attr (maybe  
> > > > takes a  
> > > > > bit  
> > > > > > > more  
> > > > > > > >    time to see the outputs but...)  
> > > > > > >
> > > > > > > Hmm. These corner cases always take a bit of figuring out.  
> > What  
> > > > > you  
> > > > > > > have
> > > > > > > here is a bit 'device specific' for my liking.
> > > > > > >
> > > > > > > So there is precedence for ABI in this area, on the frequency  
> > > > > devices  
> > > > > > > but only
> > > > > > > for devices we still haven't moved out of staging.  For those  
> > we  
> > > > > > > needed a means
> > > > > > > to define selectable phases for PSK - where the selection was  
> > > > > either  
> > > > > > > software or,
> > > > > > > much like here, a selection pin.
> > > > > > >
> > > > > > > out_altvotage0_phase0 etc
> > > > > > >
> > > > > > > so I guess the equivalent here would be
> > > > > > > out_voltageY_raw0
> > > > > > > out_voltageY_raw1
> > > > > > > and the selection would be via something like
> > > > > > > out_voltageY_symbol (0 or 1 in this case). - note this is only
> > > > > > > relevant if you have the software toggle. Any enable needed  
> > for  
> > > > > > > setting
> > > > > > > can be done as part of the write sequence for the  raw0 /  
> > raw1  
> > > > and  
> > > > > > > should
> > > > > > > ideally not be exposed to userspace (controls that require  
> > > > manual  
> > > > > > > sequencing
> > > > > > > tend to be hard to use / document).  
> > > > > >
> > > > > > Yeah, I thought about that. I was even thinking in having  
> > something  
> > > > > like  
> > > > > > *_amplitude for dither mode. In some cases, where we might  
> > be  
> > > > left  
> > > > > > in some non obvious state (eg: moved the select register to  
> > input b  
> > > > > and  
> > > > > > then we failed to set the code;), I prefer to leave things as  
> > flexible  
> > > > as  
> > > > > > possible for userspace. But I agree it adds up more complexity  
> > and  
> > > > in  
> > > > > > this case, I can just always go to 'input_a' when writing 'raw0'...
> > > > > >  
> > > > > > > However, I'm not 100% sure it really maps to this case.  What  
> > do  
> > > > > you  
> > > > > > > think?  
> > > > > >
> > > > > > I think it can work. Though out_voltageY_symbol probably  
> > needs to  
> > > > > be  
> > > > > > shared by type to be coherent with what we might have with  
> > > > TGPx.  
> > > > >
> > > > > That's fine.
> > > > >  
> > > > > > Note the sw_toggle register has a bit mask of the channels we  
> > > > want  
> > > > > to  
> > > > > > toggle which means we can toggle multiple channels at the  
> > same  
> > > > > time.
> > > > >
> > > > > Using that wired up to buffer mode might make sense.  You'd  
> > > > provide  
> > > > > multiple
> > > > > buffers and allow channels to be selected into one of them at a  
> > time.  
> > > > > Each
> > > > > buffer is then tied to a different toggle (TGP0, TGP1, etc)
> > > > >
> > > > > The same could be true for the software toggle.  It'll get a bit  
> > fiddly  
> > > > > though.
> > > > >
> > > > > Perhaps this is an advanced feature to think about later...
> > > > >  
> > > > > > It works the same with TGPx if you map multiple channels to  
> > the  
> > > > > same  
> > > > > > pin.
> > > > > >
> > > > > > There's also the question on how to handle this if a TGPx is  
> > > > provided?  
> > > > > > I guess it will just override the pin... But most likely having them  
> > > > both  
> > > > > > at the same time will lead to non desired results (unless we  
> > have a  
> > > > > > way to gate/ungate the clocks)...  
> > > > > I don't follow this bit.  You mean TGPx and software toggle. As far  
> > as I  
> > > > > can
> > > > > tell it's an either/or thing per channel.
> > > > >  
> > > >
> > > > Here I meant that if we have a TGPx pin bundled to some  
> > channel(s)  
> > > > we
> > > > do not want to also dance with the sw_toggle bit of that channel.  
> > >
> > > Just a note on this. After starting my tests with the device, I can  
> > actually  
> > > say that if we have a TGPx set in the channel settings register, the  
> > device  
> > > will pretty much ignore the sw_toggle settings for that channel. I  
> > could  
> > > see that the output voltage was not toggling at all. As soon as I  
> > removed  
> > > the TGPx setting, then dancing with the sw_toggle started to work.  
> > So, for  
> > > the HW this is not really an issue as it just ignores the sw_toggle. On a  
> > SW  
> > > perspective, I'm still not sure if I just ignore this and write whatever  
> > the  
> > > user sets or if I return some error code in the case a user tries to  
> > toggle  
> > > a channel with a binded TGPx. The first one is appealing as it makes  
> > the  
> > > code much simpler while OTHO it might make sense to be verbose  
> > here  
> > > otherwise the user might think something is happening when it  
> > isn't...
> > 
> > If we are in a static configuration (see below) then just don't expose
> > the
> > software toggle control.  Not having a big red button to press is the
> > best way to
> > tell userspace to not press the big red button...  
> 
> 
> Hmm, I get your point and that's valid if I have the sw_toggle as a per
> channel attribute. Right now, I'm doing it as shared_by_type. The reason is
> the sw_toggling is done by writing 1/0 in the toggle register and that register
> is a bitmask being the mask 16bits wide. This allows you to toggle channels
> at the same time in the same way you can do it if, say, you map 2,3 or more
> channels to the same TGPx pin.

Hmm. That will be tricky to support in a remotely 'general' way.

> 
> However, I'm also not happy for having this as shared_by_type attr. One of
> my complains is that it makes it look like a dither capable channel will also
> support this (and we already agreed that sw_toggle does not make sense
> for dither mode; so do not expose it). For instance the output of 
> 'iio_attr'  on a dither enabled channel is:
> 
> ```
> root@analog:~# iio_attr -c ltc2688 voltage0
> dev 'ltc2688', channel 'voltage0' (output), attr 'calibbias', value '0'
> dev 'ltc2688', channel 'voltage0' (output), attr 'calibscale', value '0'
> dev 'ltc2688', channel 'voltage0' (output), attr 'dither_en', value '0'
> dev 'ltc2688', channel 'voltage0' (output), attr 'dither_frequency', value '32768'
> dev 'ltc2688', channel 'voltage0' (output), attr 'dither_frequency_available', value '32768 16384 8192 4096 2048'
> dev 'ltc2688', channel 'voltage0' (output), attr 'dither_phase', value '0'
> dev 'ltc2688', channel 'voltage0' (output), attr 'dither_phase_available', value '0 90 180 270'
> dev 'ltc2688', channel 'voltage0' (output), attr 'dither_raw', value '0'
> dev 'ltc2688', channel 'voltage0' (output), attr 'dither_raw_available', value '[0 1 65535]'
> dev 'ltc2688', channel 'voltage0' (output), attr 'offset', value '0'
> dev 'ltc2688', channel 'voltage0' (output), attr 'powerdown', value '0'
> dev 'ltc2688', channel 'voltage0' (output), attr 'raw', value '0'
> dev 'ltc2688', channel 'voltage0' (output), attr 'raw_available', value '[0 1 65535]'
> dev 'ltc2688', channel 'voltage0' (output), attr 'scale', value '0.076293945'
> dev 'ltc2688', channel 'voltage0' (output), attr 'symbol', value '0'
> ```
> 
> So you can see that symbol attr which does not make sense to be there. And it's
> definitely not something wrong in the iio_attr app as the attr is shared by type.
> 
> Also, as you suggested, not having the symbol attr when it does not make sense
> to have it also makes a lot of sense to me and that is one more plus point to have
> this as a per channel thing.
> 
> Anyways, I will probably send the patch with things as I have now so you can
> have a felling of how it looks like. Unless you already tell me to just not have it
> as a shared_by_type attr (which I'm getting more and more convinced on my own;
> I guess I just need an extra push :D).

Shared by type indeed doesn't work as it's a subset - however we would need
a means to indicate what subset is used if we want to allow single write
to toggle multiple.   Mind you - the moment we get to multiple channels
this should probably be using the chrdev route rather than sysfs and I'm
not sure how that would map to this at all.

So for now maybe take the view that software control of this is a weird feature
anyway so make it per channel?

Jonathan

> 
> - Nuno Sá

RE: [RFC PATCH 0/1] LTC2688 support

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Monday, December 6, 2021 2:10 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; linux-iio@vger.kernel.org
> Subject: Re: [RFC PATCH 0/1] LTC2688 support
> 
> [External]
> 
> On Mon, 6 Dec 2021 11:07:55 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Sunday, December 5, 2021 7:04 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio@vger.kernel.org
> > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > >
> > > [External]
> > >
> > > On Thu, 2 Dec 2021 15:37:40 +0000
> > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Sent: Tuesday, November 30, 2021 3:43 PM
> > > > > To: Jonathan Cameron <jic23@kernel.org>
> > > > > Cc: linux-iio@vger.kernel.org
> > > > > Subject: RE: [RFC PATCH 0/1] LTC2688 support
> > > > >
> > > > > [External]
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > Sent: Sunday, November 21, 2021 1:18 PM
> > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > Cc: linux-iio@vger.kernel.org
> > > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > > > >
> > > > > > [External]
> > > > > >
> > > > > > On Mon, 15 Nov 2021 10:28:51 +0000
> > > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > > > > >
> > > > > > > Hi Jonathan,
> > > > > > >
> > > > > > > Thanks for your inputs...
> > > > > > >
> > > > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > > > Sent: Friday, November 12, 2021 5:15 PM
> > > > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > > > Cc: linux-iio@vger.kernel.org
> > > > > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > > > > > >
> > > > > > > > [External]
> > > > > > > >
> > > > > > > > On Thu, 11 Nov 2021 12:00:42 +0100
> > > > > > > > Nuno Sá <nuno.sa@analog.com> wrote:
> > > > > > > >
> > > > > > > > Hi Nuno,
> > > > > > > >
> > > > > > > > > The reason why this is a RFC is because I'm still waiting
> for
> > > > > proper
> > > > > > HW
> > > > > > > > > to test this thing. The reason why I'm sending this
> already is
> > > > > > because
> > > > > > > > > there's some non usual features which might require
> extra
> > > ABI.
> > > > > > > > Hence,
> > > > > > > > > while waiting I thought we could already speed up the
> > > process
> > > > > in
> > > > > > > > regards
> > > > > > > > > with the ABI.
> > > > > > > >
> > > > > > > > Wise move as this is an unusual beast :)
> > > > > > > >
> > > > > > > > >
> > > > > > > > > I still pushed the driver but the intent here is not really
> to
> > > > > review
> > > > > > it.
> > > > > > > > > Naturally, if someone already wants to give some
> feedback,
> > > > > > that's
> > > > > > > > very
> > > > > > > > > much appreciated :)
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Now, there are three main interfaces depending on the
> > > > > channel
> > > > > > > > mode:
> > > > > > > > >  1) default (no new ABI)
> > > > > > > > >  2) toggle mode
> > > > > > > > >  3) dither mode
> > > > > > > > >
> > > > > > > > > The channel mode will be a devicetree property as it
> does
> > > not
> > > > > > really
> > > > > > > > > make much sense to change between modes at runtime
> > > even
> > > > > > more
> > > > > > > > because the
> > > > > > > > > piece of HW we might want to control with a channel
> might
> > > be
> > > > > > > > different
> > > > > > > > > depending on the selected mode (I'm fairly sure on this
> > > > > between
> > > > > > > > toggle
> > > > > > > > > and other modes but not so sure between dither and
> > > default
> > > > > > mode).
> > > > > > > >
> > > > > > > > I agree on toggle vs dither definitely being different, but
> > > normal
> > > > > > you
> > > > > > > > could implement either as software toggle, or dither with
> a 0
> > > > > > > > magnitude
> > > > > > > > sine wave.  So might not be worth implementing default
> > > mode at
> > > > > > all.
> > > > > > > > No harm in doing so though if there are advantages to
> having
> > > it.
> > > > > > >
> > > > > > > My feeling is that we could probably have dither as the
> "default
> > > > > > mode".
> > > > > > > More on this below...
> > > > > > >
> > > > > > > > >
> > > > > > > > > toggle mode special ABI:
> > > > > > > > >
> > > > > > > > >  * Toggle operation enables fast switching of a DAC
> output
> > > > > > between
> > > > > > > > two
> > > > > > > > > different DAC codes without any SPI transaction. Two
> codes
> > > are
> > > > > > set
> > > > > > > > in
> > > > > > > > > input_a and input_b and then the output switches
> > > according to
> > > > > > an
> > > > > > > > input
> > > > > > > > > signal (input_a -> clk high; input_b -> clk low).
> > > > > > > > >
> > > > > > > > > out_voltageY_input_register
> > > > > > > > >  - selects between input_a and input_b. After selecting
> one
> > > > > > register,
> > > > > > > > we
> > > > > > > > >    can write the dac code in out_voltageY_raw.
> > > > > > > > > out_voltageY_toggle_en
> > > > > > > > >  - Disable/Enable toggle mode. The reason why I think
> this
> > > one
> > > > > is
> > > > > > > > >    important is because if we wanna change the 2 codes,
> we
> > > > > > should
> > > > > > > > first
> > > > > > > > >    disable this, set the codes and only then enable the
> mode
> > > > > > back...
> > > > > > > > >    As I'm writing this, It kind of came to me that we can
> > > probably
> > > > > > > > >    achieve this with out_voltageY_powerdown attr
> (maybe
> > > > > takes a
> > > > > > bit
> > > > > > > > more
> > > > > > > > >    time to see the outputs but...)
> > > > > > > >
> > > > > > > > Hmm. These corner cases always take a bit of figuring out.
> > > What
> > > > > > you
> > > > > > > > have
> > > > > > > > here is a bit 'device specific' for my liking.
> > > > > > > >
> > > > > > > > So there is precedence for ABI in this area, on the
> frequency
> > > > > > devices
> > > > > > > > but only
> > > > > > > > for devices we still haven't moved out of staging.  For
> those
> > > we
> > > > > > > > needed a means
> > > > > > > > to define selectable phases for PSK - where the selection
> was
> > > > > > either
> > > > > > > > software or,
> > > > > > > > much like here, a selection pin.
> > > > > > > >
> > > > > > > > out_altvotage0_phase0 etc
> > > > > > > >
> > > > > > > > so I guess the equivalent here would be
> > > > > > > > out_voltageY_raw0
> > > > > > > > out_voltageY_raw1
> > > > > > > > and the selection would be via something like
> > > > > > > > out_voltageY_symbol (0 or 1 in this case). - note this is
> only
> > > > > > > > relevant if you have the software toggle. Any enable
> needed
> > > for
> > > > > > > > setting
> > > > > > > > can be done as part of the write sequence for the  raw0 /
> > > raw1
> > > > > and
> > > > > > > > should
> > > > > > > > ideally not be exposed to userspace (controls that require
> > > > > manual
> > > > > > > > sequencing
> > > > > > > > tend to be hard to use / document).
> > > > > > >
> > > > > > > Yeah, I thought about that. I was even thinking in having
> > > something
> > > > > > like
> > > > > > > *_amplitude for dither mode. In some cases, where we
> might
> > > be
> > > > > left
> > > > > > > in some non obvious state (eg: moved the select register to
> > > input b
> > > > > > and
> > > > > > > then we failed to set the code;), I prefer to leave things as
> > > flexible
> > > > > as
> > > > > > > possible for userspace. But I agree it adds up more
> complexity
> > > and
> > > > > in
> > > > > > > this case, I can just always go to 'input_a' when writing
> 'raw0'...
> > > > > > >
> > > > > > > > However, I'm not 100% sure it really maps to this case.
> What
> > > do
> > > > > > you
> > > > > > > > think?
> > > > > > >
> > > > > > > I think it can work. Though out_voltageY_symbol probably
> > > needs to
> > > > > > be
> > > > > > > shared by type to be coherent with what we might have
> with
> > > > > TGPx.
> > > > > >
> > > > > > That's fine.
> > > > > >
> > > > > > > Note the sw_toggle register has a bit mask of the channels
> we
> > > > > want
> > > > > > to
> > > > > > > toggle which means we can toggle multiple channels at the
> > > same
> > > > > > time.
> > > > > >
> > > > > > Using that wired up to buffer mode might make sense.  You'd
> > > > > provide
> > > > > > multiple
> > > > > > buffers and allow channels to be selected into one of them at
> a
> > > time.
> > > > > > Each
> > > > > > buffer is then tied to a different toggle (TGP0, TGP1, etc)
> > > > > >
> > > > > > The same could be true for the software toggle.  It'll get a bit
> > > fiddly
> > > > > > though.
> > > > > >
> > > > > > Perhaps this is an advanced feature to think about later...
> > > > > >
> > > > > > > It works the same with TGPx if you map multiple channels
> to
> > > the
> > > > > > same
> > > > > > > pin.
> > > > > > >
> > > > > > > There's also the question on how to handle this if a TGPx is
> > > > > provided?
> > > > > > > I guess it will just override the pin... But most likely having
> them
> > > > > both
> > > > > > > at the same time will lead to non desired results (unless we
> > > have a
> > > > > > > way to gate/ungate the clocks)...
> > > > > > I don't follow this bit.  You mean TGPx and software toggle. As
> far
> > > as I
> > > > > > can
> > > > > > tell it's an either/or thing per channel.
> > > > > >
> > > > >
> > > > > Here I meant that if we have a TGPx pin bundled to some
> > > channel(s)
> > > > > we
> > > > > do not want to also dance with the sw_toggle bit of that
> channel.
> > > >
> > > > Just a note on this. After starting my tests with the device, I can
> > > actually
> > > > say that if we have a TGPx set in the channel settings register, the
> > > device
> > > > will pretty much ignore the sw_toggle settings for that channel. I
> > > could
> > > > see that the output voltage was not toggling at all. As soon as I
> > > removed
> > > > the TGPx setting, then dancing with the sw_toggle started to
> work.
> > > So, for
> > > > the HW this is not really an issue as it just ignores the sw_toggle.
> On a
> > > SW
> > > > perspective, I'm still not sure if I just ignore this and write
> whatever
> > > the
> > > > user sets or if I return some error code in the case a user tries to
> > > toggle
> > > > a channel with a binded TGPx. The first one is appealing as it
> makes
> > > the
> > > > code much simpler while OTHO it might make sense to be
> verbose
> > > here
> > > > otherwise the user might think something is happening when it
> > > isn't...
> > >
> > > If we are in a static configuration (see below) then just don't
> expose
> > > the
> > > software toggle control.  Not having a big red button to press is the
> > > best way to
> > > tell userspace to not press the big red button...
> >
> >
> > Hmm, I get your point and that's valid if I have the sw_toggle as a per
> > channel attribute. Right now, I'm doing it as shared_by_type. The
> reason is
> > the sw_toggling is done by writing 1/0 in the toggle register and that
> register
> > is a bitmask being the mask 16bits wide. This allows you to toggle
> channels
> > at the same time in the same way you can do it if, say, you map 2,3 or
> more
> > channels to the same TGPx pin.
> 
> Hmm. That will be tricky to support in a remotely 'general' way.
> 
> >
> > However, I'm also not happy for having this as shared_by_type attr.
> One of
> > my complains is that it makes it look like a dither capable channel will
> also
> > support this (and we already agreed that sw_toggle does not make
> sense
> > for dither mode; so do not expose it). For instance the output of
> > 'iio_attr'  on a dither enabled channel is:
> >
> > ```
> > root@analog:~# iio_attr -c ltc2688 voltage0
> > dev 'ltc2688', channel 'voltage0' (output), attr 'calibbias', value '0'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'calibscale', value '0'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'dither_en', value '0'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'dither_frequency',
> value '32768'
> > dev 'ltc2688', channel 'voltage0' (output), attr
> 'dither_frequency_available', value '32768 16384 8192 4096 2048'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'dither_phase', value '0'
> > dev 'ltc2688', channel 'voltage0' (output), attr
> 'dither_phase_available', value '0 90 180 270'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'dither_raw', value '0'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'dither_raw_available',
> value '[0 1 65535]'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'offset', value '0'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'powerdown', value '0'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'raw', value '0'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'raw_available', value
> '[0 1 65535]'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'scale', value
> '0.076293945'
> > dev 'ltc2688', channel 'voltage0' (output), attr 'symbol', value '0'
> > ```
> >
> > So you can see that symbol attr which does not make sense to be
> there. And it's
> > definitely not something wrong in the iio_attr app as the attr is
> shared by type.
> >
> > Also, as you suggested, not having the symbol attr when it does not
> make sense
> > to have it also makes a lot of sense to me and that is one more plus
> point to have
> > this as a per channel thing.
> >
> > Anyways, I will probably send the patch with things as I have now so
> you can
> > have a felling of how it looks like. Unless you already tell me to just
> not have it
> > as a shared_by_type attr (which I'm getting more and more
> convinced on my own;
> > I guess I just need an extra push :D).
> 
> Shared by type indeed doesn't work as it's a subset - however we
> would need
> a means to indicate what subset is used if we want to allow single write
> to toggle multiple.   Mind you - the moment we get to multiple
> channels
> this should probably be using the chrdev route rather than sysfs and
> I'm
> not sure how that would map to this at all.

If someone really wants to toggle multiple channels through SW, one route
would also be to use TGPx (mapping it to multiple channels) and use some
of the host GPIOs to control the pin. Not nice as we would still need to provide a
dummy clock to make the driver "happy" but it would be a possible
workaround. This would only fail if all TGPx are already allocated for dither
channels but that's very unlikely. OTHO, if we are doing the toggle through SW
maybe our timings are not that strict so toggling all the channels in a loop is
not that bad...

Well, this is just wondering and playing "if" games so better to keep
it simple and worry about this if such a usecase ever pops up.

> So for now maybe take the view that software control of this is a weird
> feature
> anyway so make it per channel?

Agreed, will make it per channel and only expose it if a TGPx mapping is
not present. I was leaning in that direction anyways :).

- Nuno Sá

Re: [RFC PATCH 0/1] LTC2688 support

[Jonathan Cameron]

On Tue, 30 Nov 2021 14:43:25 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

Hi Nuno

Hopefully I've not lost the plot on this!

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, November 21, 2021 1:18 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio@vger.kernel.org
> > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > 
> > [External]
> > 
> > On Mon, 15 Nov 2021 10:28:51 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >   
> > > Hi Jonathan,
> > >
> > > Thanks for your inputs...
> > >  
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Friday, November 12, 2021 5:15 PM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: linux-iio@vger.kernel.org
> > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > >
> > > > [External]
> > > >
> > > > On Thu, 11 Nov 2021 12:00:42 +0100
> > > > Nuno Sá <nuno.sa@analog.com> wrote:
> > > >
> > > > Hi Nuno,
> > > >  
> > > > > The reason why this is a RFC is because I'm still waiting for proper  
> > HW  
> > > > > to test this thing. The reason why I'm sending this already is  
> > because  
> > > > > there's some non usual features which might require extra ABI.  
> > > > Hence,  
> > > > > while waiting I thought we could already speed up the process in  
> > > > regards  
> > > > > with the ABI.  
> > > >
> > > > Wise move as this is an unusual beast :)
> > > >  
> > > > >
> > > > > I still pushed the driver but the intent here is not really to review  
> > it.  
> > > > > Naturally, if someone already wants to give some feedback,  
> > that's  
> > > > very  
> > > > > much appreciated :)  
> > > >  
> > > > >
> > > > > Now, there are three main interfaces depending on the channel  
> > > > mode:  
> > > > >  1) default (no new ABI)
> > > > >  2) toggle mode
> > > > >  3) dither mode
> > > > >
> > > > > The channel mode will be a devicetree property as it does not  
> > really  
> > > > > make much sense to change between modes at runtime even  
> > more  
> > > > because the  
> > > > > piece of HW we might want to control with a channel might be  
> > > > different  
> > > > > depending on the selected mode (I'm fairly sure on this between  
> > > > toggle  
> > > > > and other modes but not so sure between dither and default  
> > mode).  
> > > >
> > > > I agree on toggle vs dither definitely being different, but normal  
> > you  
> > > > could implement either as software toggle, or dither with a 0
> > > > magnitude
> > > > sine wave.  So might not be worth implementing default mode at  
> > all.  
> > > > No harm in doing so though if there are advantages to having it.  
> > >
> > > My feeling is that we could probably have dither as the "default  
> > mode".  
> > > More on this below...
> > >  
> > > > >
> > > > > toggle mode special ABI:
> > > > >
> > > > >  * Toggle operation enables fast switching of a DAC output  
> > between  
> > > > two  
> > > > > different DAC codes without any SPI transaction. Two codes are  
> > set  
> > > > in  
> > > > > input_a and input_b and then the output switches according to  
> > an  
> > > > input  
> > > > > signal (input_a -> clk high; input_b -> clk low).
> > > > >
> > > > > out_voltageY_input_register
> > > > >  - selects between input_a and input_b. After selecting one  
> > register,  
> > > > we  
> > > > >    can write the dac code in out_voltageY_raw.
> > > > > out_voltageY_toggle_en
> > > > >  - Disable/Enable toggle mode. The reason why I think this one is
> > > > >    important is because if we wanna change the 2 codes, we  
> > should  
> > > > first  
> > > > >    disable this, set the codes and only then enable the mode  
> > back...  
> > > > >    As I'm writing this, It kind of came to me that we can probably
> > > > >    achieve this with out_voltageY_powerdown attr (maybe takes a  
> > bit  
> > > > more  
> > > > >    time to see the outputs but...)  
> > > >
> > > > Hmm. These corner cases always take a bit of figuring out.  What  
> > you  
> > > > have
> > > > here is a bit 'device specific' for my liking.
> > > >
> > > > So there is precedence for ABI in this area, on the frequency  
> > devices  
> > > > but only
> > > > for devices we still haven't moved out of staging.  For those we
> > > > needed a means
> > > > to define selectable phases for PSK - where the selection was  
> > either  
> > > > software or,
> > > > much like here, a selection pin.
> > > >
> > > > out_altvotage0_phase0 etc
> > > >
> > > > so I guess the equivalent here would be
> > > > out_voltageY_raw0
> > > > out_voltageY_raw1
> > > > and the selection would be via something like
> > > > out_voltageY_symbol (0 or 1 in this case). - note this is only
> > > > relevant if you have the software toggle. Any enable needed for
> > > > setting
> > > > can be done as part of the write sequence for the  raw0 / raw1 and
> > > > should
> > > > ideally not be exposed to userspace (controls that require manual
> > > > sequencing
> > > > tend to be hard to use / document).  
> > >
> > > Yeah, I thought about that. I was even thinking in having something  
> > like  
> > > *_amplitude for dither mode. In some cases, where we might be left
> > > in some non obvious state (eg: moved the select register to input b  
> > and  
> > > then we failed to set the code;), I prefer to leave things as flexible as
> > > possible for userspace. But I agree it adds up more complexity and in
> > > this case, I can just always go to 'input_a' when writing 'raw0'...
> > >  
> > > > However, I'm not 100% sure it really maps to this case.  What do  
> > you  
> > > > think?  
> > >
> > > I think it can work. Though out_voltageY_symbol probably needs to  
> > be  
> > > shared by type to be coherent with what we might have with TGPx.  
> > 
> > That's fine.
> >   
> > > Note the sw_toggle register has a bit mask of the channels we want  
> > to  
> > > toggle which means we can toggle multiple channels at the same  
> > time.
> > 
> > Using that wired up to buffer mode might make sense.  You'd provide
> > multiple
> > buffers and allow channels to be selected into one of them at a time.
> > Each
> > buffer is then tied to a different toggle (TGP0, TGP1, etc)
> > 
> > The same could be true for the software toggle.  It'll get a bit fiddly
> > though.
> > 
> > Perhaps this is an advanced feature to think about later...
> >   
> > > It works the same with TGPx if you map multiple channels to the  
> > same  
> > > pin.
> > >
> > > There's also the question on how to handle this if a TGPx is provided?
> > > I guess it will just override the pin... But most likely having them both
> > > at the same time will lead to non desired results (unless we have a
> > > way to gate/ungate the clocks)...  
> > I don't follow this bit.  You mean TGPx and software toggle. As far as I
> > can
> > tell it's an either/or thing per channel.
> >   
> 
> Here I meant that if we have a TGPx pin bundled to some channel(s) we
> do not want to also dance with the sw_toggle bit of that channel. Ultimately,
> that's on the user responsibility but we could also add some guards I guess.
> I'm not sure if it's an either/or thing per channel... IIUC, we spoke about
> making dither and default mode the same. That might complicate things a bit
> as:
> 
> 1) We should not force a user to specify a TGPx pin for those channels (since
> it can also work with dithering disabled).
> 2) Because of 1), we should also support sw_toggle for these channels since
> someone might want to enable dither mode (at runtime) and the TGPx pin was
> not given. Hence, we need to have a way to update the DAC using the sw_toggle.
> 
> Did I understood things wrong? One thing that comes to my mind, is to return
> error (eg: EPERM or ENOTSUPP) if someone tries to enable dither mode and
> no TGPx pin was selected for that channel? Hence, we do not need to add
> the sw_toggle ABI  (out_voltage_symbol) for the default/dither mode. Or
> maybe even better, we just expose the dither ABI if a TGPx pin is given over
> dt (I try to explain the toggle/dither modes below; that might help you in
> understanding my reasoning here)?
> 
> Alternatively, we just keep the approach I have in this RFC and we keep the
> 3 different modes (being mode a dt property; in the current state I'm using
> a boolean to say a channel Is in toggle mode)... Maybe with the difference
> that we allow sw_toggle for toggle enabled channels.

The corner I'm not clear on is what we do if for example all TGPx pins are
specified in DT.  Is the mapping from channel to TGPx things in toggle mode
always a function of the external circuit or do we want to make it runtime
controllable?

I'm absolutely fine if we just make it a dt property - particularly
as those TGPx pins may well not be visible to the host processor.

We probably do want to provide some options in dt for what they might be
connnected to on the host.  I'm guessing potentially a gpio, or a clk?

You could also wire them to a non general purpose output pin - but that
would be really hard to describe in a generic fashion.

> 
> > >  
> > > > I'm not sure if whether a channel is in toggle mode is a circuit thing  
> > or  
> > > > not..
> > > > (and hence DT or userspace control?)  
> > >
> > > The only reason I can think off to have it as DT is that toggle mode  
> > seems  
> > > to be for more specific use cases so I guess the HW we want to  
> > control (  
> > > and connect to a toggle enabled channel) will be different.
> > >
> > > I'm also not seeing an use case for ping ponging between the modes  
> > mostly  
> > > because of the above...  
> > 
> > Only use I can see is to reduce traffic if you happen to be switching
> > between
> > two sets of DAC outputs repeatedly. If there wasn't an LDAC pin I'd
> > suspect
> > this was there to enable simultaneous updates but we have that
> > anyway.
> > Maybe if the LDAC isn't wired this could be used to provide similar
> > functionality?  If that's the case, we could just leave it as a possible
> > TODO if anyone wants it in future...
> > I think you could use the TGPx to provide controlled switching of
> > sets of channels.  Maybe that's something useful?  
> 
> Hmm, I think LDAC is something else. The LDAC pin updates all 16 channels
> with the value of the input register (which I assume is INPUT_A) independent
> of them being in dither/toggle mode. Honestly, I'm not even sure of how
> thing would work if someone plays with LDAC + channels in dither/toggle
> mode. I think extra care would be needed...

My guess was as that in toggle mode at least, LDAC would update the DACs to
reflect the 'current' toggle registers, but I'm not totally sure.

> 
> So, TGPx pins are indeed used to control set of channels. But note that these
> pins (as sw_toggle) are only meaningful if the channel is in dither or toggle
> mode (the channel bit in the toggle/dither enable register has to be set):

Agreed, though note that we could ensure we always were in one of these modes
if it made sense from software point of view.

> 
> * In toggle mode we have two set of input registers: INPUT_A and INPUT_B
> and we toggle between these two values. If we wire a TGPx pin and bundle
> it to a toggle enabled channel, when the input signal is high, we output INPUT_A
> and INPUT_B when the signal is low. We can mimic the same behavior by writing
> 1/0 to the sw_toggle register.

There are different ways you could use this facility if you assume the TGPx pins
are wired to the host processor rather than something out of our control.

You could use it for double buffering for example - so update INPUT_B, then toggle,
update INPUT_A then toggle etc.
Or you could assume it's for mass mode switching - so there are only two values
that will ever be set and these reflect something in the external circuits. We
might not even have software control / visibility of which state is active.

> * In dither mode, things are slightly different as INPUT_B is the value for the
> sinusoidal amplitude. And if we have an external signal on a TGPx pin, the dac
> channel is updated on the rising edge of the signal. Again, we can mimic the same
> behavior with the sw_toggle register.  
> 
> So, for dither mode, I'm not really seeing why someone would want to use the
> sw_toggle. Even for toggle mode, I have my doubts but I can more easily see it.

Does seem unlikely for dither mode as you'd need a huge amount of bus traffic
to basically provide a bad clock signal.  Toggle I can also sort of see
being useful with sw_toggle - but suspect real usecase for that is the one
where the host processor isn't controlling the toggle..

> 
> > >  
> > > > Can see that even in a case where you did want to use external
> > > > controls to
> > > > pick the values, you might also want to override from software...
> > > > Given there is a software toggle I guess we can use that as  
> > override.  
> > > > Actually that raises the question of what the point in having normal
> > > > mode is?
> > > > Can we just implement that as a software toggle toggle mode? One
> > > > less thing to
> > > > worry about if we can.  
> > >
> > > I did thought about the sw_toggle thing (it's something that is only  
> > valid  
> > > for channels in dither/toggle mode). My reasoning was that either
> > >
> > > 1) I did not supported it and made the TGPx selection mandatory (in  
> > case  
> > > dither/toggle mode enabled) or
> > > 2) I did support it  and hence the pins  are not really mandatory.
> > >
> > > I went with 1) because, honestly, I'm not seeing the point of having  
> > these  
> > > modes and use sw toggle (at least on a production system).  
> > However, if we  
> > > want to get rid of the default mode and have it as the dither mode, I  
> > agree  
> > > we need sw_toggle because If someone just wants to use the  
> > channel  
> > > without any dithering, we can't have an hard requirement to provide  
> > a  
> > > external TGPx. Moreover, if the default channel will be a dither  
> > capable  
> > > one, we need to provide full functionality and hence, sw_toggle.
> > >
> > > As I stated before, I'm just not sure on how to handle things if a TGPx  
> > is  
> > > also provided. Maybe they should be mutual exclusive? I mean, if  
> > someone  
> > > tries to toggle a channel with a mapped TGPx we return some error  
> > code?
> > 
> > Given the mapping of TGPx to channel is a software control I think
> > ultimately
> > you'd want to expose that - one way I can think of doing that is via
> > the buffer interface.
> > 
> > 4x buffers.  One of each TGP0,1,2 and SW toggle.  Enable the channels
> > you
> > want for a given 'buffer' and then they will switch together based on
> > the
> > data in the buffer.  If the buffer has a series of toggling values then
> > it's simple - if not then after each toggle the buffer would need to
> > preload
> > the next value.  The snag there is that you'd need to know a toggle
> > occurred
> > and if the toggle pins are wired to somewhere other than our host I'm
> > not sure
> > how you would know that in general? (could wire the same TGPx signal
> > to an
> > interrupt on the host controller but seems unlikely).  
> 
> Honestly I think that we would be probably over engineering things with
> this even though I get your point. OTOH, as you said, the triggering would
> be very hacky to handle and that might already tell us something :)

It may well make sense to only implement a subset but it is often useful
to walk through what a more sophisticated feature set might look like
as then we don't accidentally rule it out.

>  
> > Whether software toggle is worth bothering when we have LDAC to
> > control
> > simultaneous DAC updates isn't clear to me.  I guess it's fewer writes  
> 
> Hopefully I could make it clear that sw_toggle is something else than LDAC :)
> 
> > if we happen to be cycling between values.  Perhaps you are right and
> > that
> > feature is just for debug.
> >   
> > >  
> > > > There is also the question of whether selection of which toggle pin  
> > is  
> > > > used
> > > > should be a dt thing or a userspace control...  
> > >
> > > Well, this definitely means some HW wiring to have the external  
> > signals and  
> > > I'm not sure if there's any added valuable in being able to change the
> > > external signal at runtime?  
> > 
> > Whilst I can conjecture reasons to do this, you may well be right - real
> > usecases will know which signal groups they want to control together.
> >   
> > >  
> > > > >
> > > > > dither mode special ABI:
> > > > >
> > > > >  * Dither operation adds a small sinusoidal wave to the digital DAC
> > > > > signal path. Dithering is a signal processing technique that  
> > involves  
> > > > > the injection of ac noise to the signal path to reduce system
> > > > > nonlinearities.
> > > > >  
> > > >
> > > > This is a complex feature to describe as (if I read it correctly) we  
> > have  
> > > > a dither clocked from an external pin, or in theory from software.  
> > That  
> > > > clock
> > > > frequency must match the dither.  Realistically that means it is a  
> > clock  
> > > > in our control or we have to match the period below to the  
> > frequency  
> > > > of that
> > > > clock.  
> > >
> > > Yeah, the frequency  of the dither signal is fsig = fclk / N, where N can  
> > only  
> > > be: [4 8 16 32 64]. So, we kind of just have these available options for  
> > the  
> > > signal frequency and fclk is something we can control and know  
> > (assuming  
> > > we have TGPx mapping which I'm bundling with a clk).
> > >
> > > The only quirk with having this with frequency rather than raw N is
> > > to handle the sw_toggle where we have no idea about fclk? We  
> > could also  
> > > think of this attr as some kind of decimation...  
> > 
> > Does seems unlikely anyone would use dither with a sw toggle.
> > Perhaps best plan here is to not support that combination.  
> 
> Agree... But there are some subtleties that we need to settle 
> about the channel default mode as I raised in my previous comments.
> 
> > As to the clock, these are about controlling a sine wave frequency. I'm
> > not sure
> > decimation fits as a model. (figure 19)
> > 
> > Given you know the input clock, perhaps present this as something like
> > out_voltage0_dither_frequency
> > 
> > I don't thing dithers are always this simple, so probably want to be
> > specific its
> > a sine wave so maybe we need
> > out_voltage0_dither_type 'sine'
> > 
> > We used to have some DDS chips in staging but looks like they all got
> > dropped due
> > to end of time Those had various waveforms and IIRC all we came up
> > with was
> > descriptive terms + frequencies and magnitudes.
> > 
> >   
> > >  
> > > > > out_voltage0_dither_en
> > > > >  - Same as in toggle mode.
> > > > > out_voltage0_dither_period
> > > > > out_voltage0_dither_phase
> > > > >  - Period and phase of the signal. Only some values are valid so  
> > > > there's  
> > > > >    also *_available files for these. I'm not sure if we can use the  
> > more  
> > > > >    generic IIO_CHAN_INFO_PHASE and  
> > IIO_CHAN_INFO_FREQUENCY  
> > > > here as these  
> > > > >    parameters don't really apply to the channel output signal..  
> > > >
> > > > Possibly not helpful to do so, but you could describe the channel as  
> > an  
> > > > out_altvoltage channel that happens to have a significant offset  
> > (the  
> > > > DC
> > > > level) and phase, frequency etc as for a normal altvoltage channel.
> > > > That would hide the intention here though so perhaps not a good  
> > plan  
> > > > even if it ensures we end up with standard ABI.  
> > >
> > > I think altvoltage might not be optimal here  because the phase and  
> > frequency  
> > > are really not characteristics of the output signal of the channel.  
> > 
> > Well they kind of are if you set the magnitude high enough  - but I get
> > your
> > point.  That's not how they are intended to be used.
> >   
> > >  
> > > > > out_voltage0_input_register
> > > > >  - Same as in toggle mode. However in this mode the code set in  
> > the  
> > > > >    input_b register has a special meaning. It's the amplitude of the
> > > > >    dither signal.  
> > > > Don't do that - provide a direct attribute representing the value of
> > > > register_b and when it is written implicitly switch to the right  
> > register.  
> > > > Any ABI that requires a sequence of events is hard to use.  
> > >
> > > I guess we can just use the same raw1 attr here? Even though, in  
> > dither  
> > > mode this has special meaning (it is the amplitude)...  
> > 
> > I was thinking toggle mode here. This interface doesn't work for dither.
> > in that case there is just
> > out_voltage0_raw for the DC part and
> > out_voltage0_dither_raw for the dither amplitude.  Assumption being
> > same scaling
> > as I don't really want to support multiple scale factors if we can avoid it.  
> 
> I'm fairly sure it is the same scaling....

As long as it's a simple factor, just hit it with a 'small adjusting tool'
until it does have the same scaling.  We do that in a few similar cases
IIRC as sometimes things like this dither input have lower resolution.

If it's the same scaling then you won't have to hit it at all ;)

> 
> > 
> >   
> > >  
> > > >  
> > > > >
> > > > > One special mention to the dac scale. In this part this is something
> > > > > that can be purely controlled by SW so that I'm allowing  
> > userspace to  
> > > > > change it rather then having it in dts. The available scales are:
> > > > >
> > > > > * [0 5V] -> offset 0
> > > > > * [0 10V] -> offset 0
> > > > > * [-5 5V] -> offset -32678
> > > > > * [-10 10V] -> offset -32768
> > > > > * [-15 15V] -> offset -32768
> > > > >
> > > > > With the above, we also need to have the offset configurable  
> > and  
> > > > right  
> > > > > now I'm going to some trouble to make sure the scale + offset is
> > > > > something valid. Honestly, I think I'm overdoing it because things  
> > can  
> > > > > still go wrong with [0 10V] and [-5 5V] as the scales are the same
> > > > > here. Furthermore, there's no real arm that can be done to the  
> > HW.  
> > > > Is  
> > > > > just that the readings won't match with what someone might be  
> > > > expecting.  
> > > > > My plan is to just remove those checks and assume is up to  
> > > > userspace to  
> > > > > make it right and not have [-10 10V] scale with 0 offset for  
> > example.  
> > > >
> > > > So this is something we've debated a few times in the past.
> > > > There is a fairly strong argument for output devices that the range  
> > is  
> > > > a characteristic of the circuit.  At the very least it makes sense to
> > > > restrict it in DT even if we allow safe forms of tweaking in the  
> > driver.  
> > > > For an initial driver, I'd just have it in DT.
> > > >  
> > >
> > > No complaints against that and makes things way simpler to handle.  
> > Great.  
> > >
> > > - Nuno Sá
> > >  
> > So conclusions.. Hmm. Not strong ones yet, but for dither mode at
> > least
> > I think you want to link particular channels to a TGPx choice
> > 
> > out_voltage0_raw
> > out_voltage0_raw_available ( nice to have on DACs)  
> 
> I guess here you mean 'IIO_AVAIL_RANGE'?

No, I mean providing the read_avail() callback and setting
BIT(IIO_INFO_RAW) in info_mask_separate_available

That's how we provide range for a channel except in some unusual
corner cases and the internal interface for that is used when a DAC
is being used via the consumer interface (so some other driver wants
to set it's output).

> 
> > out_voltage0_scale
> > out_voltage0_dither_raw
> > out_voltage0_dither_raw_available
> > out_voltage0_dither_frequency
> > out_voltage0_dither_frequency_available
> > out_voltage0_dither_phase
> > out_voltage0_dither_phase_available  
> 
> > Toggle mode is less clear to me but symbol approach plus TGPx in DT
> > maybe works
> > You could allow for software override to set the symbol.  Interface to
> > unset
> > it being to write an empty string to _symbol. Maybe leave that for
> > now.
> > 
> > out_voltage0_raw0
> > out_voltage0_raw1
> > out_voltage0_scale
> > out_voltage0_symbol  
> 
> Well, in short, I do agree with this ABI. And actually, for toggle mode, I think
> this is more or less what we will have. For dither/default mode, there's still
> the questions I raised above... Maybe, at the end, we will end up with 3 different
> ABI's...

Certainly possible.  Nice to avoid if we can, but not if it means stretching
things too far.

> 
> I would only add this to the ABI:
>  * out_voltage0_dither_en
>  * out_voltage0_toggle_en
> 
> Because if someone wants to change, let's say the dither frequency, the best way
> to update things is to first disable dithering, update all the stuff, and then enable
> it again...

I'll go with 'maybe' for these.  The use for changing things doesn't make sense to me
unless we have multiple things to change at once.  If it's just the frequency it
would be more intuitive to have a write to that attribute do the disable, set value
and enable dithering again without needing to do a dance with the interface.

I'm not sure when updating needs to be done atomically across the various variables (so
would need to be done under a single disable / enable sequence.

> 
> - Nuno Sá

RE: [RFC PATCH 0/1] LTC2688 support

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, December 5, 2021 7:01 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org
> Subject: Re: [RFC PATCH 0/1] LTC2688 support
> 
> [External]
> 
> On Tue, 30 Nov 2021 14:43:25 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> Hi Nuno
> 
> Hopefully I've not lost the plot on this!

Not really. I had some days off so this was also set on hold from
my side.

> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Sunday, November 21, 2021 1:18 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio@vger.kernel.org
> > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > >
> > > [External]
> > >
> > > On Mon, 15 Nov 2021 10:28:51 +0000
> > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > >
> > > > Hi Jonathan,
> > > >
> > > > Thanks for your inputs...
> > > >
> > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > Sent: Friday, November 12, 2021 5:15 PM
> > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Cc: linux-iio@vger.kernel.org
> > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > > >
> > > > > [External]
> > > > >
> > > > > On Thu, 11 Nov 2021 12:00:42 +0100
> > > > > Nuno Sá <nuno.sa@analog.com> wrote:
> > > > >
> > > > > Hi Nuno,
> > > > >
> > > > > > The reason why this is a RFC is because I'm still waiting for
> proper
> > > HW
> > > > > > to test this thing. The reason why I'm sending this already is
> > > because
> > > > > > there's some non usual features which might require extra
> ABI.
> > > > > Hence,
> > > > > > while waiting I thought we could already speed up the
> process in
> > > > > regards
> > > > > > with the ABI.
> > > > >
> > > > > Wise move as this is an unusual beast :)
> > > > >
> > > > > >
> > > > > > I still pushed the driver but the intent here is not really to
> review
> > > it.
> > > > > > Naturally, if someone already wants to give some feedback,
> > > that's
> > > > > very
> > > > > > much appreciated :)
> > > > >
> > > > > >
> > > > > > Now, there are three main interfaces depending on the
> channel
> > > > > mode:
> > > > > >  1) default (no new ABI)
> > > > > >  2) toggle mode
> > > > > >  3) dither mode
> > > > > >
> > > > > > The channel mode will be a devicetree property as it does not
> > > really
> > > > > > make much sense to change between modes at runtime
> even
> > > more
> > > > > because the
> > > > > > piece of HW we might want to control with a channel might
> be
> > > > > different
> > > > > > depending on the selected mode (I'm fairly sure on this
> between
> > > > > toggle
> > > > > > and other modes but not so sure between dither and default
> > > mode).
> > > > >
> > > > > I agree on toggle vs dither definitely being different, but normal
> > > you
> > > > > could implement either as software toggle, or dither with a 0
> > > > > magnitude
> > > > > sine wave.  So might not be worth implementing default mode
> at
> > > all.
> > > > > No harm in doing so though if there are advantages to having it.
> > > >
> > > > My feeling is that we could probably have dither as the "default
> > > mode".
> > > > More on this below...
> > > >
> > > > > >
> > > > > > toggle mode special ABI:
> > > > > >
> > > > > >  * Toggle operation enables fast switching of a DAC output
> > > between
> > > > > two
> > > > > > different DAC codes without any SPI transaction. Two codes
> are
> > > set
> > > > > in
> > > > > > input_a and input_b and then the output switches according
> to
> > > an
> > > > > input
> > > > > > signal (input_a -> clk high; input_b -> clk low).
> > > > > >
> > > > > > out_voltageY_input_register
> > > > > >  - selects between input_a and input_b. After selecting one
> > > register,
> > > > > we
> > > > > >    can write the dac code in out_voltageY_raw.
> > > > > > out_voltageY_toggle_en
> > > > > >  - Disable/Enable toggle mode. The reason why I think this
> one is
> > > > > >    important is because if we wanna change the 2 codes, we
> > > should
> > > > > first
> > > > > >    disable this, set the codes and only then enable the mode
> > > back...
> > > > > >    As I'm writing this, It kind of came to me that we can
> probably
> > > > > >    achieve this with out_voltageY_powerdown attr (maybe
> takes a
> > > bit
> > > > > more
> > > > > >    time to see the outputs but...)
> > > > >
> > > > > Hmm. These corner cases always take a bit of figuring out.
> What
> > > you
> > > > > have
> > > > > here is a bit 'device specific' for my liking.
> > > > >
> > > > > So there is precedence for ABI in this area, on the frequency
> > > devices
> > > > > but only
> > > > > for devices we still haven't moved out of staging.  For those we
> > > > > needed a means
> > > > > to define selectable phases for PSK - where the selection was
> > > either
> > > > > software or,
> > > > > much like here, a selection pin.
> > > > >
> > > > > out_altvotage0_phase0 etc
> > > > >
> > > > > so I guess the equivalent here would be
> > > > > out_voltageY_raw0
> > > > > out_voltageY_raw1
> > > > > and the selection would be via something like
> > > > > out_voltageY_symbol (0 or 1 in this case). - note this is only
> > > > > relevant if you have the software toggle. Any enable needed
> for
> > > > > setting
> > > > > can be done as part of the write sequence for the  raw0 / raw1
> and
> > > > > should
> > > > > ideally not be exposed to userspace (controls that require
> manual
> > > > > sequencing
> > > > > tend to be hard to use / document).
> > > >
> > > > Yeah, I thought about that. I was even thinking in having
> something
> > > like
> > > > *_amplitude for dither mode. In some cases, where we might be
> left
> > > > in some non obvious state (eg: moved the select register to input
> b
> > > and
> > > > then we failed to set the code;), I prefer to leave things as
> flexible as
> > > > possible for userspace. But I agree it adds up more complexity
> and in
> > > > this case, I can just always go to 'input_a' when writing 'raw0'...
> > > >
> > > > > However, I'm not 100% sure it really maps to this case.  What do
> > > you
> > > > > think?
> > > >
> > > > I think it can work. Though out_voltageY_symbol probably needs
> to
> > > be
> > > > shared by type to be coherent with what we might have with
> TGPx.
> > >
> > > That's fine.
> > >
> > > > Note the sw_toggle register has a bit mask of the channels we
> want
> > > to
> > > > toggle which means we can toggle multiple channels at the same
> > > time.
> > >
> > > Using that wired up to buffer mode might make sense.  You'd
> provide
> > > multiple
> > > buffers and allow channels to be selected into one of them at a
> time.
> > > Each
> > > buffer is then tied to a different toggle (TGP0, TGP1, etc)
> > >
> > > The same could be true for the software toggle.  It'll get a bit fiddly
> > > though.
> > >
> > > Perhaps this is an advanced feature to think about later...
> > >
> > > > It works the same with TGPx if you map multiple channels to the
> > > same
> > > > pin.
> > > >
> > > > There's also the question on how to handle this if a TGPx is
> provided?
> > > > I guess it will just override the pin... But most likely having them
> both
> > > > at the same time will lead to non desired results (unless we have
> a
> > > > way to gate/ungate the clocks)...
> > > I don't follow this bit.  You mean TGPx and software toggle. As far
> as I
> > > can
> > > tell it's an either/or thing per channel.
> > >
> >
> > Here I meant that if we have a TGPx pin bundled to some channel(s)
> we
> > do not want to also dance with the sw_toggle bit of that channel.
> Ultimately,
> > that's on the user responsibility but we could also add some guards I
> guess.
> > I'm not sure if it's an either/or thing per channel... IIUC, we spoke
> about
> > making dither and default mode the same. That might complicate
> things a bit
> > as:
> >
> > 1) We should not force a user to specify a TGPx pin for those
> channels (since
> > it can also work with dithering disabled).
> > 2) Because of 1), we should also support sw_toggle for these
> channels since
> > someone might want to enable dither mode (at runtime) and the
> TGPx pin was
> > not given. Hence, we need to have a way to update the DAC using
> the sw_toggle.
> >
> > Did I understood things wrong? One thing that comes to my mind, is
> to return
> > error (eg: EPERM or ENOTSUPP) if someone tries to enable dither
> mode and
> > no TGPx pin was selected for that channel? Hence, we do not need
> to add
> > the sw_toggle ABI  (out_voltage_symbol) for the default/dither
> mode. Or
> > maybe even better, we just expose the dither ABI if a TGPx pin is
> given over
> > dt (I try to explain the toggle/dither modes below; that might help
> you in
> > understanding my reasoning here)?
> >
> > Alternatively, we just keep the approach I have in this RFC and we
> keep the
> > 3 different modes (being mode a dt property; in the current state I'm
> using
> > a boolean to say a channel Is in toggle mode)... Maybe with the
> difference
> > that we allow sw_toggle for toggle enabled channels.
> 
> The corner I'm not clear on is what we do if for example all TGPx pins
> are
> specified in DT.  Is the mapping from channel to TGPx things in toggle
> mode
> always a function of the external circuit or do we want to make it
> runtime
> controllable?
> 
> I'm absolutely fine if we just make it a dt property - particularly
> as those TGPx pins may well not be visible to the host processor.
> 
> We probably do want to provide some options in dt for what they
> might be
> connnected to on the host.  I'm guessing potentially a gpio, or a clk?

For each TGPx pin (from the point you bind it to some channel), I'm
actually making it mandatory to have a clock (the reasoning being, if you 
say some channel is controlled over TGPx [being for toggle or dither mode],
you need to have some input at the pin).

I might not be doing it in the way you're thinking but you can have a
look in the actual series :) ...

> You could also wire them to a non general purpose output pin - but
> that
> would be really hard to describe in a generic fashion.
> 
> >
> > > >
> > > > > I'm not sure if whether a channel is in toggle mode is a circuit
> thing
> > > or
> > > > > not..
> > > > > (and hence DT or userspace control?)
> > > >
> > > > The only reason I can think off to have it as DT is that toggle mode
> > > seems
> > > > to be for more specific use cases so I guess the HW we want to
> > > control (
> > > > and connect to a toggle enabled channel) will be different.
> > > >
> > > > I'm also not seeing an use case for ping ponging between the
> modes
> > > mostly
> > > > because of the above...
> > >
> > > Only use I can see is to reduce traffic if you happen to be switching
> > > between
> > > two sets of DAC outputs repeatedly. If there wasn't an LDAC pin I'd
> > > suspect
> > > this was there to enable simultaneous updates but we have that
> > > anyway.
> > > Maybe if the LDAC isn't wired this could be used to provide similar
> > > functionality?  If that's the case, we could just leave it as a possible
> > > TODO if anyone wants it in future...
> > > I think you could use the TGPx to provide controlled switching of
> > > sets of channels.  Maybe that's something useful?
> >
> > Hmm, I think LDAC is something else. The LDAC pin updates all 16
> channels
> > with the value of the input register (which I assume is INPUT_A)
> independent
> > of them being in dither/toggle mode. Honestly, I'm not even sure of
> how
> > thing would work if someone plays with LDAC + channels in
> dither/toggle
> > mode. I think extra care would be needed...
> 
> My guess was as that in toggle mode at least, LDAC would update the
> DACs to
> reflect the 'current' toggle registers, but I'm not totally sure.
> >
> > So, TGPx pins are indeed used to control set of channels. But note
> that these
> > pins (as sw_toggle) are only meaningful if the channel is in dither or
> toggle
> > mode (the channel bit in the toggle/dither enable register has to be
> set):
> 
> Agreed, though note that we could ensure we always were in one of
> these modes
> if it made sense from software point of view.

Probably not because I can easily see users just wanting to have channels
operating in the default mode...

> >
> > * In toggle mode we have two set of input registers: INPUT_A and
> INPUT_B
> > and we toggle between these two values. If we wire a TGPx pin and
> bundle
> > it to a toggle enabled channel, when the input signal is high, we
> output INPUT_A
> > and INPUT_B when the signal is low. We can mimic the same
> behavior by writing
> > 1/0 to the sw_toggle register.
> 
> There are different ways you could use this facility if you assume the
> TGPx pins
> are wired to the host processor rather than something out of our
> control.
> 
> You could use it for double buffering for example - so update
> INPUT_B, then toggle,
> update INPUT_A then toggle etc.
> Or you could assume it's for mass mode switching - so there are only
> two values
> that will ever be set and these reflect something in the external
> circuits. We
> might not even have software control / visibility of which state is
> active.
> 
> > * In dither mode, things are slightly different as INPUT_B is the value
> for the
> > sinusoidal amplitude. And if we have an external signal on a TGPx
> pin, the dac
> > channel is updated on the rising edge of the signal. Again, we can
> mimic the same
> > behavior with the sw_toggle register.
> >
> > So, for dither mode, I'm not really seeing why someone would want
> to use the
> > sw_toggle. Even for toggle mode, I have my doubts but I can more
> easily see it.
> 
> Does seem unlikely for dither mode as you'd need a huge amount of
> bus traffic
> to basically provide a bad clock signal.  Toggle I can also sort of see
> being useful with sw_toggle - but suspect real usecase for that is the
> one
> where the host processor isn't controlling the toggle..

Agreed (that's also my feeling)...

> >
> > > >
> > > > > Can see that even in a case where you did want to use external
> > > > > controls to
> > > > > pick the values, you might also want to override from
> software...
> > > > > Given there is a software toggle I guess we can use that as
> > > override.
> > > > > Actually that raises the question of what the point in having
> normal
> > > > > mode is?
> > > > > Can we just implement that as a software toggle toggle mode?
> One
> > > > > less thing to
> > > > > worry about if we can.
> > > >
> > > > I did thought about the sw_toggle thing (it's something that is
> only
> > > valid
> > > > for channels in dither/toggle mode). My reasoning was that
> either
> > > >
> > > > 1) I did not supported it and made the TGPx selection mandatory
> (in
> > > case
> > > > dither/toggle mode enabled) or
> > > > 2) I did support it  and hence the pins  are not really mandatory.
> > > >
> > > > I went with 1) because, honestly, I'm not seeing the point of
> having
> > > these
> > > > modes and use sw toggle (at least on a production system).
> > > However, if we
> > > > want to get rid of the default mode and have it as the dither
> mode, I
> > > agree
> > > > we need sw_toggle because If someone just wants to use the
> > > channel
> > > > without any dithering, we can't have an hard requirement to
> provide
> > > a
> > > > external TGPx. Moreover, if the default channel will be a dither
> > > capable
> > > > one, we need to provide full functionality and hence, sw_toggle.
> > > >
> > > > As I stated before, I'm just not sure on how to handle things if a
> TGPx
> > > is
> > > > also provided. Maybe they should be mutual exclusive? I mean, if
> > > someone
> > > > tries to toggle a channel with a mapped TGPx we return some
> error
> > > code?
> > >
> > > Given the mapping of TGPx to channel is a software control I think
> > > ultimately
> > > you'd want to expose that - one way I can think of doing that is via
> > > the buffer interface.
> > >
> > > 4x buffers.  One of each TGP0,1,2 and SW toggle.  Enable the
> channels
> > > you
> > > want for a given 'buffer' and then they will switch together based
> on
> > > the
> > > data in the buffer.  If the buffer has a series of toggling values then
> > > it's simple - if not then after each toggle the buffer would need to
> > > preload
> > > the next value.  The snag there is that you'd need to know a toggle
> > > occurred
> > > and if the toggle pins are wired to somewhere other than our host
> I'm
> > > not sure
> > > how you would know that in general? (could wire the same TGPx
> signal
> > > to an
> > > interrupt on the host controller but seems unlikely).
> >
> > Honestly I think that we would be probably over engineering things
> with
> > this even though I get your point. OTOH, as you said, the triggering
> would
> > be very hacky to handle and that might already tell us something :)
> 
> It may well make sense to only implement a subset but it is often
> useful
> to walk through what a more sophisticated feature set might look like
> as then we don't accidentally rule it out.
> 
> >
> > > Whether software toggle is worth bothering when we have LDAC
> to
> > > control
> > > simultaneous DAC updates isn't clear to me.  I guess it's fewer
> writes
> >
> > Hopefully I could make it clear that sw_toggle is something else than
> LDAC :)
> >
> > > if we happen to be cycling between values.  Perhaps you are right
> and
> > > that
> > > feature is just for debug.
> > >
> > > >
> > > > > There is also the question of whether selection of which toggle
> pin
> > > is
> > > > > used
> > > > > should be a dt thing or a userspace control...
> > > >
> > > > Well, this definitely means some HW wiring to have the external
> > > signals and
> > > > I'm not sure if there's any added valuable in being able to change
> the
> > > > external signal at runtime?
> > >
> > > Whilst I can conjecture reasons to do this, you may well be right -
> real
> > > usecases will know which signal groups they want to control
> together.
> > >
> > > >
> > > > > >
> > > > > > dither mode special ABI:
> > > > > >
> > > > > >  * Dither operation adds a small sinusoidal wave to the digital
> DAC
> > > > > > signal path. Dithering is a signal processing technique that
> > > involves
> > > > > > the injection of ac noise to the signal path to reduce system
> > > > > > nonlinearities.
> > > > > >
> > > > >
> > > > > This is a complex feature to describe as (if I read it correctly) we
> > > have
> > > > > a dither clocked from an external pin, or in theory from
> software.
> > > That
> > > > > clock
> > > > > frequency must match the dither.  Realistically that means it is a
> > > clock
> > > > > in our control or we have to match the period below to the
> > > frequency
> > > > > of that
> > > > > clock.
> > > >
> > > > Yeah, the frequency  of the dither signal is fsig = fclk / N, where N
> can
> > > only
> > > > be: [4 8 16 32 64]. So, we kind of just have these available options
> for
> > > the
> > > > signal frequency and fclk is something we can control and know
> > > (assuming
> > > > we have TGPx mapping which I'm bundling with a clk).
> > > >
> > > > The only quirk with having this with frequency rather than raw N
> is
> > > > to handle the sw_toggle where we have no idea about fclk? We
> > > could also
> > > > think of this attr as some kind of decimation...
> > >
> > > Does seems unlikely anyone would use dither with a sw toggle.
> > > Perhaps best plan here is to not support that combination.
> >
> > Agree... But there are some subtleties that we need to settle
> > about the channel default mode as I raised in my previous
> comments.
> >
> > > As to the clock, these are about controlling a sine wave frequency.
> I'm
> > > not sure
> > > decimation fits as a model. (figure 19)
> > >
> > > Given you know the input clock, perhaps present this as something
> like
> > > out_voltage0_dither_frequency
> > >
> > > I don't thing dithers are always this simple, so probably want to be
> > > specific its
> > > a sine wave so maybe we need
> > > out_voltage0_dither_type 'sine'
> > >
> > > We used to have some DDS chips in staging but looks like they all
> got
> > > dropped due
> > > to end of time Those had various waveforms and IIRC all we came
> up
> > > with was
> > > descriptive terms + frequencies and magnitudes.
> > >
> > >
> > > >
> > > > > > out_voltage0_dither_en
> > > > > >  - Same as in toggle mode.
> > > > > > out_voltage0_dither_period
> > > > > > out_voltage0_dither_phase
> > > > > >  - Period and phase of the signal. Only some values are valid
> so
> > > > > there's
> > > > > >    also *_available files for these. I'm not sure if we can use
> the
> > > more
> > > > > >    generic IIO_CHAN_INFO_PHASE and
> > > IIO_CHAN_INFO_FREQUENCY
> > > > > here as these
> > > > > >    parameters don't really apply to the channel output signal..
> > > > >
> > > > > Possibly not helpful to do so, but you could describe the
> channel as
> > > an
> > > > > out_altvoltage channel that happens to have a significant offset
> > > (the
> > > > > DC
> > > > > level) and phase, frequency etc as for a normal altvoltage
> channel.
> > > > > That would hide the intention here though so perhaps not a
> good
> > > plan
> > > > > even if it ensures we end up with standard ABI.
> > > >
> > > > I think altvoltage might not be optimal here  because the phase
> and
> > > frequency
> > > > are really not characteristics of the output signal of the channel.
> > >
> > > Well they kind of are if you set the magnitude high enough  - but I
> get
> > > your
> > > point.  That's not how they are intended to be used.
> > >
> > > >
> > > > > > out_voltage0_input_register
> > > > > >  - Same as in toggle mode. However in this mode the code set
> in
> > > the
> > > > > >    input_b register has a special meaning. It's the amplitude of
> the
> > > > > >    dither signal.
> > > > > Don't do that - provide a direct attribute representing the value
> of
> > > > > register_b and when it is written implicitly switch to the right
> > > register.
> > > > > Any ABI that requires a sequence of events is hard to use.
> > > >
> > > > I guess we can just use the same raw1 attr here? Even though, in
> > > dither
> > > > mode this has special meaning (it is the amplitude)...
> > >
> > > I was thinking toggle mode here. This interface doesn't work for
> dither.
> > > in that case there is just
> > > out_voltage0_raw for the DC part and
> > > out_voltage0_dither_raw for the dither amplitude.  Assumption
> being
> > > same scaling
> > > as I don't really want to support multiple scale factors if we can
> avoid it.
> >
> > I'm fairly sure it is the same scaling....
> 
> As long as it's a simple factor, just hit it with a 'small adjusting tool'
> until it does have the same scaling.  We do that in a few similar cases
> IIRC as sometimes things like this dither input have lower resolution.
> 
> If it's the same scaling then you won't have to hit it at all ;)

Hmm, I went to confirm this and I'm not so sure since our range here
is not the full 16bits but rather 13bits (2 LSB are to be set to 0). I will
have to look at this more carefully.

> >
> > >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > One special mention to the dac scale. In this part this is
> something
> > > > > > that can be purely controlled by SW so that I'm allowing
> > > userspace to
> > > > > > change it rather then having it in dts. The available scales are:
> > > > > >
> > > > > > * [0 5V] -> offset 0
> > > > > > * [0 10V] -> offset 0
> > > > > > * [-5 5V] -> offset -32678
> > > > > > * [-10 10V] -> offset -32768
> > > > > > * [-15 15V] -> offset -32768
> > > > > >
> > > > > > With the above, we also need to have the offset configurable
> > > and
> > > > > right
> > > > > > now I'm going to some trouble to make sure the scale + offset
> is
> > > > > > something valid. Honestly, I think I'm overdoing it because
> things
> > > can
> > > > > > still go wrong with [0 10V] and [-5 5V] as the scales are the
> same
> > > > > > here. Furthermore, there's no real arm that can be done to
> the
> > > HW.
> > > > > Is
> > > > > > just that the readings won't match with what someone might
> be
> > > > > expecting.
> > > > > > My plan is to just remove those checks and assume is up to
> > > > > userspace to
> > > > > > make it right and not have [-10 10V] scale with 0 offset for
> > > example.
> > > > >
> > > > > So this is something we've debated a few times in the past.
> > > > > There is a fairly strong argument for output devices that the
> range
> > > is
> > > > > a characteristic of the circuit.  At the very least it makes sense to
> > > > > restrict it in DT even if we allow safe forms of tweaking in the
> > > driver.
> > > > > For an initial driver, I'd just have it in DT.
> > > > >
> > > >
> > > > No complaints against that and makes things way simpler to
> handle.
> > > Great.
> > > >
> > > > - Nuno Sá
> > > >
> > > So conclusions.. Hmm. Not strong ones yet, but for dither mode at
> > > least
> > > I think you want to link particular channels to a TGPx choice
> > >
> > > out_voltage0_raw
> > > out_voltage0_raw_available ( nice to have on DACs)
> >
> > I guess here you mean 'IIO_AVAIL_RANGE'?
> 
> No, I mean providing the read_avail() callback and setting
> BIT(IIO_INFO_RAW) in info_mask_separate_available
> 
> That's how we provide range for a channel except in some unusual
> corner cases and the internal interface for that is used when a DAC
> is being used via the consumer interface (so some other driver wants
> to set it's output).

Yeah, I know :). I was just meaning 'IIO_AVAIL_RANGE' over 'IIO_AVAIL_LIST'.
I guess that was already obvious to you :).

> >
> > > out_voltage0_scale
> > > out_voltage0_dither_raw
> > > out_voltage0_dither_raw_available
> > > out_voltage0_dither_frequency
> > > out_voltage0_dither_frequency_available
> > > out_voltage0_dither_phase
> > > out_voltage0_dither_phase_available
> >
> > > Toggle mode is less clear to me but symbol approach plus TGPx in
> DT
> > > maybe works
> > > You could allow for software override to set the symbol.  Interface
> to
> > > unset
> > > it being to write an empty string to _symbol. Maybe leave that for
> > > now.
> > >
> > > out_voltage0_raw0
> > > out_voltage0_raw1
> > > out_voltage0_scale
> > > out_voltage0_symbol
> >
> > Well, in short, I do agree with this ABI. And actually, for toggle mode,
> I think
> > this is more or less what we will have. For dither/default mode,
> there's still
> > the questions I raised above... Maybe, at the end, we will end up
> with 3 different
> > ABI's...
> 
> Certainly possible.  Nice to avoid if we can, but not if it means
> stretching
> things too far.
> 
> >
> > I would only add this to the ABI:
> >  * out_voltage0_dither_en
> >  * out_voltage0_toggle_en
> >
> > Because if someone wants to change, let's say the dither frequency,
> the best way
> > to update things is to first disable dithering, update all the stuff, and
> then enable
> > it again...
> 
> I'll go with 'maybe' for these.  The use for changing things doesn't
> make sense to me
> unless we have multiple things to change at once.  If it's just the
> frequency it
> would be more intuitive to have a write to that attribute do the
> disable, set value
> and enable dithering again without needing to do a dance with the
> interface.

Yeah and that is something that can happen here (and probably the most
likely situation). For dither mode, you disable it, then you might want to
change all the parameters of your dither (amplitude, phase and frequency)
and then enable it again.

For toggle mode, this means, disabling it, updating input_a and input_b and
enable it again.

Anyways, I think we already have some discussion that enables me to send
the first version of this and we can continue from there. If all goes well,
it should be out by the end of the week.

- Nuno Sá

Re: [RFC PATCH 0/1] LTC2688 support

[Jonathan Cameron]

On Mon, 6 Dec 2021 10:49:17 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, December 5, 2021 7:01 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio@vger.kernel.org
> > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > 
> > [External]
> > 
> > On Tue, 30 Nov 2021 14:43:25 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > 
> > Hi Nuno
> > 
> > Hopefully I've not lost the plot on this!  
> 
> Not really. I had some days off so this was also set on hold from
> my side.
> 
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Sunday, November 21, 2021 1:18 PM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: linux-iio@vger.kernel.org
> > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > >
> > > > [External]
> > > >
> > > > On Mon, 15 Nov 2021 10:28:51 +0000
> > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > > >  
> > > > > Hi Jonathan,
> > > > >
> > > > > Thanks for your inputs...
> > > > >  
> > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > Sent: Friday, November 12, 2021 5:15 PM
> > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > Cc: linux-iio@vger.kernel.org
> > > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > > > >
> > > > > > [External]
> > > > > >
> > > > > > On Thu, 11 Nov 2021 12:00:42 +0100
> > > > > > Nuno Sá <nuno.sa@analog.com> wrote:
> > > > > >
> > > > > > Hi Nuno,
> > > > > >  
> > > > > > > The reason why this is a RFC is because I'm still waiting for  
> > proper  
> > > > HW  
> > > > > > > to test this thing. The reason why I'm sending this already is  
> > > > because  
> > > > > > > there's some non usual features which might require extra  
> > ABI.  
> > > > > > Hence,  
> > > > > > > while waiting I thought we could already speed up the  
> > process in  
> > > > > > regards  
> > > > > > > with the ABI.  
> > > > > >
> > > > > > Wise move as this is an unusual beast :)
> > > > > >  
> > > > > > >
> > > > > > > I still pushed the driver but the intent here is not really to  
> > review  
> > > > it.  
> > > > > > > Naturally, if someone already wants to give some feedback,  
> > > > that's  
> > > > > > very  
> > > > > > > much appreciated :)  
> > > > > >  
> > > > > > >
> > > > > > > Now, there are three main interfaces depending on the  
> > channel  
> > > > > > mode:  
> > > > > > >  1) default (no new ABI)
> > > > > > >  2) toggle mode
> > > > > > >  3) dither mode
> > > > > > >
> > > > > > > The channel mode will be a devicetree property as it does not  
> > > > really  
> > > > > > > make much sense to change between modes at runtime  
> > even  
> > > > more  
> > > > > > because the  
> > > > > > > piece of HW we might want to control with a channel might  
> > be  
> > > > > > different  
> > > > > > > depending on the selected mode (I'm fairly sure on this  
> > between  
> > > > > > toggle  
> > > > > > > and other modes but not so sure between dither and default  
> > > > mode).  
> > > > > >
> > > > > > I agree on toggle vs dither definitely being different, but normal  
> > > > you  
> > > > > > could implement either as software toggle, or dither with a 0
> > > > > > magnitude
> > > > > > sine wave.  So might not be worth implementing default mode  
> > at  
> > > > all.  
> > > > > > No harm in doing so though if there are advantages to having it.  
> > > > >
> > > > > My feeling is that we could probably have dither as the "default  
> > > > mode".  
> > > > > More on this below...
> > > > >  
> > > > > > >
> > > > > > > toggle mode special ABI:
> > > > > > >
> > > > > > >  * Toggle operation enables fast switching of a DAC output  
> > > > between  
> > > > > > two  
> > > > > > > different DAC codes without any SPI transaction. Two codes  
> > are  
> > > > set  
> > > > > > in  
> > > > > > > input_a and input_b and then the output switches according  
> > to  
> > > > an  
> > > > > > input  
> > > > > > > signal (input_a -> clk high; input_b -> clk low).
> > > > > > >
> > > > > > > out_voltageY_input_register
> > > > > > >  - selects between input_a and input_b. After selecting one  
> > > > register,  
> > > > > > we  
> > > > > > >    can write the dac code in out_voltageY_raw.
> > > > > > > out_voltageY_toggle_en
> > > > > > >  - Disable/Enable toggle mode. The reason why I think this  
> > one is  
> > > > > > >    important is because if we wanna change the 2 codes, we  
> > > > should  
> > > > > > first  
> > > > > > >    disable this, set the codes and only then enable the mode  
> > > > back...  
> > > > > > >    As I'm writing this, It kind of came to me that we can  
> > probably  
> > > > > > >    achieve this with out_voltageY_powerdown attr (maybe  
> > takes a  
> > > > bit  
> > > > > > more  
> > > > > > >    time to see the outputs but...)  
> > > > > >
> > > > > > Hmm. These corner cases always take a bit of figuring out.  
> > What  
> > > > you  
> > > > > > have
> > > > > > here is a bit 'device specific' for my liking.
> > > > > >
> > > > > > So there is precedence for ABI in this area, on the frequency  
> > > > devices  
> > > > > > but only
> > > > > > for devices we still haven't moved out of staging.  For those we
> > > > > > needed a means
> > > > > > to define selectable phases for PSK - where the selection was  
> > > > either  
> > > > > > software or,
> > > > > > much like here, a selection pin.
> > > > > >
> > > > > > out_altvotage0_phase0 etc
> > > > > >
> > > > > > so I guess the equivalent here would be
> > > > > > out_voltageY_raw0
> > > > > > out_voltageY_raw1
> > > > > > and the selection would be via something like
> > > > > > out_voltageY_symbol (0 or 1 in this case). - note this is only
> > > > > > relevant if you have the software toggle. Any enable needed  
> > for  
> > > > > > setting
> > > > > > can be done as part of the write sequence for the  raw0 / raw1  
> > and  
> > > > > > should
> > > > > > ideally not be exposed to userspace (controls that require  
> > manual  
> > > > > > sequencing
> > > > > > tend to be hard to use / document).  
> > > > >
> > > > > Yeah, I thought about that. I was even thinking in having  
> > something  
> > > > like  
> > > > > *_amplitude for dither mode. In some cases, where we might be  
> > left  
> > > > > in some non obvious state (eg: moved the select register to input  
> > b  
> > > > and  
> > > > > then we failed to set the code;), I prefer to leave things as  
> > flexible as  
> > > > > possible for userspace. But I agree it adds up more complexity  
> > and in  
> > > > > this case, I can just always go to 'input_a' when writing 'raw0'...
> > > > >  
> > > > > > However, I'm not 100% sure it really maps to this case.  What do  
> > > > you  
> > > > > > think?  
> > > > >
> > > > > I think it can work. Though out_voltageY_symbol probably needs  
> > to  
> > > > be  
> > > > > shared by type to be coherent with what we might have with  
> > TGPx.  
> > > >
> > > > That's fine.
> > > >  
> > > > > Note the sw_toggle register has a bit mask of the channels we  
> > want  
> > > > to  
> > > > > toggle which means we can toggle multiple channels at the same  
> > > > time.
> > > >
> > > > Using that wired up to buffer mode might make sense.  You'd  
> > provide  
> > > > multiple
> > > > buffers and allow channels to be selected into one of them at a  
> > time.  
> > > > Each
> > > > buffer is then tied to a different toggle (TGP0, TGP1, etc)
> > > >
> > > > The same could be true for the software toggle.  It'll get a bit fiddly
> > > > though.
> > > >
> > > > Perhaps this is an advanced feature to think about later...
> > > >  
> > > > > It works the same with TGPx if you map multiple channels to the  
> > > > same  
> > > > > pin.
> > > > >
> > > > > There's also the question on how to handle this if a TGPx is  
> > provided?  
> > > > > I guess it will just override the pin... But most likely having them  
> > both  
> > > > > at the same time will lead to non desired results (unless we have  
> > a  
> > > > > way to gate/ungate the clocks)...  
> > > > I don't follow this bit.  You mean TGPx and software toggle. As far  
> > as I  
> > > > can
> > > > tell it's an either/or thing per channel.
> > > >  
> > >
> > > Here I meant that if we have a TGPx pin bundled to some channel(s)  
> > we  
> > > do not want to also dance with the sw_toggle bit of that channel.  
> > Ultimately,  
> > > that's on the user responsibility but we could also add some guards I  
> > guess.  
> > > I'm not sure if it's an either/or thing per channel... IIUC, we spoke  
> > about  
> > > making dither and default mode the same. That might complicate  
> > things a bit  
> > > as:
> > >
> > > 1) We should not force a user to specify a TGPx pin for those  
> > channels (since  
> > > it can also work with dithering disabled).
> > > 2) Because of 1), we should also support sw_toggle for these  
> > channels since  
> > > someone might want to enable dither mode (at runtime) and the  
> > TGPx pin was  
> > > not given. Hence, we need to have a way to update the DAC using  
> > the sw_toggle.  
> > >
> > > Did I understood things wrong? One thing that comes to my mind, is  
> > to return  
> > > error (eg: EPERM or ENOTSUPP) if someone tries to enable dither  
> > mode and  
> > > no TGPx pin was selected for that channel? Hence, we do not need  
> > to add  
> > > the sw_toggle ABI  (out_voltage_symbol) for the default/dither  
> > mode. Or  
> > > maybe even better, we just expose the dither ABI if a TGPx pin is  
> > given over  
> > > dt (I try to explain the toggle/dither modes below; that might help  
> > you in  
> > > understanding my reasoning here)?
> > >
> > > Alternatively, we just keep the approach I have in this RFC and we  
> > keep the  
> > > 3 different modes (being mode a dt property; in the current state I'm  
> > using  
> > > a boolean to say a channel Is in toggle mode)... Maybe with the  
> > difference  
> > > that we allow sw_toggle for toggle enabled channels.  
> > 
> > The corner I'm not clear on is what we do if for example all TGPx pins
> > are
> > specified in DT.  Is the mapping from channel to TGPx things in toggle
> > mode
> > always a function of the external circuit or do we want to make it
> > runtime
> > controllable?
> > 
> > I'm absolutely fine if we just make it a dt property - particularly
> > as those TGPx pins may well not be visible to the host processor.
> > 
> > We probably do want to provide some options in dt for what they
> > might be
> > connnected to on the host.  I'm guessing potentially a gpio, or a clk?  
> 
> For each TGPx pin (from the point you bind it to some channel), I'm
> actually making it mandatory to have a clock (the reasoning being, if you 
> say some channel is controlled over TGPx [being for toggle or dither mode],
> you need to have some input at the pin).

I'm not really sure what the usecases behind toggle are...  Using a clock
with dither makes sense, but toggle might not be fixed frequency but
driven by some other type of event.

Still we don't have to support every usecase in an initial driver.
Stick to a clock and see what requests you get for other input types
later.

> 
> I might not be doing it in the way you're thinking but you can have a
> look in the actual series :) ...
> 

...



> > > > So conclusions.. Hmm. Not strong ones yet, but for dither mode at
> > > > least
> > > > I think you want to link particular channels to a TGPx choice
> > > >
> > > > out_voltage0_raw
> > > > out_voltage0_raw_available ( nice to have on DACs)  
> > >
> > > I guess here you mean 'IIO_AVAIL_RANGE'?  
> > 
> > No, I mean providing the read_avail() callback and setting
> > BIT(IIO_INFO_RAW) in info_mask_separate_available
> > 
> > That's how we provide range for a channel except in some unusual
> > corner cases and the internal interface for that is used when a DAC
> > is being used via the consumer interface (so some other driver wants
> > to set it's output).  
> 
> Yeah, I know :). I was just meaning 'IIO_AVAIL_RANGE' over 'IIO_AVAIL_LIST'.
> I guess that was already obvious to you :).

Ah.  Got you. I'd forgotten about that - indeed IIO_AVAIL_RANGE

> 
> > >  
> > > > out_voltage0_scale
> > > > out_voltage0_dither_raw
> > > > out_voltage0_dither_raw_available
> > > > out_voltage0_dither_frequency
> > > > out_voltage0_dither_frequency_available
> > > > out_voltage0_dither_phase
> > > > out_voltage0_dither_phase_available  
> > >  
> > > > Toggle mode is less clear to me but symbol approach plus TGPx in  
> > DT  
> > > > maybe works
> > > > You could allow for software override to set the symbol.  Interface  
> > to  
> > > > unset
> > > > it being to write an empty string to _symbol. Maybe leave that for
> > > > now.
> > > >
> > > > out_voltage0_raw0
> > > > out_voltage0_raw1
> > > > out_voltage0_scale
> > > > out_voltage0_symbol  
> > >
> > > Well, in short, I do agree with this ABI. And actually, for toggle mode,  
> > I think  
> > > this is more or less what we will have. For dither/default mode,  
> > there's still  
> > > the questions I raised above... Maybe, at the end, we will end up  
> > with 3 different  
> > > ABI's...  
> > 
> > Certainly possible.  Nice to avoid if we can, but not if it means
> > stretching
> > things too far.
> >   
> > >
> > > I would only add this to the ABI:
> > >  * out_voltage0_dither_en
> > >  * out_voltage0_toggle_en
> > >
> > > Because if someone wants to change, let's say the dither frequency,  
> > the best way  
> > > to update things is to first disable dithering, update all the stuff, and  
> > then enable  
> > > it again...  
> > 
> > I'll go with 'maybe' for these.  The use for changing things doesn't
> > make sense to me
> > unless we have multiple things to change at once.  If it's just the
> > frequency it
> > would be more intuitive to have a write to that attribute do the
> > disable, set value
> > and enable dithering again without needing to do a dance with the
> > interface.  
> 
> Yeah and that is something that can happen here (and probably the most
> likely situation). For dither mode, you disable it, then you might want to
> change all the parameters of your dither (amplitude, phase and frequency)
> and then enable it again.
> 
> For toggle mode, this means, disabling it, updating input_a and input_b and
> enable it again.
> 
> Anyways, I think we already have some discussion that enables me to send
> the first version of this and we can continue from there. If all goes well,
> it should be out by the end of the week.
> 
Great,

Jonathan

RE: [RFC PATCH 0/1] LTC2688 support

[Sa, Nuno]

> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Monday, December 6, 2021 2:16 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; linux-iio@vger.kernel.org
> Subject: Re: [RFC PATCH 0/1] LTC2688 support
> 
> [External]
> 
> On Mon, 6 Dec 2021 10:49:17 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Sunday, December 5, 2021 7:01 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio@vger.kernel.org
> > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > >
> > > [External]
> > >
> > > On Tue, 30 Nov 2021 14:43:25 +0000
> > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > >
> > > Hi Nuno
> > >
> > > Hopefully I've not lost the plot on this!
> >
> > Not really. I had some days off so this was also set on hold from
> > my side.
> >
> > > > > -----Original Message-----
> > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > Sent: Sunday, November 21, 2021 1:18 PM
> > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Cc: linux-iio@vger.kernel.org
> > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > > >
> > > > > [External]
> > > > >
> > > > > On Mon, 15 Nov 2021 10:28:51 +0000
> > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > > > >
> > > > > > Hi Jonathan,
> > > > > >
> > > > > > Thanks for your inputs...
> > > > > >
> > > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > > Sent: Friday, November 12, 2021 5:15 PM
> > > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > > Cc: linux-iio@vger.kernel.org
> > > > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support
> > > > > > >
> > > > > > > [External]
> > > > > > >
> > > > > > > On Thu, 11 Nov 2021 12:00:42 +0100
> > > > > > > Nuno Sá <nuno.sa@analog.com> wrote:
> > > > > > >
> > > > > > > Hi Nuno,
> > > > > > >
> > > > > > > > The reason why this is a RFC is because I'm still waiting for
> > > proper
> > > > > HW
> > > > > > > > to test this thing. The reason why I'm sending this already
> is
> > > > > because
> > > > > > > > there's some non usual features which might require
> extra
> > > ABI.
> > > > > > > Hence,
> > > > > > > > while waiting I thought we could already speed up the
> > > process in
> > > > > > > regards
> > > > > > > > with the ABI.
> > > > > > >
> > > > > > > Wise move as this is an unusual beast :)
> > > > > > >
> > > > > > > >
> > > > > > > > I still pushed the driver but the intent here is not really to
> > > review
> > > > > it.
> > > > > > > > Naturally, if someone already wants to give some
> feedback,
> > > > > that's
> > > > > > > very
> > > > > > > > much appreciated :)
> > > > > > >
> > > > > > > >
> > > > > > > > Now, there are three main interfaces depending on the
> > > channel
> > > > > > > mode:
> > > > > > > >  1) default (no new ABI)
> > > > > > > >  2) toggle mode
> > > > > > > >  3) dither mode
> > > > > > > >
> > > > > > > > The channel mode will be a devicetree property as it does
> not
> > > > > really
> > > > > > > > make much sense to change between modes at runtime
> > > even
> > > > > more
> > > > > > > because the
> > > > > > > > piece of HW we might want to control with a channel
> might
> > > be
> > > > > > > different
> > > > > > > > depending on the selected mode (I'm fairly sure on this
> > > between
> > > > > > > toggle
> > > > > > > > and other modes but not so sure between dither and
> default
> > > > > mode).
> > > > > > >
> > > > > > > I agree on toggle vs dither definitely being different, but
> normal
> > > > > you
> > > > > > > could implement either as software toggle, or dither with a
> 0
> > > > > > > magnitude
> > > > > > > sine wave.  So might not be worth implementing default
> mode
> > > at
> > > > > all.
> > > > > > > No harm in doing so though if there are advantages to
> having it.
> > > > > >
> > > > > > My feeling is that we could probably have dither as the
> "default
> > > > > mode".
> > > > > > More on this below...
> > > > > >
> > > > > > > >
> > > > > > > > toggle mode special ABI:
> > > > > > > >
> > > > > > > >  * Toggle operation enables fast switching of a DAC output
> > > > > between
> > > > > > > two
> > > > > > > > different DAC codes without any SPI transaction. Two
> codes
> > > are
> > > > > set
> > > > > > > in
> > > > > > > > input_a and input_b and then the output switches
> according
> > > to
> > > > > an
> > > > > > > input
> > > > > > > > signal (input_a -> clk high; input_b -> clk low).
> > > > > > > >
> > > > > > > > out_voltageY_input_register
> > > > > > > >  - selects between input_a and input_b. After selecting
> one
> > > > > register,
> > > > > > > we
> > > > > > > >    can write the dac code in out_voltageY_raw.
> > > > > > > > out_voltageY_toggle_en
> > > > > > > >  - Disable/Enable toggle mode. The reason why I think this
> > > one is
> > > > > > > >    important is because if we wanna change the 2 codes,
> we
> > > > > should
> > > > > > > first
> > > > > > > >    disable this, set the codes and only then enable the
> mode
> > > > > back...
> > > > > > > >    As I'm writing this, It kind of came to me that we can
> > > probably
> > > > > > > >    achieve this with out_voltageY_powerdown attr (maybe
> > > takes a
> > > > > bit
> > > > > > > more
> > > > > > > >    time to see the outputs but...)
> > > > > > >
> > > > > > > Hmm. These corner cases always take a bit of figuring out.
> > > What
> > > > > you
> > > > > > > have
> > > > > > > here is a bit 'device specific' for my liking.
> > > > > > >
> > > > > > > So there is precedence for ABI in this area, on the
> frequency
> > > > > devices
> > > > > > > but only
> > > > > > > for devices we still haven't moved out of staging.  For those
> we
> > > > > > > needed a means
> > > > > > > to define selectable phases for PSK - where the selection
> was
> > > > > either
> > > > > > > software or,
> > > > > > > much like here, a selection pin.
> > > > > > >
> > > > > > > out_altvotage0_phase0 etc
> > > > > > >
> > > > > > > so I guess the equivalent here would be
> > > > > > > out_voltageY_raw0
> > > > > > > out_voltageY_raw1
> > > > > > > and the selection would be via something like
> > > > > > > out_voltageY_symbol (0 or 1 in this case). - note this is only
> > > > > > > relevant if you have the software toggle. Any enable
> needed
> > > for
> > > > > > > setting
> > > > > > > can be done as part of the write sequence for the  raw0 /
> raw1
> > > and
> > > > > > > should
> > > > > > > ideally not be exposed to userspace (controls that require
> > > manual
> > > > > > > sequencing
> > > > > > > tend to be hard to use / document).
> > > > > >
> > > > > > Yeah, I thought about that. I was even thinking in having
> > > something
> > > > > like
> > > > > > *_amplitude for dither mode. In some cases, where we might
> be
> > > left
> > > > > > in some non obvious state (eg: moved the select register to
> input
> > > b
> > > > > and
> > > > > > then we failed to set the code;), I prefer to leave things as
> > > flexible as
> > > > > > possible for userspace. But I agree it adds up more complexity
> > > and in
> > > > > > this case, I can just always go to 'input_a' when writing
> 'raw0'...
> > > > > >
> > > > > > > However, I'm not 100% sure it really maps to this case.
> What do
> > > > > you
> > > > > > > think?
> > > > > >
> > > > > > I think it can work. Though out_voltageY_symbol probably
> needs
> > > to
> > > > > be
> > > > > > shared by type to be coherent with what we might have with
> > > TGPx.
> > > > >
> > > > > That's fine.
> > > > >
> > > > > > Note the sw_toggle register has a bit mask of the channels
> we
> > > want
> > > > > to
> > > > > > toggle which means we can toggle multiple channels at the
> same
> > > > > time.
> > > > >
> > > > > Using that wired up to buffer mode might make sense.  You'd
> > > provide
> > > > > multiple
> > > > > buffers and allow channels to be selected into one of them at a
> > > time.
> > > > > Each
> > > > > buffer is then tied to a different toggle (TGP0, TGP1, etc)
> > > > >
> > > > > The same could be true for the software toggle.  It'll get a bit
> fiddly
> > > > > though.
> > > > >
> > > > > Perhaps this is an advanced feature to think about later...
> > > > >
> > > > > > It works the same with TGPx if you map multiple channels to
> the
> > > > > same
> > > > > > pin.
> > > > > >
> > > > > > There's also the question on how to handle this if a TGPx is
> > > provided?
> > > > > > I guess it will just override the pin... But most likely having
> them
> > > both
> > > > > > at the same time will lead to non desired results (unless we
> have
> > > a
> > > > > > way to gate/ungate the clocks)...
> > > > > I don't follow this bit.  You mean TGPx and software toggle. As
> far
> > > as I
> > > > > can
> > > > > tell it's an either/or thing per channel.
> > > > >
> > > >
> > > > Here I meant that if we have a TGPx pin bundled to some
> channel(s)
> > > we
> > > > do not want to also dance with the sw_toggle bit of that channel.
> > > Ultimately,
> > > > that's on the user responsibility but we could also add some
> guards I
> > > guess.
> > > > I'm not sure if it's an either/or thing per channel... IIUC, we spoke
> > > about
> > > > making dither and default mode the same. That might complicate
> > > things a bit
> > > > as:
> > > >
> > > > 1) We should not force a user to specify a TGPx pin for those
> > > channels (since
> > > > it can also work with dithering disabled).
> > > > 2) Because of 1), we should also support sw_toggle for these
> > > channels since
> > > > someone might want to enable dither mode (at runtime) and the
> > > TGPx pin was
> > > > not given. Hence, we need to have a way to update the DAC
> using
> > > the sw_toggle.
> > > >
> > > > Did I understood things wrong? One thing that comes to my
> mind, is
> > > to return
> > > > error (eg: EPERM or ENOTSUPP) if someone tries to enable dither
> > > mode and
> > > > no TGPx pin was selected for that channel? Hence, we do not
> need
> > > to add
> > > > the sw_toggle ABI  (out_voltage_symbol) for the default/dither
> > > mode. Or
> > > > maybe even better, we just expose the dither ABI if a TGPx pin is
> > > given over
> > > > dt (I try to explain the toggle/dither modes below; that might
> help
> > > you in
> > > > understanding my reasoning here)?
> > > >
> > > > Alternatively, we just keep the approach I have in this RFC and
> we
> > > keep the
> > > > 3 different modes (being mode a dt property; in the current state
> I'm
> > > using
> > > > a boolean to say a channel Is in toggle mode)... Maybe with the
> > > difference
> > > > that we allow sw_toggle for toggle enabled channels.
> > >
> > > The corner I'm not clear on is what we do if for example all TGPx
> pins
> > > are
> > > specified in DT.  Is the mapping from channel to TGPx things in
> toggle
> > > mode
> > > always a function of the external circuit or do we want to make it
> > > runtime
> > > controllable?
> > >
> > > I'm absolutely fine if we just make it a dt property - particularly
> > > as those TGPx pins may well not be visible to the host processor.
> > >
> > > We probably do want to provide some options in dt for what they
> > > might be
> > > connnected to on the host.  I'm guessing potentially a gpio, or a clk?
> >
> > For each TGPx pin (from the point you bind it to some channel), I'm
> > actually making it mandatory to have a clock (the reasoning being, if
> you
> > say some channel is controlled over TGPx [being for toggle or dither
> mode],
> > you need to have some input at the pin).
> 
> I'm not really sure what the usecases behind toggle are...  Using a clock
> with dither makes sense, but toggle might not be fixed frequency but
> driven by some other type of event.

Taken from the datasheet (usecases for toggle mode):
"Examples include injection of a small ac bias, or independently switching
between on and off states". A clock seems to fit even though I get your
point that it might not fit all usecases for this. In that case, maybe sw_toggle
can be enough :).

Anyways, agreed that we do not have to support all the stuff at the very
beginning and it's probably a bad idea specially in cases like this where we
are not sure about all possible usecases.

- Nuno Sá