[PATCH v7 0/7] New features for the AD5686 IIO driver

[PATCH v7 0/7] New features for the AD5686 IIO driver

[Rodrigo Alencar via B4 Relay]

This is the second series of three on updating the AD5686 driver.

Initially, a big patch series was sent:
https://lore.kernel.org/r/20260422-ad5313r-iio-support-v1-0-ed7dca001d1b@analog.com

Then, the first patch series added fixes and cleanups:
https://lore.kernel.org/linux-iio/20260524-ad5686-fixes-v7-0-b6bf395d08bd@analog.com/

This one is introducing new features:
- Consume optional reset and correct power supplies;
- LDAC GPIO handling (active-low, held low when unused);
- SPI bus sync() implementation for batching multiple transfers;
- Triggered buffer support, leveraging LDAC and sync() to flush
  all channel writes atomically;
- Gain control support through the scale property.

Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
---
Changes in v7:
- Fix return value check of i2c_transfer().
- Amend refactoring of command/data macros.
- Link to v6: https://lore.kernel.org/r/20260705-ad5686-new-features-v6-0-269594c7aae5@analog.com

Changes in v6:
- Refactor command/data macros in favor of FIELD_PREP()
- Link to v5: https://lore.kernel.org/r/20260628-ad5686-new-features-v5-0-577caadbf7c0@analog.com

Changes in v5:
- Create auxiliary handler for the triggered buffer.
- Do not allow modification of double_scale in case gain_gpio is not set.
- Link to v4: https://lore.kernel.org/r/20260623-ad5686-new-features-v4-0-28962a57db0f@analog.com

Changes in v4:
- Replace anyof+const for enum in dt-bindings. 
- Address some sashiko's comments.
- Use guard(mutex)() in trigger handler and other misc changes.
- Link to v3: https://lore.kernel.org/r/20260616-ad5686-new-features-v3-0-f829fb7e9262@analog.com

Changes in v3:
- Add range-double property.
- Acquire reset control after power-up delay.
- Include cleanup.h and use guard(mutex)() in read_raw().
- Link to v2: https://lore.kernel.org/r/20260609-ad5686-new-features-v2-0-70b423f5c76d@analog.com

Changes in v2:
- Get reset control deasserted.
- Update entire spi_transfer struct rather than individual fields.
- Replace udelay() for fsleep() in probe().
- Minor changes addressing further feedback.
- Link to v1: https://lore.kernel.org/r/20260602-ad5686-new-features-v1-0-691e01883d27@analog.com

---
Rodrigo Alencar (7):
      iio: dac: ad5696: properly check i2c_transfer() return value
      iio: dac: ad5686: refactor command/data macros
      iio: dac: ad5686: introduce sync operation
      iio: dac: ad5686: implement new sync() op for the spi bus
      iio: dac: ad5686: read_raw/write_raw: use guard(mutex)()
      iio: dac: ad5686: add triggered buffer support
      iio: dac: ad5686: add gain control support

 drivers/iio/dac/Kconfig      |   2 +
 drivers/iio/dac/ad5686-spi.c | 136 ++++++++++++++++++++-------
 drivers/iio/dac/ad5686.c     | 215 +++++++++++++++++++++++++++++++++++++++----
 drivers/iio/dac/ad5686.h     |  52 ++++++++---
 drivers/iio/dac/ad5696-i2c.c |  17 ++--
 5 files changed, 345 insertions(+), 77 deletions(-)
---
base-commit: 4c0ecdbdc0d87570f74e656e21d56bae1ab6a8f1
change-id: 20260602-ad5686-new-features-e116c04bddb9

Best regards,
-- 
Rodrigo Alencar <rodrigo.alencar@analog.com>

[PATCH v7 1/7] iio: dac: ad5696: properly check i2c_transfer() return value

[Rodrigo Alencar via B4 Relay]

From: Rodrigo Alencar <rodrigo.alencar@analog.com>

Verify that the expected number of i2c messages were transferred when
ad5686_i2c_read() is called. This issue exists since the support for I2C
devices where first introduced.

Fixes: 4177381b4401 ("iio:dac:ad5686: Add AD5671R/75R/94/94R/95R/96/96R support")
Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260705114746.1485F1F000E9@smtp.kernel.org/
Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
---
 drivers/iio/dac/ad5696-i2c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/dac/ad5696-i2c.c b/drivers/iio/dac/ad5696-i2c.c
index 279309329b64..6f726e6301a6 100644
--- a/drivers/iio/dac/ad5696-i2c.c
+++ b/drivers/iio/dac/ad5696-i2c.c
@@ -40,8 +40,8 @@ static int ad5686_i2c_read(struct ad5686_state *st, u8 addr)
 				      0x00);
 
 	ret = i2c_transfer(i2c->adapter, msg, 2);
-	if (ret < 0)
-		return ret;
+	if (ret != 2)
+		return ret < 0 ? ret : -EIO;
 
 	return be16_to_cpu(st->data[0].d16);
 }

-- 
2.43.0

[PATCH v7 2/7] iio: dac: ad5686: refactor command/data macros

[Rodrigo Alencar via B4 Relay]

From: Rodrigo Alencar <rodrigo.alencar@analog.com>

Replace usage of bit shifting macros for FIELD_PREP(), which would not
ignore bit masking when preparing SPI/I2C commands. This change is a code
hardening measure to be paired with the upcoming triggered buffer support.
For the AD5310 regmap case, 16-bit data coming from the buffer may overlap
with command bits if the data field is unmasked.

*_REF_BIT_MSK and *_PD_MSK bit position macros are renamed (with a DATA
prefix) so to indicate that they are relative to the DATA field.

Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/all/20260628143026.EC6CA1F000E9@smtp.kernel.org/#t
Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
---
 drivers/iio/dac/ad5686-spi.c | 21 +++++++++++----------
 drivers/iio/dac/ad5686.c     | 10 +++++-----
 drivers/iio/dac/ad5686.h     | 22 ++++++++++++----------
 drivers/iio/dac/ad5696-i2c.c | 11 ++++++-----
 4 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/drivers/iio/dac/ad5686-spi.c b/drivers/iio/dac/ad5686-spi.c
index 6b6ef1d7071f..d3b64b4361d5 100644
--- a/drivers/iio/dac/ad5686-spi.c
+++ b/drivers/iio/dac/ad5686-spi.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/array_size.h>
+#include <linux/bitfield.h>
 #include <linux/errno.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -26,21 +27,21 @@ static int ad5686_spi_write(struct ad5686_state *st,
 
 	switch (st->chip_info->regmap_type) {
 	case AD5310_REGMAP:
-		st->data[0].d16 = cpu_to_be16(AD5310_CMD(cmd) |
-					      val);
+		st->data[0].d16 = cpu_to_be16(FIELD_PREP(AD5310_CMD_MSK, cmd) |
+					      FIELD_PREP(AD5310_DATA_MSK, val));
 		buf = &st->data[0].d8[0];
 		tx_len = 2;
 		break;
 	case AD5683_REGMAP:
-		st->data[0].d32 = cpu_to_be32(AD5686_CMD(cmd) |
-					      AD5683_DATA(val));
+		st->data[0].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
+					      FIELD_PREP(AD5683_DATA_MSK, val));
 		buf = &st->data[0].d8[1];
 		tx_len = 3;
 		break;
 	case AD5686_REGMAP:
-		st->data[0].d32 = cpu_to_be32(AD5686_CMD(cmd) |
-					      AD5686_ADDR(addr) |
-					      val);
+		st->data[0].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
+					      FIELD_PREP(AD5686_ADDR_MSK, addr) |
+					      FIELD_PREP(AD5686_DATA_MSK, val));
 		buf = &st->data[0].d8[1];
 		tx_len = 3;
 		break;
@@ -81,9 +82,9 @@ static int ad5686_spi_read(struct ad5686_state *st, u8 addr)
 		return -EINVAL;
 	}
 
-	st->data[0].d32 = cpu_to_be32(AD5686_CMD(cmd) |
-				      AD5686_ADDR(addr));
-	st->data[1].d32 = cpu_to_be32(AD5686_CMD(AD5686_CMD_NOOP));
+	st->data[0].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
+				      FIELD_PREP(AD5686_ADDR_MSK, addr));
+	st->data[1].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, AD5686_CMD_NOOP));
 
 	ret = spi_sync_transfer(spi, t, ARRAY_SIZE(t));
 	if (ret < 0)
diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
index 713fe71ad1e7..120dd095dd2e 100644
--- a/drivers/iio/dac/ad5686.c
+++ b/drivers/iio/dac/ad5686.c
@@ -35,8 +35,8 @@ static int ad5310_control_sync(struct ad5686_state *st)
 	unsigned int pd_val = st->pwr_down_mask & st->pwr_down_mode;
 
 	return ad5686_write(st, AD5686_CMD_CONTROL_REG, 0,
-			    FIELD_PREP(AD5310_PD_MSK, pd_val & AD5686_PD_MSK) |
-			    FIELD_PREP(AD5310_REF_BIT_MSK, st->use_internal_vref ? 0 : 1));
+			    FIELD_PREP(AD5310_DATA_PD_MSK, pd_val & AD5686_PD_MSK) |
+			    FIELD_PREP(AD5310_DATA_REF_MSK, st->use_internal_vref ? 0 : 1));
 }
 
 static int ad5683_control_sync(struct ad5686_state *st)
