Hi Radu,

minor comments on top of what was said already,

On Fri, 2026-03-20 at 13:03 +0200, Radu Sabau via B4 Relay wrote:

...

+
+struct ad4691_state {
+	const struct ad4691_chip_info	*info;
+	struct regmap			*regmap;
+	int				vref_uV;
+	bool				refbuf_en;
+	bool				ldo_en;
+	/*
+	 * Synchronize access to members of the driver state, and ensure
+	 * atomicity of consecutive SPI operations.
+	 */
+	struct mutex			lock;
+};

I would not use tabs and align all the members. Makes thinks harder in the future if
an update is needed. I would just use a simple space.

+
+static int ad4691_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct spi_device *spi = context;
+	u8 tx[2], rx[4];
+	int ret;
+
+	put_unaligned_be16(0x8000 | reg, tx);
+
+	switch (reg) {
+	case 0 ... AD4691_OSC_FREQ_REG:
+	case AD4691_SPARE_CONTROL ... AD4691_ACC_SAT_OVR_REG(15):
+		ret = spi_write_then_read(spi, tx, 2, rx, 1);
+		if (ret)
+			return ret;
+		*val = rx[0];
+		return 0;
+	case AD4691_STD_SEQ_CONFIG:
+	case AD4691_AVG_IN(0) ... AD4691_AVG_IN(15):
+		ret = spi_write_then_read(spi, tx, 2, rx, 2);
+		if (ret)
+			return ret;
+		*val = get_unaligned_be16(rx);
+		return 0;
+	case AD4691_AVG_STS_IN(0) ... AD4691_AVG_STS_IN(15):
+	case AD4691_ACC_IN(0) ... AD4691_ACC_IN(15):
+		ret = spi_write_then_read(spi, tx, 2, rx, 3);
+		if (ret)
+			return ret;
+		*val = get_unaligned_be24(rx);
+		return 0;
+	case AD4691_ACC_STS_DATA(0) ... AD4691_ACC_STS_DATA(15):
+		ret = spi_write_then_read(spi, tx, 2, rx, 4);
+		if (ret)
+			return ret;
+		*val = get_unaligned_be32(rx);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4691_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct spi_device *spi = context;
+	u8 tx[4];
+
+	put_unaligned_be16(reg, tx);
+
+	switch (reg) {
+	case 0 ... AD4691_OSC_FREQ_REG:
+	case AD4691_SPARE_CONTROL ... AD4691_ACC_MASK_REG - 1:
+	case AD4691_ACC_MASK_REG + 1 ... AD4691_GPIO_MODE2_REG:
+		if (val > 0xFF)
+			return -EINVAL;
+		tx[2] = val;
+		return spi_write_then_read(spi, tx, 3, NULL, 0);
+	case AD4691_ACC_MASK_REG:
+	case AD4691_STD_SEQ_CONFIG:
+		if (val > 0xFFFF)
+			return -EINVAL;
+		put_unaligned_be16(val, &tx[2]);
+		return spi_write_then_read(spi, tx, 4, NULL, 0);
+	default:
+		return -EINVAL;
+	}
+}
+
+static bool ad4691_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case AD4691_STATUS_REG:
+	case AD4691_CLAMP_STATUS1_REG:
+	case AD4691_CLAMP_STATUS2_REG:
+	case AD4691_GPIO_READ:
+	case AD4691_ACC_STATUS_FULL1_REG ... AD4691_ACC_STATUS_SAT2_REG:
+	case AD4691_ACC_SAT_OVR_REG(0) ... AD4691_ACC_SAT_OVR_REG(15):
+	case AD4691_AVG_IN(0) ... AD4691_AVG_IN(15):
+	case AD4691_AVG_STS_IN(0) ... AD4691_AVG_STS_IN(15):
+	case AD4691_ACC_IN(0) ... AD4691_ACC_IN(15):
+	case AD4691_ACC_STS_DATA(0) ... AD4691_ACC_STS_DATA(15):
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool ad4691_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case 0 ... AD4691_OSC_FREQ_REG:
+	case AD4691_SPARE_CONTROL ... AD4691_ACC_SAT_OVR_REG(15):
+	case AD4691_STD_SEQ_CONFIG:
+	case AD4691_AVG_IN(0) ... AD4691_AVG_IN(15):
+	case AD4691_AVG_STS_IN(0) ... AD4691_AVG_STS_IN(15):
+	case AD4691_ACC_IN(0) ... AD4691_ACC_IN(15):
+	case AD4691_ACC_STS_DATA(0) ... AD4691_ACC_STS_DATA(15):
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool ad4691_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case 0 ... AD4691_OSC_FREQ_REG:
+	case AD4691_STD_SEQ_CONFIG:
+	case AD4691_SPARE_CONTROL ... AD4691_GPIO_MODE2_REG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config ad4691_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 32,
+	.reg_read = ad4691_reg_read,
+	.reg_write = ad4691_reg_write,
+	.volatile_reg = ad4691_volatile_reg,
+	.readable_reg = ad4691_readable_reg,
+	.writeable_reg = ad4691_writeable_reg,
+	.max_register = AD4691_ACC_STS_DATA(15),
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static int ad4691_get_sampling_freq(struct ad4691_state *st, int *val)
+{
+	unsigned int reg_val;
+	int ret;
+
+	ret = regmap_read(st->regmap, AD4691_OSC_FREQ_REG, &reg_val);
+	if (ret)
+		return ret;
+
+	*val = ad4691_osc_freqs[FIELD_GET(AD4691_OSC_FREQ_MASK, reg_val)];
+	return IIO_VAL_INT;
+}
+
+static int ad4691_set_sampling_freq(struct iio_dev *indio_dev, int freq)
+{
+	struct ad4691_state *st = iio_priv(indio_dev);
+	unsigned int start = (st->info->max_rate == HZ_PER_MHZ) ? 0 : 1;
+	unsigned int i;
+
+	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
+	if (IIO_DEV_ACQUIRE_FAILED(claim))
+		return -EBUSY;
+
+	for (i = start; i < ARRAY_SIZE(ad4691_osc_freqs); i++) {
+		if ((int)ad4691_osc_freqs[i] == freq)

maybe just make the array signed.

+			return regmap_update_bits(st->regmap, AD4691_OSC_FREQ_REG,
+						  AD4691_OSC_FREQ_MASK, i);
+	}
+
+	return -EINVAL;
+}
+
+static int ad4691_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type,
+			     int *length, long mask)
+{
+	struct ad4691_state *st = iio_priv(indio_dev);
+	unsigned int start = (st->info->max_rate == HZ_PER_MHZ) ? 0 : 1;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (const int *)&ad4691_osc_freqs[start];
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(ad4691_osc_freqs) - start;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4691_single_shot_read(struct iio_dev *indio_dev,
+				   struct iio_chan_spec const *chan, int *val)
+{
+	struct ad4691_state *st = iio_priv(indio_dev);
+	unsigned int reg_val;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	/*
+	 * Use AUTONOMOUS mode for single-shot reads. The chip always
+	 * operates in AUTONOMOUS mode in this driver revision.
+	 */
+	ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG,
+			   AD4691_STATE_RESET_ALL);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
+			   BIT(chan->channel));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
+			   (u16)~BIT(chan->channel));
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, AD4691_OSC_FREQ_REG, &reg_val);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4691_OSC_EN_REG, 1);
+	if (ret)
+		return ret;
+
+	/*
+	 * Wait for at least 2 internal oscillator periods for the
+	 * conversion to complete.
+	 */
+	fsleep(DIV_ROUND_UP(2 * USEC_PER_SEC,
+			    ad4691_osc_freqs[FIELD_GET(AD4691_OSC_FREQ_MASK,
+						       reg_val)]));
+
+	ret = regmap_write(st->regmap, AD4691_OSC_EN_REG, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, AD4691_AVG_IN(chan->channel), &reg_val);
+	if (ret)
+		return ret;
+
+	*val = reg_val;
+
+	ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG, AD4691_STATE_RESET_ALL);
+	if (ret)
+		return ret;
+
+	return IIO_VAL_INT;
+}
+
+static int ad4691_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long info)
+{
+	struct ad4691_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW: {
+		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
+

nit: I would drop the extra new line. The check below is related to the
above.

+		if (IIO_DEV_ACQUIRE_FAILED(claim))
+			return -EBUSY;
+
+		return ad4691_single_shot_read(indio_dev, chan, val);
+	}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return ad4691_get_sampling_freq(st, val);
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->vref_uV / (MICRO / MILLI);
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4691_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return ad4691_set_sampling_freq(indio_dev, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4691_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			     unsigned int writeval, unsigned int *readval)
+{
+	struct ad4691_state *st = iio_priv(indio_dev);
+
+	guard(mutex)(&st->lock);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
+}
+
+static const struct iio_info ad4691_info = {
+	.read_raw = &ad4691_read_raw,
+	.write_raw = &ad4691_write_raw,
+	.read_avail = &ad4691_read_avail,
+	.debugfs_reg_access = &ad4691_reg_access,
+};
+
+static int ad4691_regulator_setup(struct ad4691_state *st)
+{
+	struct device *dev = regmap_get_device(st->regmap);
+	int ret;
+
+	ret = devm_regulator_get_enable(dev, "avdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get and enable AVDD\n");
+
+	ret = devm_regulator_get_enable(dev, "ldo-in");
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "Failed to get and enable LDO-IN\n");
+	st->ldo_en = (ret == -ENODEV);
+
+	ret = devm_regulator_get_enable(dev, "vio");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get and enable VIO\n");
+
+	st->vref_uV = devm_regulator_get_enable_read_voltage(dev, "ref");
+	if (st->vref_uV >= 0) {
+		st->refbuf_en = false;
+	} else if (st->vref_uV == -ENODEV) {
+		st->vref_uV = devm_regulator_get_enable_read_voltage(dev, "refin");
+		st->refbuf_en = true;
+	}
+	if (st->vref_uV < 0)
+		return dev_err_probe(dev, st->vref_uV,
+				     "Failed to get reference supply\n");
+
+	if (st->vref_uV < AD4691_VREF_uV_MIN || st->vref_uV > AD4691_VREF_uV_MAX)
+		return dev_err_probe(dev, -EINVAL,
+				     "vref(%d) must be in the range [%u...%u]\n",
+				     st->vref_uV, AD4691_VREF_uV_MIN,
+				     AD4691_VREF_uV_MAX);
+
+	return 0;
+}
+
+static int ad4691_reset(struct ad4691_state *st)
+{
+	struct device *dev = regmap_get_device(st->regmap);
+	struct reset_control *rst;
+
+	rst = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(rst))
+		return dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset\n");
+
+	if (!rst)
+		/* No hardware reset available, fall back to software reset. */
+		return regmap_write(st->regmap, AD4691_SPI_CONFIG_A_REG,
+				    AD4691_SW_RESET);
+
+	reset_control_assert(rst);

