Re: [PATCH v2 2/7] iio: adc: Add basic support for AD4170

Andy Shevchenko
2025-05-02T11:28:50Z

On Mon, Apr 28, 2025 at 09:28:03AM -0300, Marcelo Schmitt wrote:
> From: Ana-Maria Cusco <ana-maria.cusco@analog.com>
>
> Add support for the AD4170 ADC with the following features:
> - Single-shot read.
> - Analog front end PGA configuration.
> - Digital filter and sampling frequency configuration.
> - Calibration gain and offset configuration.
> - Differential and pseudo-differential input configuration.

...

> +/*
> + * Copyright (C) 2024 Analog Devices, Inc.

My calendar shows 2025...

> + * Author: Ana-Maria Cusco <ana-maria.cusco@analog.com>
> + * Author: Marcelo Schmitt <marcelo.schmitt@analog.com>
> + */

...

> +#define AD4170_DEFAULT_SAMP_RATE (125 * KILO)

HZ_PER_KHZ ?

...

> +/* Internal and external clock properties */
> +#define AD4170_INT_CLOCK_16MHZ (16 * MEGA)

HZ_PER_MHZ

> +#define AD4170_EXT_CLOCK_MHZ_MIN (1 * MEGA)
> +#define AD4170_EXT_CLOCK_MHZ_MAX (17 * MEGA)

Ditto. But are you sure that it's 17 and not 16?

...

> +static const unsigned int ad4170_sinc3_filt_fs_tbl[] = {
> + 4, 8, 12, 16, 20, 40, 48, 80, 100, 256, 500, 1000, 5000, 8332, 10000,
> + 25000, 50000, 65532,

Please, make something like 4 or 8 per line with a comment.

4, 8, 12, 16, 20, 40, 48, 80, /* 0 - 7 */
... /* 8 - 15 */
...

> +};

...

> +struct ad4170_chan_info {
> + int setup_num; /* Index to access state setup_infos array */
> + struct ad4170_setup setup; /* cached setup */
> + int input_range_uv;
> + u32 scale_tbl[10][2];
> + int offset_tbl[10];
> + bool initialized;
> + bool enabled;

`pahole` approved?

> +};

...

> +struct ad4170_state {

Ditto.

> + struct regmap *regmap;
> + struct spi_device *spi;
> + int vrefs_uv[AD4170_MAX_SUP];
> + struct mutex lock; /* Protect read-modify-write and multi write sequences */
> + struct iio_chan_spec chans[AD4170_MAX_CHANNELS];
> + struct ad4170_chan_info chan_infos[AD4170_MAX_CHANNELS];
> + struct ad4170_setup_info setup_infos[AD4170_MAX_SETUPS];
> + u32 mclk_hz;
> + int pins_fn[AD4170_NUM_ANALOG_PINS];
> + u32 int_pin_sel;
> + int sps_tbl[ARRAY_SIZE(ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE][2];
> + struct completion completion;
> + /*
> + * DMA (thus cache coherency maintenance) requires the transfer buffers
> + * to live in their own cache lines.
> + */
> + u8 tx_buf[AD4170_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> + u8 rx_buf[AD4170_SPI_MAX_XFER_LEN];
> +};

...

> +static int ad4170_debugfs_reg_access(struct iio_dev *indio_dev,
> + unsigned int reg, unsigned int writeval,
> + unsigned int *readval)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);

> + int ret = -EINVAL;

Redundant assignment.

> + if (readval)
> + ret = regmap_read(st->regmap, reg, readval);
> + else
> + ret = regmap_write(st->regmap, reg, writeval);
> +
> + return ret;
> +}

...

> +static int ad4170_find_setup(struct ad4170_state *st,
> + struct ad4170_setup *target_setup,
> + unsigned int *setup_num, bool *overwrite)
> +{
> + unsigned int i;
> +
> + *setup_num = AD4170_INVALID_SETUP;
> + *overwrite = false;
> +
> + for (i = 0; i < AD4170_MAX_SETUPS; i++) {
> + struct ad4170_setup_info *setup_info = &st->setup_infos[i];
> +
> + /* Immediately accept a matching setup. */
> + if (!memcmp(target_setup, &setup_info->setup,
> + sizeof(*target_setup))) {

Does this handle paddings okay?

> + *setup_num = i;
> + return 0;
> + }
> +
> + /* Ignore all setups which are used by enabled channels. */
> + if (setup_info->enabled_channels)
> + continue;
> +
> + /* Find the least used slot. */
> + if (*setup_num == AD4170_INVALID_SETUP ||
> + setup_info->channels < st->setup_infos[*setup_num].channels)
> + *setup_num = i;
> + }
> +
> + if (*setup_num == AD4170_INVALID_SETUP)
> + return -EINVAL;
> +
> + *overwrite = true;
> + return 0;
> +}