@@ -44,8 +44,8 @@ static int ad5683_control_sync(struct ad5686_state *st)
 	unsigned int pd_val = st->pwr_down_mask & st->pwr_down_mode;
 
 	return ad5686_write(st, AD5686_CMD_CONTROL_REG, 0,
-			    FIELD_PREP(AD5683_PD_MSK, pd_val & AD5686_PD_MSK) |
-			    FIELD_PREP(AD5683_REF_BIT_MSK, st->use_internal_vref ? 0 : 1));
+			    FIELD_PREP(AD5683_DATA_PD_MSK, pd_val & AD5686_PD_MSK) |
+			    FIELD_PREP(AD5683_DATA_REF_MSK, st->use_internal_vref ? 0 : 1));
 }
 
 static inline unsigned int ad5686_pd_mask_shift(const struct iio_chan_spec *chan)
@@ -562,7 +562,7 @@ int ad5686_probe(struct device *dev,
 		break;
 	case AD5686_REGMAP:
 		ret = ad5686_write(st, AD5686_CMD_INTERNAL_REFER_SETUP, 0,
-				   st->use_internal_vref ? 0 : AD5686_REF_BIT_MSK);
+				   st->use_internal_vref ? 0 : AD5686_DATA_REF_MSK);
 		if (ret)
 			return ret;
 		break;
diff --git a/drivers/iio/dac/ad5686.h b/drivers/iio/dac/ad5686.h
index c424720f8f72..32cb3931413c 100644
--- a/drivers/iio/dac/ad5686.h
+++ b/drivers/iio/dac/ad5686.h
@@ -14,12 +14,14 @@
 
 #include <linux/iio/iio.h>
 
-#define AD5310_CMD(x)				((x) << 12)
+#define AD5310_CMD_MSK				GENMASK(15, 12)
+#define AD5310_DATA_MSK				GENMASK(11, 0)
 
-#define AD5683_DATA(x)				((x) << 4)
+#define AD5683_DATA_MSK				GENMASK(19, 4)
 
-#define AD5686_ADDR(x)				((x) << 16)
-#define AD5686_CMD(x)				((x) << 20)
+#define AD5686_CMD_MSK				GENMASK(23, 20)
+#define AD5686_ADDR_MSK				GENMASK(19, 16)
+#define AD5686_DATA_MSK				GENMASK(15, 0)
 
 #define AD5686_ADDR_DAC(chan)			(0x1 << (chan))
 #define AD5686_ADDR_ALL_DAC			0xF
@@ -38,15 +40,15 @@
 #define AD5686_CMD_CONTROL_REG			0x4
 #define AD5686_CMD_READBACK_ENABLE_V2		0x5
 
-#define AD5310_REF_BIT_MSK			BIT(8)
-#define AD5310_PD_MSK				GENMASK(10, 9)
+#define AD5310_DATA_REF_MSK			BIT(8)
+#define AD5310_DATA_PD_MSK			GENMASK(10, 9)
 
-#define AD5683_REF_BIT_MSK			BIT(12)
-#define AD5683_PD_MSK				GENMASK(14, 13)
+#define AD5683_DATA_REF_MSK			BIT(12) /* DB16 */
+#define AD5683_DATA_PD_MSK			GENMASK(14, 13) /* DB18:DB17 */
+
+#define AD5686_DATA_REF_MSK			BIT(0)
 
-#define AD5686_REF_BIT_MSK			BIT(0)
 #define AD5686_PD_MSK				GENMASK(1, 0)
-
 #define AD5686_PD_MODE_1K_TO_GND		0x1
 #define AD5686_PD_MODE_100K_TO_GND		0x2
 #define AD5686_PD_MODE_THREE_STATE		0x3
diff --git a/drivers/iio/dac/ad5696-i2c.c b/drivers/iio/dac/ad5696-i2c.c
index 6f726e6301a6..73dcd2e6f51b 100644
--- a/drivers/iio/dac/ad5696-i2c.c
+++ b/drivers/iio/dac/ad5696-i2c.c
@@ -7,6 +7,7 @@
  * Copyright 2018 Analog Devices Inc.
  */
 
+#include <linux/bitfield.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
@@ -35,9 +36,8 @@ static int ad5686_i2c_read(struct ad5686_state *st, u8 addr)
 	};
 	int ret;
 
-	st->data[0].d32 = cpu_to_be32(AD5686_CMD(AD5686_CMD_NOOP) |
-				      AD5686_ADDR(addr) |
-				      0x00);
+	st->data[0].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, AD5686_CMD_NOOP) |
+				      FIELD_PREP(AD5686_ADDR_MSK, addr));
 
 	ret = i2c_transfer(i2c->adapter, msg, 2);
 	if (ret != 2)
@@ -52,8 +52,9 @@ static int ad5686_i2c_write(struct ad5686_state *st,
 	struct i2c_client *i2c = to_i2c_client(st->dev);
 	int ret;
 
-	st->data[0].d32 = cpu_to_be32(AD5686_CMD(cmd) | AD5686_ADDR(addr)
-				      | val);
+	st->data[0].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
+				      FIELD_PREP(AD5686_ADDR_MSK, addr) |
+				      FIELD_PREP(AD5686_DATA_MSK, val));
 
 	ret = i2c_master_send(i2c, &st->data[0].d8[1], 3);
 	if (ret < 0)

-- 
2.43.0

[PATCH v7 3/7] iio: dac: ad5686: introduce sync operation

[Rodrigo Alencar via B4 Relay]

From: Rodrigo Alencar <rodrigo.alencar@analog.com>

Add sync() to operation to ad5686_bus_ops, which can be used to flush
multiple pending data transfers at once. This is going to be used when
implementing triggered buffer support.

Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
---
 drivers/iio/dac/ad5686.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/dac/ad5686.h b/drivers/iio/dac/ad5686.h
index 32cb3931413c..ae9aeda2d201 100644
--- a/drivers/iio/dac/ad5686.h
+++ b/drivers/iio/dac/ad5686.h
@@ -70,10 +70,12 @@ struct ad5686_state;
  * struct ad5686_bus_ops - bus specific read/write operations
  * @read: read a register value at the given address
  * @write: write a command, address and value to the device
+ * @sync: ensure the completion of the write operation (optional)
  */
 struct ad5686_bus_ops {
 	int (*read)(struct ad5686_state *st, u8 addr);
 	int (*write)(struct ad5686_state *st, u8 cmd, u8 addr, u16 val);
+	int (*sync)(struct ad5686_state *st);
 };
 
 /**
@@ -162,7 +164,13 @@ int ad5686_probe(struct device *dev,
 
 static inline int ad5686_write(struct ad5686_state *st, u8 cmd, u8 addr, u16 val)
 {
-	return st->ops->write(st, cmd, addr, val);
+	int ret;
+
+	ret = st->ops->write(st, cmd, addr, val);
+	if (ret)
+		return ret;
+
+	return st->ops->sync ? st->ops->sync(st) : 0;
 }
 
 static inline int ad5686_read(struct ad5686_state *st, u8 addr)

-- 
2.43.0

[PATCH v7 4/7] iio: dac: ad5686: implement new sync() op for the spi bus

[Rodrigo Alencar via B4 Relay]

From: Rodrigo Alencar <rodrigo.alencar@analog.com>

Use of local SPI bus data to manage a collection of SPI transfers and
flush them to the SPI platform driver with the sync() operation. This
allows for faster handling of multiple channel DAC writes, avoiding kernel
overhead per spi_sync() call, which will be helpful when enabling
triggered buffer support.

Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
---
 drivers/iio/dac/ad5686-spi.c | 129 ++++++++++++++++++++++++++++++++-----------
 drivers/iio/dac/ad5686.c     |   4 +-
 drivers/iio/dac/ad5686.h     |   8 ++-
 drivers/iio/dac/ad5696-i2c.c |   2 +-
 4 files changed, 108 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/dac/ad5686-spi.c b/drivers/iio/dac/ad5686-spi.c
index d3b64b4361d5..ae74a17ba48e 100644
--- a/drivers/iio/dac/ad5686-spi.c
+++ b/drivers/iio/dac/ad5686-spi.c
@@ -13,59 +13,95 @@
 #include <linux/errno.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/spi/spi.h>
 
 #include <asm/byteorder.h>
 
 #include "ad5686.h"
 
+/**
+ * struct ad5686_spi_data - SPI bus specific data
+ * @msg: SPI message used for transfers
+ * @size: number of transfers currently in the message
+ * @capacity: maximum number of transfers that can be added to the message
+ * @xfers: array of SPI transfers, allocated with the provided capacity
+ */
+struct ad5686_spi_data {
+	struct spi_message msg;
+	unsigned int size;
+	unsigned int capacity;
+	struct spi_transfer xfers[] __counted_by(capacity);
+};
+
 static int ad5686_spi_write(struct ad5686_state *st,
 			    u8 cmd, u8 addr, u16 val)
 {
-	struct spi_device *spi = to_spi_device(st->dev);
-	u8 tx_len, *buf;
+	struct ad5686_spi_data *bus_data = st->bus_data;
+	struct spi_transfer *xfer;
 
+	if (bus_data->size >= bus_data->capacity)
+		return -E2BIG;
+
+	if (bus_data->size)
+		bus_data->xfers[bus_data->size - 1].cs_change = 1;
+	else
+		spi_message_init(&bus_data->msg);
+
+	xfer = &bus_data->xfers[bus_data->size];
 	switch (st->chip_info->regmap_type) {
 	case AD5310_REGMAP:
-		st->data[0].d16 = cpu_to_be16(FIELD_PREP(AD5310_CMD_MSK, cmd) |
-					      FIELD_PREP(AD5310_DATA_MSK, val));
-		buf = &st->data[0].d8[0];
-		tx_len = 2;
+		st->data[bus_data->size].d16 =
+			cpu_to_be16(FIELD_PREP(AD5310_CMD_MSK, cmd) |
+				    FIELD_PREP(AD5310_DATA_MSK, val));
+		*xfer = (struct spi_transfer) {
+			.tx_buf = &st->data[bus_data->size].d16,
+			.len = sizeof(st->data[bus_data->size].d16),
+		};
 		break;
 	case AD5683_REGMAP:
-		st->data[0].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
-					      FIELD_PREP(AD5683_DATA_MSK, val));
-		buf = &st->data[0].d8[1];
-		tx_len = 3;
+		st->data[bus_data->size].d32 =
+			cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
+				    FIELD_PREP(AD5683_DATA_MSK, val));
+		*xfer = (struct spi_transfer) {
+			.tx_buf = &st->data[bus_data->size].d8[1],
+			.len = sizeof(st->data[bus_data->size].d8) - 1,
+		};
 		break;
 	case AD5686_REGMAP:
