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, 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
© 2016 - 2025 Red Hat, Inc.