| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9c02b2bd-dabf-4818-8adf-83c9127946d1@baylibre.com>
Date: Wed, 30 Apr 2025 11:14:28 -0500
From: David Lechner <dlechner@...libre.com>
To: Nuno Sá <noname.nuno@...il.com>,
Angelo Dureghello <adureghello@...libre.com>,
Jonathan Cameron <jic23@...nel.org>, Nuno Sá
<nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH 3/5] iio: adc: ad7606: add offset and phase calibration
support
On 4/30/25 10:36 AM, Nuno Sá wrote:
> On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:
>> From: Angelo Dureghello <adureghello@...libre.com>
>>
>> Add support for offset and phase calibration, only for
>> devices that support software mode, that are:
>>
>> ad7606b
>> ad7606c-16
>> ad7606c-18
>>
>> Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
>> ---
>> drivers/iio/adc/ad7606.c | 160
>> +++++++++++++++++++++++++++++++++++++++++++++++
>> drivers/iio/adc/ad7606.h | 9 +++
>> 2 files changed, 169 insertions(+)
>>
>> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
>> index
>> ad5e6b5e1d5d2edc7f8ac7ed9a8a4e6e43827b85..ec063dd4a67eb94610c41c473e2d8040c919
>> 74cf 100644
>> --- a/drivers/iio/adc/ad7606.c
>> +++ b/drivers/iio/adc/ad7606.c
>> @@ -95,6 +95,22 @@ static const unsigned int ad7616_oversampling_avail[8] = {
>> 1, 2, 4, 8, 16, 32, 64, 128,
>> };
>>
>> +static const int ad7606_calib_offset_avail[3] = {
>> + -128, 1, 127,
>> +};
>> +
>> +static const int ad7606c_18bit_calib_offset_avail[3] = {
>> + -512, 4, 511,
>> +};
>
> From the DS, it seems this is 508?
>
>> +
>> +static const int ad7606b_calib_phase_avail[][2] = {
>> + { 0, 0 }, { 0, 1250 }, { 0, 318750 },
>> +};
>> +
>> +static const int ad7606c_calib_phase_avail[][2] = {
>> + { 0, 0 }, { 0, 1000 }, { 0, 255000 },
>> +};
>> +
>> static int ad7606c_18bit_chan_scale_setup(struct iio_dev *indio_dev,
>> struct iio_chan_spec *chan);
>> static int ad7606c_16bit_chan_scale_setup(struct iio_dev *indio_dev,
>> @@ -164,6 +180,8 @@ const struct ad7606_chip_info ad7606b_info = {
>> .scale_setup_cb = ad7606_16bit_chan_scale_setup,
>> .sw_setup_cb = ad7606b_sw_mode_setup,
>> .offload_storagebits = 32,
>> + .calib_offset_avail = ad7606_calib_offset_avail,
>> + .calib_phase_avail = ad7606b_calib_phase_avail,
>> };
>> EXPORT_SYMBOL_NS_GPL(ad7606b_info, "IIO_AD7606");
>>
>> @@ -177,6 +195,8 @@ const struct ad7606_chip_info ad7606c_16_info = {
>> .scale_setup_cb = ad7606c_16bit_chan_scale_setup,
>> .sw_setup_cb = ad7606b_sw_mode_setup,
>> .offload_storagebits = 32,
>> + .calib_offset_avail = ad7606_calib_offset_avail,
>> + .calib_phase_avail = ad7606c_calib_phase_avail,
>> };
>> EXPORT_SYMBOL_NS_GPL(ad7606c_16_info, "IIO_AD7606");
>>
>> @@ -226,6 +246,8 @@ const struct ad7606_chip_info ad7606c_18_info = {
>> .scale_setup_cb = ad7606c_18bit_chan_scale_setup,
>> .sw_setup_cb = ad7606b_sw_mode_setup,
>> .offload_storagebits = 32,
>> + .calib_offset_avail = ad7606c_18bit_calib_offset_avail,
>> + .calib_phase_avail = ad7606c_calib_phase_avail,
>> };
>> EXPORT_SYMBOL_NS_GPL(ad7606c_18_info, "IIO_AD7606");
>>
>> @@ -683,6 +705,40 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev,
>> unsigned int ch,
>> return ret;
>> }
>>
>> +static int ad7606_get_calib_offset(struct ad7606_state *st, int ch, int *val)
>> +{
>> + int ret;
>> +
>> + ret = st->bops->reg_read(st, AD7606_CALIB_OFFSET(ch));
>> + if (ret < 0)
>> + return ret;
>> +
>> + *val = st->chip_info->calib_offset_avail[0] +
>> + ret * st->chip_info->calib_offset_avail[1];
>> +
>> + return 0;
>> +}
>> +
>> +static int ad7606_get_calib_phase(struct ad7606_state *st, int ch, int *val,
>> + int *val2)
>> +{
>> + int ret;
>> +
>> + ret = st->bops->reg_read(st, AD7606_CALIB_PHASE(ch));
>> + if (ret < 0)
>> + return ret;
>> +
>> + *val = 0;
>> +
>> + /*
>> + * ad7606b: phase delay from 0 to 318.75 μs in steps of 1.25 μs.
>> + * ad7606c-16/18: phase delay from 0 µs to 255 µs in steps of 1 µs.
>> + */
>> + *val2 = ret * st->chip_info->calib_phase_avail[1][1];
>> +
>> + return 0;
>> +}
>> +
>> static int ad7606_read_raw(struct iio_dev *indio_dev,
>> struct iio_chan_spec const *chan,
>> int *val,
>> @@ -717,6 +773,22 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>> pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
>> *val = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
>> cnvst_pwm_state.period);
>> return IIO_VAL_INT;
>> + case IIO_CHAN_INFO_CALIBBIAS:
>> + if (!iio_device_claim_direct(indio_dev))
>> + return -EBUSY;
>> + ret = ad7606_get_calib_offset(st, chan->scan_index, val);
>> + iio_device_release_direct(indio_dev);
>> + if (ret)
>> + return ret;
>> + return IIO_VAL_INT;
>> + case IIO_CHAN_INFO_CALIBPHASE_DELAY:
>> + if (!iio_device_claim_direct(indio_dev))
>> + return -EBUSY;
>> + ret = ad7606_get_calib_phase(st, chan->scan_index, val,
>> val2);
>> + iio_device_release_direct(indio_dev);
>> + if (ret)
>> + return ret;
>> + return IIO_VAL_INT_PLUS_NANO;
>> }
>> return -EINVAL;
>> }
>> @@ -767,6 +839,64 @@ static int ad7606_write_os_hw(struct iio_dev *indio_dev,
>> int val)
>> return 0;
>> }
>>
>> +static int ad7606_set_calib_offset(struct ad7606_state *st, int ch, int val)
>> +{
>> + int start_val, step_val, stop_val;
>> +
>> + start_val = st->chip_info->calib_offset_avail[0];
>> + step_val = st->chip_info->calib_offset_avail[1];
>> + stop_val = st->chip_info->calib_offset_avail[2];
>> +
>> + if (val < start_val || val > stop_val)
>> + return -EINVAL;
>> +
>> + val += start_val;
>
> Shouldn't this be val -= start_val?
>
> I also don't think we have any strict rules in the ABI for units for these kind
> of interfaces so using "raw" values is easier. But FWIW, I think we could have
> this in mv (would naturally depend on scale)
>
> - Nuno Sá
>
>From testing, it seems to be working as expected for me, so I think this is
correct. The register value is not signed. 0x80 is no offset.
Also, I like having the scaling so that the units are the same LSB as the raw
value like it is now. It makes calibration easy since I can generate a constant
voltage and do a buffered read. Then I can take the average of all samples and
see how it compares to the expected value. Then take the difference and that is
the exact value to enter into the attribute. Millivolts would work to but that
requires applying the scale to the average of the raw values to get the number
that you would need to enter into the calibration attribute.
Powered by blists - more mailing lists