[v1] Add support for AD4062 device family

[PATCH 3/7] iio: adc: Add support for ad4062

Posted by Jorge Marques 2 months, 1 week ago

The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
register (SAR) analog-to-digital converter (ADC) that enables low-power,
high-density data acquisition solutions without sacrificing precision.
This ADC offers a unique balance of performance and power efficiency,
plus innovative features for seamlessly switching between
high-resolution and low-power modes tailored to the immediate needs of
the system. The AD4060/AD4062 are ideal for battery-powered, compact
data acquisition and edge sensing applications.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 MAINTAINERS              |   1 +
 drivers/iio/adc/Kconfig  |  11 +
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad4062.c | 905 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 918 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ce012c6c719023d3c0355676a335a55d92cf424c..ab4c95e331014c18895dc13699d616a91a080f8e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1406,6 +1406,7 @@ S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml
 F:	Documentation/iio/ad4062.rst
+F:	drivers/iio/adc/ad4062.c
 
 ANALOG DEVICES INC AD4080 DRIVER
 M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 58a14e6833f60008511838176b8b3ec0789dc95b..490c01d701bdd1543809cdefad4ed5573c051c24 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -70,6 +70,17 @@ config AD4030
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad4030.
 
+config AD4062
+	tristate "Analog Devices AD4062 Driver"
+	depends on I3C
+	select REGMAP_I3C
+	help
+	  Say yes here to build support for Analog Devices AD4062 I3C analog
+	  to digital converters (ADC).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad4062.
+
 config AD4080
 	tristate "Analog Devices AD4080 high speed ADC"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d008f78dc010aff5f76f838283be294e0ebc27dd..ed71b2549c647b9d59d999e107ca1ee256d8ec04 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4000) += ad4000.o
 obj-$(CONFIG_AD4030) += ad4030.o
+obj-$(CONFIG_AD4062) += ad4062.o
 obj-$(CONFIG_AD4080) += ad4080.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD4170_4) += ad4170-4.o
diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
new file mode 100644
index 0000000000000000000000000000000000000000..e55a69c62694a71a4e29f29b9a2bfeec3b16c990
--- /dev/null
+++ b/drivers/iio/adc/ad4062.c
@@ -0,0 +1,905 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Analog Devices AD4062 I3C ADC driver
+ *
+ * Copyright 2025 Analog Devices Inc.
+ */
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i3c/device.h>
+#include <linux/i3c/master.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/units.h>
+#include <linux/unaligned.h>
+
+#define AD4062_REG_INTERFACE_CONFIG_A			0x00
+#define AD4062_REG_DEVICE_CONFIG			0x02
+#define     AD4062_REG_DEVICE_CONFIG_POWER_MODE_MSK	GENMASK(1, 0)
+#define     AD4062_REG_DEVICE_CONFIG_LOW_POWER_MODE	3
+#define AD4062_REG_PROD_ID_1				0x05
+#define AD4062_REG_DEVICE_GRADE				0x06
+#define AD4062_REG_SCRATCH_PAD				0x0A
+#define AD4062_REG_VENDOR_H				0x0D
+#define AD4062_REG_STREAM_MODE				0x0E
+#define AD4062_REG_INTERFACE_STATUS			0x11
+#define     AD4062_REG_INTERFACE_STATUS_NOT_RDY		BIT(7)
+#define AD4062_REG_MODE_SET				0x20
+#define     AD4062_REG_MODE_SET_ENTER_ADC		BIT(0)
+#define AD4062_REG_ADC_MODES				0x21
+#define     AD4062_REG_ADC_MODES_MODE_MSK		GENMASK(1, 0)
+#define     AD4062_REG_ADC_MODES_DATA_FORMAT		BIT(7)
+#define AD4062_REG_ADC_CONFIG				0x22
+#define     AD4062_REG_ADC_CONFIG_REF_EN_MSK		BIT(5)
+#define     AD4062_REG_ADC_CONFIG_SCALE_EN_MSK		BIT(4)
+#define AD4062_REG_AVG_CONFIG				0x23
+#define AD4062_REG_GP_CONF				0x24
+#define     AD4062_REG_GP_CONF_MODE_MSK_0		GENMASK(2, 0)
+#define     AD4062_REG_GP_CONF_MODE_MSK_1		GENMASK(6, 4)
+#define AD4062_REG_INTR_CONF				0x25
+#define     AD4062_REG_INTR_CONF_EN_MSK_0		GENMASK(1, 0)
+#define     AD4062_REG_INTR_CONF_EN_MSK_1		GENMASK(5, 4)
+#define AD4062_REG_TIMER_CONFIG				0x27
+#define     AD4062_REG_TIMER_CONFIG_FS_MASK		GENMASK(7, 4)
+#define     AD4062_REG_TIMER_CONFIG_300KSPS		0x2
+#define AD4062_REG_MAX_LIMIT				0x29
+#define AD4062_REG_MIN_LIMIT				0x2B
+#define AD4062_REG_MAX_HYST				0x2C
+#define AD4062_REG_MIN_HYST				0x2D
+#define AD4062_REG_MON_VAL				0x2F
+#define AD4062_REG_ADC_IBI_EN				0x31
+#define AD4062_REG_ADC_IBI_EN_CONV_TRIGGER		BIT(2)
+#define AD4062_REG_ADC_IBI_EN_MAX			BIT(1)
+#define AD4062_REG_ADC_IBI_EN_MIN			BIT(0)
+#define AD4062_REG_FUSE_CRC				0x40
+#define AD4062_REG_DEVICE_STATUS			0x41
+#define     AD4062_REG_DEVICE_STATUS_DEVICE_RDY		BIT(7)
+#define     AD4062_REG_DEVICE_STATUS_DEVICE_RESET	BIT(6)
+#define AD4062_REG_MIN_SAMPLE				0x45
+#define AD4062_REG_IBI_STATUS				0x48
+#define AD4062_REG_CONV_READ_LSB			0x50
+#define AD4062_REG_CONV_READ				0x53
+#define AD4062_REG_CONV_TRIGGER				0x59
+#define AD4062_REG_CONV_AUTO				0x61
+#define AD4062_MAX_REG					0x61
+
+#define AD4062_I3C_VENDOR	0x0177
+
+#define AD4050_MAX_AVG		0x7
+#define AD4062_MAX_AVG		0xB
+#define AD4062_MAX_RATE(x)	((x) == AD4062_2MSPS ? 2000000 : 500000)
+#define AD4062_FS_OFFSET(g)	((g) == AD4062_2MSPS ? 0 : 2)
+#define AD4062_FS(g)		(&ad4062_conversion_freqs[AD4062_FS_OFFSET(g)])
+#define AD4062_FS_LEN(g)	(ARRAY_SIZE(ad4062_conversion_freqs) - (AD4062_FS_OFFSET(g)))
+#define AD4062_MON_VAL_MAX_GAIN		1999970
+#define AD4062_MON_VAL_MIDDLE_POINT	0x8000
+#define AD4062_T_CNVH_NS	10
+#define AD4062_VIO_3V3		3300000
+#define AD4062_SPI_MAX_ADC_XFER_SPEED(x)	((x) >= AD4062_VIO_3V3 ? 83333333 : 58823529)
+#define AD4062_SPI_MAX_REG_XFER_SPEED		16000000
+
+enum ad4062_grade {
+	AD4062_2MSPS,
+};
+
+enum ad4062_operation_mode {
+	AD4062_SAMPLE_MODE = 0,
+	AD4062_BURST_AVERAGING_MODE = 1,
+	AD4062_MONITOR_MODE = 3,
+};
+
+enum ad4062_gp_mode {
+	AD4062_GP_DISABLED,
+	AD4062_GP_INTR,
+	AD4062_GP_DRDY,
+};
+
+enum ad4062_interrupt_en {
+	AD4062_INTR_EN_NEITHER,
+	AD4062_INTR_EN_MIN,
+	AD4062_INTR_EN_MAX,
+	AD4062_INTR_EN_EITHER,
+};
+
+struct ad4062_chip_info {
+	const struct iio_chan_spec channels[1];
+	const char *name;
+	u16 prod_id;
+	u8 max_avg;
+	u8 grade;
+};
+
+enum {
+	AD4062_SCAN_TYPE_SAMPLE,
+	AD4062_SCAN_TYPE_BURST_AVG,
+};
+
+static const struct iio_scan_type ad4062_scan_type_12_s[] = {
+	[AD4062_SCAN_TYPE_SAMPLE] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+	[AD4062_SCAN_TYPE_BURST_AVG] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+};
+
+static const struct iio_scan_type ad4062_scan_type_16_s[] = {
+	[AD4062_SCAN_TYPE_SAMPLE] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+	[AD4062_SCAN_TYPE_BURST_AVG] = {
+		.sign = 's',
+		.realbits = 24,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+};
+
+struct ad4062_state {
+	const struct ad4062_chip_info *chip;
+	const struct ad4062_bus_ops *ops;
+	enum ad4062_operation_mode mode;
+	struct completion completion;
+	struct iio_trigger *trigger;
+	struct iio_dev *indio_dev;
+	struct i3c_device *i3cdev;
+	struct regmap *regmap;
+	u16 sampling_frequency;
+	int vref_uv;
+	u8 raw[4] __aligned(IIO_DMA_MINALIGN);
+};
+
+static const struct regmap_range ad4062_regmap_rd_ranges[] = {
+	regmap_reg_range(AD4062_REG_INTERFACE_CONFIG_A, AD4062_REG_DEVICE_GRADE),
+	regmap_reg_range(AD4062_REG_SCRATCH_PAD, AD4062_REG_INTERFACE_STATUS),
+	regmap_reg_range(AD4062_REG_MODE_SET, AD4062_REG_ADC_IBI_EN),
+	regmap_reg_range(AD4062_REG_FUSE_CRC, AD4062_REG_IBI_STATUS),
+	regmap_reg_range(AD4062_REG_CONV_READ_LSB, AD4062_REG_CONV_AUTO),
+};
+
+static const struct regmap_access_table ad4062_regmap_rd_table = {
+	.yes_ranges = ad4062_regmap_rd_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad4062_regmap_rd_ranges),
+};
+
+static const struct regmap_range ad4062_regmap_wr_ranges[] = {
+	regmap_reg_range(AD4062_REG_INTERFACE_CONFIG_A, AD4062_REG_DEVICE_CONFIG),
+	regmap_reg_range(AD4062_REG_SCRATCH_PAD, AD4062_REG_SCRATCH_PAD),
+	regmap_reg_range(AD4062_REG_STREAM_MODE, AD4062_REG_INTERFACE_STATUS),
+	regmap_reg_range(AD4062_REG_MODE_SET, AD4062_REG_ADC_IBI_EN),
+	regmap_reg_range(AD4062_REG_FUSE_CRC, AD4062_REG_DEVICE_STATUS),
+};
+
+static const struct regmap_access_table ad4062_regmap_wr_table = {
+	.yes_ranges = ad4062_regmap_wr_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad4062_regmap_wr_ranges),
+};
+
+static const char *const ad4062_conversion_freqs[] = {
+	"2000000", "1000000", "300000", "100000",	/*  0 -  3 */
+	"33300", "10000", "3000", "500",		/*  4 -  7 */
+	"333", "250", "200", "166",			/*  8 - 11 */
+	"140", "124", "111",				/* 12 - 15 */
+};
+
+static int ad4062_conversion_frequency_set(struct ad4062_state *st, u8 val)
+{
+	val += AD4062_FS_OFFSET(st->chip->grade);
+	return regmap_write(st->regmap, AD4062_REG_TIMER_CONFIG,
+			    FIELD_PREP(AD4062_REG_TIMER_CONFIG_FS_MASK, val));
+}
+
+static int ad4062_sampling_frequency_get(struct iio_dev *indio_dev,
+					     const struct iio_chan_spec *chan)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+
+	return st->sampling_frequency - AD4062_FS_OFFSET(st->chip->grade);
+}
+
+static int ad4062_sampling_frequency_set(struct iio_dev *indio_dev,
+					     const struct iio_chan_spec *chan,
+					     unsigned int val)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+
+	val += AD4062_FS_OFFSET(st->chip->grade);
+	st->sampling_frequency = val;
+
+	return 0;
+}
+
+static const struct iio_enum AD4062_2MSPS_conversion_freq_enum = {
+	.items = AD4062_FS(AD4062_2MSPS),
+	.num_items = AD4062_FS_LEN(AD4062_2MSPS),
+	.set = ad4062_sampling_frequency_set,
+	.get = ad4062_sampling_frequency_get,
+};
+
+#define AD4062_EXT_INFO(grade)						\
+static struct iio_chan_spec_ext_info grade##_ext_info[] = {		\
+	IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL,		\
+		 &grade##_conversion_freq_enum),			\
+	IIO_ENUM_AVAILABLE("sampling_frequency", IIO_SHARED_BY_ALL,	\
+			   &grade##_conversion_freq_enum),		\
+	{ }								\
+}
+
+AD4062_EXT_INFO(AD4062_2MSPS);
+
+#define AD4062_CHAN(bits, grade) {							\
+	.type = IIO_VOLTAGE,								\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) |				\
+				    BIT(IIO_CHAN_INFO_SCALE) |				\
+				    BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
+				    BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),		\
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
+	.indexed = 1,									\
+	.channel = 0,									\
+	.has_ext_scan_type = 1,								\
+	.ext_scan_type = ad4062_scan_type_##bits##_s,					\
+	.num_ext_scan_type = ARRAY_SIZE(ad4062_scan_type_##bits##_s),			\
+	.ext_info = grade##_ext_info,							\
+}
+
+static const struct ad4062_chip_info ad4060_chip_info = {
+	.name = "ad4060",
+	.channels = { AD4062_CHAN(12, AD4062_2MSPS) },
+	.prod_id = 0x7A,
+	.max_avg = AD4050_MAX_AVG,
+	.grade = AD4062_2MSPS,
+};
+
+static const struct ad4062_chip_info ad4062_chip_info = {
+	.name = "ad4062",
+	.channels = { AD4062_CHAN(16, AD4062_2MSPS) },
+	.prod_id = 0x7C,
+	.max_avg = AD4062_MAX_AVG,
+	.grade = AD4062_2MSPS,
+};
+
+static int ad4062_set_oversampling_ratio(struct iio_dev *indio_dev,
+					 const struct iio_chan_spec *chan,
+					 unsigned int val)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (val < 1 || val > BIT(st->chip->max_avg + 1))
+		return -EINVAL;
+
+	/* 1 disables oversampling */
+	if (val == 1) {
+		st->mode = AD4062_SAMPLE_MODE;
+	} else {
+		val = ilog2(val);
+		st->mode = AD4062_BURST_AVERAGING_MODE;
+		ret = regmap_write(st->regmap, AD4062_REG_AVG_CONFIG, val - 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ad4062_get_oversampling_ratio(struct ad4062_state *st,
+					 unsigned int *val)
+{
+	int ret, buf;
+
+	if (st->mode == AD4062_SAMPLE_MODE) {
+		*val = 1;
+		return 0;
+	}
+
+	ret = regmap_read(st->regmap, AD4062_REG_AVG_CONFIG, &buf);
+	if (ret)
+		return ret;
+
+	*val = BIT(buf + 1);
+
+	return 0;
+}
+
+static int ad4062_check_ids(struct ad4062_state *st)
+{
+	int ret;
+	u16 val;
+
+	ret = regmap_bulk_read(st->regmap, AD4062_REG_PROD_ID_1, &st->raw, 2);
+	if (ret)
+		return ret;
+
+	val = get_unaligned_be16(st->raw);
+	if (val != st->chip->prod_id)
+		dev_warn(&st->i3cdev->dev,
+			 "Production ID x%x does not match known values", val);
+
+	ret = regmap_bulk_read(st->regmap, AD4062_REG_VENDOR_H, &st->raw, 2);
+	if (ret)
+		return ret;
+
+	val = get_unaligned_be16(st->raw);
+	if (val != AD4062_I3C_VENDOR) {
+		dev_err(&st->i3cdev->dev,
+			"Vendor ID x%x does not match expected value\n", val);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int ad4062_set_operation_mode(struct ad4062_state *st,
+				     enum ad4062_operation_mode mode)
+{
+	int ret;
+
+	if (mode == AD4062_BURST_AVERAGING_MODE) {
+		ret = ad4062_conversion_frequency_set(st, st->sampling_frequency);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_MODES,
+				 AD4062_REG_ADC_MODES_MODE_MSK, mode);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD4062_REG_MODE_SET,
+			    AD4062_REG_MODE_SET_ENTER_ADC);
+}
+
+static int ad4062_soft_reset(struct ad4062_state *st)
+{
+	u8 val = 0x81;
+	int ret;
+
+	ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
+	if (ret)
+		return ret;
+
+	/* Wait AD4062 treset time */
+	fsleep(5000);
+
+	return 0;
+}
+
+static int ad4062_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			const bool *ref_sel)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
+	int ret;
+
+	scan_type = iio_get_current_scan_type(indio_dev, chan);
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
+	u8 val = FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, AD4062_GP_INTR) |
+		 FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, AD4062_GP_DRDY);
+
+	ret = regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
+				 AD4062_REG_GP_CONF_MODE_MSK_1 | AD4062_REG_GP_CONF_MODE_MSK_0,
+				 val);
+	if (ret)
+		return ret;
+
+	val = FIELD_PREP(AD4062_REG_INTR_CONF_EN_MSK_0, (AD4062_INTR_EN_EITHER)) |
+	      FIELD_PREP(AD4062_REG_INTR_CONF_EN_MSK_1, (AD4062_INTR_EN_NEITHER));
+
+	ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_MODES,
+				 AD4062_REG_ADC_CONFIG_REF_EN_MSK,
+				 FIELD_PREP(AD4062_REG_ADC_CONFIG_REF_EN_MSK,
+					    *ref_sel));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4062_REG_DEVICE_STATUS,
+			   AD4062_REG_DEVICE_STATUS_DEVICE_RESET);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(st->regmap, AD4062_REG_INTR_CONF,
+				  AD4062_REG_INTR_CONF_EN_MSK_0 | AD4062_REG_INTR_CONF_EN_MSK_1,
+				  val);
+}
+
+static irqreturn_t ad4062_irq_handler_drdy(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct ad4062_state *st = iio_priv(indio_dev);
+
+	complete(&st->completion);
+
+	return IRQ_HANDLED;
+}
+
+static void ad4062_ibi_handler(struct i3c_device *i3cdev,
+			       const struct i3c_ibi_payload *payload)
+{
+	struct ad4062_state *st = i3cdev_get_drvdata(i3cdev);
+
+	complete(&st->completion);
+}
+
+static int ad4062_request_ibi(struct i3c_device *i3cdev)
+{
+	const struct i3c_ibi_setup ibireq = {
+		.max_payload_len = 1,
+		.num_slots = 1,
+		.handler = ad4062_ibi_handler,
+	};
+	int ret;
+
+	ret = i3c_device_request_ibi(i3cdev, &ibireq);
+	if (ret)
+		return ret;
+
+	ret = i3c_device_enable_ibi(i3cdev);
+	if (ret)
+		goto err_enable_ibi;
+	return 0;
+
+err_enable_ibi:
+	i3c_device_free_ibi(i3cdev);
+	return ret;
+}
+
+static int ad4062_request_irq(struct iio_dev *indio_dev)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->i3cdev->dev;
+	int ret;
+
+	ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
+	if (ret >= 0) {
+		ret = devm_request_threaded_irq(dev, ret, NULL,
+						 ad4062_irq_handler_drdy,
+						 IRQF_ONESHOT, indio_dev->name,
+						 indio_dev);
+	} else if (ret != -EPROBE_DEFER) {
+		ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
+					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER,
+					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER);
+	}
+
+	return ret;
+}
+
+static const int ad4062_oversampling_avail[] = {
+	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
+};
+
+static int ad4062_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, const int **vals,
+			     int *type, int *len, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = ad4062_oversampling_avail;
+		*len = ARRAY_SIZE(ad4062_oversampling_avail);
+		*type = IIO_VAL_INT;
+
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4062_get_chan_scale(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 int *val, int *val2)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
+
+	scan_type = iio_get_current_scan_type(indio_dev, st->chip->channels);
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
+	*val = (st->vref_uv * 2) / MILLI;
+
+	*val2 = scan_type->realbits;
+
+	return IIO_VAL_FRACTIONAL_LOG2;
+}
+
+static int ad4062_get_chan_calibscale(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      int *val, int *val2)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	u16 gain;
+	int ret;
+
+	ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL, &st->raw, 2);
+	if (ret)
+		return ret;
+
+	gain = get_unaligned_be16(&st->raw);
+
+	/* From datasheet: code out = code in × mon_val/0x8000 */
+	*val = gain / AD4062_MON_VAL_MIDDLE_POINT;
+	*val2 = mul_u64_u32_div(gain % AD4062_MON_VAL_MIDDLE_POINT, NANO,
+				AD4062_MON_VAL_MIDDLE_POINT);
+
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static int ad4062_set_chan_calibscale(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      int gain_int, int gain_frac)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	u64 gain;
+	int ret;
+
+	if (gain_int < 0 || gain_frac < 0)
+		return -EINVAL;
+
+	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
+
+	if (gain > AD4062_MON_VAL_MAX_GAIN)
+		return -EINVAL;
+
+	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
+						 MICRO),
+			   &st->raw);
+
+	ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL, &st->raw, 2);
+	if (ret)
+		return ret;
+
+	/* Enable scale if gain is not one. */
+	return regmap_update_bits(st->regmap, AD4062_REG_ADC_MODES,
+				  AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
+				  FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
+					     !(gain_int == 1 && gain_frac == 0)));
+}
+
+static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
+{
+	struct i3c_device *i3cdev = st->i3cdev;
+	u8 addr = AD4062_REG_CONV_TRIGGER;
+	struct i3c_priv_xfer t[2] = {
+		{
+			.data.out = &addr,
+			.len = 1,
+			.rnw = false,
+		},
+		{
+			.data.in = &st->raw,
+			.len = 4,
+			.rnw = true,
+		}
+	};
+	int ret;
+
+	reinit_completion(&st->completion);
+	/* Change address pointer to trigger conversion */
+	ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
+	if (ret)
+		return ret;
+	/*
+	 * Single sample read should be used only for oversampling and
+	 * sampling frequency pairs that take less than 1 sec.
+	 */
+	ret = wait_for_completion_timeout(&st->completion,
+					  msecs_to_jiffies(1000));
+	if (!ret)
+		return -ETIMEDOUT;
+
+	ret = i3c_device_do_priv_xfers(i3cdev, &t[1], 1);
+	if (ret)
+		return ret;
+	*val = get_unaligned_be32(st->raw);
+	return ret;
+}
+
+static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
+	if (ret)
+		return ret;
+
+	ret = ad4062_set_operation_mode(st, st->mode);
+	if (ret)
+		goto out_error;
+
+	ret = __ad4062_read_chan_raw(st, val);
+
+out_error:
+	pm_runtime_put_autosuspend(&st->i3cdev->dev);
+	return ret;
+}
+
+static int ad4062_read_raw_dispatch(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan, int *val,
+				    int *val2, long info)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		return ad4062_read_chan_raw(indio_dev, val);
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return ad4062_get_chan_calibscale(indio_dev, chan, val, val2);
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return ad4062_get_oversampling_ratio(st, val);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4062_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long info)
+{
+	int ret;
+
+	if (info == IIO_CHAN_INFO_SCALE)
+		return ad4062_get_chan_scale(indio_dev, chan, val, val2);
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = ad4062_read_raw_dispatch(indio_dev, chan, val, val2, info);
+
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : IIO_VAL_INT;
+}
+
+static int ad4062_write_raw_dispatch(struct iio_dev *indio_dev,
+				     struct iio_chan_spec const *chan, int val,
+				     int val2, long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return ad4062_set_oversampling_ratio(indio_dev, chan, val);
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return ad4062_set_chan_calibscale(indio_dev, chan, val, val2);
+
+	default:
+		return -EINVAL;
+	}
+};
+
+static int ad4062_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val,
+			    int val2, long info)
+{
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = ad4062_write_raw_dispatch(indio_dev, chan, val, val2, info);
+
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+				     unsigned int writeval, unsigned int *readval)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (readval)
+		ret = regmap_read(st->regmap, reg, readval);
+	else
+		ret = regmap_write(st->regmap, reg, writeval);
+
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int ad4062_get_current_scan_type(const struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+
+	return st->mode == AD4062_BURST_AVERAGING_MODE ?
+			   AD4062_SCAN_TYPE_BURST_AVG :
+			   AD4062_SCAN_TYPE_SAMPLE;
+}
+
+static const struct iio_info ad4062_info = {
+	.read_raw = ad4062_read_raw,
+	.write_raw = ad4062_write_raw,
+	.read_avail = ad4062_read_avail,
+	.get_current_scan_type = &ad4062_get_current_scan_type,
+	.debugfs_reg_access = &ad4062_debugfs_reg_access,
+};
+
+static const struct regmap_config ad4062_regmap_config = {
+	.name = "ad4062",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AD4062_MAX_REG,
+	.rd_table = &ad4062_regmap_rd_table,
+	.wr_table = &ad4062_regmap_wr_table,
+	.can_sleep = true,
+};
+
+static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
+{
+	struct device *dev = &st->i3cdev->dev;
+	int uv;
+
+	uv = devm_regulator_get_enable_read_voltage(dev, "vio");
+	if (uv < 0)
+		return dev_err_probe(dev, uv,
+				     "Failed to enable and read vio voltage\n");
+
+	uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
+	if (uv < 0)
+		return dev_err_probe(dev, uv,
+				     "Failed to enable vdd regulator\n");
+
+	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
+	*ref_sel = st->vref_uv == -ENODEV;
+	if (st->vref_uv == -ENODEV)
+		st->vref_uv = uv;
+	else if (st->vref_uv < 0)
+		return dev_err_probe(dev, st->vref_uv,
+				     "Failed to enable and read ref voltage\n");
+	return 0;
+}
+
+static const struct i3c_device_id ad4062_id_table[] = {
+	I3C_DEVICE(AD4062_I3C_VENDOR, ad4060_chip_info.prod_id, &ad4060_chip_info),
+	I3C_DEVICE(AD4062_I3C_VENDOR, ad4062_chip_info.prod_id, &ad4062_chip_info),
+	{}
+};
+MODULE_DEVICE_TABLE(i3c, ad4062_id_table);
+
+static int ad4062_probe(struct i3c_device *i3cdev)
+{
+	const struct i3c_device_id *id = i3c_device_match_id(i3cdev, ad4062_id_table);
+	const struct ad4062_chip_info *chip = id->data;
+	struct device *dev = &i3cdev->dev;
+	struct iio_dev *indio_dev;
+	struct ad4062_state *st;
+	bool ref_sel;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->i3cdev = i3cdev;
+	i3cdev_set_drvdata(i3cdev, st);
+	init_completion(&st->completion);
+
+	ret = ad4062_regulators_get(st, &ref_sel);
+	if (ret)
+		return ret;
+
+	st->regmap = devm_regmap_init_i3c(i3cdev, &ad4062_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap),
+				     "Failed to initialize regmap\n");
+
+	st->mode = AD4062_SAMPLE_MODE;
+	st->chip = chip;
+	st->sampling_frequency = AD4062_FS_OFFSET(st->chip->grade);
+	st->indio_dev = indio_dev;
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = 1;
+	indio_dev->info = &ad4062_info;
+	indio_dev->name = chip->name;
+	indio_dev->channels = chip->channels;
+
+	ret = ad4062_soft_reset(st);
+	if (ret)
+		return dev_err_probe(dev, ret, "AD4062 failed to soft reset\n");
+
+	ret = ad4062_check_ids(st);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "AD4062 fields assertions failed\n");
+
+	ret = ad4062_setup(indio_dev, indio_dev->channels, &ref_sel);
+	if (ret)
+		return ret;
+
+	ret = ad4062_request_irq(indio_dev);
+	if (ret)
+		return ret;
+
+	pm_runtime_set_active(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable pm_runtime\n");
+
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+
+	ret = ad4062_request_ibi(i3cdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request i3c ibi\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static void ad4062_remove(struct i3c_device *i3cdev)
+{
+	i3c_device_disable_ibi(i3cdev);
+	i3c_device_free_ibi(i3cdev);
+}
+
+static int ad4062_runtime_suspend(struct device *dev)
+{
+	struct ad4062_state *st = dev_get_drvdata(dev);
+
+	return regmap_write(st->regmap, AD4062_REG_DEVICE_CONFIG,
+			    FIELD_PREP(AD4062_REG_DEVICE_CONFIG_POWER_MODE_MSK,
+				       AD4062_REG_DEVICE_CONFIG_LOW_POWER_MODE));
+}
+
+static int ad4062_runtime_resume(struct device *dev)
+{
+	struct ad4062_state *st = dev_get_drvdata(dev);
+	int ret;
+
+	ret = regmap_clear_bits(st->regmap, AD4062_REG_DEVICE_CONFIG,
+				AD4062_REG_DEVICE_CONFIG_POWER_MODE_MSK);
+
+	fsleep(4000);
+	return ret;
+}
+
+static const struct dev_pm_ops ad4062_pm_ops = {
+	SET_RUNTIME_PM_OPS(ad4062_runtime_suspend, ad4062_runtime_resume, NULL)
+};
+
+static struct i3c_driver ad4062_driver = {
+	.driver = {
+		.name = "ad4062",
+		.pm = pm_ptr(&ad4062_pm_ops),
+	},
+	.probe = ad4062_probe,
+	.remove = ad4062_remove,
+	.id_table = ad4062_id_table,
+};
+module_i3c_driver(ad4062_driver);
+
+MODULE_AUTHOR("Jorge Marques <jorge.marques@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD4062");
+MODULE_LICENSE("GPL");

-- 
2.49.0

Re: [PATCH 3/7] iio: adc: Add support for ad4062

Posted by Jonathan Cameron 2 months ago

On Mon, 13 Oct 2025 09:28:01 +0200
Jorge Marques <jorge.marques@analog.com> wrote:

> The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> register (SAR) analog-to-digital converter (ADC) that enables low-power,
> high-density data acquisition solutions without sacrificing precision.
> This ADC offers a unique balance of performance and power efficiency,
> plus innovative features for seamlessly switching between
> high-resolution and low-power modes tailored to the immediate needs of
> the system. The AD4060/AD4062 are ideal for battery-powered, compact
> data acquisition and edge sensing applications.

Little bit marketing heavy for a patch description.. I'd stick to
type, resolution and typical usecases. Skip the sacrificing precision etc
wording!  It'll just make the readers eyes glaze over ;)

The mixing of regmap and direct accesses is a little unfortunate but seems unavoidable.


> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>

Various comments inline.  In general looks good for a v1.


> diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..e55a69c62694a71a4e29f29b9a2bfeec3b16c990
> --- /dev/null
> +++ b/drivers/iio/adc/ad4062.c
> @@ -0,0 +1,905 @@

> +#define     AD4062_REG_DEVICE_STATUS_DEVICE_RESET	BIT(6)
> +#define AD4062_REG_MIN_SAMPLE				0x45
> +#define AD4062_REG_IBI_STATUS				0x48
> +#define AD4062_REG_CONV_READ_LSB			0x50
> +#define AD4062_REG_CONV_READ				0x53
> +#define AD4062_REG_CONV_TRIGGER				0x59
> +#define AD4062_REG_CONV_AUTO				0x61
> +#define AD4062_MAX_REG					0x61

I'd define this in terms of the reg that it happens to be
#define AD4052_MAX_REG AD6062_REG_CONF_AUTO


> +#define AD4062_VIO_3V3		3300000

This is a real value rather than a magic number so better to not hide it in a define.
Just use the value inline.


> +#define AD4062_SPI_MAX_ADC_XFER_SPEED(x)	((x) >= AD4062_VIO_3V3 ? 83333333 : 58823529)
> +#define AD4062_SPI_MAX_REG_XFER_SPEED		16000000
So these might explain the locking in debugfs functions, but they aren't currently used.
So either use them or drop them.

> +
> +enum ad4062_grade {
> +	AD4062_2MSPS,
> +};
> +
> +enum ad4062_operation_mode {
> +	AD4062_SAMPLE_MODE = 0,
> +	AD4062_BURST_AVERAGING_MODE = 1,
> +	AD4062_MONITOR_MODE = 3,
> +};
> +
> +enum ad4062_gp_mode {
> +	AD4062_GP_DISABLED,
> +	AD4062_GP_INTR,
> +	AD4062_GP_DRDY,
> +};
> +
> +enum ad4062_interrupt_en {
> +	AD4062_INTR_EN_NEITHER,
Where these correspond to specific register values make sure
to specify those values with = 0x0 etc

I'd use defines unless the type is useful, for example as ad4062_operation_mode above is.

> +	AD4062_INTR_EN_MIN,
> +	AD4062_INTR_EN_MAX,
> +	AD4062_INTR_EN_EITHER,
> +};

> +struct ad4062_state {
> +	const struct ad4062_chip_info *chip;
> +	const struct ad4062_bus_ops *ops;
> +	enum ad4062_operation_mode mode;
> +	struct completion completion;
> +	struct iio_trigger *trigger;
> +	struct iio_dev *indio_dev;
> +	struct i3c_device *i3cdev;
> +	struct regmap *regmap;
> +	u16 sampling_frequency;
> +	int vref_uv;
> +	u8 raw[4] __aligned(IIO_DMA_MINALIGN);
As I mention below, many of the uses of this seem to be a single __be16 so
I'd suggestion a union with one of those so we know it's aligned and can use
sizeof() instead of hard coded 2s.  If you prefer just add a __be16 variable
after this as that always fits anyway (IIO_DMA_MINALIGN >= 8).

> +};

> +
> +static const struct iio_enum AD4062_2MSPS_conversion_freq_enum = {
> +	.items = AD4062_FS(AD4062_2MSPS),
> +	.num_items = AD4062_FS_LEN(AD4062_2MSPS),
> +	.set = ad4062_sampling_frequency_set,
> +	.get = ad4062_sampling_frequency_get,
> +};
> +
> +#define AD4062_EXT_INFO(grade)						\
> +static struct iio_chan_spec_ext_info grade##_ext_info[] = {		\
> +	IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL,		\
> +		 &grade##_conversion_freq_enum),			\

if it's shared by all why can't you use the standard
 info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) ?
That enables simple in kernel use etc that is a pain with ext info.


> +	IIO_ENUM_AVAILABLE("sampling_frequency", IIO_SHARED_BY_ALL,	\
> +			   &grade##_conversion_freq_enum),		\

And this should use the read_avail handling rather than being done this way.
ext_info is for stuff that isn't particularly standard or which doesn't
fit in the normal interfaces (like mount matrix).

> +	{ }								\
> +}
> +
> +AD4062_EXT_INFO(AD4062_2MSPS);
> +
> +#define AD4062_CHAN(bits, grade) {							\
> +	.type = IIO_VOLTAGE,								\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) |				\
> +				    BIT(IIO_CHAN_INFO_SCALE) |				\
> +				    BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
> +				    BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),		\
> +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
> +	.indexed = 1,									\
> +	.channel = 0,									\
> +	.has_ext_scan_type = 1,								\
> +	.ext_scan_type = ad4062_scan_type_##bits##_s,					\
> +	.num_ext_scan_type = ARRAY_SIZE(ad4062_scan_type_##bits##_s),			\
> +	.ext_info = grade##_ext_info,							\
> +}
> +
> +static const struct ad4062_chip_info ad4060_chip_info = {
> +	.name = "ad4060",
> +	.channels = { AD4062_CHAN(12, AD4062_2MSPS) },
> +	.prod_id = 0x7A,
> +	.max_avg = AD4050_MAX_AVG,
> +	.grade = AD4062_2MSPS,
> +};
> +
> +static const struct ad4062_chip_info ad4062_chip_info = {
> +	.name = "ad4062",
> +	.channels = { AD4062_CHAN(16, AD4062_2MSPS) },
> +	.prod_id = 0x7C,
> +	.max_avg = AD4062_MAX_AVG,
> +	.grade = AD4062_2MSPS,

We'd normally only introduce elements to chip_info when then differ between
supported chips.  For now this .grade seems unnecessary complexity. 

> +};


