| Message ID | 20240820-ad4695-gain-offset-v1-2-c8f6e3b47551@baylibre.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: adc: ad4695: implement calibration support | expand |
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
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;
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; > > >
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; > > > > > > >
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 >
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), + ®_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), + ®_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, };
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(-)