[PATCH 0/4] iio: adc: ad4695: implement calibration support

[PATCH 0/4] iio: adc: ad4695: implement calibration support

[David Lechner]

This series adds calibration support to the ad4695 driver.

This has the same "odd" gain and offset registers that are being used
for calibration as discussed recently in the ad4030 series [1]. So if
the ranges in the *_available attributes seem a bit big for calibration,
that is why. The official datasheet explanation for this feature is,
"The AD4695/AD4696 include offset and gain error correction
functionality to correct for first-order nonidealities in a full
[analog front end] signal chain."

[1]: https://lore.kernel.org/linux-iio/20240814193814.78fe45cc@jic23-huawei/

---
David Lechner (4):
      iio: adc: ad4695: add 2nd regmap for 16-bit registers
      iio: adc: ad4695: implement calibration support
      doc: iio: ad4695: update for calibration support
      iio: ABI: document ad4695 new attributes

 Documentation/ABI/testing/sysfs-bus-iio |   3 +
 Documentation/iio/ad4695.rst            |   7 +-
 drivers/iio/adc/ad4695.c                | 242 +++++++++++++++++++++++++++++---
 3 files changed, 234 insertions(+), 18 deletions(-)
---
base-commit: 0f718e10da81446df0909c9939dff2b77e3b4e95
change-id: 20240819-ad4695-gain-offset-c748d7addf27

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH 1/4] iio: adc: ad4695: add 2nd regmap for 16-bit registers

[David Lechner]

The AD4695 and similar chips have some multibyte registers that have
to be read/written in a single operation. So we need to add a 2nd regmap
for these registers.

These registers are removed from the 8-bit regmap allowable ranges and
AD4695_MAX_REG is dropped since it would be ambiguous now.

debugfs register access is also updated to automatically use the correct
regmap depending on the register address.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad4695.c | 83 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 68 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index b8547914f00f..63d816ad2d1f 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -34,6 +34,7 @@
 /* AD4695 registers */
 #define AD4695_REG_SPI_CONFIG_A				0x0000
 #define   AD4695_REG_SPI_CONFIG_A_SW_RST		  (BIT(7) | BIT(0))
+#define   AD4695_REG_SPI_CONFIG_A_ADDR_DIR		  BIT(5)
 #define AD4695_REG_SPI_CONFIG_B				0x0001
 #define   AD4695_REG_SPI_CONFIG_B_INST_MODE		  BIT(7)
 #define AD4695_REG_DEVICE_TYPE				0x0003
@@ -77,7 +78,6 @@
 #define AD4695_REG_GAIN_IN(n)				(0x00C0 | (2 * (n)))
 #define AD4695_REG_AS_SLOT(n)				(0x0100 | (n))
 #define   AD4695_REG_AS_SLOT_INX			  GENMASK(3, 0)
-#define AD4695_MAX_REG					0x017F
 
 /* Conversion mode commands */
 #define AD4695_CMD_EXIT_CNV_MODE	0x0A