...

> +static int ad4170_link_channel_setup(struct ad4170_state *st,
> + unsigned int chan_addr,
> + unsigned int setup_num)
> +{
> + struct ad4170_setup_info *setup_info = &st->setup_infos[setup_num];
> + struct ad4170_chan_info *chan_info = &st->chan_infos[chan_addr];
> + int ret;
> +
> + ret = regmap_update_bits(st->regmap, AD4170_CHAN_SETUP_REG(chan_addr),
> + AD4170_CHAN_SETUP_SETUP_MSK,

> + FIELD_PREP(AD4170_CHAN_SETUP_SETUP_MSK,
> + setup_num));

I think it's fine to have these two on one line.

> + if (ret)
> + return ret;
> +
> + chan_info->setup_num = setup_num;
> + setup_info->channels++;
> + return 0;
> +}

...

> + /* Case 4 */
> + if (on_enable && chan_info->setup_num != AD4170_INVALID_SETUP)
> + return 0;
> +
> + if (!on_enable && !chan_info->enabled) {
> + if (chan_info->setup_num != AD4170_INVALID_SETUP)
> + /* Case 3 */
> + ad4170_unlink_channel(st, chan_addr);
> +
> + /* Cases 3 & 5 */
> + return 0;
> + }

I believe this can be refactored a bit.

/* Cases 3, 4, and 5 */
if (chan_info->setup_num != AD4170_INVALID_SETUP) {
/* Case 4 */
if (on_enable)
return 0;

/* Case 3 */
if (!chan_info->enabled) {
ad4170_unlink_channel(st, chan_addr);
return 0;
}
} else if (!on_enable && !chan_info->enabled) {
/* Case 5 */
return 0;
}

In my opinion it gives each case separated and hence better understanding of
the code flow. For the curiosity one may check the object file sizes (with help
of bloat-o-meter script).

...

> +static int ad4170_set_filter_type(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + unsigned int val)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address];
> + struct ad4170_setup *setup = &chan_info->setup;
> + unsigned int old_filter_fs, old_filter, filter_type_conf;

> + int ret = 0;

Redundant assignment.

...

> + switch (ain_n) {
> + case AD4170_CHAN_MAP_AVDD_AVSS_N:
> + *ain_voltage = (st->vrefs_uv[AD4170_AVDD_SUP]
> + - st->vrefs_uv[AD4170_AVSS_SUP]) / 5;

Please use better indentation and split.

... tmp;

tmp = st->vrefs_uv[AD4170_AVDD_SUP] - st->vrefs_uv[AD4170_AVSS_SUP];
*ain_voltage = tmp / 5;

> + return 0;
> + case AD4170_CHAN_MAP_IOVDD_DGND_N:
> + *ain_voltage = st->vrefs_uv[AD4170_IOVDD_SUP] / 5;
> + return 0;
> + case AD4170_CHAN_MAP_AVSS:
> + *ain_voltage = st->vrefs_uv[AD4170_AVSS_SUP];
> + return 0;
> + case AD4170_CHAN_MAP_DGND:
> + *ain_voltage = 0;
> + return 0;
> + case AD4170_CHAN_MAP_REFIN1_P:
> + if (st->vrefs_uv[AD4170_REFIN1P_SUP] == -ENODEV)
> + return dev_err_probe(dev, -ENODEV,
> + "input set to REFIN+ but ref not provided\n");
> +
> + *ain_voltage = st->vrefs_uv[AD4170_REFIN1P_SUP];
> + return 0;
> + case AD4170_CHAN_MAP_REFIN1_N:
> + if (st->vrefs_uv[AD4170_REFIN1N_SUP] == -ENODEV)
> + return dev_err_probe(dev, -ENODEV,
> + "input set to REFIN- but ref not provided\n");
> +
> + *ain_voltage = st->vrefs_uv[AD4170_REFIN1N_SUP];
> + return 0;
> + case AD4170_CHAN_MAP_REFIN2_P:
> + if (st->vrefs_uv[AD4170_REFIN2P_SUP] == -ENODEV)
> + return dev_err_probe(dev, -ENODEV,
> + "input set to REFIN2+ but ref not provided\n");
> +
> + *ain_voltage = st->vrefs_uv[AD4170_REFIN2P_SUP];
> + return 0;
> + case AD4170_CHAN_MAP_REFIN2_N:
> + if (st->vrefs_uv[AD4170_REFIN2N_SUP] == -ENODEV)
> + return dev_err_probe(dev, -ENODEV,
> + "input set to REFIN2- but ref not provided\n");
> +
> + *ain_voltage = st->vrefs_uv[AD4170_REFIN2N_SUP];
> + return 0;
> + case AD4170_CHAN_MAP_REFOUT:
> + /* REFOUT is 2.5V relative to AVSS so take that into account */
> + *ain_voltage = st->vrefs_uv[AD4170_AVSS_SUP] + (2500 * MILLI);

Using 2500000 here is probably slightly better choice as MILLI a bit ambiguous
(since the result is in MICROvolts).

> + return 0;
> + }