> +static int ad4062_check_ids(struct ad4062_state *st)
> +{
> +	int ret;
> +	u16 val;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4062_REG_PROD_ID_1, &st->raw, 2);

You do a lot of __be16 reads in this driver. Maybe use a union for raw so we can
type these correctly and use sizeof() for that 2.

> +	if (ret)
> +		return ret;
> +
> +	val = get_unaligned_be16(st->raw);
> +	if (val != st->chip->prod_id)
> +		dev_warn(&st->i3cdev->dev,
> +			 "Production ID x%x does not match known values", val);
> +
> +	ret = regmap_bulk_read(st->regmap, AD4062_REG_VENDOR_H, &st->raw, 2);
> +	if (ret)
> +		return ret;
> +
> +	val = get_unaligned_be16(st->raw);
> +	if (val != AD4062_I3C_VENDOR) {
> +		dev_err(&st->i3cdev->dev,
> +			"Vendor ID x%x does not match expected value\n", val);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}

> +
> +static int ad4062_soft_reset(struct ad4062_state *st)
> +{
> +	u8 val = 0x81;

That probably wants to a define as definitely a magic number.

> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait AD4062 treset time */
> +	fsleep(5000);
> +
> +	return 0;
> +}
> +
> +static int ad4062_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			const bool *ref_sel)
> +{
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +	int ret;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	u8 val = FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, AD4062_GP_INTR) |