-		st->data[0].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
-					      FIELD_PREP(AD5686_ADDR_MSK, addr) |
-					      FIELD_PREP(AD5686_DATA_MSK, val));
-		buf = &st->data[0].d8[1];
-		tx_len = 3;
+		st->data[bus_data->size].d32 =
+			cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
+				    FIELD_PREP(AD5686_ADDR_MSK, addr) |
+				    FIELD_PREP(AD5686_DATA_MSK, val));
+		*xfer = (struct spi_transfer) {
+			.tx_buf = &st->data[bus_data->size].d8[1],
+			.len = sizeof(st->data[bus_data->size].d8) - 1,
+		};
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	return spi_write(spi, buf, tx_len);
+	spi_message_add_tail(xfer, &bus_data->msg);
+	bus_data->size++;
+
+	return 0;
+}
+
+static int ad5686_spi_sync(struct ad5686_state *st)
+{
+	struct spi_device *spi = to_spi_device(st->dev);
+	struct ad5686_spi_data *bus_data = st->bus_data;
+
+	bus_data->size = 0; /* always reset, even on sync failure */
+	return spi_sync(spi, &bus_data->msg);
 }
 
 static int ad5686_spi_read(struct ad5686_state *st, u8 addr)
 {
-	struct spi_transfer t[] = {
-		{
-			.tx_buf = &st->data[0].d8[1],
-			.len = 3,
-			.cs_change = 1,
-		}, {
-			.tx_buf = &st->data[1].d8[1],
-			.rx_buf = &st->data[2].d8[1],
-			.len = 3,
-		},
-	};
 	struct spi_device *spi = to_spi_device(st->dev);
+	struct ad5686_spi_data *bus_data = st->bus_data;
+	struct spi_transfer *xfer = &bus_data->xfers[0];
 	u8 cmd = 0;
 	int ret;
 
@@ -86,8 +122,21 @@ static int ad5686_spi_read(struct ad5686_state *st, u8 addr)
 				      FIELD_PREP(AD5686_ADDR_MSK, addr));
 	st->data[1].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, AD5686_CMD_NOOP));
 
-	ret = spi_sync_transfer(spi, t, ARRAY_SIZE(t));
-	if (ret < 0)
+	xfer[0] = (struct spi_transfer) {
+		.tx_buf = &st->data[0].d8[1],
+		.len = sizeof(st->data[0].d8) - 1,
+		.cs_change = 1,
+	};
+	xfer[1] = (struct spi_transfer) {
+		.tx_buf = &st->data[1].d8[1],
+		.rx_buf = &st->data[2].d8[1],
+		.len = sizeof(st->data[1].d8) - 1,
+	};
+
+	spi_message_init_with_transfers(&bus_data->msg, xfer, 2);
+
+	ret = spi_sync(spi, &bus_data->msg);
+	if (ret)
 		return ret;
 
 	return be32_to_cpu(st->data[2].d32);