> + return -EINVAL;

Make this default case.

...

> +static int ad4170_validate_channel(struct ad4170_state *st,
> + struct iio_chan_spec const *chan)
> +{
> + int ret;
> +
> + ret = ad4170_validate_channel_input(st, chan->channel, false);
> + if (ret < 0)

Why ' < 0'? Same Q to all similar checks. (And here you even have it
inconsistent with the below)

> + return ret;
> +
> + return ad4170_validate_channel_input(st, chan->channel2,
> + !chan->differential);
> +}

...

> +static int ad4170_get_input_range(struct ad4170_state *st,
> + struct iio_chan_spec const *chan,
> + unsigned int ch_reg, unsigned int ref_sel)
> +{
> + bool bipolar = chan->scan_type.sign == 's';
> + struct device *dev = &st->spi->dev;
> + int refp, refn, ain_voltage, ret;
> +
> + switch (ref_sel) {
> + case AD4170_REF_REFIN1:
> + if (st->vrefs_uv[AD4170_REFIN1P_SUP] == -ENODEV ||
> + st->vrefs_uv[AD4170_REFIN1N_SUP] == -ENODEV)
> + return dev_err_probe(dev, -ENODEV,
> + "REFIN+, REFIN− selected but not provided\n");

Maybe

"REFIN± selected but not provided\n");

?

> + refp = st->vrefs_uv[AD4170_REFIN1P_SUP];
> + refn = st->vrefs_uv[AD4170_REFIN1N_SUP];
> + break;
> + case AD4170_REF_REFIN2:
> + if (st->vrefs_uv[AD4170_REFIN2P_SUP] == -ENODEV ||
> + st->vrefs_uv[AD4170_REFIN2N_SUP] == -ENODEV)
> + return dev_err_probe(dev, -ENODEV,
> + "REFIN2+, REFIN2− selected but not provided\n");

Ditto.

> + refp = st->vrefs_uv[AD4170_REFIN2P_SUP];
> + refn = st->vrefs_uv[AD4170_REFIN2N_SUP];
> + break;
> + case AD4170_REF_AVDD:
> + refp = st->vrefs_uv[AD4170_AVDD_SUP];
> + refn = st->vrefs_uv[AD4170_AVSS_SUP];
> + break;
> + case AD4170_REF_REFOUT:
> + /* REFOUT is 2.5 V relative to AVSS */
> + refp = st->vrefs_uv[AD4170_AVSS_SUP] + (2500 * MILLI);

2500000 ? Perhaps define this constant. I see it second time already.

> + refn = st->vrefs_uv[AD4170_AVSS_SUP];
> + break;
> + default:
> + return -EINVAL;
> + }

> + /*
> + * Find out the analog input range from the channel type, polarity, and
> + * voltage reference selection.
> + * AD4170 channels are either differential or pseudo-differential.
> + * Diff input voltage range: −VREF/gain to +VREF/gain (datasheet page 6)
> + * Pseudo-diff input voltage range: 0 to VREF/gain (datasheet page 6)
> + */
> + if (chan->differential) {
> + if (!bipolar)
> + return dev_err_probe(&st->spi->dev, -EINVAL,
> + "Channel %u differential unipolar\n",
> + ch_reg);
> +
> + /*
> + * Differential bipolar channel.
> + * avss-supply is never above 0V.
> + * Assuming refin1n-supply not above 0V.
> + * Assuming refin2n-supply not above 0V.
> + */
> + return refp + abs(refn);
> + }
> + /*
> + * Some configurations can lead to invalid setups.
> + * For example, if AVSS = -2.5V, REF_SELECT set to REFOUT (REFOUT/AVSS),
> + * and pseudo-diff channel configuration set, then the input range
> + * should go from 0V to +VREF (single-ended - datasheet pg 10), but
> + * REFOUT/AVSS range would be -2.5V to 0V.
> + * Check the positive reference is higher than 0V for pseudo-diff
> + * channels.
> + */
> + if (refp <= 0)
> + return dev_err_probe(&st->spi->dev, -EINVAL,
> + "REF+ <= GND for pseudo-diff chan %u\n",
> + ch_reg);
> +
> + if (bipolar)
> + return refp;
> +
> + /*
> + * Pseudo-differential unipolar channel.
> + * Input expected to swing from IN- to +VREF.
> + */
> + ret = ad4170_get_ain_voltage_uv(st, chan->channel2, &ain_voltage);
> + if (ret < 0)
> + return ret;
> +
> + if (refp - ain_voltage <= 0)
> + return dev_err_probe(&st->spi->dev, -EINVAL,
> + "Negative input >= REF+ for pseudo-diff chan %u\n",
> + ch_reg);
> +
> + return refp - ain_voltage;
> +}