I'm a bit confused on why this routes anything to GP0 given we haven't requested that interrupt.

> +		 FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, AD4062_GP_DRDY);
> +
> +	ret = regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
> +				 AD4062_REG_GP_CONF_MODE_MSK_1 | AD4062_REG_GP_CONF_MODE_MSK_0,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(AD4062_REG_INTR_CONF_EN_MSK_0, (AD4062_INTR_EN_EITHER)) |
> +	      FIELD_PREP(AD4062_REG_INTR_CONF_EN_MSK_1, (AD4062_INTR_EN_NEITHER));

Filling val up here and using it much later seems odd. Probably better moved down.
Also excess brackets.
I'm lost on what this is doing though. Seems to be enabling interrupts that aren't
routed anywhere.

> +
> +	ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_MODES,
> +				 AD4062_REG_ADC_CONFIG_REF_EN_MSK,
> +				 FIELD_PREP(AD4062_REG_ADC_CONFIG_REF_EN_MSK,
> +					    *ref_sel));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4062_REG_DEVICE_STATUS,
> +			   AD4062_REG_DEVICE_STATUS_DEVICE_RESET);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(st->regmap, AD4062_REG_INTR_CONF,
> +				  AD4062_REG_INTR_CONF_EN_MSK_0 | AD4062_REG_INTR_CONF_EN_MSK_1,
> +				  val);
> +}

> +
> +static int ad4062_request_irq(struct iio_dev *indio_dev)
> +{
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->i3cdev->dev;
> +	int ret;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
> +	if (ret >= 0) {
> +		ret = devm_request_threaded_irq(dev, ret, NULL,
> +						 ad4062_irq_handler_drdy,
> +						 IRQF_ONESHOT, indio_dev->name,
> +						 indio_dev);
return directly here.
> +	} else if (ret != -EPROBE_DEFER) {
I'd prefer we did the error handling first. It's a little fiddly but
something like

	if (ret == -EPROBE_DEFER)
		return ret;
	if (ret < 0) { //I'd prefer an explicit match against whatever indicates nothing provided.
		return ...

	return devm_request_threaded_irq();

> +		ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
> +					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER,
> +					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER);
> +	}
> +
> +	return ret;
> +}
> +
> +static const int ad4062_oversampling_avail[] = {
> +	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
I don't mind this style, but you'll likely get feedback that you might as well
use
	BIT(0), BIT(1),
etc here as it makes it less likely a typo will hid in these powers of 2.

> +};

> +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
> +{
> +	struct i3c_device *i3cdev = st->i3cdev;
> +	u8 addr = AD4062_REG_CONV_TRIGGER;
> +	struct i3c_priv_xfer t[2] = {
> +		{
> +			.data.out = &addr,
> +			.len = 1,
> +			.rnw = false,
> +		},
> +		{
> +			.data.in = &st->raw,
> +			.len = 4,
> +			.rnw = true,
> +		}
> +	};
> +	int ret;
> +
> +	reinit_completion(&st->completion);
> +	/* Change address pointer to trigger conversion */
> +	ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
> +	if (ret)
> +		return ret;
> +	/*
> +	 * Single sample read should be used only for oversampling and
> +	 * sampling frequency pairs that take less than 1 sec.
> +	 */
> +	ret = wait_for_completion_timeout(&st->completion,
> +					  msecs_to_jiffies(1000));
> +	if (!ret)
> +		return -ETIMEDOUT;
> +
> +	ret = i3c_device_do_priv_xfers(i3cdev, &t[1], 1);
> +	if (ret)
> +		return ret;
> +	*val = get_unaligned_be32(st->raw);
> +	return ret;

return 0;
Makes it clear it must be 0 if we get here and that this is the 'good' exit path.

> +}
> +
> +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> +{
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
There is a nice new
	ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
let you do this using cleanup.h style.

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c

This looks like a perfect example of where those help.

When I catch up with review backlog I plan to look for other
places to use that infrastructure in IIO.


> +	if (ret)
> +		return ret;
> +
> +	ret = ad4062_set_operation_mode(st, st->mode);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = __ad4062_read_chan_raw(st, val);
> +
> +out_error:
> +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> +	return ret;
> +}