@@ -96,12 +145,30 @@ static int ad5686_spi_read(struct ad5686_state *st, u8 addr)
 static const struct ad5686_bus_ops ad5686_spi_ops = {
 	.write = ad5686_spi_write,
 	.read = ad5686_spi_read,
+	.sync = ad5686_spi_sync,
 };
 
 static int ad5686_spi_probe(struct spi_device *spi)
 {
-	return ad5686_probe(&spi->dev, spi_get_device_match_data(spi),
-			    spi->modalias, &ad5686_spi_ops);
+	const struct ad5686_chip_info *info;
+	struct ad5686_spi_data *bus_data;
+	struct device *dev = &spi->dev;
+	unsigned int capacity;
+
+	info = spi_get_device_match_data(spi);
+	if (!info)
+		return -ENODEV;
+
+	/* read operation requires at least 2 transfers */
+	capacity = max(info->num_channels, 2);
+	bus_data = devm_kzalloc(dev, struct_size(bus_data, xfers, capacity),
+				GFP_KERNEL);
+	if (!bus_data)
+		return -ENOMEM;
+
+	bus_data->capacity = capacity;
+
+	return ad5686_probe(dev, info, spi->modalias, &ad5686_spi_ops, bus_data);
 }
 
 static const struct spi_device_id ad5686_spi_id[] = {
diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
index 120dd095dd2e..316f9ccf54d9 100644
--- a/drivers/iio/dac/ad5686.c
+++ b/drivers/iio/dac/ad5686.c
@@ -472,7 +472,8 @@ EXPORT_SYMBOL_NS_GPL(ad5679r_chip_info, "IIO_AD5686");
 
 int ad5686_probe(struct device *dev,
 		 const struct ad5686_chip_info *chip_info,
-		 const char *name, const struct ad5686_bus_ops *ops)
+		 const char *name, const struct ad5686_bus_ops *ops,
+		 void *bus_data)
 {
 	struct reset_control *rstc;
 	struct ad5686_state *st;
@@ -487,6 +488,7 @@ int ad5686_probe(struct device *dev,
 
 	st->dev = dev;
 	st->ops = ops;
+	st->bus_data = bus_data;
 	st->chip_info = chip_info;
 
 	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
diff --git a/drivers/iio/dac/ad5686.h b/drivers/iio/dac/ad5686.h
index ae9aeda2d201..2c3d7d5b1d6b 100644
--- a/drivers/iio/dac/ad5686.h
+++ b/drivers/iio/dac/ad5686.h
@@ -25,6 +25,7 @@
 
 #define AD5686_ADDR_DAC(chan)			(0x1 << (chan))
 #define AD5686_ADDR_ALL_DAC			0xF
+#define AD5686_MAX_CHANNELS			16
 
 #define AD5686_CMD_NOOP				0x0
 #define AD5686_CMD_WRITE_INPUT_N		0x1
@@ -132,6 +133,7 @@ extern const struct ad5686_chip_info ad5679r_chip_info;
  * @use_internal_vref:	set to true if the internal reference voltage is used
  * @lock:		lock to protect access to state fields, which includes
  *			the data buffer during regmap ops
+ * @bus_data:		bus specific data
  * @data:		transfer buffers
  */
 struct ad5686_state {
@@ -144,6 +146,7 @@ struct ad5686_state {
 	unsigned int			pwr_down_mode;
 	bool				use_internal_vref;
 	struct mutex			lock;
+	void				*bus_data;
 
 	/*
 	 * DMA (thus cache coherency maintenance) may require the
@@ -154,13 +157,14 @@ struct ad5686_state {
 		__be32 d32;
 		__be16 d16;
 		u8 d8[4];
-	} data[3] __aligned(IIO_DMA_MINALIGN);
+	} data[AD5686_MAX_CHANNELS] __aligned(IIO_DMA_MINALIGN);
 };
 
 
 int ad5686_probe(struct device *dev,
 		 const struct ad5686_chip_info *chip_info,
-		 const char *name, const struct ad5686_bus_ops *ops);
+		 const char *name, const struct ad5686_bus_ops *ops,
+		 void *bus_data);
 
 static inline int ad5686_write(struct ad5686_state *st, u8 cmd, u8 addr, u16 val)
 {
diff --git a/drivers/iio/dac/ad5696-i2c.c b/drivers/iio/dac/ad5696-i2c.c
index 73dcd2e6f51b..b122d8d938e4 100644
--- a/drivers/iio/dac/ad5696-i2c.c
+++ b/drivers/iio/dac/ad5696-i2c.c
@@ -71,7 +71,7 @@ static const struct ad5686_bus_ops ad5686_i2c_ops = {
 static int ad5686_i2c_probe(struct i2c_client *i2c)
 {
 	return ad5686_probe(&i2c->dev, i2c_get_match_data(i2c),
-			    i2c->name, &ad5686_i2c_ops);
+			    i2c->name, &ad5686_i2c_ops, NULL);
 }
 
 static const struct i2c_device_id ad5686_i2c_id[] = {

-- 
2.43.0

[PATCH v7 5/7] iio: dac: ad5686: read_raw/write_raw: use guard(mutex)()

[Rodrigo Alencar via B4 Relay]

From: Rodrigo Alencar <rodrigo.alencar@analog.com>

Use guarded mutex lock to facilitate code review when adding new
attributes. This will allow for early returns, avoiding error-prone
locking and unlocking in error paths. This also adds missing include
linux/cleanup.h. Gain-control support will allow the scale attribute
to be configurable.

Reviewed-by: Maxwell Doose <m32285159@gmail.com>
Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
---
 drivers/iio/dac/ad5686.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
index 316f9ccf54d9..df32f46db81e 100644
--- a/drivers/iio/dac/ad5686.c
+++ b/drivers/iio/dac/ad5686.c
@@ -8,6 +8,7 @@
 #include <linux/array_size.h>
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/dev_printk.h>
 #include <linux/errno.h>
@@ -177,11 +178,11 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
 	struct ad5686_state *st = iio_priv(indio_dev);
 	int ret;
 
+	guard(mutex)(&st->lock);
+
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&st->lock);
 		ret = ad5686_read(st, chan->address);
-		mutex_unlock(&st->lock);
 		if (ret < 0)
 			return ret;
 		*val = (ret >> chan->scan_type.shift) &
@@ -202,23 +203,19 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
 			    long mask)
 {
 	struct ad5686_state *st = iio_priv(indio_dev);
-	int ret;
+
+	guard(mutex)(&st->lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		if (val >= (1 << chan->scan_type.realbits) || val < 0)
 			return -EINVAL;
 
-		mutex_lock(&st->lock);
-		ret = ad5686_write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
-				   chan->address, val << chan->scan_type.shift);
-		mutex_unlock(&st->lock);
-		break;
+		return ad5686_write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
+				    chan->address, val << chan->scan_type.shift);
 	default:
-		ret = -EINVAL;
+		return -EINVAL;
 	}
-
-	return ret;
 }
 
 static const struct iio_info ad5686_info = {

-- 
2.43.0

[PATCH v7 6/7] iio: dac: ad5686: add triggered buffer support

[Rodrigo Alencar via B4 Relay]

From: Rodrigo Alencar <rodrigo.alencar@analog.com>

Implement trigger handler by leveraging the LDAC gpio to update all DAC
channels at once when it is available. Also, the multiple channel writes
can be flushed at once with the sync() operation.

Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
---
 drivers/iio/dac/Kconfig  |  2 ++
 drivers/iio/dac/ad5686.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 17529509da9d..d6d560c09e25 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -243,6 +243,8 @@ config LTC2688
 
 config AD5686
 	tristate
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 
 config AD5686_SPI
 	tristate "Analog Devices AD5686 and similar multi-channel DACs (SPI)"
diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
index df32f46db81e..eeeff5c6cf38 100644
--- a/drivers/iio/dac/ad5686.c
+++ b/drivers/iio/dac/ad5686.c
@@ -21,7 +21,11 @@
 #include <linux/sysfs.h>
 #include <linux/wordpart.h>
 
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 #include "ad5686.h"
 
@@ -243,6 +247,7 @@ static const struct iio_chan_spec_ext_info ad5686_ext_info[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
 		.address = addr,				\
+		.scan_index = chan,				\
 		.scan_type = {					\
 			.sign = 'u',				\
 			.realbits = (bits),			\
@@ -467,6 +472,60 @@ const struct ad5686_chip_info ad5679r_chip_info = {
 };
 EXPORT_SYMBOL_NS_GPL(ad5679r_chip_info, "IIO_AD5686");
 
+static void do_ad5686_trigger_handler(struct iio_dev *indio_dev)
+{
+	struct iio_buffer *buffer = indio_dev->buffer;
+	struct ad5686_state *st = iio_priv(indio_dev);
+	u16 val[AD5686_MAX_CHANNELS] = { };
+	unsigned int scan_count, ch, i;
+	bool async_update;
+	u8 cmd;
+
+	if (iio_pop_from_buffer(buffer, val))
+		return;
+
+	guard(mutex)(&st->lock);
+
+	scan_count = bitmap_weight(indio_dev->active_scan_mask,
+				   iio_get_masklength(indio_dev));
+	async_update = st->ldac_gpio && scan_count > 1;
+	if (async_update) {
+		/* use LDAC to update all channels simultaneously */
+		cmd = AD5686_CMD_WRITE_INPUT_N;
+		gpiod_set_value_cansleep(st->ldac_gpio, 0);
+	} else {
+		cmd = AD5686_CMD_WRITE_INPUT_N_UPDATE_N;
+	}
+
+	i = 0;
+	iio_for_each_active_channel(indio_dev, ch) {
+		if (st->ops->write(st, cmd, indio_dev->channels[ch].address, val[i++]))
+			break;
+	}
+
+	/*
+	 * If sync() is available, it is called here regardless of write
+	 * failure to allow bus implementation to reset. In that case, partial
+	 * writes are unlikely as the write operations would just queue up
+	 * the transfers.
+	 */
+	if (st->ops->sync)
+		st->ops->sync(st);
+
+	if (async_update)
+		gpiod_set_value_cansleep(st->ldac_gpio, 1);
+}
+
+static irqreturn_t ad5686_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+
+	do_ad5686_trigger_handler(indio_dev);
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
 int ad5686_probe(struct device *dev,
 		 const struct ad5686_chip_info *chip_info,
 		 const char *name, const struct ad5686_bus_ops *ops,
@@ -569,6 +628,13 @@ int ad5686_probe(struct device *dev,
 		return -EINVAL;
 	}
 
+	ret = devm_iio_triggered_buffer_setup_ext(dev, indio_dev, NULL,
+						  &ad5686_trigger_handler,
+						  IIO_BUFFER_DIRECTION_OUT,
+						  NULL, NULL);
+	if (ret)
+		return ret;
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(ad5686_probe, "IIO_AD5686");

-- 
2.43.0

[PATCH v7 7/7] iio: dac: ad5686: add gain control support

[Rodrigo Alencar via B4 Relay]

From: Rodrigo Alencar <rodrigo.alencar@analog.com>

Most of the supported devices rely on a GAIN pin to control a 2x
multiplier applied to the output voltage. Other devices, e.g. the
single-channel ones, provides a gain control through a bit field in
the control register. Some designs might have the GAIN pin hardwired
to VDD/VLOGIC or GND, which would have no "gain-gpios" device property,
being able to set "adi,range-double" if it is hardwired to VDD. The
vref_mv field is moved down in the struct ad5686_state, so that the
overall size increase is reduced.

Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
---
 drivers/iio/dac/ad5686.c | 120 +++++++++++++++++++++++++++++++++++++++++++++--
 drivers/iio/dac/ad5686.h |  12 ++++-
 2 files changed, 125 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
index eeeff5c6cf38..18ecd4abdfea 100644
--- a/drivers/iio/dac/ad5686.c
+++ b/drivers/iio/dac/ad5686.c
@@ -15,10 +15,13 @@
 #include <linux/export.h>
 #include <linux/gpio/consumer.h>
 #include <linux/kstrtox.h>
+#include <linux/math64.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/sysfs.h>
+#include <linux/units.h>
 #include <linux/wordpart.h>
 
 #include <linux/iio/buffer.h>
@@ -41,7 +44,8 @@ static int ad5310_control_sync(struct ad5686_state *st)
 
 	return ad5686_write(st, AD5686_CMD_CONTROL_REG, 0,
 			    FIELD_PREP(AD5310_DATA_PD_MSK, pd_val & AD5686_PD_MSK) |
-			    FIELD_PREP(AD5310_DATA_REF_MSK, st->use_internal_vref ? 0 : 1));
+			    FIELD_PREP(AD5310_DATA_REF_MSK, st->use_internal_vref ? 0 : 1) |
+			    FIELD_PREP(AD5310_DATA_GAIN_MSK, st->double_scale ? 1 : 0));
 }
 
 static int ad5683_control_sync(struct ad5686_state *st)
@@ -50,7 +54,8 @@ static int ad5683_control_sync(struct ad5686_state *st)
 
 	return ad5686_write(st, AD5686_CMD_CONTROL_REG, 0,
 			    FIELD_PREP(AD5683_DATA_PD_MSK, pd_val & AD5686_PD_MSK) |
-			    FIELD_PREP(AD5683_DATA_REF_MSK, st->use_internal_vref ? 0 : 1));
+			    FIELD_PREP(AD5683_DATA_REF_MSK, st->use_internal_vref ? 0 : 1) |
+			    FIELD_PREP(AD5683_DATA_GAIN_MSK, st->double_scale ? 1 : 0));
 }
 
 static inline unsigned int ad5686_pd_mask_shift(const struct iio_chan_spec *chan)