@@ -121,6 +121,7 @@ struct ad4695_channel_config {
 struct ad4695_state {
 	struct spi_device *spi;
 	struct regmap *regmap;
+	struct regmap *regmap16;
 	struct gpio_desc *reset_gpio;
 	/* voltages channels plus temperature and timestamp */
 	struct iio_chan_spec iio_chan[AD4695_MAX_CHANNELS + 2];
@@ -150,8 +151,10 @@ static const struct regmap_range ad4695_regmap_rd_ranges[] = {
 	regmap_reg_range(AD4695_REG_SPI_CONFIG_C, AD4695_REG_SPI_STATUS),
 	regmap_reg_range(AD4695_REG_STATUS, AD4695_REG_ALERT_STATUS2),
 	regmap_reg_range(AD4695_REG_CLAMP_STATUS, AD4695_REG_CLAMP_STATUS),
-	regmap_reg_range(AD4695_REG_SETUP, AD4695_REG_TEMP_CTRL),
-	regmap_reg_range(AD4695_REG_CONFIG_IN(0), AD4695_MAX_REG),
+	regmap_reg_range(AD4695_REG_SETUP, AD4695_REG_AC_CTRL),
+	regmap_reg_range(AD4695_REG_GPIO_CTRL, AD4695_REG_TEMP_CTRL),
+	regmap_reg_range(AD4695_REG_CONFIG_IN(0), AD4695_REG_CONFIG_IN(15)),
+	regmap_reg_range(AD4695_REG_AS_SLOT(0), AD4695_REG_AS_SLOT(127)),
 };
 
 static const struct regmap_access_table ad4695_regmap_rd_table = {
@@ -164,8 +167,10 @@ static const struct regmap_range ad4695_regmap_wr_ranges[] = {
 	regmap_reg_range(AD4695_REG_SCRATCH_PAD, AD4695_REG_SCRATCH_PAD),
 	regmap_reg_range(AD4695_REG_LOOP_MODE, AD4695_REG_LOOP_MODE),
 	regmap_reg_range(AD4695_REG_SPI_CONFIG_C, AD4695_REG_SPI_STATUS),
-	regmap_reg_range(AD4695_REG_SETUP, AD4695_REG_TEMP_CTRL),
-	regmap_reg_range(AD4695_REG_CONFIG_IN(0), AD4695_MAX_REG),
+	regmap_reg_range(AD4695_REG_SETUP, AD4695_REG_AC_CTRL),
+	regmap_reg_range(AD4695_REG_GPIO_CTRL, AD4695_REG_TEMP_CTRL),
+	regmap_reg_range(AD4695_REG_CONFIG_IN(0), AD4695_REG_CONFIG_IN(15)),
+	regmap_reg_range(AD4695_REG_AS_SLOT(0), AD4695_REG_AS_SLOT(127)),
 };
 
 static const struct regmap_access_table ad4695_regmap_wr_table = {
@@ -174,15 +179,47 @@ static const struct regmap_access_table ad4695_regmap_wr_table = {
 };
 
 static const struct regmap_config ad4695_regmap_config = {
-	.name = "ad4695",
+	.name = "ad4695-8",
 	.reg_bits = 16,
 	.val_bits = 8,
-	.max_register = AD4695_MAX_REG,
+	.max_register = AD4695_REG_AS_SLOT(127),
 	.rd_table = &ad4695_regmap_rd_table,
 	.wr_table = &ad4695_regmap_wr_table,
 	.can_multi_write = true,
 };
 
+static const struct regmap_range ad4695_regmap16_rd_ranges[] = {
+	regmap_reg_range(AD4695_REG_STD_SEQ_CONFIG, AD4695_REG_STD_SEQ_CONFIG),
+	regmap_reg_range(AD4695_REG_UPPER_IN(0), AD4695_REG_GAIN_IN(15)),
+};
+
+static const struct regmap_access_table ad4695_regmap16_rd_table = {
+	.yes_ranges = ad4695_regmap16_rd_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad4695_regmap16_rd_ranges),
+};
+
+static const struct regmap_range ad4695_regmap16_wr_ranges[] = {
+	regmap_reg_range(AD4695_REG_STD_SEQ_CONFIG, AD4695_REG_STD_SEQ_CONFIG),
+	regmap_reg_range(AD4695_REG_UPPER_IN(0), AD4695_REG_GAIN_IN(15)),
+};
+
+static const struct regmap_access_table ad4695_regmap16_wr_table = {
+	.yes_ranges = ad4695_regmap16_wr_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad4695_regmap16_wr_ranges),
+};
+
+static const struct regmap_config ad4695_regmap16_config = {
+	.name = "ad4695-16",
+	.reg_bits = 16,
+	.reg_stride = 2,
+	.val_bits = 16,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.max_register = AD4695_REG_GAIN_IN(15),
+	.rd_table = &ad4695_regmap16_rd_table,
+	.wr_table = &ad4695_regmap16_wr_table,
+	.can_multi_write = true,
+};
+
 static const struct iio_chan_spec ad4695_channel_template = {
 	.type = IIO_VOLTAGE,
 	.indexed = 1,
@@ -646,13 +683,24 @@ static int ad4695_debugfs_reg_access(struct iio_dev *indio_dev,
 	struct ad4695_state *st = iio_priv(indio_dev);
 
 	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
-		if (readval)
-			return regmap_read(st->regmap, reg, readval);
-
-		return regmap_write(st->regmap, reg, writeval);
+		if (readval) {
+			if (regmap_check_range_table(st->regmap, reg,
+						     &ad4695_regmap_rd_table))
+				return regmap_read(st->regmap, reg, readval);
+			if (regmap_check_range_table(st->regmap16, reg,
+						     &ad4695_regmap16_rd_table))
+				return regmap_read(st->regmap16, reg, readval);
+		} else {
+			if (regmap_check_range_table(st->regmap, reg,
+						     &ad4695_regmap_wr_table))
+				return regmap_write(st->regmap, reg, writeval);
+			if (regmap_check_range_table(st->regmap16, reg,
+						     &ad4695_regmap16_wr_table))
+				return regmap_write(st->regmap16, reg, writeval);
+		}
 	}
 
-	unreachable();
+	return -EINVAL;
 }
 
 static const struct iio_info ad4695_info = {
@@ -807,6 +855,11 @@ static int ad4695_probe(struct spi_device *spi)
 		return dev_err_probe(dev, PTR_ERR(st->regmap),
 				     "Failed to initialize regmap\n");
 
+	st->regmap16 = devm_regmap_init_spi(spi, &ad4695_regmap16_config);
+	if (IS_ERR(st->regmap16))
+		return dev_err_probe(dev, PTR_ERR(st->regmap16),
+				     "Failed to initialize regmap16\n");
+
 	ret = devm_regulator_bulk_get_enable(dev,
 					     ARRAY_SIZE(ad4695_power_supplies),
 					     ad4695_power_supplies);
@@ -876,9 +929,9 @@ static int ad4695_probe(struct spi_device *spi)
 		msleep(AD4695_T_WAKEUP_SW_MS);
 	}
 
-	/* Needed for debugfs since it only access registers 1 byte at a time. */
-	ret = regmap_set_bits(st->regmap, AD4695_REG_SPI_CONFIG_C,
-			      AD4695_REG_SPI_CONFIG_C_MB_STRICT);
+	/* Needed for regmap16 to be able to work correctly. */
+	ret = regmap_set_bits(st->regmap, AD4695_REG_SPI_CONFIG_A,
+			      AD4695_REG_SPI_CONFIG_A_ADDR_DIR);
 	if (ret)
 		return ret;
 

-- 
2.43.0

[PATCH 2/4] iio: adc: ad4695: implement calibration support

[David Lechner]

The AD4695 has a calibration feature that allows the user to compensate
for variations in the analog front end. This implements this feature in
the driver using the standard `calibgain` and `calibbias` attributes.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad4695.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 157 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 63d816ad2d1f..181c4146188c 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -23,6 +23,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/trigger_consumer.h>
+#include <linux/minmax.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -225,7 +226,11 @@ static const struct iio_chan_spec ad4695_channel_template = {
 	.indexed = 1,
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			      BIT(IIO_CHAN_INFO_SCALE) |
-			      BIT(IIO_CHAN_INFO_OFFSET),
+			      BIT(IIO_CHAN_INFO_OFFSET) |
+			      BIT(IIO_CHAN_INFO_CALIBSCALE) |
+			      BIT(IIO_CHAN_INFO_CALIBBIAS),
+	.info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBSCALE) |
+					BIT(IIO_CHAN_INFO_CALIBBIAS),
 	.scan_type = {
 		.sign = 'u',
 		.realbits = 16,
@@ -619,7 +624,8 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
 	struct ad4695_state *st = iio_priv(indio_dev);
 	struct ad4695_channel_config *cfg = &st->channels_cfg[chan->scan_index];
 	u8 realbits = chan->scan_type.realbits;
-	int ret;
+	unsigned int reg_val;
+	int ret, tmp;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -670,6 +676,153 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	case IIO_CHAN_INFO_CALIBSCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+				ret = regmap_read(st->regmap16,
+					AD4695_REG_GAIN_IN(chan->scan_index),
+					&reg_val);
+				if (ret)
+					return ret;
+
+				*val = reg_val;
+				*val2 = 15;
+
+				return IIO_VAL_FRACTIONAL_LOG2;
+			}
+			unreachable();
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_CALIBBIAS:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+				ret = regmap_read(st->regmap16,
+					AD4695_REG_OFFSET_IN(chan->scan_index),
+					&reg_val);
+				if (ret)
+					return ret;
+
+				tmp = sign_extend32(reg_val, 15);
+
+				*val = tmp / 4;
+				*val2 = abs(tmp) % 4 * MICRO / 4;
+
+				if (tmp < 0 && *val2) {
+					*val *= -1;
+					*val2 *= -1;
+				}
+
+				return IIO_VAL_INT_PLUS_MICRO;
+			}
+			unreachable();
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4695_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct ad4695_state *st = iio_priv(indio_dev);
+	unsigned int reg_val;
+	int ret;
+
+	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+		switch (mask) {
+		case IIO_CHAN_INFO_CALIBSCALE:
+			switch (chan->type) {
+			case IIO_VOLTAGE:
+				if (val < 0 || val2 < 0)
+					reg_val = 0;
+				else if (val > 1)
+					reg_val = U16_MAX;
+				else
+					reg_val = (val * (1 << 16) +
+						   mul_u64_u32_div(val2, 1 << 16,
+								   MICRO)) / 2;
+
+				return regmap_write(st->regmap16,
+					AD4695_REG_GAIN_IN(chan->scan_index),
+					reg_val);
+			default:
+				return -EINVAL;
+			}
+		case IIO_CHAN_INFO_CALIBBIAS:
+			switch (chan->type) {
+			case IIO_VOLTAGE:
+				if (val2 >= 0 && val > S16_MAX / 4)
+					reg_val = S16_MAX;
+				else if ((val2 < 0 ? -val : val) < S16_MIN / 4)
+					reg_val = S16_MIN;
+				else if (val2 < 0)
+					reg_val = clamp_t(int,
+						-(val * 4 + -val2 * 4 / MICRO),
+						S16_MIN, S16_MAX);
+				else if (val < 0)
+					reg_val = clamp_t(int,
+						val * 4 - val2 * 4 / MICRO,
+						S16_MIN, S16_MAX);
+				else
+					reg_val = clamp_t(int,
+						val * 4 + val2 * 4 / MICRO,
+						S16_MIN, S16_MAX);
+
+				return regmap_write(st->regmap16,
+					AD4695_REG_OFFSET_IN(chan->scan_index),
+					reg_val);
+			default:
+				return -EINVAL;
+			}
+		default:
+			return -EINVAL;
+		}
+	}
+	unreachable();
+}
+
+static int ad4695_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	static const int ad4695_calibscale_available[6] = {
+		/* Range of 0 (inclusive) to 2 (exclusive) */
+		0, 15, 1, 15, U16_MAX, 15
+	};
+	static const int ad4695_calibbias_available[6] = {
+		/*
+		 * Datasheet says FSR/8 which translates to signed/4. The step
+		 * depends on oversampling ratio which is always 1 for now.
+		 */
+		S16_MIN / 4, 0, 0, MICRO / 4, S16_MAX / 4, S16_MAX % 4 * MICRO / 4
+	};
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBSCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*vals = ad4695_calibscale_available;
+			*type = IIO_VAL_FRACTIONAL_LOG2;
+			return IIO_AVAIL_RANGE;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_CALIBBIAS:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*vals = ad4695_calibbias_available;
+			*type = IIO_VAL_INT_PLUS_MICRO;
+			return IIO_AVAIL_RANGE;
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -705,6 +858,8 @@ static int ad4695_debugfs_reg_access(struct iio_dev *indio_dev,
 
 static const struct iio_info ad4695_info = {
 	.read_raw = &ad4695_read_raw,
+	.write_raw = &ad4695_write_raw,
+	.read_avail = &ad4695_read_avail,
 	.debugfs_reg_access = &ad4695_debugfs_reg_access,
 };
 

-- 
2.43.0

[PATCH 3/4] doc: iio: ad4695: update for calibration support

[David Lechner]

Calibration support has been added to the ad4695 driver, so update the
documentation to reflect this.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 Documentation/iio/ad4695.rst | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/iio/ad4695.rst b/Documentation/iio/ad4695.rst
index 7612596bb6e9..33ed29b7c98a 100644
--- a/Documentation/iio/ad4695.rst
+++ b/Documentation/iio/ad4695.rst
@@ -143,13 +143,18 @@ present, then the external reference voltage is used and the internal buffer is
 disabled. If ``refin-supply`` is present, then the internal buffered reference
 voltage is used.
 
+Gain/offset calibration
+-----------------------
+
+System calibration is supported using the channel gain and offset registers via
+the ``calibscale`` and ``calibbias`` attributes respectively.
+
 Unimplemented features
 ----------------------
 
 - Additional wiring modes
 - Threshold events
 - Oversampling
-- Gain/offset calibration
 - GPIO support
 - CRC support
 

-- 
2.43.0

[PATCH 4/4] iio: ABI: document ad4695 new attributes

[David Lechner]

The ad4695 driver now supports calibration using the
in_voltageY_calib{scale,bias}[_available] attributes.

Only one of these was documented before. This adds rest.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 345d58535dc9..89943c2d54e8 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -541,6 +541,7 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_calibbias
 What:		/sys/bus/iio/devices/iio:deviceX/in_proximity0_calibbias
 What:		/sys/bus/iio/devices/iio:deviceX/in_resistance_calibbias
 What:		/sys/bus/iio/devices/iio:deviceX/in_temp_calibbias
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibbias
 What:		/sys/bus/iio/devices/iio:deviceX/out_currentY_calibbias
 What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_calibbias
 KernelVersion:	2.6.35
@@ -556,6 +557,7 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_accel_calibbias_available
 What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_calibbias_available
 What:		/sys/bus/iio/devices/iio:deviceX/in_temp_calibbias_available
 What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_calibbias_available
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibbias_available
 What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_calibbias_available
 KernelVersion:  5.8
 Contact:        linux-iio@vger.kernel.org
@@ -603,6 +605,7 @@ Description:
 What:		/sys/bus/iio/devices/iio:deviceX/in_illuminanceY_calibscale_available
 What:		/sys/bus/iio/devices/iio:deviceX/in_intensityY_calibscale_available
 What:		/sys/bus/iio/devices/iio:deviceX/in_proximityY_calibscale_available
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibscale_available
 KernelVersion:	4.8
 Contact:	linux-iio@vger.kernel.org
 Description:

-- 
2.43.0

Re: [PATCH 2/4] iio: adc: ad4695: implement calibration support

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 0f718e10da81446df0909c9939dff2b77e3b4e95]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Lechner/iio-adc-ad4695-add-2nd-regmap-for-16-bit-registers/20240821-000102
base:   0f718e10da81446df0909c9939dff2b77e3b4e95
patch link:    https://lore.kernel.org/r/20240820-ad4695-gain-offset-v1-2-c8f6e3b47551%40baylibre.com
patch subject: [PATCH 2/4] iio: adc: ad4695: implement calibration support
config: i386-buildonly-randconfig-002-20240821 (https://download.01.org/0day-ci/archive/20240821/202408211207.fmYTjQDK-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240821/202408211207.fmYTjQDK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408211207.fmYTjQDK-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/ad4695.c:735:6: warning: unused variable 'ret' [-Wunused-variable]
     735 |         int ret;
         |             ^~~
   1 warning generated.


vim +/ret +735 drivers/iio/adc/ad4695.c

   728	
   729	static int ad4695_write_raw(struct iio_dev *indio_dev,
   730				    struct iio_chan_spec const *chan,
   731				    int val, int val2, long mask)
   732	{
   733		struct ad4695_state *st = iio_priv(indio_dev);
   734		unsigned int reg_val;
 > 735		int ret;
   736	
   737		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
   738			switch (mask) {
   739			case IIO_CHAN_INFO_CALIBSCALE:
   740				switch (chan->type) {
   741				case IIO_VOLTAGE:
   742					if (val < 0 || val2 < 0)
   743						reg_val = 0;
   744					else if (val > 1)
   745						reg_val = U16_MAX;
   746					else
   747						reg_val = (val * (1 << 16) +
   748							   mul_u64_u32_div(val2, 1 << 16,
   749									   MICRO)) / 2;
   750	
   751					return regmap_write(st->regmap16,
   752						AD4695_REG_GAIN_IN(chan->scan_index),
   753						reg_val);
   754				default:
   755					return -EINVAL;
   756				}
   757			case IIO_CHAN_INFO_CALIBBIAS:
   758				switch (chan->type) {
   759				case IIO_VOLTAGE:
   760					if (val2 >= 0 && val > S16_MAX / 4)
   761						reg_val = S16_MAX;
   762					else if ((val2 < 0 ? -val : val) < S16_MIN / 4)
   763						reg_val = S16_MIN;
   764					else if (val2 < 0)
   765						reg_val = clamp_t(int,
   766							-(val * 4 + -val2 * 4 / MICRO),
   767							S16_MIN, S16_MAX);
   768					else if (val < 0)
   769						reg_val = clamp_t(int,
   770							val * 4 - val2 * 4 / MICRO,
   771							S16_MIN, S16_MAX);
   772					else
   773						reg_val = clamp_t(int,
   774							val * 4 + val2 * 4 / MICRO,
   775							S16_MIN, S16_MAX);
   776	
   777					return regmap_write(st->regmap16,
   778						AD4695_REG_OFFSET_IN(chan->scan_index),
   779						reg_val);
   780				default:
   781					return -EINVAL;
   782				}
   783			default:
   784				return -EINVAL;
   785			}
   786		}
   787		unreachable();
   788	}
   789	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/4] iio: adc: ad4695: implement calibration support