Can't we ask for the reset in the asserted state already?

+	/* Reset delay required. See datasheet Table 5. */
+	fsleep(300);
+	reset_control_deassert(rst);
+
+	return 0;
+}
+
+static int ad4691_config(struct ad4691_state *st)
+{
+	struct device *dev = regmap_get_device(st->regmap);
+	enum ad4691_ref_ctrl ref_val;
+	int ret;
+
+	switch (st->vref_uV) {
+	case AD4691_VREF_uV_MIN ... AD4691_VREF_2P5_uV_MAX:
+		ref_val = AD4691_VREF_2P5;
+		break;
+	case AD4691_VREF_2P5_uV_MAX + 1 ... AD4691_VREF_3P0_uV_MAX:
+		ref_val = AD4691_VREF_3P0;
+		break;
+	case AD4691_VREF_3P0_uV_MAX + 1 ... AD4691_VREF_3P3_uV_MAX:
+		ref_val = AD4691_VREF_3P3;
+		break;
+	case AD4691_VREF_3P3_uV_MAX + 1 ... AD4691_VREF_4P096_uV_MAX:
+		ref_val = AD4691_VREF_4P096;
+		break;
+	case AD4691_VREF_4P096_uV_MAX + 1 ... AD4691_VREF_uV_MAX:
+		ref_val = AD4691_VREF_5P0;
+		break;
+	default:
+		return dev_err_probe(dev, -EINVAL,
+				     "Unsupported vref voltage: %d uV\n",
+				     st->vref_uV);
+	}
+
+	ret = regmap_write(st->regmap, AD4691_REF_CTRL,
+			   FIELD_PREP(AD4691_REF_CTRL_MASK, ref_val) |
+			   (st->refbuf_en ? AD4691_REFBUF_EN : 0));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to write REF_CTRL\n");
+
+	ret = regmap_write(st->regmap, AD4691_DEVICE_SETUP,
+			   st->ldo_en ? AD4691_LDO_EN : 0);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to write DEVICE_SETUP\n");
+
+	/*
+	 * Set the internal oscillator to the highest valid rate for this chip.
+	 * Index 0 (1 MHz) is valid only for AD4692/AD4694; AD4691/AD4693 start
+	 * at index 1 (500 kHz).
+	 */
+	ret = regmap_write(st->regmap, AD4691_OSC_FREQ_REG,
+			   (st->info->max_rate == HZ_PER_MHZ) ? 0 : 1);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to write OSC_FREQ\n");
+
+	/* Device always operates in AUTONOMOUS mode. */

Kind of obvious from the code. Drop the comment please.


- Nuno Sá