| 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: <aBSsa-QnloxBbBN8@smile.fi.intel.com>
Date: Fri, 2 May 2025 14:28:43 +0300
From: Andy Shevchenko <andy@...nel.org>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org,
Ana-Maria Cusco <ana-maria.cusco@...log.com>, jic23@...nel.org,
lars@...afoo.de, Michael.Hennerich@...log.com,
dlechner@...libre.com, nuno.sa@...log.com, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v2 2/7] iio: adc: Add basic support for AD4170
On Mon, Apr 28, 2025 at 09:28:03AM -0300, Marcelo Schmitt wrote:
> From: Ana-Maria Cusco <ana-maria.cusco@...log.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@...log.com>
> + * Author: Marcelo Schmitt <marcelo.schmitt@...log.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
Powered by blists - more mailing lists