> +static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +				     unsigned int writeval, unsigned int *readval)
> +{
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
Add a comment on why reading registers isn't fine when buffered mode is in use.

If it's just a case of preventing writes that will change the state, then we typically
don't prevent those. It's a debug interface, no problem with people shooting themselves
in the foot.

If you are avoiding disrupting RMW sequences then use a local lock to ensure that
is atomic, not this big heavy one.

> +		return -EBUSY;
> +
> +	if (readval)
> +		ret = regmap_read(st->regmap, reg, readval);
> +	else
> +		ret = regmap_write(st->regmap, reg, writeval);
> +
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}

> +
> +static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
> +{
> +	struct device *dev = &st->i3cdev->dev;
> +	int uv;
> +
> +	uv = devm_regulator_get_enable_read_voltage(dev, "vio");

Why are you reading the voltages then throwing them away?
If we don't need to read it just use
devm_regulator_get_enabled()


> +	if (uv < 0)
> +		return dev_err_probe(dev, uv,
> +				     "Failed to enable and read vio voltage\n");
> +
> +	uv = devm_regulator_get_enable_read_voltage(dev, "vdd");

Unless there is some ordering constraint I don't know of, you shouldn't
fail on being unable to read the vdd voltage unless we are actually using it.
So try ref first and if that succeeds use devm_regulator_get_enabled(), if not
try the voltage reading approach..

> +	if (uv < 0)
> +		return dev_err_probe(dev, uv,
> +				     "Failed to enable vdd regulator\n");
> +
> +	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	*ref_sel = st->vref_uv == -ENODEV;
> +	if (st->vref_uv == -ENODEV)
> +		st->vref_uv = uv;
> +	else if (st->vref_uv < 0)
> +		return dev_err_probe(dev, st->vref_uv,
> +				     "Failed to enable and read ref voltage\n");
> +	return 0;
> +}
> +
> +static const struct i3c_device_id ad4062_id_table[] = {
> +	I3C_DEVICE(AD4062_I3C_VENDOR, ad4060_chip_info.prod_id, &ad4060_chip_info),
> +	I3C_DEVICE(AD4062_I3C_VENDOR, ad4062_chip_info.prod_id, &ad4062_chip_info),
> +	{}
Trivial but I'm trying to slowly standardize spacing in IIO for these and
a while back chose at random (more or less) to go with
	{ }

> +};
> +MODULE_DEVICE_TABLE(i3c, ad4062_id_table);
> +
> +static int ad4062_probe(struct i3c_device *i3cdev)
> +{
> +	const struct i3c_device_id *id = i3c_device_match_id(i3cdev, ad4062_id_table);
> +	const struct ad4062_chip_info *chip = id->data;
> +	struct device *dev = &i3cdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad4062_state *st;
> +	bool ref_sel;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->i3cdev = i3cdev;
> +	i3cdev_set_drvdata(i3cdev, st);
> +	init_completion(&st->completion);
> +
> +	ret = ad4062_regulators_get(st, &ref_sel);
> +	if (ret)
> +		return ret;
> +
> +	st->regmap = devm_regmap_init_i3c(i3cdev, &ad4062_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "Failed to initialize regmap\n");
> +
> +	st->mode = AD4062_SAMPLE_MODE;
> +	st->chip = chip;
> +	st->sampling_frequency = AD4062_FS_OFFSET(st->chip->grade);
> +	st->indio_dev = indio_dev;
> +
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = 1;
> +	indio_dev->info = &ad4062_info;
> +	indio_dev->name = chip->name;
> +	indio_dev->channels = chip->channels;
> +
> +	ret = ad4062_soft_reset(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "AD4062 failed to soft reset\n");
> +
> +	ret = ad4062_check_ids(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "AD4062 fields assertions failed\n");
Seems like you print enough in the check_ids function that this print is
perhaps just confusing?  Maybe add a print in the only path in there that
doesn't already have one.
> +
> +	ret = ad4062_setup(indio_dev, indio_dev->channels, &ref_sel);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4062_request_irq(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_active(dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable pm_runtime\n");
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = ad4062_request_ibi(i3cdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request i3c ibi\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);

Related to below. There is a race on tear down as you'll remove some of the
i3c infrastructure before removing the userspace ABI that might result in
that being used.

> +}
> +
> +static void ad4062_remove(struct i3c_device *i3cdev)
> +{
> +	i3c_device_disable_ibi(i3cdev);
> +	i3c_device_free_ibi(i3cdev);
You should never mix devm_ based cleanup and manual cleanup.

Basic rule is that the 1st time you add something that needs non devm cleanup
for some reason then you must stop using any devm_ calls after that in probe().

There are some handy helpers for defining your own cleanup functions.
devm_add_action_or_reset() is helpful for extending how far we can use devm_
and typically solves this problem.

> +}

> +
> +static int ad4062_runtime_resume(struct device *dev)
> +{
> +	struct ad4062_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regmap_clear_bits(st->regmap, AD4062_REG_DEVICE_CONFIG,
> +				AD4062_REG_DEVICE_CONFIG_POWER_MODE_MSK);
If it failed, little point in sleeping.  I'd do
	if (ret)
		return ret;
	
	fsleep(4000);
	return 0;

So the flow on error is clearer.
> +
> +	fsleep(4000);
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops ad4062_pm_ops = {
> +	SET_RUNTIME_PM_OPS(ad4062_runtime_suspend, ad4062_runtime_resume, NULL)
Should have trailing comma.

Also rare that we can't just enable the system suspend/resume stuff that uses
the runtime PM callbacks by using:
DEFINE_RUNTIME_DEV_PM_OPS().  If we can't I'd add a comment on why not alongside
this code.

> +};

Re: [PATCH 3/7] iio: adc: Add support for ad4062

Posted by Jorge Marques 3 weeks, 4 days ago

On Sat, Oct 18, 2025 at 05:10:32PM +0100, Jonathan Cameron wrote:
> On Mon, 13 Oct 2025 09:28:01 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
> 
> > The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> > register (SAR) analog-to-digital converter (ADC) that enables low-power,
> > high-density data acquisition solutions without sacrificing precision.
> > This ADC offers a unique balance of performance and power efficiency,
> > plus innovative features for seamlessly switching between
> > high-resolution and low-power modes tailored to the immediate needs of
> > the system. The AD4060/AD4062 are ideal for battery-powered, compact
> > data acquisition and edge sensing applications.
Hi Jonathan,

Mostly acknowledgements and explanations, except a comment on ACQUIRE usage.

> 
> Little bit marketing heavy for a patch description.. I'd stick to
> type, resolution and typical usecases. Skip the sacrificing precision etc
> wording!  It'll just make the readers eyes glaze over ;)
> 
Yep, will tune down a bit. The description was from the website.
> The mixing of regmap and direct accesses is a little unfortunate but seems unavoidable.
> 
Yes, it is not possible to use only the regmap due to the adc sample
transfers that require reads without writing the address.
> 
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> 
> Various comments inline.  In general looks good for a v1.
> 
> 
> > diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..e55a69c62694a71a4e29f29b9a2bfeec3b16c990
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4062.c
> > @@ -0,0 +1,905 @@
> 
> > +#define     AD4062_REG_DEVICE_STATUS_DEVICE_RESET	BIT(6)
> > +#define AD4062_REG_MIN_SAMPLE				0x45
> > +#define AD4062_REG_IBI_STATUS				0x48
> > +#define AD4062_REG_CONV_READ_LSB			0x50
> > +#define AD4062_REG_CONV_READ				0x53
> > +#define AD4062_REG_CONV_TRIGGER				0x59
> > +#define AD4062_REG_CONV_AUTO				0x61
> > +#define AD4062_MAX_REG					0x61
> 
> I'd define this in terms of the reg that it happens to be
> #define AD4052_MAX_REG AD6062_REG_CONF_AUTO
> 
Ack.
> 
> > +#define AD4062_VIO_3V3		3300000
> 
> This is a real value rather than a magic number so better to not hide it in a define.
> Just use the value inline.
> 
Ack.
> 
> > +#define AD4062_SPI_MAX_ADC_XFER_SPEED(x)	((x) >= AD4062_VIO_3V3 ? 83333333 : 58823529)
> > +#define AD4062_SPI_MAX_REG_XFER_SPEED		16000000
> So these might explain the locking in debugfs functions, but they aren't currently used.
> So either use them or drop them.
> 
Those are dead code for this driver, I will remove as well as others
unused defines.
> > +
> > +enum ad4062_grade {
> > +	AD4062_2MSPS,
> > +};

Since only one speed grade is available in the market, this will be
dropped and all usage simplified.

> > +
> > +enum ad4062_operation_mode {
> > +	AD4062_SAMPLE_MODE = 0,
> > +	AD4062_BURST_AVERAGING_MODE = 1,
> > +	AD4062_MONITOR_MODE = 3,
> > +};
> > +
> > +enum ad4062_gp_mode {
> > +	AD4062_GP_DISABLED,
> > +	AD4062_GP_INTR,
> > +	AD4062_GP_DRDY,
> > +};
> > +
> > +enum ad4062_interrupt_en {
> > +	AD4062_INTR_EN_NEITHER,
> Where these correspond to specific register values make sure
> to specify those values with = 0x0 etc
> 
> I'd use defines unless the type is useful, for example as ad4062_operation_mode above is.
> 
Ack.
> > +	AD4062_INTR_EN_MIN,
> > +	AD4062_INTR_EN_MAX,
> > +	AD4062_INTR_EN_EITHER,
> > +};
> 
> > +struct ad4062_state {
> > +	const struct ad4062_chip_info *chip;
> > +	const struct ad4062_bus_ops *ops;
> > +	enum ad4062_operation_mode mode;
> > +	struct completion completion;
> > +	struct iio_trigger *trigger;
> > +	struct iio_dev *indio_dev;
> > +	struct i3c_device *i3cdev;
> > +	struct regmap *regmap;
> > +	u16 sampling_frequency;
> > +	int vref_uv;
> > +	u8 raw[4] __aligned(IIO_DMA_MINALIGN);
> As I mention below, many of the uses of this seem to be a single __be16 so
> I'd suggestion a union with one of those so we know it's aligned and can use
> sizeof() instead of hard coded 2s.  If you prefer just add a __be16 variable
> after this as that always fits anyway (IIO_DMA_MINALIGN >= 8).
> 
Will do

        union {
                __be32 be32;
                __be16 be16;
                u8 bytes[4];
        } buf __aligned(IIO_DMA_MINALIGN);

> > +};
> 
> > +
> > +static const struct iio_enum AD4062_2MSPS_conversion_freq_enum = {
> > +	.items = AD4062_FS(AD4062_2MSPS),
> > +	.num_items = AD4062_FS_LEN(AD4062_2MSPS),
> > +	.set = ad4062_sampling_frequency_set,
> > +	.get = ad4062_sampling_frequency_get,
> > +};
> > +
> > +#define AD4062_EXT_INFO(grade)						\
> > +static struct iio_chan_spec_ext_info grade##_ext_info[] = {		\
> > +	IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL,		\
> > +		 &grade##_conversion_freq_enum),			\
> 
> if it's shared by all why can't you use the standard
>  info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) ?
> That enables simple in kernel use etc that is a pain with ext info.
> 

Yes definitely.
> 
> > +	IIO_ENUM_AVAILABLE("sampling_frequency", IIO_SHARED_BY_ALL,	\
> > +			   &grade##_conversion_freq_enum),		\
> 
> And this should use the read_avail handling rather than being done this way.
> ext_info is for stuff that isn't particularly standard or which doesn't
> fit in the normal interfaces (like mount matrix).
> 
Ack.
> > +	{ }								\
> > +}
> > +
> > +AD4062_EXT_INFO(AD4062_2MSPS);
> > +
> > +#define AD4062_CHAN(bits, grade) {							\
> > +	.type = IIO_VOLTAGE,								\
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) |				\
> > +				    BIT(IIO_CHAN_INFO_SCALE) |				\
> > +				    BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
> > +				    BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),		\
> > +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
> > +	.indexed = 1,									\
> > +	.channel = 0,									\
> > +	.has_ext_scan_type = 1,								\
> > +	.ext_scan_type = ad4062_scan_type_##bits##_s,					\
> > +	.num_ext_scan_type = ARRAY_SIZE(ad4062_scan_type_##bits##_s),			\
> > +	.ext_info = grade##_ext_info,							\
> > +}
> > +
> > +static const struct ad4062_chip_info ad4060_chip_info = {
> > +	.name = "ad4060",
> > +	.channels = { AD4062_CHAN(12, AD4062_2MSPS) },
> > +	.prod_id = 0x7A,
> > +	.max_avg = AD4050_MAX_AVG,
> > +	.grade = AD4062_2MSPS,
> > +};
> > +
> > +static const struct ad4062_chip_info ad4062_chip_info = {
> > +	.name = "ad4062",
> > +	.channels = { AD4062_CHAN(16, AD4062_2MSPS) },
> > +	.prod_id = 0x7C,
> > +	.max_avg = AD4062_MAX_AVG,
> > +	.grade = AD4062_2MSPS,
> 
> We'd normally only introduce elements to chip_info when then differ between
> supported chips.  For now this .grade seems unnecessary complexity. 
> 
Yes, grade will be completely removed since the is only one speed grade
in the market.
> > +};
> 
> 
> > +static int ad4062_check_ids(struct ad4062_state *st)
> > +{
> > +	int ret;
> > +	u16 val;
> > +
> > +	ret = regmap_bulk_read(st->regmap, AD4062_REG_PROD_ID_1, &st->raw, 2);
> 
> You do a lot of __be16 reads in this driver. Maybe use a union for raw so we can
> type these correctly and use sizeof() for that 2.
Ack.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	val = get_unaligned_be16(st->raw);
> > +	if (val != st->chip->prod_id)
> > +		dev_warn(&st->i3cdev->dev,
> > +			 "Production ID x%x does not match known values", val);
> > +
> > +	ret = regmap_bulk_read(st->regmap, AD4062_REG_VENDOR_H, &st->raw, 2);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val = get_unaligned_be16(st->raw);
> > +	if (val != AD4062_I3C_VENDOR) {
> > +		dev_err(&st->i3cdev->dev,
> > +			"Vendor ID x%x does not match expected value\n", val);
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> > +
> > +static int ad4062_soft_reset(struct ad4062_state *st)
> > +{
> > +	u8 val = 0x81;
> 
> That probably wants to a define as definitely a magic number.
> 
Ack.
> > +	int ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Wait AD4062 treset time */
> > +	fsleep(5000);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ad4062_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > +			const bool *ref_sel)
> > +{
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +	const struct iio_scan_type *scan_type;
> > +	int ret;
> > +
> > +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> > +	if (IS_ERR(scan_type))
> > +		return PTR_ERR(scan_type);
> > +
> > +	u8 val = FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, AD4062_GP_INTR) |
> 
> I'm a bit confused on why this routes anything to GP0 given we haven't requested that interrupt.
> 
Is used in the next commit, on v2 I will be more careful to define things
only for the particular patch.
> > +		 FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, AD4062_GP_DRDY);
> > +
> > +	ret = regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
> > +				 AD4062_REG_GP_CONF_MODE_MSK_1 | AD4062_REG_GP_CONF_MODE_MSK_0,
> > +				 val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val = FIELD_PREP(AD4062_REG_INTR_CONF_EN_MSK_0, (AD4062_INTR_EN_EITHER)) |
> > +	      FIELD_PREP(AD4062_REG_INTR_CONF_EN_MSK_1, (AD4062_INTR_EN_NEITHER));
> 
> Filling val up here and using it much later seems odd. Probably better moved down.
> Also excess brackets.
> I'm lost on what this is doing though. Seems to be enabling interrupts that aren't
> routed anywhere.
Ack, reason same as above, belongs to the next commit.
> 
> > +
> > +	ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_MODES,
> > +				 AD4062_REG_ADC_CONFIG_REF_EN_MSK,
> > +				 FIELD_PREP(AD4062_REG_ADC_CONFIG_REF_EN_MSK,
> > +					    *ref_sel));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4062_REG_DEVICE_STATUS,
> > +			   AD4062_REG_DEVICE_STATUS_DEVICE_RESET);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_update_bits(st->regmap, AD4062_REG_INTR_CONF,
> > +				  AD4062_REG_INTR_CONF_EN_MSK_0 | AD4062_REG_INTR_CONF_EN_MSK_1,
> > +				  val);
> > +}
> 
> > +
> > +static int ad4062_request_irq(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +	struct device *dev = &st->i3cdev->dev;
> > +	int ret;
> > +
> > +	ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
> > +	if (ret >= 0) {
> > +		ret = devm_request_threaded_irq(dev, ret, NULL,
> > +						 ad4062_irq_handler_drdy,
> > +						 IRQF_ONESHOT, indio_dev->name,
> > +						 indio_dev);
> return directly here.
Ack
> > +	} else if (ret != -EPROBE_DEFER) {
> I'd prefer we did the error handling first. It's a little fiddly but
> something like
Ack.
> 
> 	if (ret == -EPROBE_DEFER)
> 		return ret;
> 	if (ret < 0) { //I'd prefer an explicit match against whatever indicates nothing provided.
> 		return ...
> 
> 	return devm_request_threaded_irq();
> 
> > +		ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
> > +					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER,
> > +					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static const int ad4062_oversampling_avail[] = {
> > +	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
> I don't mind this style, but you'll likely get feedback that you might as well
> use
> 	BIT(0), BIT(1),
> etc here as it makes it less likely a typo will hid in these powers of 2.
> 
I think it is fine as is.
> > +};
> 
> > +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
> > +{
> > +	struct i3c_device *i3cdev = st->i3cdev;
> > +	u8 addr = AD4062_REG_CONV_TRIGGER;
> > +	struct i3c_priv_xfer t[2] = {
> > +		{
> > +			.data.out = &addr,
> > +			.len = 1,
> > +			.rnw = false,
> > +		},
> > +		{
> > +			.data.in = &st->raw,
> > +			.len = 4,
> > +			.rnw = true,
> > +		}
> > +	};
> > +	int ret;
> > +
> > +	reinit_completion(&st->completion);
> > +	/* Change address pointer to trigger conversion */
> > +	ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
> > +	if (ret)
> > +		return ret;
> > +	/*
> > +	 * Single sample read should be used only for oversampling and
> > +	 * sampling frequency pairs that take less than 1 sec.
> > +	 */
> > +	ret = wait_for_completion_timeout(&st->completion,
> > +					  msecs_to_jiffies(1000));
> > +	if (!ret)
> > +		return -ETIMEDOUT;
> > +
> > +	ret = i3c_device_do_priv_xfers(i3cdev, &t[1], 1);
> > +	if (ret)
> > +		return ret;
> > +	*val = get_unaligned_be32(st->raw);
> > +	return ret;
> 
> return 0;
> Makes it clear it must be 0 if we get here and that this is the 'good' exit path.
> 
Ack.
> > +}
> > +
> > +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> > +{
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> There is a nice new
> 	ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
> let you do this using cleanup.h style.
> 
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
> 
> This looks like a perfect example of where those help.
> 
> When I catch up with review backlog I plan to look for other
> places to use that infrastructure in IIO.
> 
I tried implementing, here becomes

        ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
        ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);

At buffer and monitor, since we put the device as active during the
lifetime of the buffer and monitor mode, either I leave as is, or I bump
the counter with pm_runtime_get_noresume, so when the method leaves, the
counter drops to 1 and not 0, then on disable I drop the counter back to
0 and queue the autosuspend with pm_runtime_put_autosuspend.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4062_set_operation_mode(st, st->mode);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	ret = __ad4062_read_chan_raw(st, val);
> > +
> > +out_error:
> > +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > +	return ret;
> > +}
> 
> 
> > +static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> > +				     unsigned int writeval, unsigned int *readval)
> > +{
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> Add a comment on why reading registers isn't fine when buffered mode is in use.
> 
> If it's just a case of preventing writes that will change the state, then we typically
> don't prevent those. It's a debug interface, no problem with people shooting themselves
> in the foot.
> 
> If you are avoiding disrupting RMW sequences then use a local lock to ensure that
> is atomic, not this big heavy one.
> 
I dropped the lock and will let the user mess-up the transfer.
On buffer or monitor mode, a register access will cause the device to
drop of those modes. For the buffer acquisition, it will stop filling up
the buffer. The user can then recover the state by disabling and
enabling again the buffer or monitor mode.
> > +		return -EBUSY;
> > +
> > +	if (readval)
> > +		ret = regmap_read(st->regmap, reg, readval);
> > +	else
> > +		ret = regmap_write(st->regmap, reg, writeval);
> > +
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> > +}
> 
> > +
> > +static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
> > +{
> > +	struct device *dev = &st->i3cdev->dev;
> > +	int uv;
> > +
> > +	uv = devm_regulator_get_enable_read_voltage(dev, "vio");
> 
> Why are you reading the voltages then throwing them away?
> If we don't need to read it just use
> devm_regulator_get_enabled()
> 
Ack, I will just enable vio, not read it.
> 
> > +	if (uv < 0)
> > +		return dev_err_probe(dev, uv,
> > +				     "Failed to enable and read vio voltage\n");
> > +
> > +	uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
> 
> Unless there is some ordering constraint I don't know of, you shouldn't
> fail on being unable to read the vdd voltage unless we are actually using it.
> So try ref first and if that succeeds use devm_regulator_get_enabled(), if not
> try the voltage reading approach..
> 

Ack, I reordered it and won't read vdd if ref exists (!= -ENODEV).

> > +	if (uv < 0)
> > +		return dev_err_probe(dev, uv,
> > +				     "Failed to enable vdd regulator\n");
> > +
> > +	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> > +	*ref_sel = st->vref_uv == -ENODEV;
> > +	if (st->vref_uv == -ENODEV)
> > +		st->vref_uv = uv;
> > +	else if (st->vref_uv < 0)
> > +		return dev_err_probe(dev, st->vref_uv,
> > +				     "Failed to enable and read ref voltage\n");
> > +	return 0;
> > +}
> > +
> > +static const struct i3c_device_id ad4062_id_table[] = {
> > +	I3C_DEVICE(AD4062_I3C_VENDOR, ad4060_chip_info.prod_id, &ad4060_chip_info),
> > +	I3C_DEVICE(AD4062_I3C_VENDOR, ad4062_chip_info.prod_id, &ad4062_chip_info),
> > +	{}
> Trivial but I'm trying to slowly standardize spacing in IIO for these and
> a while back chose at random (more or less) to go with
> 	{ }
Ack.
> 
> > +};
> > +MODULE_DEVICE_TABLE(i3c, ad4062_id_table);
> > +
> > +static int ad4062_probe(struct i3c_device *i3cdev)
> > +{
> > +	const struct i3c_device_id *id = i3c_device_match_id(i3cdev, ad4062_id_table);
> > +	const struct ad4062_chip_info *chip = id->data;
> > +	struct device *dev = &i3cdev->dev;
> > +	struct iio_dev *indio_dev;
> > +	struct ad4062_state *st;
> > +	bool ref_sel;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +	st->i3cdev = i3cdev;
> > +	i3cdev_set_drvdata(i3cdev, st);
> > +	init_completion(&st->completion);
> > +
> > +	ret = ad4062_regulators_get(st, &ref_sel);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->regmap = devm_regmap_init_i3c(i3cdev, &ad4062_regmap_config);
> > +	if (IS_ERR(st->regmap))
> > +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> > +				     "Failed to initialize regmap\n");
> > +
> > +	st->mode = AD4062_SAMPLE_MODE;
> > +	st->chip = chip;
> > +	st->sampling_frequency = AD4062_FS_OFFSET(st->chip->grade);
> > +	st->indio_dev = indio_dev;
> > +
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->num_channels = 1;
> > +	indio_dev->info = &ad4062_info;
> > +	indio_dev->name = chip->name;
> > +	indio_dev->channels = chip->channels;
> > +
> > +	ret = ad4062_soft_reset(st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "AD4062 failed to soft reset\n");
> > +
> > +	ret = ad4062_check_ids(st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "AD4062 fields assertions failed\n");
> Seems like you print enough in the check_ids function that this print is
> perhaps just confusing?  Maybe add a print in the only path in there that
> doesn't already have one.
Ack, I will remove this error message.
> > +
> > +	ret = ad4062_setup(indio_dev, indio_dev->channels, &ref_sel);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4062_request_irq(indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pm_runtime_set_active(dev);
> > +	ret = devm_pm_runtime_enable(dev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to enable pm_runtime\n");
> > +
> > +	pm_runtime_set_autosuspend_delay(dev, 1000);
> > +	pm_runtime_use_autosuspend(dev);
> > +
> > +	ret = ad4062_request_ibi(i3cdev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to request i3c ibi\n");
> > +
> > +	return devm_iio_device_register(dev, indio_dev);
> 
> Related to below. There is a race on tear down as you'll remove some of the
> i3c infrastructure before removing the userspace ABI that might result in
> that being used.
I submitted a patch at
https://lore.kernel.org/linux-i3c/aRYLc%2F+KAD13g7T7@lizhi-Precision-Tower-5810/T/#t
to allow using the devm_ based cleanup.
As is, the devm_ calls occurs after i3c_device_remove, this that calls
i3c_device_free_ibi and causes an exception if i3c_device_disable_ibi is
not called.

> 
> > +}
> > +
> > +static void ad4062_remove(struct i3c_device *i3cdev)
> > +{
> > +	i3c_device_disable_ibi(i3cdev);
> > +	i3c_device_free_ibi(i3cdev);
> You should never mix devm_ based cleanup and manual cleanup.
> 
> Basic rule is that the 1st time you add something that needs non devm cleanup
> for some reason then you must stop using any devm_ calls after that in probe().
> 
> There are some handy helpers for defining your own cleanup functions.
> devm_add_action_or_reset() is helpful for extending how far we can use devm_
> and typically solves this problem.
> 
Explained above, will use the devm_ helper in v2.
> > +}
> 
> > +
> > +static int ad4062_runtime_resume(struct device *dev)
> > +{
> > +	struct ad4062_state *st = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = regmap_clear_bits(st->regmap, AD4062_REG_DEVICE_CONFIG,
> > +				AD4062_REG_DEVICE_CONFIG_POWER_MODE_MSK);
> If it failed, little point in sleeping.  I'd do
> 	if (ret)
> 		return ret;
> 	
> 	fsleep(4000);
> 	return 0;
> 
> So the flow on error is clearer.
Ack.
> > +
> > +	fsleep(4000);
> > +	return ret;
> > +}
> > +
> > +static const struct dev_pm_ops ad4062_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(ad4062_runtime_suspend, ad4062_runtime_resume, NULL)
> Should have trailing comma.
> 
> Also rare that we can't just enable the system suspend/resume stuff that uses
> the runtime PM callbacks by using:
> DEFINE_RUNTIME_DEV_PM_OPS().  If we can't I'd add a comment on why not alongside
> this code.
> 
> > +};
> 
> 
Ack.

Best regards,
Jorge

Re: [PATCH 3/7] iio: adc: Add support for ad4062

Posted by Andy Shevchenko 3 weeks, 4 days ago

On Sun, Nov 23, 2025 at 08:48:09PM +0100, Jorge Marques wrote:
> On Sat, Oct 18, 2025 at 05:10:32PM +0100, Jonathan Cameron wrote:
> > On Mon, 13 Oct 2025 09:28:01 +0200
> > Jorge Marques <jorge.marques@analog.com> wrote:

> Mostly acknowledgements and explanations, except a comment on ACQUIRE usage.

...

> > > +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> > > +{
> > > +	struct ad4062_state *st = iio_priv(indio_dev);
> > > +	int ret;
> > > +
> > > +	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > There is a nice new
> > 	ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
> > let you do this using cleanup.h style.
> > 
> > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
> > 
> > This looks like a perfect example of where those help.
> > 
> > When I catch up with review backlog I plan to look for other
> > places to use that infrastructure in IIO.
> > 
> I tried implementing, here becomes
> 
>         ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
>         ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> 
> At buffer and monitor, since we put the device as active during the
> lifetime of the buffer and monitor mode, either I leave as is, or I bump
> the counter with pm_runtime_get_noresume, so when the method leaves, the
> counter drops to 1 and not 0, then on disable I drop the counter back to
> 0 and queue the autosuspend with pm_runtime_put_autosuspend.
> > 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = ad4062_set_operation_mode(st, st->mode);
> > > +	if (ret)
> > > +		goto out_error;
> > > +
> > > +	ret = __ad4062_read_chan_raw(st, val);
> > > +
> > > +out_error:
> > > +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > > +	return ret;
> > > +}

I read the above code, I read it again, I don't understand the reasoning.
The ACQUIRE() doesn't change the behaviour of the above code.

If you need to bump the reference counter, it should be done somewhere else
where it affects the flow, or this code has a bug.

If I miss something, please elaborate.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/7] iio: adc: Add support for ad4062

Posted by Jorge Marques 3 weeks, 4 days ago

On Mon, Nov 24, 2025 at 09:43:09AM +0200, Andy Shevchenko wrote:
> On Sun, Nov 23, 2025 at 08:48:09PM +0100, Jorge Marques wrote:
> > On Sat, Oct 18, 2025 at 05:10:32PM +0100, Jonathan Cameron wrote:
> > > On Mon, 13 Oct 2025 09:28:01 +0200
> > > Jorge Marques <jorge.marques@analog.com> wrote:
> 
> > Mostly acknowledgements and explanations, except a comment on ACQUIRE usage.
> 
> ...
> 

Hi Andy,

> > > > +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> > > > +{
> > > > +	struct ad4062_state *st = iio_priv(indio_dev);
> > > > +	int ret;
> > > > +
> > > > +	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > > There is a nice new
> > > 	ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
> > > let you do this using cleanup.h style.
> > > 
> > > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
> > > 
> > > This looks like a perfect example of where those help.
> > > 
> > > When I catch up with review backlog I plan to look for other
> > > places to use that infrastructure in IIO.
> > > 
> > I tried implementing, here becomes
> > 
> >         ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> >         ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> > 
> > At buffer and monitor, since we put the device as active during the
> > lifetime of the buffer and monitor mode, either I leave as is, or I bump
> > the counter with pm_runtime_get_noresume, so when the method leaves, the
> > counter drops to 1 and not 0, then on disable I drop the counter back to
> > 0 and queue the autosuspend with pm_runtime_put_autosuspend.
> > > 
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = ad4062_set_operation_mode(st, st->mode);
> > > > +	if (ret)
> > > > +		goto out_error;
> > > > +
> > > > +	ret = __ad4062_read_chan_raw(st, val);
> > > > +
> > > > +out_error:
> > > > +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > > > +	return ret;
> > > > +}
> 
> I read the above code, I read it again, I don't understand the reasoning.
> The ACQUIRE() doesn't change the behaviour of the above code.
> 
> If you need to bump the reference counter, it should be done somewhere else
> where it affects the flow, or this code has a bug.
> 
> If I miss something, please elaborate.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

The part highlighted does not require bumping the reference counter, but
at the buffer acquisition and monitor mode, to not put the device back
in low power mode during the lifetime of those operations.

Buffer more:

  static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
  {
          struct ad4062_state *st = iio_priv(indio_dev);
          int ret;

	  // [ Some code ]

          ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
          ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
          if (ret)
                  return ret;

	  // [ More code ]

          pm_runtime_get_noresume(&st->i3cdev->dev);
          return 0;
  }

  static int ad4062_triggered_buffer_predisable(struct iio_dev *indio_dev)
  {
          struct ad4062_state *st = iio_priv(indio_dev);

          pm_runtime_put_autosuspend(&st->i3cdev->dev);
          return 0;
  }

Monitor mode:

  static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
  {
          int ret = 0;

          if (!enable) {
                  pm_runtime_put_autosuspend(&st->i3cdev->dev);
                  return 0;
          }

          ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
          ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
          if (ret)
                  return ret;

	  // [ Some code ]

          pm_runtime_get_noresume(&st->i3cdev->dev);
          return 0;
  }

The raw read does not require this behaviour, when the method returns,
the device returns to low power mode:

  static int ad4062_read_chan_raw(struct ad4062_state *st, int *val)
  {
          int ret;

          ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
          ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
          if (ret)
                  return ret;

          ret = ad4062_set_operation_mode(st, st->mode);
          if (ret)
                  return ret;

          return __ad4062_read_chan_raw(st, val);
  }

I will submitted v2 shortly.

Best Regards,
Jorge

Re: [PATCH 3/7] iio: adc: Add support for ad4062

Posted by Andy Shevchenko 3 weeks, 4 days ago

On Mon, Nov 24, 2025 at 09:57:26AM +0100, Jorge Marques wrote:
> On Mon, Nov 24, 2025 at 09:43:09AM +0200, Andy Shevchenko wrote:

...

> I will submitted v2 shortly.

I think the "shortly" is not needed, please take your time, this seems missed
v6.19-rc1 anyway.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/7] iio: adc: Add support for ad4062

