The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
register (SAR) analog-to-digital converter (ADC) with low-power and
threshold monitoring modes.
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 | 881 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 894 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8fc28b789d639..003f51cfb0d07 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1438,6 +1438,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 58da8255525e4..e506dbe83f488 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 7cc8f9a12f763..a897252eeed40 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 0000000000000..6866393ffef8d
--- /dev/null
+++ b/drivers/iio/adc/ad4062.c
@@ -0,0 +1,881 @@
+// 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>
+#include <linux/util_macros.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_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_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_1 GENMASK(6, 4)
+#define AD4062_REG_INTR_CONF 0x25
+#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_MON_VAL 0x2F
+#define AD4062_REG_ADC_IBI_EN 0x31
+#define AD4062_REG_ADC_IBI_EN_CONV_TRIGGER BIT(2)
+#define AD4062_REG_FUSE_CRC 0x40
+#define AD4062_REG_DEVICE_STATUS 0x41
+#define AD4062_REG_DEVICE_STATUS_DEVICE_RESET BIT(6)
+#define AD4062_REG_IBI_STATUS 0x48
+#define AD4062_REG_CONV_READ_LSB 0x50
+#define AD4062_REG_CONV_TRIGGER 0x59
+#define AD4062_REG_CONV_AUTO 0x61
+#define AD4062_MAX_REG AD4062_REG_CONV_AUTO
+
+#define AD4062_I3C_VENDOR 0x0177
+
+#define AD4062_SOFT_RESET 0x81
+#define AD4060_MAX_AVG 0x7
+#define AD4062_MAX_AVG 0xB
+#define AD4062_MON_VAL_MAX_GAIN 1999970
+#define AD4062_MON_VAL_MIDDLE_POINT 0x8000
+#define AD4062_GP_DRDY 0x2
+#define AD4062_INTR_EN_NEITHER 0x0
+#define AD4062_TCONV_NS 270
+
+enum ad4062_operation_mode {
+ AD4062_SAMPLE_MODE = 0x0,
+ AD4062_BURST_AVERAGING_MODE = 0x1,
+ AD4062_MONITOR_MODE = 0x3,
+};
+
+struct ad4062_chip_info {
+ const struct iio_chan_spec channels[1];
+ const char *name;
+ u16 prod_id;
+ u8 max_avg;
+};
+
+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,
+ },
+};
+
+static const int 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 */
+};
+
+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;
+ int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
+ u8 oversamp_ratio;
+ union {
+ __be32 be32;
+ __be16 be16;
+ u8 bytes[4];
+ } buf __aligned(IIO_DMA_MINALIGN);
+ u8 reg_addr_conv;
+};
+
+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 int ad4062_conversion_frequency_set(struct ad4062_state *st, u8 val)
+{
+ return regmap_write(st->regmap, AD4062_REG_TIMER_CONFIG,
+ FIELD_PREP(AD4062_REG_TIMER_CONFIG_FS_MASK, val));
+}
+
+#define AD4062_CHAN(bits) { \
+ .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_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+ .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+ .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), \
+}
+
+static const struct ad4062_chip_info ad4060_chip_info = {
+ .name = "ad4060",
+ .channels = { AD4062_CHAN(12) },
+ .prod_id = 0x7A,
+ .max_avg = AD4060_MAX_AVG,
+};
+
+static const struct ad4062_chip_info ad4062_chip_info = {
+ .name = "ad4062",
+ .channels = { AD4062_CHAN(16) },
+ .prod_id = 0x7C,
+ .max_avg = AD4062_MAX_AVG,
+};
+
+static int ad4062_set_oversampling_ratio(struct ad4062_state *st, unsigned int val)
+{
+ int ret;
+
+ if (val < 1 || val > BIT(st->chip->max_avg + 1))
+ return -EINVAL;
+
+ /* 1 disables oversampling */
+ val = ilog2(val);
+ if (val == 0) {
+ st->mode = AD4062_SAMPLE_MODE;
+ } else {
+ st->mode = AD4062_BURST_AVERAGING_MODE;
+ ret = regmap_write(st->regmap, AD4062_REG_AVG_CONFIG, val - 1);
+ if (ret)
+ return ret;
+ }
+ st->oversamp_ratio = BIT(val);
+
+ 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);
+ return 0;
+}
+
+static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
+{
+ /* See datasheet page 31 */
+ u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
+
+ return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
+}
+
+static int ad4062_populate_sampling_frequency(struct ad4062_state *st)
+{
+ for (int i = 0; i < ARRAY_SIZE(ad4062_conversion_freqs); i++)
+ st->samp_freqs[i] = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
+ st->oversamp_ratio);
+ return 0;
+}
+
+static int ad4062_get_sampling_frequency(struct ad4062_state *st, int *val)
+{
+ *val = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[st->sampling_frequency],
+ st->oversamp_ratio);
+ return 0;
+}
+
+static int ad4062_set_sampling_frequency(struct ad4062_state *st, int val)
+{
+ int ret;
+
+ ret = ad4062_populate_sampling_frequency(st);
+ if (ret)
+ return ret;
+
+ st->sampling_frequency = find_closest_descending(val, st->samp_freqs,
+ ARRAY_SIZE(ad4062_conversion_freqs));
+ 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->buf.be16, sizeof(st->buf.be16));
+ if (ret)
+ return ret;
+
+ val = get_unaligned_be16(st->buf.bytes);
+ 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->buf.be16, sizeof(st->buf.be16));
+ if (ret)
+ return ret;
+
+ val = get_unaligned_be16(st->buf.bytes);
+ 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 = AD4062_SOFT_RESET;
+ 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;
+ u8 val;
+
+ scan_type = iio_get_current_scan_type(indio_dev, chan);
+ if (IS_ERR(scan_type))
+ return PTR_ERR(scan_type);
+
+ val = 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, val);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
+ 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;
+
+ val = FIELD_PREP(AD4062_REG_INTR_CONF_EN_MSK_1, AD4062_INTR_EN_NEITHER);
+ ret = regmap_update_bits(st->regmap, AD4062_REG_INTR_CONF,
+ AD4062_REG_INTR_CONF_EN_MSK_1, val);
+ if (ret)
+ return ret;
+
+ put_unaligned_be16(AD4062_MON_VAL_MIDDLE_POINT, st->buf.bytes);
+ return regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
+ &st->buf.be16, sizeof(st->buf.be16));
+}
+
+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 void ad4062_remove_ibi(void *data)
+{
+ struct i3c_device *i3cdev = data;
+
+ i3c_device_disable_ibi(i3cdev);
+ i3c_device_free_ibi(i3cdev);
+}
+
+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 devm_add_action_or_reset(&i3cdev->dev, ad4062_remove_ibi, i3cdev);
+
+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 == -EPROBE_DEFER) {
+ return ret;
+ } else if (ret < 0) {
+ 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);
+ } else {
+ ret = devm_request_threaded_irq(dev, ret,
+ ad4062_irq_handler_drdy,
+ NULL, IRQF_ONESHOT, indio_dev->name,
+ indio_dev);
+ }
+
+ 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)
+{
+ struct ad4062_state *st = iio_priv(indio_dev);
+ int ret;
+
+ 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;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ ret = ad4062_populate_sampling_frequency(st);
+ if (ret)
+ return ret;
+ *vals = st->samp_freqs;
+ *len = st->oversamp_ratio != 1 ? ARRAY_SIZE(ad4062_conversion_freqs) : 1;
+ *type = IIO_VAL_INT;
+
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad4062_get_chan_scale(struct iio_dev *indio_dev, 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 - 1; /* signed */
+
+ return IIO_VAL_FRACTIONAL_LOG2;
+}
+
+static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
+{
+ u16 gain;
+ int ret;
+
+ ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
+ &st->buf.be16, sizeof(st->buf.be16));
+ if (ret)
+ return ret;
+
+ gain = get_unaligned_be16(st->buf.bytes);
+
+ /* 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 ad4062_state *st, int gain_int, int gain_frac)
+{
+ 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->buf.bytes);
+
+ ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
+ &st->buf.be16, sizeof(st->buf.be16));
+ if (ret)
+ return ret;
+
+ /* Enable scale if gain is not one. */
+ return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
+ 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;
+ struct i3c_priv_xfer t[2] = {
+ {
+ .data.out = &st->reg_addr_conv,
+ .len = sizeof(st->reg_addr_conv),
+ .rnw = false,
+ },
+ {
+ .data.in = &st->buf.be32,
+ .len = sizeof(st->buf.be32),
+ .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->buf.bytes);
+ return 0;
+}
+
+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);
+}
+
+static int ad4062_read_raw_dispatch(struct ad4062_state *st, int *val, int *val2,
+ long info)
+{
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ return ad4062_read_chan_raw(st, val);
+
+ case IIO_CHAN_INFO_CALIBSCALE:
+ return ad4062_get_chan_calibscale(st, val, val2);
+
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ return ad4062_get_oversampling_ratio(st, val);
+
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return ad4062_get_sampling_frequency(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)
+{
+ struct ad4062_state *st = iio_priv(indio_dev);
+ int ret;
+
+ if (info == IIO_CHAN_INFO_SCALE)
+ return ad4062_get_chan_scale(indio_dev, val, val2);
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = ad4062_read_raw_dispatch(st, val, val2, info);
+
+ iio_device_release_direct(indio_dev);
+ return ret ? ret : IIO_VAL_INT;
+}
+
+static int ad4062_write_raw_dispatch(struct ad4062_state *st, int val, int val2,
+ long info)
+{
+ switch (info) {
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ return ad4062_set_oversampling_ratio(st, val);
+
+ case IIO_CHAN_INFO_CALIBSCALE:
+ return ad4062_set_chan_calibscale(st, val, val2);
+
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return ad4062_set_sampling_frequency(st, val);
+
+ 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)
+{
+ struct ad4062_state *st = iio_priv(indio_dev);
+ int ret;
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = ad4062_write_raw_dispatch(st, 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 (readval)
+ ret = regmap_read(st->regmap, reg, readval);
+ else
+ ret = regmap_write(st->regmap, reg, writeval);
+
+ 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 ret;
+
+ ret = devm_regulator_get_enable(dev, "vio");
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to enable vio voltage\n");
+
+ st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
+ *ref_sel = st->vref_uv == -ENODEV;
+ if (st->vref_uv < 0 && st->vref_uv != -ENODEV) {
+ return dev_err_probe(dev, st->vref_uv,
+ "Failed to enable and read ref voltage\n");
+ } else if (st->vref_uv == -ENODEV) {
+ st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
+ if (st->vref_uv < 0)
+ return dev_err_probe(dev, st->vref_uv,
+ "Failed to enable and read vdd voltage\n");
+ } else {
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to enable vdd regulator\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 = 0;
+ st->oversamp_ratio = BIT(0);
+ st->indio_dev = indio_dev;
+ st->reg_addr_conv = AD4062_REG_CONV_TRIGGER;
+
+ 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 ret;
+
+ 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 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);
+ if (ret)
+ return ret;
+
+ fsleep(4000);
+ return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_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,
+ .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.51.1
On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
> The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> register (SAR) analog-to-digital converter (ADC) with low-power and
> threshold monitoring modes.
...
> +#define AD4062_SOFT_RESET 0x81
The grouping seems a bit strange. Haven't you forgotten a blank line here?
Ditto for other similar cases.
> +#define AD4060_MAX_AVG 0x7
> +#define AD4062_MAX_AVG 0xB
> +#define AD4062_MON_VAL_MAX_GAIN 1999970
This is decimal...
> +#define AD4062_MON_VAL_MIDDLE_POINT 0x8000
...and this is hexadecimal. Can you make these consistent?
Also, is there any explanation of the number above? To me
it looks like 2000000 - 30. Is it so? Or is this a fraction
number multiplied by 1000000 or so? In any case some elaboration
would be good to have.
> +#define AD4062_GP_DRDY 0x2
> +#define AD4062_INTR_EN_NEITHER 0x0
> +#define AD4062_TCONV_NS 270
...
> +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;
> + int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
> + u8 oversamp_ratio;
> + union {
> + __be32 be32;
> + __be16 be16;
> + u8 bytes[4];
> + } buf __aligned(IIO_DMA_MINALIGN);
> + u8 reg_addr_conv;
Can't we group u8:s to save a few bytes of memory?
> +};
...
> +static int ad4062_set_oversampling_ratio(struct ad4062_state *st, unsigned int val)
> +{
> + int ret;
> +
> + if (val < 1 || val > BIT(st->chip->max_avg + 1))
in_range() ?
in_range(val, 1, GENMASK(st->chip->max_avg, 0))
if I am not mistaken. Also note, the GENMASK() approach makes possible
to have all 32 bits set, however it's most unlikely to happen here anyway.
> + return -EINVAL;
> +
> + /* 1 disables oversampling */
> + val = ilog2(val);
> + if (val == 0) {
> + st->mode = AD4062_SAMPLE_MODE;
> + } else {
> + st->mode = AD4062_BURST_AVERAGING_MODE;
> + ret = regmap_write(st->regmap, AD4062_REG_AVG_CONFIG, val - 1);
> + if (ret)
> + return ret;
> + }
> + st->oversamp_ratio = BIT(val);
> +
> + 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);
> + return 0;
This is strange piece of code. Why do we have ret at all?
Please, try to compile kernel also with `make LLVM=1 W=1 ...`
assuming you have clang installed. It catches such issues quite
well.
> +}
...
> +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> +{
> + /* See datasheet page 31 */
> + u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> +
> + return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
Why u64?
The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
more than 4 billions?
> +}
> +
> +static int ad4062_populate_sampling_frequency(struct ad4062_state *st)
> +{
> + for (int i = 0; i < ARRAY_SIZE(ad4062_conversion_freqs); i++)
Why signed iterator?
> + st->samp_freqs[i] = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
> + st->oversamp_ratio);
Perhaps
st->samp_freqs[i] =
ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
st->oversamp_ratio);
But I am not insisting on this case and similar.
> + return 0;
> +}
> +static int ad4062_get_sampling_frequency(struct ad4062_state *st, int *val)
> +{
> + *val = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[st->sampling_frequency],
> + st->oversamp_ratio);
Oh, temporary variable makes this better for readability.
> + return 0;
> +}
...
> +static int ad4062_check_ids(struct ad4062_state *st)
> +{
struct device *dev = &st->i3cdev->dev;
> + int ret;
> + u16 val;
> +
> + ret = regmap_bulk_read(st->regmap, AD4062_REG_PROD_ID_1,
> + &st->buf.be16, sizeof(st->buf.be16));
> + if (ret)
> + return ret;
> +
> + val = get_unaligned_be16(st->buf.bytes);
> + if (val != st->chip->prod_id)
> + dev_warn(&st->i3cdev->dev,
> + "Production ID x%x does not match known values", val);
dev_warn(dev, "Production ID x%x does not match known values", val);
> + ret = regmap_bulk_read(st->regmap, AD4062_REG_VENDOR_H,
> + &st->buf.be16, sizeof(st->buf.be16));
> + if (ret)
> + return ret;
> +
> + val = get_unaligned_be16(st->buf.bytes);
> + if (val != AD4062_I3C_VENDOR) {
> + dev_err(&st->i3cdev->dev,
> + "Vendor ID x%x does not match expected value\n", val);
dev_err(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 = AD4062_SOFT_RESET;
> + int ret;
> +
> + ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> + if (ret)
> + return ret;
> +
> + /* Wait AD4062 treset time */
> + fsleep(5000);
5 * USEC_PER_MSEC
This gives a hint on the units without even a need to comment or look somewhere
else.
> + return 0;
> +}
...
> +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 == -EPROBE_DEFER) {
> + return ret;
> + } else if (ret < 0) {
Redundant 'else'
> + 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);
> + } else {
> + ret = devm_request_threaded_irq(dev, ret,
> + ad4062_irq_handler_drdy,
> + NULL, IRQF_ONESHOT, indio_dev->name,
> + indio_dev);
> + }
> +
> + return ret;
> +}
...
> +static const int ad4062_oversampling_avail[] = {
> + 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
It's not easy to count them at glance, please add a comment with indices.
> +};
...
> +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)
> +{
> + struct ad4062_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + 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;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = ad4062_populate_sampling_frequency(st);
> + if (ret)
> + return ret;
> + *vals = st->samp_freqs;
> + *len = st->oversamp_ratio != 1 ? ARRAY_SIZE(ad4062_conversion_freqs) : 1;
Why not using positive conditional?
Funny trick that Elvis operator can be used in this case, but please don't,
it will make code harder to follow.
> + *type = IIO_VAL_INT;
> +
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int ad4062_get_chan_scale(struct iio_dev *indio_dev, 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;
It's most likely (MICRO / MILLI) instead of MILLI. Am I right?
> + *val2 = scan_type->realbits - 1; /* signed */
> +
> + return IIO_VAL_FRACTIONAL_LOG2;
> +}
...
> +static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> +{
> + u16 gain;
> + int ret;
> +
> + ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> + &st->buf.be16, sizeof(st->buf.be16));
> + if (ret)
> + return ret;
> +
> + gain = get_unaligned_be16(st->buf.bytes);
> +
> + /* 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);
I don't see the need for 64-bit division. Can you elaborate what I miss here?
> + return IIO_VAL_INT_PLUS_NANO;
> +}
...
> +static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)
Forgot to wrap this line.
> +{
> + u64 gain;
> + int ret;
> +
> + if (gain_int < 0 || gain_frac < 0)
> + return -EINVAL;
> +
> + gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> +
Redundant blank line.
> + if (gain > AD4062_MON_VAL_MAX_GAIN)
> + return -EINVAL;
> +
> + put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
> + MICRO),
> + st->buf.bytes);
Also in doubt here about 64-bit division.
> + ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> + &st->buf.be16, sizeof(st->buf.be16));
> + if (ret)
> + return ret;
> +
> + /* Enable scale if gain is not one. */
"...is not equal to one."
Also be consistent with the style for one-line comments. Choose one and
use it everywhere. Usual cases:
- my one-line comment
- My one-line comment
- My one-line comment.
> + return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> + 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)
Can be named without leading double underscore? Preference is to use
the suffix, like _no_pm (but you can find better one).
> +{
> + struct i3c_device *i3cdev = st->i3cdev;
> + struct i3c_priv_xfer t[2] = {
> + {
> + .data.out = &st->reg_addr_conv,
> + .len = sizeof(st->reg_addr_conv),
> + .rnw = false,
> + },
> + {
> + .data.in = &st->buf.be32,
> + .len = sizeof(st->buf.be32),
> + .rnw = true,
> + }
> + };
> + int ret;
> +
> + reinit_completion(&st->completion);
> + /* Change address pointer to trigger conversion */
> + ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
Why array? Just split them on per transfer and use separately. This gives a bit
odd feeling that the two goes together, but no. They are semi-related as we
have a special condition after the first one.
> + 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->buf.bytes);
> + return 0;
> +}
...
> +static int ad4062_read_raw_dispatch(struct ad4062_state *st, int *val, int *val2,
> + long info)
The parameters are split in a logical way here...
(however preference is
static int ad4062_read_raw_dispatch(struct ad4062_state *st,
int *val, int *val2, long info)
to fit 80 characters)
...
> +static int ad4062_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long info)
...but here. Why not
static int ad4062_read_raw(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);
> + int ret;
> +
> + if (info == IIO_CHAN_INFO_SCALE)
> + return ad4062_get_chan_scale(indio_dev, val, val2);
> +
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> +
> + ret = ad4062_read_raw_dispatch(st, val, val2, info);
> +
> + iio_device_release_direct(indio_dev);
> + return ret ? ret : IIO_VAL_INT;
return ret ?: IIO_VAL_INT;
> +}
...
> +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 (readval)
> + ret = regmap_read(st->regmap, reg, readval);
> + else
> + ret = regmap_write(st->regmap, reg, writeval);
> +
> + return ret;
Do you expand this in the following patches? If not, ret is not needed.
Just return directly.
> +}
...
> +static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
> +{
> + struct device *dev = &st->i3cdev->dev;
> + int ret;
> +
> + ret = devm_regulator_get_enable(dev, "vio");
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable vio voltage\n");
> +
> + st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> + *ref_sel = st->vref_uv == -ENODEV;
_uV ?
> + if (st->vref_uv < 0 && st->vref_uv != -ENODEV) {
You already has the second part
if (st->vref_uV < 0 && !*ref_sel) {
I believe this is better to understand as we check that ref_sel is not chosen.
> + return dev_err_probe(dev, st->vref_uv,
> + "Failed to enable and read ref voltage\n");
> + } else if (st->vref_uv == -ENODEV) {
Redundant 'else'
if (*ref_sel) {
(also in similar way as above)
I don't know if the above was asked specifically, but if so, I ask
the requestor(s) to reconsider.
> + st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
> + if (st->vref_uv < 0)
> + return dev_err_probe(dev, st->vref_uv,
> + "Failed to enable and read vdd voltage\n");
> + } else {
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable vdd regulator\n");
> + }
> +
> + return 0;
> +}
...
> +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 (ret)
> + return ret;
> +
> + fsleep(4000);
4 * USEC_PER_MSEC, also would be good to add a comment for this long delay.
> + return 0;
> +}
...
> +static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops, ad4062_runtime_suspend,
> + ad4062_runtime_resume, NULL);
I think the logical split is slightly better:
static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops,
ad4062_runtime_suspend, ad4062_runtime_resume, NULL);
OR
static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops,
ad4062_runtime_suspend,
ad4062_runtime_resume,
NULL);
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
> > The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> > register (SAR) analog-to-digital converter (ADC) with low-power and
> > threshold monitoring modes.
>
> ...
>
Hi Andy,
> > +#define AD4062_SOFT_RESET 0x81
>
> The grouping seems a bit strange. Haven't you forgotten a blank line here?
> Ditto for other similar cases.
>
Ack.
> > +#define AD4060_MAX_AVG 0x7
> > +#define AD4062_MAX_AVG 0xB
>
> > +#define AD4062_MON_VAL_MAX_GAIN 1999970
>
> This is decimal...
>
> > +#define AD4062_MON_VAL_MIDDLE_POINT 0x8000
>
> ...and this is hexadecimal. Can you make these consistent?
> Also, is there any explanation of the number above? To me
> it looks like 2000000 - 30. Is it so? Or is this a fraction
> number multiplied by 1000000 or so? In any case some elaboration
> would be good to have.
>
Since this is not a magic number, I will use directly below.
It MAX_MON_VAL/MON_VAL_MIDDLE_POINT = 0xFFFF/0x8000
> > +#define AD4062_GP_DRDY 0x2
> > +#define AD4062_INTR_EN_NEITHER 0x0
> > +#define AD4062_TCONV_NS 270
>
> ...
>
> > +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;
> > + int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
> > + u8 oversamp_ratio;
> > + union {
> > + __be32 be32;
> > + __be16 be16;
> > + u8 bytes[4];
> > + } buf __aligned(IIO_DMA_MINALIGN);
> > + u8 reg_addr_conv;
>
> Can't we group u8:s to save a few bytes of memory?
>
Sure
struct ad4062_state {
// ...
union {
__be32 be32;
__be16 be16;
u8 bytes[4];
} buf __aligned(IIO_DMA_MINALIGN);
u16 sampling_frequency;
u8 oversamp_ratio;
u8 reg_addr_conv;
};
> > +};
>
> ...
>
> > +static int ad4062_set_oversampling_ratio(struct ad4062_state *st, unsigned int val)
> > +{
> > + int ret;
> > +
> > + if (val < 1 || val > BIT(st->chip->max_avg + 1))
>
> in_range() ?
>
> in_range(val, 1, GENMASK(st->chip->max_avg, 0))
>
> if I am not mistaken. Also note, the GENMASK() approach makes possible
> to have all 32 bits set, however it's most unlikely to happen here anyway.
>
Sure, but requires locals to not trigger suspicious usage of sizeof.
// ...
const u32 _max = GENMASK(st->chip->max_avg, 0);
const u32 _min = 1;
int ret;
if (in_range(val, _min, _max))
> > + return -EINVAL;
> > +
> > + /* 1 disables oversampling */
> > + val = ilog2(val);
> > + if (val == 0) {
> > + st->mode = AD4062_SAMPLE_MODE;
> > + } else {
> > + st->mode = AD4062_BURST_AVERAGING_MODE;
> > + ret = regmap_write(st->regmap, AD4062_REG_AVG_CONFIG, val - 1);
> > + if (ret)
> > + return ret;
> > + }
> > + st->oversamp_ratio = BIT(val);
> > +
> > + 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);
> > + return 0;
>
> This is strange piece of code. Why do we have ret at all?
> Please, try to compile kernel also with `make LLVM=1 W=1 ...`
> assuming you have clang installed. It catches such issues quite
> well.
>
Can return directly yes.
> > +}
>
> ...
>
> > +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> > +{
> > + /* See datasheet page 31 */
> > + u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> > +
> > + return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
>
> Why u64?
>
> The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
> more than 4 billions?
>
This is necessary since at fosc 111 Hz and avg 4096 it does take longer
than 4 seconds, even though I do timeout after 1 seconds in the raw
acquisition.
> > +}
> > +
> > +static int ad4062_populate_sampling_frequency(struct ad4062_state *st)
> > +{
> > + for (int i = 0; i < ARRAY_SIZE(ad4062_conversion_freqs); i++)
>
> Why signed iterator?
>
Ack, can be u8.
> > + st->samp_freqs[i] = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
> > + st->oversamp_ratio);
>
> Perhaps
>
> st->samp_freqs[i] =
> ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
> st->oversamp_ratio);
>
> But I am not insisting on this case and similar.
>
>
Ack.
> > + return 0;
> > +}
>
> > +static int ad4062_get_sampling_frequency(struct ad4062_state *st, int *val)
> > +{
> > + *val = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[st->sampling_frequency],
> > + st->oversamp_ratio);
>
> Oh, temporary variable makes this better for readability.
>
Ack.
> > + return 0;
> > +}
>
> ...
>
> > +static int ad4062_check_ids(struct ad4062_state *st)
> > +{
>
> struct device *dev = &st->i3cdev->dev;
>
Ack.
> > + int ret;
> > + u16 val;
> > +
> > + ret = regmap_bulk_read(st->regmap, AD4062_REG_PROD_ID_1,
> > + &st->buf.be16, sizeof(st->buf.be16));
> > + if (ret)
> > + return ret;
> > +
> > + val = get_unaligned_be16(st->buf.bytes);
> > + if (val != st->chip->prod_id)
> > + dev_warn(&st->i3cdev->dev,
> > + "Production ID x%x does not match known values", val);
>
> dev_warn(dev, "Production ID x%x does not match known values", val);
>
Ack.
> > + ret = regmap_bulk_read(st->regmap, AD4062_REG_VENDOR_H,
> > + &st->buf.be16, sizeof(st->buf.be16));
> > + if (ret)
> > + return ret;
> > +
> > + val = get_unaligned_be16(st->buf.bytes);
> > + if (val != AD4062_I3C_VENDOR) {
> > + dev_err(&st->i3cdev->dev,
> > + "Vendor ID x%x does not match expected value\n", val);
>
> dev_err(dev, "Vendor ID x%x does not match expected value\n", val);
>
Ack.
> > + return -ENODEV;
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int ad4062_soft_reset(struct ad4062_state *st)
> > +{
> > + u8 val = AD4062_SOFT_RESET;
> > + int ret;
> > +
> > + ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> > + if (ret)
> > + return ret;
> > +
> > + /* Wait AD4062 treset time */
> > + fsleep(5000);
>
> 5 * USEC_PER_MSEC
>
> This gives a hint on the units without even a need to comment or look somewhere
> else.
>
// TODO
Since the device functional blocks are powered when voltage is supplied,
here we can stick with the treset datasheet value 60ns (ndelay(60)).
> > + return 0;
> > +}
>
> ...
>
> > +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 == -EPROBE_DEFER) {
> > + return ret;
>
> > + } else if (ret < 0) {
>
> Redundant 'else'
>
Ack.
> > + 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);
> > + } else {
> > + ret = devm_request_threaded_irq(dev, ret,
> > + ad4062_irq_handler_drdy,
> > + NULL, IRQF_ONESHOT, indio_dev->name,
> > + indio_dev);
> > + }
> > +
> > + return ret;
> > +}
>
> ...
>
> > +static const int ad4062_oversampling_avail[] = {
> > + 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
>
> It's not easy to count them at glance, please add a comment with indices.
>
Ack, will use
static const int ad4062_oversampling_avail[] = {
BIT(0), BIT(1), BIT(2), BIT(3), BIT(4), BIT(5), BIT(6), BIT(7), BIT(8),
BIT(9), BIT(10), BIT(11), BIT(12),
};
> > +};
>
> ...
>
> > +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)
> > +{
> > + struct ad4062_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + 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;
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + ret = ad4062_populate_sampling_frequency(st);
> > + if (ret)
> > + return ret;
> > + *vals = st->samp_freqs;
> > + *len = st->oversamp_ratio != 1 ? ARRAY_SIZE(ad4062_conversion_freqs) : 1;
>
> Why not using positive conditional?
>
> Funny trick that Elvis operator can be used in this case, but please don't,
> it will make code harder to follow.
>
Ack.
> > + *type = IIO_VAL_INT;
> > +
> > + return IIO_AVAIL_LIST;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
>
> ...
>
> > +static int ad4062_get_chan_scale(struct iio_dev *indio_dev, 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;
>
> It's most likely (MICRO / MILLI) instead of MILLI. Am I right?
>
Sure.
> > + *val2 = scan_type->realbits - 1; /* signed */
> > +
> > + return IIO_VAL_FRACTIONAL_LOG2;
> > +}
>
> ...
>
> > +static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> > +{
> > + u16 gain;
> > + int ret;
> > +
> > + ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> > + &st->buf.be16, sizeof(st->buf.be16));
> > + if (ret)
> > + return ret;
> > +
> > + gain = get_unaligned_be16(st->buf.bytes);
> > +
> > + /* 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);
>
> I don't see the need for 64-bit division. Can you elaborate what I miss here?
>
> > + return IIO_VAL_INT_PLUS_NANO;
> > +}
>
Can be improved to
static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
{
int ret;
ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
&st->buf.be16, sizeof(st->buf.be16));
if (ret)
return ret;
/* From datasheet: code out = code in × mon_val/0x8000 */
*val = get_unaligned_be16(st->buf.bytes) * 2;
*val2 = 16;
return IIO_VAL_FRACTIONAL_LOG2;
}
> ...
>
> > +static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)
>
> Forgot to wrap this line.
>
ack
> > +{
> > + u64 gain;
> > + int ret;
> > +
> > + if (gain_int < 0 || gain_frac < 0)
> > + return -EINVAL;
> > +
> > + gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
>
> > +
>
> Redundant blank line.
>
Ack.
> > + if (gain > AD4062_MON_VAL_MAX_GAIN)
> > + return -EINVAL;
> > +
> > + put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
> > + MICRO),
> > + st->buf.bytes);
>
> Also in doubt here about 64-bit division.
>
This can be slightly improved to
static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
int gain_frac)
{
u32 gain;
int ret;
if (gain_int < 0 || gain_frac < 0)
return -EINVAL;
gain = gain_int * MICRO + gain_frac;
if (gain > 1999970)
return -EINVAL;
put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
MICRO),
st->buf.bytes);
ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
&st->buf.be16, sizeof(st->buf.be16));
if (ret)
return ret;
/* Enable scale if gain is not equal to one */
return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
!(gain_int == 1 && gain_frac == 0)));
}
To provide the enough resolution to compute every step (e.g., 0xFFFF and
0xFFFE) with the arbitrary user input.
> > + ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> > + &st->buf.be16, sizeof(st->buf.be16));
> > + if (ret)
> > + return ret;
> > +
> > + /* Enable scale if gain is not one. */
>
> "...is not equal to one."
>
> Also be consistent with the style for one-line comments. Choose one and
> use it everywhere. Usual cases:
> - my one-line comment
> - My one-line comment
> - My one-line comment.
>
Ack.
>
> > + return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> > + 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)
>
> Can be named without leading double underscore? Preference is to use
> the suffix, like _no_pm (but you can find better one).
>
Since there is one usage of this method, can be merged into ad4062_read_chan_raw.
> > +{
> > + struct i3c_device *i3cdev = st->i3cdev;
> > + struct i3c_priv_xfer t[2] = {
> > + {
> > + .data.out = &st->reg_addr_conv,
> > + .len = sizeof(st->reg_addr_conv),
> > + .rnw = false,
> > + },
> > + {
> > + .data.in = &st->buf.be32,
> > + .len = sizeof(st->buf.be32),
> > + .rnw = true,
> > + }
> > + };
> > + int ret;
> > +
> > + reinit_completion(&st->completion);
> > + /* Change address pointer to trigger conversion */
> > + ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
>
> Why array? Just split them on per transfer and use separately. This gives a bit
> odd feeling that the two goes together, but no. They are semi-related as we
> have a special condition after the first one.
>
For this commit sure, but in the next a fallback method is introduced
for when the gp1 gpio line is not connected.
There are two register to trigger and read samples:
* write CONV_READ -> read dummy value - [conversion] -> read value -> [conv ...
* write CONV_TRIGGER - [conversion] -> read value -> write ...
The first allows almost twice the sampling frequency, but does not work
with the fallback because In-Band-Interrupt for CONV_READ are not
yielded.
> > + 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->buf.bytes);
> > + return 0;
> > +}
>
> ...
>
> > +static int ad4062_read_raw_dispatch(struct ad4062_state *st, int *val, int *val2,
> > + long info)
>
> The parameters are split in a logical way here...
>
> (however preference is
>
> static int ad4062_read_raw_dispatch(struct ad4062_state *st,
> int *val, int *val2, long info)
>
> to fit 80 characters)
>
> ...
Ack.
>
> > +static int ad4062_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int *val,
> > + int *val2, long info)
>
> ...but here. Why not
>
> static int ad4062_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long info)
>
> ?
>
Ack.
> > + struct ad4062_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + if (info == IIO_CHAN_INFO_SCALE)
> > + return ad4062_get_chan_scale(indio_dev, val, val2);
> > +
> > + if (!iio_device_claim_direct(indio_dev))
> > + return -EBUSY;
> > +
> > + ret = ad4062_read_raw_dispatch(st, val, val2, info);
> > +
> > + iio_device_release_direct(indio_dev);
> > + return ret ? ret : IIO_VAL_INT;
>
> return ret ?: IIO_VAL_INT;
>
> > +}
>
> ...
Ack.
>
> > +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 (readval)
> > + ret = regmap_read(st->regmap, reg, readval);
> > + else
> > + ret = regmap_write(st->regmap, reg, writeval);
> > +
> > + return ret;
>
> Do you expand this in the following patches? If not, ret is not needed.
> Just return directly.
>
Not anymore, will just return.
> > +}
>
> ...
>
> > +static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
> > +{
> > + struct device *dev = &st->i3cdev->dev;
> > + int ret;
> > +
> > + ret = devm_regulator_get_enable(dev, "vio");
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "Failed to enable vio voltage\n");
> > +
> > + st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> > + *ref_sel = st->vref_uv == -ENODEV;
>
> _uV ?
>
Ack.
> > + if (st->vref_uv < 0 && st->vref_uv != -ENODEV) {
>
> You already has the second part
>
> if (st->vref_uV < 0 && !*ref_sel) {
>
> I believe this is better to understand as we check that ref_sel is not chosen.
>
Ack.
> > + return dev_err_probe(dev, st->vref_uv,
> > + "Failed to enable and read ref voltage\n");
>
> > + } else if (st->vref_uv == -ENODEV) {
>
> Redundant 'else'
>
> if (*ref_sel) {
>
> (also in similar way as above)
>
> I don't know if the above was asked specifically, but if so, I ask
> the requestor(s) to reconsider.
>
Ack. Asked for returning error first, good path return 0 and not return
ret;
> > + st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
> > + if (st->vref_uv < 0)
> > + return dev_err_probe(dev, st->vref_uv,
> > + "Failed to enable and read vdd voltage\n");
> > + } else {
> > + ret = devm_regulator_get_enable(dev, "vdd");
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "Failed to enable vdd regulator\n");
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > +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 (ret)
> > + return ret;
> > +
> > + fsleep(4000);
>
> 4 * USEC_PER_MSEC, also would be good to add a comment for this long delay.
>
Will add
/* Wait device functional blocks to power up */
Based on hardware tests, I can drop to 2 * USEC_PER_MSEC, lower than
that the device is not ready to switch to acquisition mode for
conversions.
> > + return 0;
> > +}
>
> ...
>
> > +static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops, ad4062_runtime_suspend,
> > + ad4062_runtime_resume, NULL);
>
> I think the logical split is slightly better:
>
> static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops,
> ad4062_runtime_suspend, ad4062_runtime_resume, NULL);
>
> OR
Ack.
>
> static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops,
> ad4062_runtime_suspend,
> ad4062_runtime_resume,
> NULL);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Best Regards,
Jorge
On Wed, 26 Nov 2025 12:40:00 +0100
Jorge Marques <gastmaier@gmail.com> wrote:
> On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
> > > The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> > > register (SAR) analog-to-digital converter (ADC) with low-power and
> > > threshold monitoring modes.
> >
> > ...
> >
> Hi Andy,
> > > +#define AD4062_SOFT_RESET 0x81
> >
> > The grouping seems a bit strange. Haven't you forgotten a blank line here?
> > Ditto for other similar cases.
> >
> Ack.
Side note. For efficiency, if you agree with something just delete that
block of the thread and don't say so. Reviewers should be safe to assume
that anything the author agrees with will just be fixed in the next version.
That lets us focus in very fast on the key discussions.
> >
> > > +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;
> > > + int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
> > > + u8 oversamp_ratio;
> > > + union {
> > > + __be32 be32;
> > > + __be16 be16;
> > > + u8 bytes[4];
> > > + } buf __aligned(IIO_DMA_MINALIGN);
> > > + u8 reg_addr_conv;
> >
> > Can't we group u8:s to save a few bytes of memory?
> >
> Sure
>
> struct ad4062_state {
> // ...
> union {
> __be32 be32;
> __be16 be16;
> u8 bytes[4];
> } buf __aligned(IIO_DMA_MINALIGN);
> u16 sampling_frequency;
> u8 oversamp_ratio;
> u8 reg_addr_conv;
Unless my assumption is wrong and those 3 values are passed
as buffers for DMA (or otherwise very carefully protected
against access racing with the DMA buffers) then this is very wrong.
Refresh your memory on why we do the __aligned(IIO_DMA_MINALIGN) and
exactly how that works.
Short answer, nothing must come after it in the structure as it
only forces the start of the buffer, not it's end and hence the
following data ends up in the same cacheline and fun data corruption
is the result.
> };
On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
...
> > > +#define AD4062_MON_VAL_MAX_GAIN 1999970
> >
> > This is decimal...
> >
> > > +#define AD4062_MON_VAL_MIDDLE_POINT 0x8000
> >
> > ...and this is hexadecimal. Can you make these consistent?
> > Also, is there any explanation of the number above? To me
> > it looks like 2000000 - 30. Is it so? Or is this a fraction
> > number multiplied by 1000000 or so? In any case some elaboration
> > would be good to have.
> >
> Since this is not a magic number, I will use directly below.
> It MAX_MON_VAL/MON_VAL_MIDDLE_POINT = 0xFFFF/0x8000
Okay, at least it will explain the value.
...
> > > + if (val < 1 || val > BIT(st->chip->max_avg + 1))
> >
> > in_range() ?
> >
> > in_range(val, 1, GENMASK(st->chip->max_avg, 0))
> >
> > if I am not mistaken. Also note, the GENMASK() approach makes possible
> > to have all 32 bits set, however it's most unlikely to happen here anyway.
> >
> Sure, but requires locals to not trigger suspicious usage of sizeof.
> // ...
> const u32 _max = GENMASK(st->chip->max_avg, 0);
> const u32 _min = 1;
> int ret;
>
> if (in_range(val, _min, _max))
> > > + return -EINVAL;
It's fine.
...
> > > +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> > > +{
> > > + /* See datasheet page 31 */
> > > + u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> > > +
> > > + return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
> >
> > Why u64?
> >
> > The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
> > more than 4 billions?
> >
> This is necessary since at fosc 111 Hz and avg 4096 it does take longer
> than 4 seconds, even though I do timeout after 1 seconds in the raw
> acquisition.
Values above NSEC_PER_SEC+1 do not make sense (it will return 0),
and that fits u32. Can you refactor to avoid 64-bit arithmetics?
> > > +}
...
> > > +static int ad4062_soft_reset(struct ad4062_state *st)
> > > +{
> > > + u8 val = AD4062_SOFT_RESET;
> > > + int ret;
> > > +
> > > + ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Wait AD4062 treset time */
> > > + fsleep(5000);
> >
> > 5 * USEC_PER_MSEC
> >
> > This gives a hint on the units without even a need to comment or look somewhere
> > else.
> >
> // TODO
> Since the device functional blocks are powered when voltage is supplied,
> here we can stick with the treset datasheet value 60ns (ndelay(60)).
Add a comment and it will work for me, thanks!
> > > + return 0;
> > > +}
...
> > > +static const int ad4062_oversampling_avail[] = {
> > > + 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
> >
> > It's not easy to count them at glance, please add a comment with indices.
> >
> Ack, will use
> static const int ad4062_oversampling_avail[] = {
> BIT(0), BIT(1), BIT(2), BIT(3), BIT(4), BIT(5), BIT(6), BIT(7), BIT(8),
> BIT(9), BIT(10), BIT(11), BIT(12),
> };
Of course you can use bit notations, but what I meant is to have
1, 2, 4, 8, 16, 32, 64, 128, /* 0 - 7 */
256, 512, 1024, 2048, 4096, /* 8 - 12 */
(or something alike).
> > > +};
...
> > > +static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> > > +{
> > > + u16 gain;
> > > + int ret;
> > > +
> > > + ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> > > + &st->buf.be16, sizeof(st->buf.be16));
> > > + if (ret)
> > > + return ret;
> > > +
> > > + gain = get_unaligned_be16(st->buf.bytes);
> > > +
> > > + /* 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);
> >
> > I don't see the need for 64-bit division. Can you elaborate what I miss here?
> >
> > > + return IIO_VAL_INT_PLUS_NANO;
> > > +}
> >
> Can be improved to
>
> static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> {
> int ret;
>
> ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> &st->buf.be16, sizeof(st->buf.be16));
> if (ret)
> return ret;
>
> /* From datasheet: code out = code in × mon_val/0x8000 */
> *val = get_unaligned_be16(st->buf.bytes) * 2;
> *val2 = 16;
>
> return IIO_VAL_FRACTIONAL_LOG2;
> }
Much better, thanks!
...
> > > +static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)
> >
> > Forgot to wrap this line.
> >
> ack
> > > +{
> > > + u64 gain;
> > > + int ret;
> > > +
> > > + if (gain_int < 0 || gain_frac < 0)
> > > + return -EINVAL;
> > > +
> > > + gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> >
> > > +
> >
> > Redundant blank line.
> >
> Ack.
> > > + if (gain > AD4062_MON_VAL_MAX_GAIN)
> > > + return -EINVAL;
> > > +
> > > + put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
> > > + MICRO),
> > > + st->buf.bytes);
> >
> > Also in doubt here about 64-bit division.
> >
> This can be slightly improved to
>
> static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> int gain_frac)
> {
> u32 gain;
> int ret;
>
> if (gain_int < 0 || gain_frac < 0)
> return -EINVAL;
>
> gain = gain_int * MICRO + gain_frac;
> if (gain > 1999970)
But this magic should be changed to what you explained to me
(as in 0xffff/0x8000 with the proper precision, and this
can be done in 32-bit space).
Or even better
if (gain_int < 0 || gain_int > 1)
return -EINVAL;
if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
return -EINVAL;
> return -EINVAL;
>
> put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
> MICRO),
...with temporary variable at minimum.
But again, I still don't see the need for 64-bit space.
> st->buf.bytes);
>
> ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> &st->buf.be16, sizeof(st->buf.be16));
> if (ret)
> return ret;
>
> /* Enable scale if gain is not equal to one */
> return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> !(gain_int == 1 && gain_frac == 0)));
> }
>
> To provide the enough resolution to compute every step (e.g., 0xFFFF and
> 0xFFFE) with the arbitrary user input.
...
> > > +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
> >
> > Can be named without leading double underscore? Preference is to use
> > the suffix, like _no_pm (but you can find better one).
> >
> Since there is one usage of this method, can be merged into ad4062_read_chan_raw.
Good choice!
...
> > > + struct i3c_priv_xfer t[2] = {
> > > + {
> > > + .data.out = &st->reg_addr_conv,
> > > + .len = sizeof(st->reg_addr_conv),
> > > + .rnw = false,
> > > + },
> > > + {
> > > + .data.in = &st->buf.be32,
> > > + .len = sizeof(st->buf.be32),
> > > + .rnw = true,
> > > + }
> > > + };
> > > + /* Change address pointer to trigger conversion */
> > > + ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
> >
> > Why array? Just split them on per transfer and use separately. This gives a bit
> > odd feeling that the two goes together, but no. They are semi-related as we
> > have a special condition after the first one.
> >
> For this commit sure, but in the next a fallback method is introduced
> for when the gp1 gpio line is not connected.
> There are two register to trigger and read samples:
>
> * write CONV_READ -> read dummy value - [conversion] -> read value -> [conv ...
> * write CONV_TRIGGER - [conversion] -> read value -> write ...
>
> The first allows almost twice the sampling frequency, but does not work
> with the fallback because In-Band-Interrupt for CONV_READ are not
> yielded.
Do you mean that the same array is reused differently? If so, then okay.
> > > + if (ret)
> > > + return ret;
...
> > > + fsleep(4000);
> >
> > 4 * USEC_PER_MSEC, also would be good to add a comment for this long delay.
> >
> Will add
> /* Wait device functional blocks to power up */
> Based on hardware tests, I can drop to 2 * USEC_PER_MSEC, lower than
> that the device is not ready to switch to acquisition mode for
> conversions.
Good!
--
With Best Regards,
Andy Shevchenko
On Thu, Nov 27, 2025 at 10:58:24AM +0200, Andy Shevchenko wrote:
> On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> > On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
>
> ...
>
> > > > +#define AD4062_MON_VAL_MAX_GAIN 1999970
> > >
> > > This is decimal...
> > >
> > > > +#define AD4062_MON_VAL_MIDDLE_POINT 0x8000
> > >
> > > ...and this is hexadecimal. Can you make these consistent?
> > > Also, is there any explanation of the number above? To me
> > > it looks like 2000000 - 30. Is it so? Or is this a fraction
> > > number multiplied by 1000000 or so? In any case some elaboration
> > > would be good to have.
> > >
> > Since this is not a magic number, I will use directly below.
> > It MAX_MON_VAL/MON_VAL_MIDDLE_POINT = 0xFFFF/0x8000
>
> Okay, at least it will explain the value.
>
> ...
>
> > > > + if (val < 1 || val > BIT(st->chip->max_avg + 1))
> > >
> > > in_range() ?
> > >
> > > in_range(val, 1, GENMASK(st->chip->max_avg, 0))
> > >
> > > if I am not mistaken. Also note, the GENMASK() approach makes possible
> > > to have all 32 bits set, however it's most unlikely to happen here anyway.
> > >
> > Sure, but requires locals to not trigger suspicious usage of sizeof.
> > // ...
> > const u32 _max = GENMASK(st->chip->max_avg, 0);
> > const u32 _min = 1;
> > int ret;
> >
> > if (in_range(val, _min, _max))
> > > > + return -EINVAL;
>
> It's fine.
>
> ...
>
> > > > +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> > > > +{
> > > > + /* See datasheet page 31 */
> > > > + u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> > > > +
> > > > + return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
> > >
> > > Why u64?
> > >
> > > The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
> > > more than 4 billions?
> > >
> > This is necessary since at fosc 111 Hz and avg 4096 it does take longer
> > than 4 seconds, even though I do timeout after 1 seconds in the raw
> > acquisition.
>
> Values above NSEC_PER_SEC+1 do not make sense (it will return 0),
> and that fits u32. Can you refactor to avoid 64-bit arithmetics?
>
Ok, any frequency lower than 1 Hz does not make sense.
static int ad4062_calc_sampling_frequency(int fosc, unsigned int oversamp_ratio)
{
/* See datasheet page 31 */
u32 period = NSEC_PER_SEC / fosc;
u32 n_avg = BIT(oversamp_ratio) - 1;
/* Result is less than 1 Hz */
if (n_avg >= fosc)
return 1;
return NSEC_PER_SEC / (n_avg * period + AD4062_TCONV_NS);
}
> > > > +}
>
> ...
>
> > > > +static int ad4062_soft_reset(struct ad4062_state *st)
> > > > +{
> > > > + u8 val = AD4062_SOFT_RESET;
> > > > + int ret;
> > > > +
> > > > + ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + /* Wait AD4062 treset time */
> > > > + fsleep(5000);
> > >
> > > 5 * USEC_PER_MSEC
> > >
> > > This gives a hint on the units without even a need to comment or look somewhere
> > > else.
> > >
> > // TODO
> > Since the device functional blocks are powered when voltage is supplied,
> > here we can stick with the treset datasheet value 60ns (ndelay(60)).
>
> Add a comment and it will work for me, thanks!
>
> > > > + return 0;
> > > > +}
>
> ...
>
> > > > +static const int ad4062_oversampling_avail[] = {
> > > > + 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
> > >
> > > It's not easy to count them at glance, please add a comment with indices.
> > >
> > Ack, will use
> > static const int ad4062_oversampling_avail[] = {
> > BIT(0), BIT(1), BIT(2), BIT(3), BIT(4), BIT(5), BIT(6), BIT(7), BIT(8),
> > BIT(9), BIT(10), BIT(11), BIT(12),
> > };
>
> Of course you can use bit notations, but what I meant is to have
>
> 1, 2, 4, 8, 16, 32, 64, 128, /* 0 - 7 */
> 256, 512, 1024, 2048, 4096, /* 8 - 12 */
>
> (or something alike).
>
Ack
> > > > +};
>
> ...
>
> > > > +static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> > > > +{
> > > > + u16 gain;
> > > > + int ret;
> > > > +
> > > > + ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> > > > + &st->buf.be16, sizeof(st->buf.be16));
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + gain = get_unaligned_be16(st->buf.bytes);
> > > > +
> > > > + /* 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);
> > >
> > > I don't see the need for 64-bit division. Can you elaborate what I miss here?
> > >
> > > > + return IIO_VAL_INT_PLUS_NANO;
> > > > +}
> > >
> > Can be improved to
> >
> > static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> > {
> > int ret;
> >
> > ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> > &st->buf.be16, sizeof(st->buf.be16));
> > if (ret)
> > return ret;
> >
> > /* From datasheet: code out = code in × mon_val/0x8000 */
> > *val = get_unaligned_be16(st->buf.bytes) * 2;
> > *val2 = 16;
> >
> > return IIO_VAL_FRACTIONAL_LOG2;
> > }
>
> Much better, thanks!
>
> ...
>
> > > > +static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)
> > >
> > > Forgot to wrap this line.
> > >
> > ack
> > > > +{
> > > > + u64 gain;
> > > > + int ret;
> > > > +
> > > > + if (gain_int < 0 || gain_frac < 0)
> > > > + return -EINVAL;
> > > > +
> > > > + gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> > >
> > > > +
> > >
> > > Redundant blank line.
> > >
> > Ack.
> > > > + if (gain > AD4062_MON_VAL_MAX_GAIN)
> > > > + return -EINVAL;
> > > > +
> > > > + put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
> > > > + MICRO),
> > > > + st->buf.bytes);
> > >
> > > Also in doubt here about 64-bit division.
> > >
> > This can be slightly improved to
> >
> > static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> > int gain_frac)
> > {
> > u32 gain;
> > int ret;
> >
> > if (gain_int < 0 || gain_frac < 0)
> > return -EINVAL;
> >
> > gain = gain_int * MICRO + gain_frac;
> > if (gain > 1999970)
>
> But this magic should be changed to what you explained to me
> (as in 0xffff/0x8000 with the proper precision, and this
> can be done in 32-bit space).
>
> Or even better
>
> if (gain_int < 0 || gain_int > 1)
> return -EINVAL;
>
> if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
> return -EINVAL;
>
gain_frac would be 999999 max, or 999970 for the limit that fits in the
register after the math. I think > 1.999.970 is self explanatory.
> > return -EINVAL;
> >
> > put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
> > MICRO),
>
> ...with temporary variable at minimum.
>
> But again, I still don't see the need for 64-bit space.
>
Well, by dividing mon_val and micro values by a common divisor the
operation fit in 32-bits:
static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
int gain_frac)
{
const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
const u32 micro = MICRO / 64;
const u32 gain = gain_int * MICRO + gain_frac;
int ret;
if (gain_int < 0 || gain_frac < 0)
return -EINVAL;
if (gain > 1999970)
return -EINVAL;
put_unaligned_be16(DIV_ROUND_CLOSEST(gain * mon_val, micro), st->buf.bytes);
ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
&st->buf.be16, sizeof(st->buf.be16));
if (ret)
return ret;
/* Enable scale if gain is not equal to one */
return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
!(gain_int == 1 && gain_frac == 0)));
}
> > st->buf.bytes);
> >
> > ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> > &st->buf.be16, sizeof(st->buf.be16));
> > if (ret)
> > return ret;
> >
> > /* Enable scale if gain is not equal to one */
> > return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> > AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > !(gain_int == 1 && gain_frac == 0)));
> > }
> >
> > To provide the enough resolution to compute every step (e.g., 0xFFFF and
> > 0xFFFE) with the arbitrary user input.
>
> ...
>
> > > > +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
> > >
> > > Can be named without leading double underscore? Preference is to use
> > > the suffix, like _no_pm (but you can find better one).
> > >
> > Since there is one usage of this method, can be merged into ad4062_read_chan_raw.
>
> Good choice!
>
> ...
>
> > > > + struct i3c_priv_xfer t[2] = {
> > > > + {
> > > > + .data.out = &st->reg_addr_conv,
> > > > + .len = sizeof(st->reg_addr_conv),
> > > > + .rnw = false,
> > > > + },
> > > > + {
> > > > + .data.in = &st->buf.be32,
> > > > + .len = sizeof(st->buf.be32),
> > > > + .rnw = true,
> > > > + }
> > > > + };
>
> > > > + /* Change address pointer to trigger conversion */
> > > > + ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
> > >
> > > Why array? Just split them on per transfer and use separately. This gives a bit
> > > odd feeling that the two goes together, but no. They are semi-related as we
> > > have a special condition after the first one.
> > >
> > For this commit sure, but in the next a fallback method is introduced
> > for when the gp1 gpio line is not connected.
> > There are two register to trigger and read samples:
> >
> > * write CONV_READ -> read dummy value - [conversion] -> read value -> [conv ...
> > * write CONV_TRIGGER - [conversion] -> read value -> write ...
> >
> > The first allows almost twice the sampling frequency, but does not work
> > with the fallback because In-Band-Interrupt for CONV_READ are not
> > yielded.
>
> Do you mean that the same array is reused differently? If so, then okay.
>
Yes
> > > > + if (ret)
> > > > + return ret;
>
> ...
>
> > > > + fsleep(4000);
> > >
> > > 4 * USEC_PER_MSEC, also would be good to add a comment for this long delay.
> > >
> > Will add
> > /* Wait device functional blocks to power up */
> > Based on hardware tests, I can drop to 2 * USEC_PER_MSEC, lower than
> > that the device is not ready to switch to acquisition mode for
> > conversions.
>
> Good!
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Best regards,
Jorge
On Fri, Nov 28, 2025 at 07:50:02PM +0100, Jorge Marques wrote:
> On Thu, Nov 27, 2025 at 10:58:24AM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> > > On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
Please, remove the context you are agree with or which has no need
to be answered, it helps to parse and reply.
...
> > > > > +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> > > > > +{
> > > > > + /* See datasheet page 31 */
> > > > > + u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> > > > > +
> > > > > + return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
> > > >
> > > > Why u64?
> > > >
> > > > The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
> > > > more than 4 billions?
> > > >
> > > This is necessary since at fosc 111 Hz and avg 4096 it does take longer
> > > than 4 seconds, even though I do timeout after 1 seconds in the raw
> > > acquisition.
> >
> > Values above NSEC_PER_SEC+1 do not make sense (it will return 0),
> > and that fits u32. Can you refactor to avoid 64-bit arithmetics?
>
> Ok, any frequency lower than 1 Hz does not make sense.
Depends on the cases, we have sub-Hz sensors or some other stuff.
So, "...does not make sense in _this_ case." That's what I implied.
> static int ad4062_calc_sampling_frequency(int fosc, unsigned int oversamp_ratio)
Shouldn't fosc be unsigned?
> {
> /* See datasheet page 31 */
It's fine, but better to add a formula here or more information about
the calculations done in the function.
> u32 period = NSEC_PER_SEC / fosc;
period_ns ?
(We usually add units to this kind of variables for better understanding
of the calculations)
> u32 n_avg = BIT(oversamp_ratio) - 1;
>
> /* Result is less than 1 Hz */
> if (n_avg >= fosc)
> return 1;
+ blank line.
> return NSEC_PER_SEC / (n_avg * period + AD4062_TCONV_NS);
> }
LGTM, thanks!
> > > > > +}
...
> > > static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> > > int gain_frac)
> > > {
> > > u32 gain;
> > > int ret;
> > >
> > > if (gain_int < 0 || gain_frac < 0)
> > > return -EINVAL;
> > >
> > > gain = gain_int * MICRO + gain_frac;
> > > if (gain > 1999970)
> >
> > But this magic should be changed to what you explained to me
> > (as in 0xffff/0x8000 with the proper precision, and this
> > can be done in 32-bit space).
> >
> > Or even better
> >
> > if (gain_int < 0 || gain_int > 1)
> > return -EINVAL;
> >
> > if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
> > return -EINVAL;
> gain_frac would be 999999 max, or 999970 for the limit that fits in the
> register after the math. I think > 1.999.970 is self explanatory.
On the place of unprepared reader this is a complete magic number without
scale, without understanding where it came from, etc.
So, can you define it as a derivative from the other constants and with
a comment perhaps?
> > > return -EINVAL;
> > >
> > > put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
> > > MICRO),
> >
> > ...with temporary variable at minimum.
> >
> > But again, I still don't see the need for 64-bit space.
>
> Well, by dividing mon_val and micro values by a common divisor the
> operation fit in 32-bits:
>
> static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> int gain_frac)
> {
/* Divide numerator and denumerator by known great common divider */
> const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
> const u32 micro = MICRO / 64;
Yep, I suggested the same in another patch under review (not yours) for
the similar cases where we definitely may easily avoid overflow.
Alternatively you can use gcd().
> const u32 gain = gain_int * MICRO + gain_frac;
> int ret;
>
> if (gain_int < 0 || gain_frac < 0)
> return -EINVAL;
>
> if (gain > 1999970)
> return -EINVAL;
>
> put_unaligned_be16(DIV_ROUND_CLOSEST(gain * mon_val, micro), st->buf.bytes);
>
> ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> &st->buf.be16, sizeof(st->buf.be16));
> if (ret)
> return ret;
>
> /* Enable scale if gain is not equal to one */
> return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> !(gain_int == 1 && gain_frac == 0)));
Btw, I think you can move this check up and save in a temporary variable which
might affect the binary size of the compiled object as accesses to the gain_int
and gain_frac will be grouped in the same place with potential of the reusing
the CPU register(s)..
> }
> > > st->buf.bytes);
> > >
> > > ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> > > &st->buf.be16, sizeof(st->buf.be16));
> > > if (ret)
> > > return ret;
> > >
> > > /* Enable scale if gain is not equal to one */
> > > return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> > > AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > > FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > > !(gain_int == 1 && gain_frac == 0)));
> > > }
> > >
> > > To provide the enough resolution to compute every step (e.g., 0xFFFF and
> > > 0xFFFE) with the arbitrary user input.
--
With Best Regards,
Andy Shevchenko
On Fri, Nov 28, 2025 at 09:25:50PM +0200, Andy Shevchenko wrote:
Hi Andy,
> On Fri, Nov 28, 2025 at 07:50:02PM +0100, Jorge Marques wrote:
> > On Thu, Nov 27, 2025 at 10:58:24AM +0200, Andy Shevchenko wrote:
> > > On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> > > > On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
>
> > static int ad4062_calc_sampling_frequency(int fosc, unsigned int oversamp_ratio)
>
> Shouldn't fosc be unsigned?
>
Yep
> > {
> > /* See datasheet page 31 */
>
> It's fine, but better to add a formula here or more information about
> the calculations done in the function.
>
> > u32 period = NSEC_PER_SEC / fosc;
>
> period_ns ?
>
> (We usually add units to this kind of variables for better understanding
> of the calculations)
>
Ack.
> > > > static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> > > > int gain_frac)
> > > > {
> > > > ...
> > > >
> > > > if (gain > 1999970)
> > >
> > > But this magic should be changed to what you explained to me
> > > (as in 0xffff/0x8000 with the proper precision, and this
> > > can be done in 32-bit space).
> > >
> > > Or even better
> > >
> > > if (gain_int < 0 || gain_int > 1)
> > > return -EINVAL;
> > >
> > > if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
> > > return -EINVAL;
>
> > gain_frac would be 999999 max, or 999970 for the limit that fits in the
> > register after the math. I think > 1.999.970 is self explanatory.
>
> On the place of unprepared reader this is a complete magic number without
> scale, without understanding where it came from, etc.
>
> So, can you define it as a derivative from the other constants and with
> a comment perhaps?
>
(my proposal is after all your comments below)
> > > > return -EINVAL;
> > > >
> > > > put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
> > > > MICRO),
> > >
> > > ...with temporary variable at minimum.
> > >
> > > But again, I still don't see the need for 64-bit space.
> >
> > Well, by dividing mon_val and micro values by a common divisor the
> > operation fit in 32-bits:
> >
> > static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> > int gain_frac)
> > {
>
> /* Divide numerator and denumerator by known great common divider */
>
> > const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
> > const u32 micro = MICRO / 64;
>
> Yep, I suggested the same in another patch under review (not yours) for
> the similar cases where we definitely may easily avoid overflow.
>
> Alternatively you can use gcd().
>
> > put_unaligned_be16(DIV_ROUND_CLOSEST(gain * mon_val, micro), st->buf.bytes);
>
> Btw, I think you can move this check up and save in a temporary variable which
> might affect the binary size of the compiled object as accesses to the gain_int
> and gain_frac will be grouped in the same place with potential of the reusing
> the CPU register(s)..
>
> > }
>
I believe this is clear and fits all points:
/* Divide numerator and denumerator by known great common divider */
const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
const u32 micro = MICRO / 64;
const u32 gain_fp = gain_int * MICRO + gain_frac;
const u32 reg_val = DIV_ROUND_CLOSEST(gain_fp * mon_val, micro);
int ret;
/* Checks if the gain is in range and the value fits the field */
if (gain_int < 0 || gain_int > 1 || reg_val > BIT(16) - 1)
return -EINVAL;
put_unaligned_be16(reg_val, st->buf.bytes);
Explains 64 value. Checks if is in range [0, 2), then if fits the
register field for corner case of range (1.999970, 2) (0x10000). Full
formula is in the previous method ad4062_get_chan_calibscale.
> > > > st->buf.bytes);
> > > >
> > > > ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> > > > &st->buf.be16, sizeof(st->buf.be16));
> > > > if (ret)
> > > > return ret;
> > > >
> > > > /* Enable scale if gain is not equal to one */
> > > > return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> > > > AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > > > FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > > > !(gain_int == 1 && gain_frac == 0)));
> > > > }
> > > >
> > > > To provide the enough resolution to compute every step (e.g., 0xFFFF and
> > > > 0xFFFE) with the arbitrary user input.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Best Regards,
Jorge
On Thu, Dec 4, 2025 at 11:37 PM Jorge Marques <gastmaier@gmail.com> wrote:
> On Fri, Nov 28, 2025 at 09:25:50PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 28, 2025 at 07:50:02PM +0100, Jorge Marques wrote:
> > > On Thu, Nov 27, 2025 at 10:58:24AM +0200, Andy Shevchenko wrote:
> > > > On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> > > > > On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > > > > > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
...
> > > > > static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> > > > > int gain_frac)
> > > > > {
> > > > > ...
> > > > >
> > > > > if (gain > 1999970)
> > > >
> > > > But this magic should be changed to what you explained to me
> > > > (as in 0xffff/0x8000 with the proper precision, and this
> > > > can be done in 32-bit space).
> > > >
> > > > Or even better
> > > >
> > > > if (gain_int < 0 || gain_int > 1)
> > > > return -EINVAL;
> > > >
> > > > if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
> > > > return -EINVAL;
> >
> > > gain_frac would be 999999 max, or 999970 for the limit that fits in the
> > > register after the math. I think > 1.999.970 is self explanatory.
> >
> > On the place of unprepared reader this is a complete magic number without
> > scale, without understanding where it came from, etc.
> >
> > So, can you define it as a derivative from the other constants and with
> > a comment perhaps?
> >
> (my proposal is after all your comments below)
> > > > > return -EINVAL;
> > > > >
> > > > > put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
> > > > > MICRO),
> > > >
> > > > ...with temporary variable at minimum.
> > > >
> > > > But again, I still don't see the need for 64-bit space.
> > >
> > > Well, by dividing mon_val and micro values by a common divisor the
> > > operation fit in 32-bits:
> > >
> > > static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> > > int gain_frac)
> > > {
> >
> > /* Divide numerator and denumerator by known great common divider */
> >
> > > const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
> > > const u32 micro = MICRO / 64;
> >
> > Yep, I suggested the same in another patch under review (not yours) for
> > the similar cases where we definitely may easily avoid overflow.
> >
> > Alternatively you can use gcd().
> >
> > > put_unaligned_be16(DIV_ROUND_CLOSEST(gain * mon_val, micro), st->buf.bytes);
> >
> > Btw, I think you can move this check up and save in a temporary variable which
> > might affect the binary size of the compiled object as accesses to the gain_int
> > and gain_frac will be grouped in the same place with potential of the reusing
> > the CPU register(s)..
> >
> > > }
> >
> I believe this is clear and fits all points:
>
> /* Divide numerator and denumerator by known great common divider */
> const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
> const u32 micro = MICRO / 64;
> const u32 gain_fp = gain_int * MICRO + gain_frac;
> const u32 reg_val = DIV_ROUND_CLOSEST(gain_fp * mon_val, micro);
> int ret;
>
> /* Checks if the gain is in range and the value fits the field */
> if (gain_int < 0 || gain_int > 1 || reg_val > BIT(16) - 1)
> return -EINVAL;
>
> put_unaligned_be16(reg_val, st->buf.bytes);
>
> Explains 64 value. Checks if is in range [0, 2), then if fits the
> register field for corner case of range (1.999970, 2) (0x10000). Full
> formula is in the previous method ad4062_get_chan_calibscale.
Yes, LGTM now, thanks.
> > > > > st->buf.bytes);
> > > > >
> > > > > ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> > > > > &st->buf.be16, sizeof(st->buf.be16));
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > /* Enable scale if gain is not equal to one */
> > > > > return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> > > > > AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > > > > FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > > > > !(gain_int == 1 && gain_frac == 0)));
> > > > > }
> > > > >
> > > > > To provide the enough resolution to compute every step (e.g., 0xFFFF and
> > > > > 0xFFFE) with the arbitrary user input.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.