From: Andy Shevchenko <andy@kernel.org>
To: Marcelo Schmitt <marcelo.schmitt@analog.com>
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
Ana-Maria Cusco <ana-maria.cusco@analog.com>,
jic23@kernel.org, lars@metafoo.de, Michael.Hennerich@analog.com,
dlechner@baylibre.com, nuno.sa@analog.com, robh@kernel.org,
krzk+dt@kernel.org, conor+dt@kernel.org,
marcelo.schmitt1@gmail.com
Subject: Re: [PATCH v2 2/7] iio: adc: Add basic support for AD4170
Date: Fri, 2 May 2025 14:28:43 +0300 [thread overview]
Message-ID: <aBSsa-QnloxBbBN8@smile.fi.intel.com> (raw)
In-Reply-To: <01ac3a81f9aa7f1fe48478ff60c0033dd02aefb1.1745841276.git.marcelo.schmitt@analog.com>
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
next prev parent reply other threads:[~2025-05-02 11:28 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-04-28 12:27 [PATCH v2 0/7] iio: adc: Add support for AD4170 series of ADCs Marcelo Schmitt
2025-04-28 12:27 ` [PATCH v2 1/7] dt-bindings: iio: adc: Add AD4170 Marcelo Schmitt
2025-05-09 18:56 ` Rob Herring
2025-05-11 15:27 ` Marcelo Schmitt
2025-04-28 12:28 ` [PATCH v2 2/7] iio: adc: Add basic support for AD4170 Marcelo Schmitt
2025-05-02 11:28 ` Andy Shevchenko [this message]
2025-05-12 13:23 ` Marcelo Schmitt
2025-05-04 18:21 ` Jonathan Cameron
2025-04-28 12:28 ` [PATCH v2 3/7] iio: adc: ad4170: Add support for buffered data capture Marcelo Schmitt
2025-04-29 22:00 ` Andy Shevchenko
2025-04-30 13:40 ` Marcelo Schmitt
2025-05-02 8:56 ` Andy Shevchenko
2025-05-04 17:27 ` Jonathan Cameron
2025-05-04 17:57 ` Jonathan Cameron
2025-04-28 12:28 ` [PATCH v2 4/7] iio: adc: ad4170: Add clock provider support Marcelo Schmitt
2025-04-29 22:10 ` Andy Shevchenko
2025-05-06 8:21 ` kernel test robot
2025-04-28 12:28 ` [PATCH v2 5/7] iio: adc: ad4170: Add GPIO controller support Marcelo Schmitt
2025-04-29 22:14 ` Andy Shevchenko
2025-05-04 17:50 ` Jonathan Cameron
2025-04-28 12:29 ` [PATCH v2 6/7] iio: adc: ad4170: Add support for internal temperature sensor Marcelo Schmitt
2025-04-29 22:16 ` Andy Shevchenko
2025-05-04 17:44 ` Jonathan Cameron
2025-04-28 12:29 ` [PATCH v2 7/7] iio: adc: ad4170: Add support for weigh scale and RTD sensors Marcelo Schmitt
2025-04-29 22:26 ` Andy Shevchenko
2025-05-01 19:50 ` kernel test robot
2025-05-04 17:42 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aBSsa-QnloxBbBN8@smile.fi.intel.com \
--to=andy@kernel.org \
--cc=Michael.Hennerich@analog.com \
--cc=ana-maria.cusco@analog.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dlechner@baylibre.com \
--cc=jic23@kernel.org \
--cc=krzk+dt@kernel.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marcelo.schmitt1@gmail.com \
--cc=marcelo.schmitt@analog.com \
--cc=nuno.sa@analog.com \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.