Posted by Jorge Marques 3 weeks, 4 days ago

On Mon, Nov 24, 2025 at 11:11:41AM +0200, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 09:57:26AM +0100, Jorge Marques wrote:
> > On Mon, Nov 24, 2025 at 09:43:09AM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > I will submitted v2 shortly.
> 
> I think the "shortly" is not needed, please take your time, this seems missed
> v6.19-rc1 anyway.

Hi Andy,

I was submitting when this reached my mailbox. Still, noted the comment
on your next e-mail and no pressure on the release window, I just didn't
have further changes for this revision.

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
Thanks,
Jorge

Re: [PATCH 3/7] iio: adc: Add support for ad4062

Posted by Andy Shevchenko 3 weeks, 4 days ago

On Mon, Nov 24, 2025 at 09:57:26AM +0100, Jorge Marques wrote:
> On Mon, Nov 24, 2025 at 09:43:09AM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 23, 2025 at 08:48:09PM +0100, Jorge Marques wrote:
> > > On Sat, Oct 18, 2025 at 05:10:32PM +0100, Jonathan Cameron wrote:
> > > > On Mon, 13 Oct 2025 09:28:01 +0200
> > > > Jorge Marques <jorge.marques@analog.com> wrote:

> > > Mostly acknowledgements and explanations, except a comment on ACQUIRE usage.

