| 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: <ebdc963c-d1f7-41fd-a49e-803bfb7a4408@baylibre.com>
Date: Tue, 3 Jun 2025 10:52:06 -0500
From: David Lechner <dlechner@...libre.com>
To: 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 v8 6/6] iio: adc: ad7606: add gain calibration support
On 6/3/25 9:36 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@...libre.com>
>
> Add gain calibration support, using resistor values set on devicetree,
> values to be set accordingly with ADC external RFilter, as explained in
> the ad7606c-16 datasheet, rev0, page 37.
>
> Usage example in the fdt yaml documentation.
>
> Tested-by: David Lechner <dlechner@...libre.com>
> Reviewed-by: Nuno Sá <nuno.sa@...log.com>
The patch has changed significantly since these tags were given, so
these tags should have been dropped.
> Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
> ---
> drivers/iio/adc/ad7606.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/iio/adc/ad7606.h | 6 +++++
> 2 files changed, 72 insertions(+)
>
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index e0a666cc0e14255754e74daa9e1e88bc4ad1665c..22dcb52ced57d4305db6401605c064fc438d5be4 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -33,6 +33,10 @@
>
> #include "ad7606.h"
>
> +#define AD7606_CALIB_GAIN_MIN 0
> +#define AD7606_CALIB_GAIN_STEP 1024
> +#define AD7606_CALIB_GAIN_MAX (63 * AD7606_CALIB_GAIN_STEP)
> +
> /*
> * Scales are computed as 5000/32768 and 10000/32768 respectively,
> * so that when applied to the raw values they provide mV values.
> @@ -125,6 +129,7 @@ static int ad7609_chan_scale_setup(struct iio_dev *indio_dev,
> struct iio_chan_spec *chan);
> static int ad7616_sw_mode_setup(struct iio_dev *indio_dev);
> static int ad7606b_sw_mode_setup(struct iio_dev *indio_dev);
> +static int ad7606_chan_calib_gain_setup(struct iio_dev *indio_dev);
>
> const struct ad7606_chip_info ad7605_4_info = {
> .max_samplerate = 300 * KILO,
> @@ -180,6 +185,7 @@ 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_gain_setup_cb = ad7606_chan_calib_gain_setup,
> .calib_offset_avail = ad7606_calib_offset_avail,
> .calib_phase_avail = ad7606b_calib_phase_avail,
> };
> @@ -195,6 +201,7 @@ 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_gain_setup_cb = ad7606_chan_calib_gain_setup,
> .calib_offset_avail = ad7606_calib_offset_avail,
> .calib_phase_avail = ad7606c_calib_phase_avail,
> };
> @@ -246,6 +253,7 @@ 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_gain_setup_cb = ad7606_chan_calib_gain_setup,
> .calib_offset_avail = ad7606c_18bit_calib_offset_avail,
> .calib_phase_avail = ad7606c_calib_phase_avail,
> };
> @@ -355,6 +363,36 @@ static int ad7606_get_chan_config(struct iio_dev *indio_dev, int ch,
> return 0;
> }
>
> +static int ad7606_chan_calib_gain_setup(struct iio_dev *indio_dev)
> +{
> + struct ad7606_state *st = iio_priv(indio_dev);
> + unsigned int num_channels = st->chip_info->num_adc_channels;
> + struct device *dev = st->dev;
> + int ret;
> +
> + device_for_each_child_node_scoped(dev, child) {
Now that I had a deep dive into this in v7, I'm wondering what is the
benefit of having a separate function and iterating over child nodes
a second time compared to just having a bool flag in chip_info and
doing this in the existing iterator in ad7606_probe_channels().
It seems like we could do the same thing in much fewer lines of code
if we avoid adding a callback.
> + u32 reg, r_gain;
> +
> + ret = fwnode_property_read_u32(child, "reg", ®);
> + if (ret)
> + return ret;
> +
> + /* Chan reg is a 1-based index. */
> + if (reg < 1 || reg > num_channels)
> + return -EINVAL;
> +
> + r_gain = 0;
> + ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
> + &r_gain);
ret is set but never read here.
> + if (r_gain > AD7606_CALIB_GAIN_MAX)
> + return -EINVAL;
> +
> + st->r_gain[reg - 1] = r_gain;
> + }
> +
> + return 0;
> +}
> +
> static int ad7606c_18bit_chan_scale_setup(struct iio_dev *indio_dev,
> struct iio_chan_spec *chan)
> {
> @@ -1352,6 +1390,21 @@ static int ad7606b_sw_mode_setup(struct iio_dev *indio_dev)
> return st->bops->sw_mode_config(indio_dev);
> }
>
> +static int ad7606_set_gain_calib(struct ad7606_state *st)
> +{
> + int i, ret;
> +
> + for (i = 0; i < st->chip_info->num_adc_channels; i++) {
> + ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(i),
> + DIV_ROUND_CLOSEST(st->r_gain[i],
> + AD7606_CALIB_GAIN_STEP));
I think typical kernel style would be to have at least one more tab
on this line (assuming aligning to "(" makes the line too long).
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int ad7606_probe_channels(struct iio_dev *indio_dev)
> {
> struct ad7606_state *st = iio_priv(indio_dev);
> @@ -1444,6 +1497,13 @@ static int ad7606_probe_channels(struct iio_dev *indio_dev)
> if (slow_bus)
> channels[i] = (struct iio_chan_spec)IIO_CHAN_SOFT_TIMESTAMP(i);
>
> + /* Getting gain calibration values for all channels. */
> + if (st->sw_mode_en && st->chip_info->calib_gain_setup_cb) {
> + ret = st->chip_info->calib_gain_setup_cb(indio_dev);
> + if (ret)
> + return ret;
> + }
> +
> indio_dev->channels = channels;
>
> return 0;
> @@ -1620,6 +1680,12 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
> st->chip_info->sw_setup_cb(indio_dev);
> }
>
> + if (st->sw_mode_en && st->chip_info->calib_gain_setup_cb) {
> + ret = ad7606_set_gain_calib(st);
> + if (ret)
> + return ret;
> + }
> +
> return devm_iio_device_register(dev, indio_dev);
> }
> EXPORT_SYMBOL_NS_GPL(ad7606_probe, "IIO_AD7606");
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index f613583a7fa4095115b0b28e3f8e51cd32b93524..a5b0d318e2f4d73d3708288536e807957c5de68c 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> @@ -50,6 +50,7 @@ struct ad7606_state;
> typedef int (*ad7606_scale_setup_cb_t)(struct iio_dev *indio_dev,
> struct iio_chan_spec *chan);
> typedef int (*ad7606_sw_setup_cb_t)(struct iio_dev *indio_dev);
> +typedef int (*ad7606_calib_gain_setup_cb_t)(struct iio_dev *indio_dev);
>
> /**
> * struct ad7606_chip_info - chip specific information
> @@ -66,6 +67,7 @@ typedef int (*ad7606_sw_setup_cb_t)(struct iio_dev *indio_dev);
> * @init_delay_ms: required delay in milliseconds for initialization
> * after a restart
> * @offload_storagebits: storage bits used by the offload hw implementation
> + * @calib_gain_setup_cb: callback to setup of gain calibration
> * @calib_offset_avail: pointer to offset calibration range/limits array
> * @calib_phase_avail: pointer to phase calibration range/limits array
> */
> @@ -81,6 +83,7 @@ struct ad7606_chip_info {
> bool os_req_reset;
> unsigned long init_delay_ms;
> u8 offload_storagebits;
> + ad7606_calib_gain_setup_cb_t calib_gain_setup_cb;
> const int *calib_offset_avail;
> const int (*calib_phase_avail)[2];
> };
> @@ -131,6 +134,7 @@ struct ad7606_chan_scale {
> * @data: buffer for reading data from the device
> * @offload_en: SPI offload enabled
> * @bus_data: bus-specific variables
> + * @r_gain: array to store gain calibration resistor value in ohm
> * @d16: be16 buffer for reading data from the device
> */
> struct ad7606_state {
> @@ -161,6 +165,8 @@ struct ad7606_state {
> bool offload_en;
> void *bus_data;
>
> + int r_gain[AD760X_MAX_CHANNELS];
This isn't used outside of probe, so we could possibly get away with
putting it somewhere else.
> +
> /*
> * DMA (thus cache coherency maintenance) may require the
> * transfer buffers to live in their own cache lines.
>
Powered by blists - more mailing lists