@@ -193,9 +198,14 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
 			GENMASK(chan->scan_type.realbits - 1, 0);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		*val = st->vref_mv;
-		*val2 = chan->scan_type.realbits;
-		return IIO_VAL_FRACTIONAL_LOG2;
+		if (st->double_scale) {
+			*val = st->scale_avail[2];
+			*val2 = st->scale_avail[3];
+		} else {
+			*val = st->scale_avail[0];
+			*val2 = st->scale_avail[1];
+		}
+		return IIO_VAL_INT_PLUS_NANO;
 	}
 	return -EINVAL;
 }
@@ -207,6 +217,8 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
 			    long mask)
 {
 	struct ad5686_state *st = iio_priv(indio_dev);
+	bool double_scale;
+	int ret;
 
 	guard(mutex)(&st->lock);
 
@@ -217,6 +229,82 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
 
 		return ad5686_write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
 				    chan->address, val << chan->scan_type.shift);
+	case IIO_CHAN_INFO_SCALE:
+		if (val == st->scale_avail[0] && val2 == st->scale_avail[1])
+			double_scale = false;
+		else if (val == st->scale_avail[2] && val2 == st->scale_avail[3])
+			double_scale = true;
+		else
+			return -EINVAL;
+
+		if (st->double_scale == double_scale)
+			return 0; /* no change */
+
+		if (st->chip_info->regmap_type == AD5686_REGMAP && !st->gain_gpio)
+			return -EINVAL; /* GAIN pin is board-strapped */
+
+		st->double_scale = double_scale;
+		switch (st->chip_info->regmap_type) {
+		case AD5310_REGMAP:
+			ret = ad5310_control_sync(st);
+			break;
+		case AD5683_REGMAP:
+			ret = ad5683_control_sync(st);
+			break;
+		case AD5686_REGMAP:
+			ret = gpiod_set_value_cansleep(st->gain_gpio,
+						       st->double_scale ? 1 : 0);
+			break;
+		default:
+			ret = -EINVAL;
+		}
+		if (ret)
+			st->double_scale = !double_scale; /* revert on failure */
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad5686_write_raw_get_fmt(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan,
+				    long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad5686_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	struct ad5686_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*type = IIO_VAL_INT_PLUS_NANO;
+
+		if (st->chip_info->regmap_type == AD5686_REGMAP && !st->gain_gpio) {
+			/*
+			 * GAIN pin is board-strapped, so only the current
+			 * scale is available.
+			 */
+			*vals = st->double_scale ? &st->scale_avail[2] :
+						   &st->scale_avail[0];
+			*length = 2;
+			return IIO_AVAIL_LIST;
+		}
+
+		*vals = st->scale_avail;
+		*length = ARRAY_SIZE(st->scale_avail);
+		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;
 	}
@@ -225,6 +313,8 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
 static const struct iio_info ad5686_info = {
 	.read_raw = ad5686_read_raw,
 	.write_raw = ad5686_write_raw,
+	.write_raw_get_fmt = ad5686_write_raw_get_fmt,
+	.read_avail = ad5686_read_avail,
 };
 
 static const struct iio_chan_spec_ext_info ad5686_ext_info[] = {
@@ -246,6 +336,7 @@ static const struct iio_chan_spec_ext_info ad5686_ext_info[] = {
 		.channel = chan,				\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
+		.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),\
 		.address = addr,				\
 		.scan_index = chan,				\
 		.scan_type = {					\
@@ -472,6 +563,15 @@ const struct ad5686_chip_info ad5679r_chip_info = {
 };
 EXPORT_SYMBOL_NS_GPL(ad5679r_chip_info, "IIO_AD5686");
 
+static void ad5686_init_scale_avail(struct ad5686_state *st)
+{
+	int realbits = st->chip_info->channels[0].scan_type.realbits;
+	s64 tmp = 2ULL * st->vref_mv * NANO >> realbits;
+
+	st->scale_avail[2] = div_s64_rem(tmp, NANO, &st->scale_avail[3]);
+	st->scale_avail[0] = div_s64_rem(tmp >> 1, NANO, &st->scale_avail[1]);
+}
+
 static void do_ad5686_trigger_handler(struct iio_dev *indio_dev)
 {
 	struct iio_buffer *buffer = indio_dev->buffer;
@@ -585,6 +685,16 @@ int ad5686_probe(struct device *dev,
 		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
 				     "Failed to get LDAC GPIO\n");
 
+	st->double_scale = device_property_read_bool(dev, "adi,range-double");
+	st->gain_gpio = devm_gpiod_get_optional(dev, "gain",
+						st->double_scale ? GPIOD_OUT_HIGH :
+								   GPIOD_OUT_LOW);
+	if (IS_ERR(st->gain_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->gain_gpio),
+				     "Failed to get GAIN GPIO\n");
+
+	ad5686_init_scale_avail(st);
+
 	/* Initialize masks to all ones */
 	st->pwr_down_mask = ~0;
 	st->pwr_down_mode = ~0;
diff --git a/drivers/iio/dac/ad5686.h b/drivers/iio/dac/ad5686.h
index 2c3d7d5b1d6b..c1222ef83a4f 100644
--- a/drivers/iio/dac/ad5686.h
+++ b/drivers/iio/dac/ad5686.h
@@ -41,9 +41,11 @@
 #define AD5686_CMD_CONTROL_REG			0x4
 #define AD5686_CMD_READBACK_ENABLE_V2		0x5
 
+#define AD5310_DATA_GAIN_MSK			BIT(7)
 #define AD5310_DATA_REF_MSK			BIT(8)
 #define AD5310_DATA_PD_MSK			GENMASK(10, 9)
 
+#define AD5683_DATA_GAIN_MSK			BIT(11) /* DB15 */
 #define AD5683_DATA_REF_MSK			BIT(12) /* DB16 */
 #define AD5683_DATA_PD_MSK			GENMASK(14, 13) /* DB18:DB17 */
 
@@ -127,9 +129,12 @@ extern const struct ad5686_chip_info ad5679r_chip_info;
  * @chip_info:		chip model specific constants, available modes etc
  * @ops:		bus specific operations
  * @ldac_gpio:		LDAC pin GPIO descriptor
- * @vref_mv:		actual reference voltage used
+ * @gain_gpio:		GAIN pin GPIO descriptor
  * @pwr_down_mask:	power down mask
  * @pwr_down_mode:	current power down mode
+ * @scale_avail:	pre-calculated available scale values
+ * @vref_mv:		actual reference voltage used
+ * @double_scale:	flag to indicate the gain multiplier is applied
  * @use_internal_vref:	set to true if the internal reference voltage is used
  * @lock:		lock to protect access to state fields, which includes
  *			the data buffer during regmap ops
@@ -141,9 +146,12 @@ struct ad5686_state {
 	const struct ad5686_chip_info	*chip_info;
 	const struct ad5686_bus_ops	*ops;
 	struct gpio_desc		*ldac_gpio;
-	unsigned short			vref_mv;
+	struct gpio_desc		*gain_gpio;
 	unsigned int			pwr_down_mask;
 	unsigned int			pwr_down_mode;
+	int				scale_avail[4];
+	unsigned short			vref_mv;
+	bool				double_scale;
 	bool				use_internal_vref;
 	struct mutex			lock;
 	void				*bus_data;

-- 
2.43.0

Re: [PATCH v7 1/7] iio: dac: ad5696: properly check i2c_transfer() return value

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The I2C read transaction uses overlapping transmit and receive buffers, causing a DMA mapping violation and potential cacheline corruption.
- [High] Missing NULL check for `chip_info` during driver probe when bound via sysfs.
--

--- Patch [1]: [PATCH v7 1/7] iio: dac: ad5696: properly check i2c_transfer() return value ---
commit 7918e0fc4a2bf3dec679b7e0bd608081aff01ef3
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: dac: ad5696: properly check i2c_transfer() return value

Verify that the expected number of i2c messages were transferred when
ad5686_i2c_read() is called.

Closes: https://lore.kernel.org/all/20260705114746.1485F1F000E9@smtp.kernel.org/

> diff --git a/drivers/iio/dac/ad5696-i2c.c b/drivers/iio/dac/ad5696-i2c.c
> index 279309329b64b..6f726e6301a6e 100644
> --- a/drivers/iio/dac/ad5696-i2c.c
> +++ b/drivers/iio/dac/ad5696-i2c.c
> @@ -40,8 +40,8 @@ static int ad5686_i2c_read(struct ad5686_state *st, u8 addr)
>  				      0x00);
>  
>  	ret = i2c_transfer(i2c->adapter, msg, 2);
> -	if (ret < 0)
> -		return ret;
> +	if (ret != 2)
> +		return ret < 0 ? ret : -EIO;

[Severity: High]
This is a pre-existing issue, but does this I2C read transaction use
overlapping transmit and receive buffers?

Looking at the msg array initialization earlier in ad5686_i2c_read():

    struct i2c_msg msg[2] = {
        {
            ...
            .buf = &st->data[0].d8[1],
        },
        {
            ...
            .buf = (char *)&st->data[0].d16,
        },
    };

The transmit buffer and the receive buffer are mapped to the same
st->data[0] union, meaning they overlap and share the same cacheline.

When the I2C controller uses non-cache-coherent DMA, mapping overlapping
buffers for both DMA_TO_DEVICE and DMA_FROM_DEVICE within the same
cacheline violates the DMA API.

Could the cache invalidation for the receive buffer clobber the dirty
cacheline containing the transmit buffer's data, leading to corrupted data
being sent to the DAC or corrupted readback?

>  	return be16_to_cpu(st->data[0].d16);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing NULL
check for chip_info during driver probe when bound via sysfs?

In drivers/iio/dac/ad5696-i2c.c:ad5686_i2c_probe():

    return ad5686_probe(&i2c->dev, i2c_get_match_data(i2c), ...);

If a user binds the driver using driver_override via sysfs with a device
name not present in the driver's id_table or of_match_table,
i2c_get_match_data() will return NULL.

This NULL pointer is then passed to ad5686_probe() in
drivers/iio/dac/ad5686.c:

    st->vref_mv = st->use_internal_vref ? st->chip_info->int_vref_mv : ret / 1000;

Will this unconditionally dereference chip_info and cause a kernel panic
during device probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ad5686-new-features-v7-0-1bcc8c280e4d@analog.com?part=1

Re: [PATCH v7 2/7] iio: dac: ad5686: refactor command/data macros

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] DMA API Cacheline Sharing Violation in SPI Read
--

--- Patch [2]: [PATCH v7 2/7] iio: dac: ad5686: refactor command/data macros ---
commit 7ffa72bdbbf5952d81c8c2ac98d1aa1023cddf2f
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>
iio: dac: ad5686: refactor command/data macros

This commit refactors bit shifting macros in the AD5686 driver to use
FIELD_PREP(). It hardens the code to prepare for upcoming triggered buffer
support by ensuring bit masking is applied correctly.

Link: https://lore.kernel.org/all/20260628143026.EC6CA1F000E9@smtp.kernel.org/#t

> diff --git a/drivers/iio/dac/ad5686-spi.c b/drivers/iio/dac/ad5686-spi.c
> index 6b6ef1d7071fe..d3b64b4361d51 100644
> --- a/drivers/iio/dac/ad5686-spi.c
> +++ b/drivers/iio/dac/ad5686-spi.c
[ ... ]
> @@ -81,9 +82,9 @@ static int ad5686_spi_read(struct ad5686_state *st, u8 addr)
>  		return -EINVAL;
>  	}
>  
> -	st->data[0].d32 = cpu_to_be32(AD5686_CMD(cmd) |
> -				      AD5686_ADDR(addr));
> -	st->data[1].d32 = cpu_to_be32(AD5686_CMD(AD5686_CMD_NOOP));
> +	st->data[0].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
> +				      FIELD_PREP(AD5686_ADDR_MSK, addr));
> +	st->data[1].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, AD5686_CMD_NOOP));
>  
>  	ret = spi_sync_transfer(spi, t, ARRAY_SIZE(t));
>  	if (ret < 0)