...

> > > > > +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> > > > > +{
> > > > > +	struct ad4062_state *st = iio_priv(indio_dev);
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > > > There is a nice new
> > > > 	ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
> > > > let you do this using cleanup.h style.
> > > > 
> > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
> > > > 
> > > > This looks like a perfect example of where those help.
> > > > 
> > > > When I catch up with review backlog I plan to look for other
> > > > places to use that infrastructure in IIO.
> > > > 
> > > I tried implementing, here becomes
> > > 
> > >         ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> > >         ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> > > 
> > > At buffer and monitor, since we put the device as active during the
> > > lifetime of the buffer and monitor mode, either I leave as is, or I bump
> > > the counter with pm_runtime_get_noresume, so when the method leaves, the
> > > counter drops to 1 and not 0, then on disable I drop the counter back to
> > > 0 and queue the autosuspend with pm_runtime_put_autosuspend.
> > > > 
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	ret = ad4062_set_operation_mode(st, st->mode);
> > > > > +	if (ret)
> > > > > +		goto out_error;
> > > > > +
> > > > > +	ret = __ad4062_read_chan_raw(st, val);
> > > > > +
> > > > > +out_error:
> > > > > +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > > > > +	return ret;
> > > > > +}
> > 
> > I read the above code, I read it again, I don't understand the reasoning.
> > The ACQUIRE() doesn't change the behaviour of the above code.
> > 
> > If you need to bump the reference counter, it should be done somewhere else
> > where it affects the flow, or this code has a bug.
> > 
> > If I miss something, please elaborate.
> 
> The part highlighted does not require bumping the reference counter, but
> at the buffer acquisition and monitor mode, to not put the device back
> in low power mode during the lifetime of those operations.
> 
> Buffer more:
> 
>   static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
>   {
>           struct ad4062_state *st = iio_priv(indio_dev);
>           int ret;
> 
> 	  // [ Some code ]
> 
>           ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
>           ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
>           if (ret)
>                   return ret;
> 
> 	  // [ More code ]

>           pm_runtime_get_noresume(&st->i3cdev->dev);


Yes, this looks good if it makes the error paths cleaner.
Also consider adding

	struct device *dev = &st->i3cdev->dev;

at the top of the functions that use it, it might make code better to read.

>           return 0;
>   }
> 
>   static int ad4062_triggered_buffer_predisable(struct iio_dev *indio_dev)
>   {
>           struct ad4062_state *st = iio_priv(indio_dev);
> 
>           pm_runtime_put_autosuspend(&st->i3cdev->dev);
>           return 0;
>   }
> 
> Monitor mode:
> 
>   static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
>   {
>           int ret = 0;
> 
>           if (!enable) {
>                   pm_runtime_put_autosuspend(&st->i3cdev->dev);
>                   return 0;
>           }
> 
>           ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
>           ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
>           if (ret)
>                   return ret;
> 
> 	  // [ Some code ]
> 
>           pm_runtime_get_noresume(&st->i3cdev->dev);
>           return 0;
>   }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/7] iio: adc: Add support for ad4062