...

> + settling_time_ms = DIV_ROUND_UP(6291164 * MILLI, st->mclk_hz);

Would it work on 32-bit? int*long == long.

...

> + ret = ad4170_get_ain_voltage_uv(st, chan->channel2, &ainm_voltage);
> + if (ret < 0)

Why ' < 0' ?

> + return dev_err_probe(&st->spi->dev, ret,

struct device = &st->spi->dev;

> + "Failed to fill scale table\n");

will help in this and many other cases to make them shorter.

...

> + nv = (u64)chan_info->input_range_uv * NANO;
> + lshift = (pga >> 3 & 1); /* handle cases 8 and 9 */

lshift = !!(pga & BIT(3));

?

> + rshift = precision_bits - bipolar + (pga & 0x7) - lshift;

GENMASK() ?

> + chan_info->scale_tbl[pga][0] = 0;
> + chan_info->scale_tbl[pga][1] = div_u64(nv >> rshift, MILLI);

...

> + for (i = 0; i < filt_fs_tbl_size; i++) {
> + if (st->sps_tbl[f_type][i][0] == val &&
> + st->sps_tbl[f_type][i][1] == val2)
> + break;
> + }
> + if (i >= filt_fs_tbl_size)

Why '>' ?

> + return -EINVAL;

...

> + ret = fwnode_property_read_u32_array(child, "diff-channels", pins,
> + ARRAY_SIZE(pins));
> + if (!ret) {

Use usual patter, checking for errors first.

> + chan->differential = true;
> + chan->channel = pins[0];
> + chan->channel2 = pins[1];
> + return 0;
> + }
> + return dev_err_probe(dev, ret,
> + "Channel must define one of diff-channels or single-channel.\n");
> +}

...

> +static int ad4170_parse_channels(struct iio_dev *indio_dev)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->spi->dev;
> + unsigned int num_channels;
> + unsigned int chan_num;
> + int ret;
> +
> + num_channels = device_get_child_node_count(dev);

> +

Redundant blank line.

> + if (num_channels > AD4170_MAX_CHANNELS)
> + return dev_err_probe(dev, -EINVAL, "Too many channels\n");
> +
> + device_for_each_child_node_scoped(dev, child) {
> + ret = ad4170_parse_channel_node(indio_dev, child, chan_num++);
> + if (ret)
> + return ret;
> + }
> +
> + indio_dev->num_channels = num_channels;
> + indio_dev->channels = st->chans;
> + return 0;
> +}

...

> +static int ad4170_parse_firmware(struct iio_dev *indio_dev)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->spi->dev;
> + int reg_data, ret, i;

Why i is signed? Ditto for many similar cases.

> + st->mclk_hz = AD4170_INT_CLOCK_16MHZ;
> +
> + for (i = 0; i < AD4170_NUM_ANALOG_PINS; i++)
> + st->pins_fn[i] = AD4170_PIN_UNASIGNED;
> +
> + /* On power on, device defaults to using SDO pin for data ready signal */
> + st->int_pin_sel = AD4170_INT_PIN_SDO;
> + ret = device_property_match_property_string(dev, "interrupt-names",
> + ad4170_int_pin_names,
> + ARRAY_SIZE(ad4170_int_pin_names));
> + if (ret >= 0)
> + st->int_pin_sel = ret;
> +
> + reg_data = FIELD_PREP(AD4170_PIN_MUXING_DIG_AUX1_CTRL_MSK,
> + st->int_pin_sel == AD4170_INT_PIN_DIG_AUX1 ?
> + AD4170_PIN_MUXING_DIG_AUX1_RDY :
> + AD4170_PIN_MUXING_DIG_AUX1_DISABLED);
> +
> + ret = regmap_update_bits(st->regmap, AD4170_PIN_MUXING_REG,
> + AD4170_PIN_MUXING_DIG_AUX1_CTRL_MSK, reg_data);
> + if (ret)
> + return ret;
> +
> + return ad4170_parse_channels(indio_dev);
> +}

...

This was really hard to review and I am sure I missed something.

Please, split this to 3+ patches (basic + feature1 + feature2 + ...).

--
With Best Regards,
Andy Shevchenko