[Severity: High]
This isn't a bug introduced by this patch, but is it possible this setup
causes a DMA API cacheline sharing violation during SPI reads?

In ad5686_spi_read(), the SPI transfers use adjacent elements from the
st->data array:

    struct spi_transfer t[] = {
        {
            .tx_buf = &st->data[0].d8[1],
            .len = 3,
            .cs_change = 1,
        }, {
            .tx_buf = &st->data[1].d8[1],
            .rx_buf = &st->data[2].d8[1],
            .len = 3,
        },
    };

Since st->data elements are 4-byte unions, they reside within the same
CPU cacheline. When spi_sync_transfer() maps st->data[0] and st->data[1]
as DMA_TO_DEVICE and st->data[2] as DMA_FROM_DEVICE simultaneously, does
this create overlapping mappings that violate the DMA API?

This could trigger CONFIG_DMA_API_DEBUG warnings for overlapping
mappings, and on non-coherent architectures, could there be a risk of
memory corruption when DMA_FROM_DEVICE cache invalidations interact
with active transfers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-ad5686-new-features-v7-0-1bcc8c280e4d@analog.com?part=2

Re: [PATCH v7 4/7] iio: dac: ad5686: implement new sync() op for the spi bus

[David Lechner]

On 7/10/26 6:20 AM, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Use of local SPI bus data to manage a collection of SPI transfers and
> flush them to the SPI platform driver with the sync() operation. This
> allows for faster handling of multiple channel DAC writes, avoiding kernel
> overhead per spi_sync() call, which will be helpful when enabling
> triggered buffer support.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> ---
>  drivers/iio/dac/ad5686-spi.c | 129 ++++++++++++++++++++++++++++++++-----------
>  drivers/iio/dac/ad5686.c     |   4 +-
>  drivers/iio/dac/ad5686.h     |   8 ++-
>  drivers/iio/dac/ad5696-i2c.c |   2 +-
>  4 files changed, 108 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5686-spi.c b/drivers/iio/dac/ad5686-spi.c
> index d3b64b4361d5..ae74a17ba48e 100644
> --- a/drivers/iio/dac/ad5686-spi.c
> +++ b/drivers/iio/dac/ad5686-spi.c
> @@ -13,59 +13,95 @@
>  #include <linux/errno.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/overflow.h>
>  #include <linux/spi/spi.h>
>  
>  #include <asm/byteorder.h>
>  
>  #include "ad5686.h"
>  
> +/**
> + * struct ad5686_spi_data - SPI bus specific data
> + * @msg: SPI message used for transfers
> + * @size: number of transfers currently in the message
> + * @capacity: maximum number of transfers that can be added to the message
> + * @xfers: array of SPI transfers, allocated with the provided capacity
> + */
> +struct ad5686_spi_data {
> +	struct spi_message msg;
> +	unsigned int size;
> +	unsigned int capacity;
> +	struct spi_transfer xfers[] __counted_by(capacity);
> +};
> +
>  static int ad5686_spi_write(struct ad5686_state *st,
>  			    u8 cmd, u8 addr, u16 val)
>  {
> -	struct spi_device *spi = to_spi_device(st->dev);
> -	u8 tx_len, *buf;
> +	struct ad5686_spi_data *bus_data = st->bus_data;
> +	struct spi_transfer *xfer;
>  

This could use some comments similar to the explanation in the
commit message that this function is just queuing writes to
be sent over the bus when sync() is called.

> +	if (bus_data->size >= bus_data->capacity)
> +		return -E2BIG;
> +
> +	if (bus_data->size)
> +		bus_data->xfers[bus_data->size - 1].cs_change = 1;
> +	else
> +		spi_message_init(&bus_data->msg);
> +
> +	xfer = &bus_data->xfers[bus_data->size];

Why not a local variable for st->data[bus_data->size] too so we don't
have to write it so many times?

>  	switch (st->chip_info->regmap_type) {
>  	case AD5310_REGMAP:
> -		st->data[0].d16 = cpu_to_be16(FIELD_PREP(AD5310_CMD_MSK, cmd) |
> -					      FIELD_PREP(AD5310_DATA_MSK, val));
> -		buf = &st->data[0].d8[0];
> -		tx_len = 2;
> +		st->data[bus_data->size].d16 =
> +			cpu_to_be16(FIELD_PREP(AD5310_CMD_MSK, cmd) |
> +				    FIELD_PREP(AD5310_DATA_MSK, val));
> +		*xfer = (struct spi_transfer) {
> +			.tx_buf = &st->data[bus_data->size].d16,
> +			.len = sizeof(st->data[bus_data->size].d16),
> +		};
>  		break;
>  	case AD5683_REGMAP:
> -		st->data[0].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
> -					      FIELD_PREP(AD5683_DATA_MSK, val));
> -		buf = &st->data[0].d8[1];
> -		tx_len = 3;
> +		st->data[bus_data->size].d32 =
> +			cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
> +				    FIELD_PREP(AD5683_DATA_MSK, val));
> +		*xfer = (struct spi_transfer) {
> +			.tx_buf = &st->data[bus_data->size].d8[1],
> +			.len = sizeof(st->data[bus_data->size].d8) - 1,
> +		};
>  		break;
>  	case AD5686_REGMAP:
> -		st->data[0].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
> -					      FIELD_PREP(AD5686_ADDR_MSK, addr) |
> -					      FIELD_PREP(AD5686_DATA_MSK, val));
> -		buf = &st->data[0].d8[1];
> -		tx_len = 3;
> +		st->data[bus_data->size].d32 =
> +			cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
> +				    FIELD_PREP(AD5686_ADDR_MSK, addr) |
> +				    FIELD_PREP(AD5686_DATA_MSK, val));
> +		*xfer = (struct spi_transfer) {
> +			.tx_buf = &st->data[bus_data->size].d8[1],
> +			.len = sizeof(st->data[bus_data->size].d8) - 1,
> +		};
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	return spi_write(spi, buf, tx_len);
> +	spi_message_add_tail(xfer, &bus_data->msg);
> +	bus_data->size++;
> +
> +	return 0;
> +}
> +
> +static int ad5686_spi_sync(struct ad5686_state *st)
> +{
> +	struct spi_device *spi = to_spi_device(st->dev);
> +	struct ad5686_spi_data *bus_data = st->bus_data;
> +
> +	bus_data->size = 0; /* always reset, even on sync failure */
> +	return spi_sync(spi, &bus_data->msg);
>  }
>  
>  static int ad5686_spi_read(struct ad5686_state *st, u8 addr)
>  {
> -	struct spi_transfer t[] = {
> -		{
> -			.tx_buf = &st->data[0].d8[1],
> -			.len = 3,
> -			.cs_change = 1,
> -		}, {
> -			.tx_buf = &st->data[1].d8[1],
> -			.rx_buf = &st->data[2].d8[1],
> -			.len = 3,
> -		},
> -	};
>  	struct spi_device *spi = to_spi_device(st->dev);
> +	struct ad5686_spi_data *bus_data = st->bus_data;
> +	struct spi_transfer *xfer = &bus_data->xfers[0];

It would be nice if the commit message also explained why the
read() function needs to be changed to use bus_data.

>  	u8 cmd = 0;
>  	int ret;
>  
> @@ -86,8 +122,21 @@ static int ad5686_spi_read(struct ad5686_state *st, u8 addr)
>  				      FIELD_PREP(AD5686_ADDR_MSK, addr));
>  	st->data[1].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, AD5686_CMD_NOOP));
>  
> -	ret = spi_sync_transfer(spi, t, ARRAY_SIZE(t));
> -	if (ret < 0)
> +	xfer[0] = (struct spi_transfer) {
> +		.tx_buf = &st->data[0].d8[1],
> +		.len = sizeof(st->data[0].d8) - 1,
> +		.cs_change = 1,
> +	};
> +	xfer[1] = (struct spi_transfer) {
> +		.tx_buf = &st->data[1].d8[1],
> +		.rx_buf = &st->data[2].d8[1],
> +		.len = sizeof(st->data[1].d8) - 1,
> +	};
> +
> +	spi_message_init_with_transfers(&bus_data->msg, xfer, 2);
> +
> +	ret = spi_sync(spi, &bus_data->msg);
> +	if (ret)
>  		return ret;
>  
>  	return be32_to_cpu(st->data[2].d32);