Posted by Jonathan Cameron 1 week, 5 days ago

On Mon, 24 Nov 2025 11:10:32 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Nov 24, 2025 at 09:57:26AM +0100, Jorge Marques wrote:
> > On Mon, Nov 24, 2025 at 09:43:09AM +0200, Andy Shevchenko wrote:  
> > > On Sun, Nov 23, 2025 at 08:48:09PM +0100, Jorge Marques wrote:  
> > > > On Sat, Oct 18, 2025 at 05:10:32PM +0100, Jonathan Cameron wrote:  
> > > > > On Mon, 13 Oct 2025 09:28:01 +0200
> > > > > Jorge Marques <jorge.marques@analog.com> wrote:  
> 
> > > > Mostly acknowledgements and explanations, except a comment on ACQUIRE usage.  
> 
> ...
> 
> > > > > > +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> > > > > > +{
> > > > > > +	struct ad4062_state *st = iio_priv(indio_dev);
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);  
> > > > > There is a nice new
> > > > > 	ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
> > > > > let you do this using cleanup.h style.
> > > > > 
> > > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
> > > > > 
> > > > > This looks like a perfect example of where those help.
> > > > > 
> > > > > When I catch up with review backlog I plan to look for other
> > > > > places to use that infrastructure in IIO.
> > > > >   
> > > > I tried implementing, here becomes
> > > > 
> > > >         ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> > > >         ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> > > > 
> > > > At buffer and monitor, since we put the device as active during the
> > > > lifetime of the buffer and monitor mode, either I leave as is, or I bump
> > > > the counter with pm_runtime_get_noresume, so when the method leaves, the
> > > > counter drops to 1 and not 0, then on disable I drop the counter back to
> > > > 0 and queue the autosuspend with pm_runtime_put_autosuspend.  
> > > > >   
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	ret = ad4062_set_operation_mode(st, st->mode);
> > > > > > +	if (ret)
> > > > > > +		goto out_error;
> > > > > > +
> > > > > > +	ret = __ad4062_read_chan_raw(st, val);
> > > > > > +
> > > > > > +out_error:
> > > > > > +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > > > > > +	return ret;
> > > > > > +}  
> > > 
> > > I read the above code, I read it again, I don't understand the reasoning.
> > > The ACQUIRE() doesn't change the behaviour of the above code.
> > > 
> > > If you need to bump the reference counter, it should be done somewhere else
> > > where it affects the flow, or this code has a bug.
> > > 
> > > If I miss something, please elaborate.  
> > 
> > The part highlighted does not require bumping the reference counter, but
> > at the buffer acquisition and monitor mode, to not put the device back
> > in low power mode during the lifetime of those operations.
> > 
> > Buffer more:
> > 
> >   static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> >   {
> >           struct ad4062_state *st = iio_priv(indio_dev);
> >           int ret;
> > 
> > 	  // [ Some code ]
> > 
> >           ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> >           ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> >           if (ret)
> >                   return ret;
> > 
> > 	  // [ More code ]  
> 
> >           pm_runtime_get_noresume(&st->i3cdev->dev);  
> 
> 
> Yes, this looks good if it makes the error paths cleaner.
> Also consider adding
> 
> 	struct device *dev = &st->i3cdev->dev;
> 
> at the top of the functions that use it, it might make code better to read.
> 

Sorry I'm late to respond to this.  If we do end up doing this
Raphael has been posting some new macros to help and I'm rather dubious
about using an acquire to grab a first reference then upgrading with with
a second on the use paths.  I think ACQUIRE in this particular place may
just not be appropriate.

Jonathan

> >           return 0;
> >   }
> > 
> >   static int ad4062_triggered_buffer_predisable(struct iio_dev *indio_dev)
> >   {
> >           struct ad4062_state *st = iio_priv(indio_dev);
> > 
> >           pm_runtime_put_autosuspend(&st->i3cdev->dev);
> >           return 0;
> >   }
> > 
> > Monitor mode:
> > 
> >   static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
> >   {
> >           int ret = 0;
> > 
> >           if (!enable) {
> >                   pm_runtime_put_autosuspend(&st->i3cdev->dev);
> >                   return 0;
> >           }
> > 
> >           ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> >           ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> >           if (ret)
> >                   return ret;
> > 
> > 	  // [ Some code ]
> > 
> >           pm_runtime_get_noresume(&st->i3cdev->dev);
> >           return 0;
> >   }  
>

[PATCH 1/7] dt-bindings: iio: adc: Add adi,ad4062
[PATCH 2/7] docs: iio: New docs for ad4062 driver
[PATCH 3/7] iio: adc: Add support for ad4062
[PATCH 4/7] docs: iio: ad4062: Add IIO Trigger support
[PATCH 5/7] iio: adc: ad4062: Add IIO Trigger support
[PATCH 6/7] docs: iio: ad4062: Add IIO Events support
[PATCH 7/7] iio: adc: ad4062: Add IIO Events support