[Jonathan Cameron]

On Tue, 20 Aug 2024 10:58:36 -0500
David Lechner <dlechner@baylibre.com> wrote:

> The AD4695 has a calibration feature that allows the user to compensate
> for variations in the analog front end. This implements this feature in
> the driver using the standard `calibgain` and `calibbias` attributes.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Hi David,

Whilst some of the messy value manipulation is unavoidable
(oh for signed integer zero!), I wonder if we can at least
move one case into the core.

See below.

> +
> +static int ad4695_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ad4695_state *st = iio_priv(indio_dev);
> +	unsigned int reg_val;
> +	int ret;
> +
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +		switch (mask) {
> +		case IIO_CHAN_INFO_CALIBSCALE:
> +			switch (chan->type) {
> +			case IIO_VOLTAGE:
> +				if (val < 0 || val2 < 0)
> +					reg_val = 0;
> +				else if (val > 1)
> +					reg_val = U16_MAX;
> +				else
> +					reg_val = (val * (1 << 16) +
> +						   mul_u64_u32_div(val2, 1 << 16,
> +								   MICRO)) / 2;
Maybe worth extending iio_write_channel_info() to handle
IIO_VAL_FRACTIONAL_LOG2()?
It'll look much like this and you'll need to provide write_raw_get_fmt()
so the core know what to do with the value formatting.

I don't really like the mixture here between the read path being able
to rely on the core code to deal with the /2^X and the write path not.
> +
> +				return regmap_write(st->regmap16,
> +					AD4695_REG_GAIN_IN(chan->scan_index),
> +					reg_val);
> +			default:
> +				return -EINVAL;

Re: [PATCH 2/4] iio: adc: ad4695: implement calibration support

[David Lechner]

On 8/21/24 8:16 AM, Jonathan Cameron wrote:
> On Tue, 20 Aug 2024 10:58:36 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> The AD4695 has a calibration feature that allows the user to compensate
>> for variations in the analog front end. This implements this feature in
>> the driver using the standard `calibgain` and `calibbias` attributes.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
> Hi David,
> 
> Whilst some of the messy value manipulation is unavoidable
> (oh for signed integer zero!), I wonder if we can at least
> move one case into the core.
> 
> See below.
> 
>> +
>> +static int ad4695_write_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int val, int val2, long mask)
>> +{
>> +	struct ad4695_state *st = iio_priv(indio_dev);
>> +	unsigned int reg_val;
>> +	int ret;
>> +
>> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
>> +		switch (mask) {
>> +		case IIO_CHAN_INFO_CALIBSCALE:
>> +			switch (chan->type) {
>> +			case IIO_VOLTAGE:
>> +				if (val < 0 || val2 < 0)
>> +					reg_val = 0;
>> +				else if (val > 1)
>> +					reg_val = U16_MAX;
>> +				else
>> +					reg_val = (val * (1 << 16) +
>> +						   mul_u64_u32_div(val2, 1 << 16,
>> +								   MICRO)) / 2;
> Maybe worth extending iio_write_channel_info() to handle
> IIO_VAL_FRACTIONAL_LOG2()?
> It'll look much like this and you'll need to provide write_raw_get_fmt()
> so the core know what to do with the value formatting.
> 
> I don't really like the mixture here between the read path being able
> to rely on the core code to deal with the /2^X and the write path not.

Sounds like a good idea to me.

It seems like we would need to add an extra out parameter to 
write_raw_get_fmt to say what we want the X in 2^X to be. For
example, we would want to make sure the val2 we get in write_raw
for this driver is 15.

>> +
>> +				return regmap_write(st->regmap16,
>> +					AD4695_REG_GAIN_IN(chan->scan_index),
>> +					reg_val);
>> +			default:
>> +				return -EINVAL;
> 
> 
> 

Re: [PATCH 2/4] iio: adc: ad4695: implement calibration support

[Jonathan Cameron]

On Wed, 21 Aug 2024 11:12:12 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 8/21/24 8:16 AM, Jonathan Cameron wrote:
> > On Tue, 20 Aug 2024 10:58:36 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> The AD4695 has a calibration feature that allows the user to compensate
> >> for variations in the analog front end. This implements this feature in
> >> the driver using the standard `calibgain` and `calibbias` attributes.
> >>
> >> Signed-off-by: David Lechner <dlechner@baylibre.com>  
> > Hi David,
> > 
> > Whilst some of the messy value manipulation is unavoidable
> > (oh for signed integer zero!), I wonder if we can at least
> > move one case into the core.
> > 
> > See below.
> >   
> >> +
> >> +static int ad4695_write_raw(struct iio_dev *indio_dev,
> >> +			    struct iio_chan_spec const *chan,
> >> +			    int val, int val2, long mask)
> >> +{
> >> +	struct ad4695_state *st = iio_priv(indio_dev);
> >> +	unsigned int reg_val;
> >> +	int ret;
> >> +
> >> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> >> +		switch (mask) {
> >> +		case IIO_CHAN_INFO_CALIBSCALE:
> >> +			switch (chan->type) {
> >> +			case IIO_VOLTAGE:
> >> +				if (val < 0 || val2 < 0)
> >> +					reg_val = 0;
> >> +				else if (val > 1)
> >> +					reg_val = U16_MAX;
> >> +				else
> >> +					reg_val = (val * (1 << 16) +
> >> +						   mul_u64_u32_div(val2, 1 << 16,
> >> +								   MICRO)) / 2;  
> > Maybe worth extending iio_write_channel_info() to handle
> > IIO_VAL_FRACTIONAL_LOG2()?
> > It'll look much like this and you'll need to provide write_raw_get_fmt()
> > so the core know what to do with the value formatting.
> > 
> > I don't really like the mixture here between the read path being able
> > to rely on the core code to deal with the /2^X and the write path not.  
> 
> Sounds like a good idea to me.
> 
> It seems like we would need to add an extra out parameter to 
> write_raw_get_fmt to say what we want the X in 2^X to be. For
> example, we would want to make sure the val2 we get in write_raw
> for this driver is 15.
Yes.  (I tried to reply to say I'd neglected this but managed to
just email myself. oops.)

Maybe it's too complex to bother given that.  Definitely a job
for another day rather than something to block this series on.


Jonathan

> 
> >> +
> >> +				return regmap_write(st->regmap16,
> >> +					AD4695_REG_GAIN_IN(chan->scan_index),
> >> +					reg_val);
> >> +			default:
> >> +				return -EINVAL;  
> > 
> > 
> >   
> 

Re: [PATCH 0/4] iio: adc: ad4695: implement calibration support

[Jonathan Cameron]

On Tue, 20 Aug 2024 10:58:34 -0500
David Lechner <dlechner@baylibre.com> wrote:

> This series adds calibration support to the ad4695 driver.
> 
> This has the same "odd" gain and offset registers that are being used
> for calibration as discussed recently in the ad4030 series [1]. So if
> the ranges in the *_available attributes seem a bit big for calibration,
> that is why. The official datasheet explanation for this feature is,
> "The AD4695/AD4696 include offset and gain error correction
> functionality to correct for first-order nonidealities in a full
> [analog front end] signal chain."
> 
> [1]: https://lore.kernel.org/linux-iio/20240814193814.78fe45cc@jic23-huawei/
Series LGTM
Applied to the togreg branch of iio.git and pushed out as testing.
Note I'll be rebasing after the previous pull request (hopefully)
gets picked up by Greg.

Jonathan

> 
> ---
> David Lechner (4):
>       iio: adc: ad4695: add 2nd regmap for 16-bit registers
>       iio: adc: ad4695: implement calibration support
>       doc: iio: ad4695: update for calibration support
>       iio: ABI: document ad4695 new attributes
> 
>  Documentation/ABI/testing/sysfs-bus-iio |   3 +
>  Documentation/iio/ad4695.rst            |   7 +-
>  drivers/iio/adc/ad4695.c                | 242 +++++++++++++++++++++++++++++---
>  3 files changed, 234 insertions(+), 18 deletions(-)
> ---
> base-commit: 0f718e10da81446df0909c9939dff2b77e3b4e95
> change-id: 20240819-ad4695-gain-offset-c748d7addf27
> 
> Best regards,

Re: [PATCH 2/4] iio: adc: ad4695: implement calibration support

[Jonathan Cameron]

On Wed, 21 Aug 2024 12:21:09 +0800
kernel test robot <lkp@intel.com> wrote:

> Hi David,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on 0f718e10da81446df0909c9939dff2b77e3b4e95]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/David-Lechner/iio-adc-ad4695-add-2nd-regmap-for-16-bit-registers/20240821-000102
> base:   0f718e10da81446df0909c9939dff2b77e3b4e95
> patch link:    https://lore.kernel.org/r/20240820-ad4695-gain-offset-v1-2-c8f6e3b47551%40baylibre.com
> patch subject: [PATCH 2/4] iio: adc: ad4695: implement calibration support
> config: i386-buildonly-randconfig-002-20240821 (https://download.01.org/0day-ci/archive/20240821/202408211207.fmYTjQDK-lkp@intel.com/config)
> compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240821/202408211207.fmYTjQDK-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202408211207.fmYTjQDK-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
> >> drivers/iio/adc/ad4695.c:735:6: warning: unused variable 'ret' [-Wunused-variable]  
>      735 |         int ret;
>          |             ^~~
>    1 warning generated.
I dropped this whilst applying.

> 
> 
> vim +/ret +735 drivers/iio/adc/ad4695.c
> 
>    728	
>    729	static int ad4695_write_raw(struct iio_dev *indio_dev,
>    730				    struct iio_chan_spec const *chan,
>    731				    int val, int val2, long mask)
>    732	{
>    733		struct ad4695_state *st = iio_priv(indio_dev);
>    734		unsigned int reg_val;
>  > 735		int ret;  
>    736	
>    737		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
>    738			switch (mask) {
>    739			case IIO_CHAN_INFO_CALIBSCALE:
>    740				switch (chan->type) {
>    741				case IIO_VOLTAGE:
>    742					if (val < 0 || val2 < 0)
>    743						reg_val = 0;
>    744					else if (val > 1)
>    745						reg_val = U16_MAX;
>    746					else
>    747						reg_val = (val * (1 << 16) +
>    748							   mul_u64_u32_div(val2, 1 << 16,
>    749									   MICRO)) / 2;
>    750	
>    751					return regmap_write(st->regmap16,
>    752						AD4695_REG_GAIN_IN(chan->scan_index),
>    753						reg_val);
>    754				default:
>    755					return -EINVAL;
>    756				}
>    757			case IIO_CHAN_INFO_CALIBBIAS:
>    758				switch (chan->type) {
>    759				case IIO_VOLTAGE:
>    760					if (val2 >= 0 && val > S16_MAX / 4)
>    761						reg_val = S16_MAX;
>    762					else if ((val2 < 0 ? -val : val) < S16_MIN / 4)
>    763						reg_val = S16_MIN;
>    764					else if (val2 < 0)
>    765						reg_val = clamp_t(int,
>    766							-(val * 4 + -val2 * 4 / MICRO),
>    767							S16_MIN, S16_MAX);
>    768					else if (val < 0)
>    769						reg_val = clamp_t(int,
>    770							val * 4 - val2 * 4 / MICRO,
>    771							S16_MIN, S16_MAX);
>    772					else
>    773						reg_val = clamp_t(int,
>    774							val * 4 + val2 * 4 / MICRO,
>    775							S16_MIN, S16_MAX);
>    776	
>    777					return regmap_write(st->regmap16,
>    778						AD4695_REG_OFFSET_IN(chan->scan_index),
>    779						reg_val);
>    780				default:
>    781					return -EINVAL;
>    782				}
>    783			default:
>    784				return -EINVAL;
>    785			}
>    786		}
>    787		unreachable();
>    788	}
>    789	
>