Re: [PATCH v7 5/7] iio: dac: ad5686: read_raw/write_raw: use guard(mutex)()

[David Lechner]

On 7/10/26 6:20 AM, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Use guarded mutex lock to facilitate code review when adding new
> attributes. This will allow for early returns, avoiding error-prone
> locking and unlocking in error paths. This also adds missing include
> linux/cleanup.h. Gain-control support will allow the scale attribute
> to be configurable.
> 
> Reviewed-by: Maxwell Doose <m32285159@gmail.com>
> Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> ---
>  drivers/iio/dac/ad5686.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index 316f9ccf54d9..df32f46db81e 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c
> @@ -8,6 +8,7 @@
>  #include <linux/array_size.h>
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/dev_printk.h>
>  #include <linux/errno.h>
> @@ -177,11 +178,11 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
>  	struct ad5686_state *st = iio_priv(indio_dev);
>  	int ret;
>  
> +	guard(mutex)(&st->lock);

The commit message should explain why the location we are taking the lock
has moved. Although, I don't see a reason to move it.

> +
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
> -		mutex_lock(&st->lock);
>  		ret = ad5686_read(st, chan->address);
> -		mutex_unlock(&st->lock);
>  		if (ret < 0)
>  			return ret;
>  		*val = (ret >> chan->scan_type.shift) &
> @@ -202,23 +203,19 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
>  			    long mask)
>  {
>  	struct ad5686_state *st = iio_priv(indio_dev);
> -	int ret;
> +
> +	guard(mutex)(&st->lock);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		if (val >= (1 << chan->scan_type.realbits) || val < 0)
>  			return -EINVAL;
>  
> -		mutex_lock(&st->lock);
> -		ret = ad5686_write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
> -				   chan->address, val << chan->scan_type.shift);
> -		mutex_unlock(&st->lock);
> -		break;
> +		return ad5686_write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
> +				    chan->address, val << chan->scan_type.shift);
>  	default:
> -		ret = -EINVAL;
> +		return -EINVAL;
>  	}
> -
> -	return ret;
>  }
>  
>  static const struct iio_info ad5686_info = {
> 

Re: [PATCH v7 6/7] iio: dac: ad5686: add triggered buffer support

[David Lechner]

On 7/10/26 6:20 AM, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Implement trigger handler by leveraging the LDAC gpio to update all DAC
> channels at once when it is available. Also, the multiple channel writes
> can be flushed at once with the sync() operation.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> ---
>  drivers/iio/dac/Kconfig  |  2 ++
>  drivers/iio/dac/ad5686.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 17529509da9d..d6d560c09e25 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -243,6 +243,8 @@ config LTC2688
>  
>  config AD5686
>  	tristate
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  
>  config AD5686_SPI
>  	tristate "Analog Devices AD5686 and similar multi-channel DACs (SPI)"
> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index df32f46db81e..eeeff5c6cf38 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c
> @@ -21,7 +21,11 @@
>  #include <linux/sysfs.h>
>  #include <linux/wordpart.h>
>  
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  
>  #include "ad5686.h"
>  
> @@ -243,6 +247,7 @@ static const struct iio_chan_spec_ext_info ad5686_ext_info[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
>  		.address = addr,				\
> +		.scan_index = chan,				\
>  		.scan_type = {					\
>  			.sign = 'u',				\
>  			.realbits = (bits),			\
> @@ -467,6 +472,60 @@ const struct ad5686_chip_info ad5679r_chip_info = {
>  };
>  EXPORT_SYMBOL_NS_GPL(ad5679r_chip_info, "IIO_AD5686");
>  
> +static void do_ad5686_trigger_handler(struct iio_dev *indio_dev)
> +{
> +	struct iio_buffer *buffer = indio_dev->buffer;

Only used once and doesn't save a line wrap, so no need for a local variable.

> +	struct ad5686_state *st = iio_priv(indio_dev);
> +	u16 val[AD5686_MAX_CHANNELS] = { };
> +	unsigned int scan_count, ch, i;
> +	bool async_update;
> +	u8 cmd;
> +
> +	if (iio_pop_from_buffer(buffer, val))
> +		return;
> +
> +	guard(mutex)(&st->lock);
> +
> +	scan_count = bitmap_weight(indio_dev->active_scan_mask,
> +				   iio_get_masklength(indio_dev));
> +	async_update = st->ldac_gpio && scan_count > 1;
> +	if (async_update) {
> +		/* use LDAC to update all channels simultaneously */
> +		cmd = AD5686_CMD_WRITE_INPUT_N;
> +		gpiod_set_value_cansleep(st->ldac_gpio, 0);
> +	} else {
> +		cmd = AD5686_CMD_WRITE_INPUT_N_UPDATE_N;
> +	}
> +
> +	i = 0;
> +	iio_for_each_active_channel(indio_dev, ch) {
> +		if (st->ops->write(st, cmd, indio_dev->channels[ch].address, val[i++]))
> +			break;
> +	}
> +
> +	/*
> +	 * If sync() is available, it is called here regardless of write
> +	 * failure to allow bus implementation to reset. In that case, partial
> +	 * writes are unlikely as the write operations would just queue up
> +	 * the transfers.
> +	 */
> +	if (st->ops->sync)
> +		st->ops->sync(st);
> +
> +	if (async_update)
> +		gpiod_set_value_cansleep(st->ldac_gpio, 1);
> +}
> +

Re: [PATCH v7 7/7] iio: dac: ad5686: add gain control support

[David Lechner]

On 7/10/26 6:20 AM, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Most of the supported devices rely on a GAIN pin to control a 2x
> multiplier applied to the output voltage. Other devices, e.g. the
> single-channel ones, provides a gain control through a bit field in
> the control register. Some designs might have the GAIN pin hardwired
> to VDD/VLOGIC or GND, which would have no "gain-gpios" device property,
> being able to set "adi,range-double" if it is hardwired to VDD. The
> vref_mv field is moved down in the struct ad5686_state, so that the
> overall size increase is reduced.
> 

...

> @@ -217,6 +229,82 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
>  
>  		return ad5686_write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
>  				    chan->address, val << chan->scan_type.shift);
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val == st->scale_avail[0] && val2 == st->scale_avail[1])
> +			double_scale = false;
> +		else if (val == st->scale_avail[2] && val2 == st->scale_avail[3])
> +			double_scale = true;
> +		else
> +			return -EINVAL;
> +
> +		if (st->double_scale == double_scale)
> +			return 0; /* no change */
> +
> +		if (st->chip_info->regmap_type == AD5686_REGMAP && !st->gain_gpio)

This condition is used a couple of times. It could be nice to just add a
st->pin_strapped_gain field to make it self-documenting and use that instead.

> +			return -EINVAL; /* GAIN pin is board-strapped */
> +
> +		st->double_scale = double_scale;
> +		switch (st->chip_info->regmap_type) {
> +		case AD5310_REGMAP:
> +			ret = ad5310_control_sync(st);
> +			break;
> +		case AD5683_REGMAP:
> +			ret = ad5683_control_sync(st);
> +			break;
> +		case AD5686_REGMAP:
> +			ret = gpiod_set_value_cansleep(st->gain_gpio,
> +						       st->double_scale ? 1 : 0);
> +			break;
> +		default:
> +			ret = -EINVAL;
> +		}
> +		if (ret)
> +			st->double_scale = !double_scale; /* revert on failure */
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

Re: [PATCH v7 0/7] New features for the AD5686 IIO driver

[David Lechner]

On 7/10/26 6:20 AM, Rodrigo Alencar via B4 Relay wrote:
> This is the second series of three on updating the AD5686 driver.
> 
> Initially, a big patch series was sent:
> https://lore.kernel.org/r/20260422-ad5313r-iio-support-v1-0-ed7dca001d1b@analog.com
> 
> Then, the first patch series added fixes and cleanups:
> https://lore.kernel.org/linux-iio/20260524-ad5686-fixes-v7-0-b6bf395d08bd@analog.com/
> 
> This one is introducing new features:
> - Consume optional reset and correct power supplies;
> - LDAC GPIO handling (active-low, held low when unused);
> - SPI bus sync() implementation for batching multiple transfers;
> - Triggered buffer support, leveraging LDAC and sync() to flush
>   all channel writes atomically;
> - Gain control support through the scale property.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> ---

I made a few comments, but nothing critical, so...

Reviewed-by: David Lechner <dlechner@baylibre.com>

Re: [PATCH v7 1/7] iio: dac: ad5696: properly check i2c_transfer() return value

[Jonathan Cameron]

On Fri, 10 Jul 2026 12:20:45 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Verify that the expected number of i2c messages were transferred when
> ad5686_i2c_read() is called. This issue exists since the support for I2C
> devices where first introduced.
> 
> Fixes: 4177381b4401 ("iio:dac:ad5686: Add AD5671R/75R/94/94R/95R/96/96R support")
> Reported-by: sashiko-bot@kernel.org
> Closes: https://lore.kernel.org/all/20260705114746.1485F1F000E9@smtp.kernel.org/
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
FWIW, as I understand it, these are vanishingly rare unless the device is
doing something unusual so I'll leave this for merge with the rest of the series. 

Jonathan

> ---
>  drivers/iio/dac/ad5696-i2c.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5696-i2c.c b/drivers/iio/dac/ad5696-i2c.c
> index 279309329b64..6f726e6301a6 100644
> --- a/drivers/iio/dac/ad5696-i2c.c
> +++ b/drivers/iio/dac/ad5696-i2c.c
> @@ -40,8 +40,8 @@ static int ad5686_i2c_read(struct ad5686_state *st, u8 addr)
>  				      0x00);
>  
>  	ret = i2c_transfer(i2c->adapter, msg, 2);
> -	if (ret < 0)
> -		return ret;
> +	if (ret != 2)
> +		return ret < 0 ? ret : -EIO;
>  
>  	return be16_to_cpu(st->data[0].d16);
>  }
> 

Re: [PATCH v7 0/7] New features for the AD5686 IIO driver

[Jonathan Cameron]

On Sat, 11 Jul 2026 10:23:56 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 7/10/26 6:20 AM, Rodrigo Alencar via B4 Relay wrote:
> > This is the second series of three on updating the AD5686 driver.
> > 
> > Initially, a big patch series was sent:
> > https://lore.kernel.org/r/20260422-ad5313r-iio-support-v1-0-ed7dca001d1b@analog.com
> > 
> > Then, the first patch series added fixes and cleanups:
> > https://lore.kernel.org/linux-iio/20260524-ad5686-fixes-v7-0-b6bf395d08bd@analog.com/
> > 
> > This one is introducing new features:
> > - Consume optional reset and correct power supplies;
> > - LDAC GPIO handling (active-low, held low when unused);
> > - SPI bus sync() implementation for batching multiple transfers;
> > - Triggered buffer support, leveraging LDAC and sync() to flush
> >   all channel writes atomically;
> > - Gain control support through the scale property.
> > 
> > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > ---  
> 
> I made a few comments, but nothing critical, so...
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> 
> 
I didn't find anything to add, so should be good with a v8
covering the stuff David pointed out.  FWIW sashiko is clean
though it does raise a missing null check if anyone forces
a bind that might be good to fix.

As to the DMA cache line issues, I'd ignore those. If they
are a real problem with any controller (or more likely
the interconnect / caches in front of it) than I think it
is up to the controller driver to bounce the data.

J


J

Re: [PATCH v7 1/7] iio: dac: ad5696: properly check i2c_transfer() return value

[Andy Shevchenko]

On Fri, Jul 10, 2026 at 12:20:45PM +0100, Rodrigo Alencar via B4 Relay wrote:

> Verify that the expected number of i2c messages were transferred when
> ad5686_i2c_read() is called. This issue exists since the support for I2C
> devices where first introduced.

...

>  	ret = i2c_transfer(i2c->adapter, msg, 2);
> -	if (ret < 0)
> -		return ret;

No need to touch this.

> +	if (ret != 2)
> +		return ret < 0 ? ret : -EIO;

Make it use standard pattern:

	ret = i2c_transfer(i2c->adapter, msg, ARRAY_SIZE(msg));
	if (ret < 0)
		return ret;
	if (ret != ARRAY_SIZE(msg))
		return -EIO;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 4/7] iio: dac: ad5686: implement new sync() op for the spi bus

[Andy Shevchenko]

On Fri, Jul 10, 2026 at 12:20:48PM +0100, Rodrigo Alencar via B4 Relay wrote:

> Use of local SPI bus data to manage a collection of SPI transfers and
> flush them to the SPI platform driver with the sync() operation. This
> allows for faster handling of multiple channel DAC writes, avoiding kernel
> overhead per spi_sync() call, which will be helpful when enabling
> triggered buffer support.

...

>  int ad5686_probe(struct device *dev,
>  		 const struct ad5686_chip_info *chip_info,
> -		 const char *name, const struct ad5686_bus_ops *ops)
> +		 const char *name, const struct ad5686_bus_ops *ops,
> +		 void *bus_data)

Can't you utilise the dev->platform_data for this? I believe it's exactly
the case where it suits.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 2/7] iio: dac: ad5686: refactor command/data macros

[Andy Shevchenko]

On Fri, Jul 10, 2026 at 12:20:46PM +0100, Rodrigo Alencar via B4 Relay wrote:

> Replace usage of bit shifting macros for FIELD_PREP(), which would not
> ignore bit masking when preparing SPI/I2C commands. This change is a code
> hardening measure to be paired with the upcoming triggered buffer support.
> For the AD5310 regmap case, 16-bit data coming from the buffer may overlap
> with command bits if the data field is unmasked.
> 
> *_REF_BIT_MSK and *_PD_MSK bit position macros are renamed (with a DATA
> prefix) so to indicate that they are relative to the DATA field.

> Reported-by: sashiko-bot@kernel.org
> Link: https://lore.kernel.org/all/20260628143026.EC6CA1F000E9@smtp.kernel.org/#t

Reported <--> Closes.
Also, '#t' part may be dropped from the URL.
Doesn't checkpatch make a warning on this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 5/7] iio: dac: ad5686: read_raw/write_raw: use guard(mutex)()

[Andy Shevchenko]

On Sat, Jul 11, 2026 at 10:09:51AM -0500, David Lechner wrote:
> On 7/10/26 6:20 AM, Rodrigo Alencar via B4 Relay wrote:

> > Use guarded mutex lock to facilitate code review when adding new
> > attributes. This will allow for early returns, avoiding error-prone
> > locking and unlocking in error paths. This also adds missing include
> > linux/cleanup.h. Gain-control support will allow the scale attribute
> > to be configurable.

...

> > +	guard(mutex)(&st->lock);
> 
> The commit message should explain why the location we are taking the lock
> has moved. Although, I don't see a reason to move it.

As I understand it's a compromise between simplicity and extra expressions that
are appear under the lock. Since it's mutex and not a spinlock, and the code is
not an utterly hotpath (something like very core kernel stuff), I think this
is okay. And IIRC there was even Jonathan's suggestion to go this way (maybe not
in this series, but semantically the same situation).

So, for the sake of simplicity, I am fine with this
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko