| 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: <CAHp75VcGG_h+wpo7hHL=ERYqbrvvAaufwPAYBsEbRn3dB8-dfA@mail.gmail.com>
Date: Sun, 24 Aug 2025 22:43:23 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Jonathan Santos <Jonathan.Santos@...log.com>
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, Michael.Hennerich@...log.com, lars@...afoo.de,
jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
marcelo.schmitt1@...il.com
Subject: Re: [PATCH v2 4/4] iio: adc: ad7768-1: add support for ADAQ776x-1 ADC Family
On Sun, Aug 24, 2025 at 7:10 AM Jonathan Santos
<Jonathan.Santos@...log.com> wrote:
>
> Add support for ADAQ7767/68/69-1 series, which includes PGIA and
> Anti-aliasing filter (AAF) gains. Unlike the AD7768-1, they do not
> provide a VCM regulator interface.
>
> The PGA gain is configured in run-time through the scale attribute,
> if supported by the device. PGA is enabled and controlled by the GPIO
> controller (GPIOs 0, 1 and 2) provided by the device with the SPI
> interface.
>
> The AAF gain is defined by hardware connections and should be specified
> in device tree.
the device
...
> +enum {
> + AD7768_PGA_GAIN_0,
> + AD7768_PGA_GAIN_1,
> + AD7768_PGA_GAIN_2,
> + AD7768_PGA_GAIN_3,
> + AD7768_PGA_GAIN_4,
> + AD7768_PGA_GAIN_5,
> + AD7768_PGA_GAIN_6,
> + AD7768_PGA_GAIN_7,
> + AD7768_MAX_PGA_GAIN,
No trailing comma for the terminator line and I haven't noticed if
it's even being used in the code...
> +};
...
> +/* PGA and AAF gains in V/V */
> +static const int adaq7768_gains[7] = {
The 7 is redundant. Leave it for the compiler.
> + [AD7768_PGA_GAIN_0] = 325, /* 0.325 */
> + [AD7768_PGA_GAIN_1] = 650, /* 0.650 */
> + [AD7768_PGA_GAIN_2] = 1300, /* 1.300 */
> + [AD7768_PGA_GAIN_3] = 2600, /* 2.600 */
> + [AD7768_PGA_GAIN_4] = 5200, /* 5.200 */
> + [AD7768_PGA_GAIN_5] = 10400, /* 10.400 */
> + [AD7768_PGA_GAIN_6] = 20800 /* 20.800 */
Please, leave a trailing comma.
> +};
> +
> +static const int adaq7769_gains[8] = {
8 is redundant.
> + [AD7768_PGA_GAIN_0] = 1000, /* 1.000 */
> + [AD7768_PGA_GAIN_1] = 2000, /* 2.000 */
> + [AD7768_PGA_GAIN_2] = 4000, /* 4.000 */
> + [AD7768_PGA_GAIN_3] = 8000, /* 8.000 */
> + [AD7768_PGA_GAIN_4] = 16000, /* 16.000 */
> + [AD7768_PGA_GAIN_5] = 32000, /* 32.000 */
> + [AD7768_PGA_GAIN_6] = 64000, /* 64.000 */
> + [AD7768_PGA_GAIN_7] = 128000 /* 128.000 */
Please, leave a trailing comma.
> +};
> +static const int ad7768_aaf_gains[3] = {
3 is redundant,
> + [AD7768_AAF_IN1] = 1000, /* 1.000 */
> + [AD7768_AAF_IN2] = 364, /* 0.364 */
> + [AD7768_AAF_IN3] = 143 /* 0.143 */
Please, leave a trailing comma.
> +};
...
> static const int ad7768_filter_3db_odr_multiplier[] = {
> [AD7768_FILTER_SINC5] = 204, /* 0.204 */
> const unsigned long *available_masks;
> const struct iio_chan_spec *channel_spec;
> int num_channels;
> + const int *pga_gains;
> + int num_pga_modes;
> + int default_pga_mode;
> + int pgia_mode2pin_offset;
> + bool has_pga;
> + bool has_variable_aaf;
> + bool has_vcm_regulator;
> };
Same comments as per below.
...
> struct ad7768_state {
> unsigned int samp_freq;
> unsigned int samp_freq_avail[ARRAY_SIZE(ad7768_mclk_div_rates)];
> unsigned int samp_freq_avail_len;
> + int pga_gain_mode;
> + int aaf_gain;
> + int scale_tbl[ADAQ776X_MAX_GAIN_MODES][2];
Why all signed?
> struct completion completion;
> struct iio_trigger *trig;
> struct gpio_desc *gpio_sync_in;
> struct gpio_chip gpiochip;
> const struct ad7768_chip_info *chip;
> bool en_spi_sync;
> + struct mutex pga_lock; /* protect PGA value access */
> }
Have you run `pahole`? Does it suggest a better layout?
...
> +static void ad7768_fill_scale_tbl(struct iio_dev *dev)
> +{
> + struct ad7768_state *st = iio_priv(dev);
> + const struct iio_scan_type *scan_type;
> + int val, val2, tmp0, tmp1, i;
> + unsigned long denominator, numerator;
struct u32_fract fract;
> + u64 tmp2;
> +
> + scan_type = iio_get_current_scan_type(dev, &dev->channels[0]);
> + if (scan_type->sign == 's')
> + val2 = scan_type->realbits - 1;
> + else
> + val2 = scan_type->realbits;
> +
> + for (i = 0; i < st->chip->num_pga_modes; i++) {
> + /* Convert gain to a fraction format */
> + numerator = st->chip->pga_gains[i];
> + denominator = MILLI;
> + if (st->chip->has_variable_aaf) {
> + numerator *= ad7768_aaf_gains[st->aaf_gain];
> + denominator *= MILLI;
> + }
> +
> + rational_best_approximation(numerator, denominator, INT_MAX, INT_MAX,
> + &numerator, &denominator);
> +
> + val = mult_frac(st->vref_uv, denominator, numerator);
> + /* Would multiply by NANO here, but value is already in milli */
> + tmp2 = shift_right((u64)val * MICRO, val2);
> + tmp0 = (int)div_u64_rem(tmp2, NANO, &tmp1);
Why casting here?
> + st->scale_tbl[i][0] = tmp0; /* Integer part */
> + st->scale_tbl[i][1] = abs(tmp1); /* Fractional part */
> + }
> +}
...
> +static int ad7768_set_pga_gain(struct ad7768_state *st,
> + int gain_mode)
> +{
> + int pgia_pins_value = abs(gain_mode - st->chip->pgia_mode2pin_offset);
> + int check_val;
> + int ret;
> +
> + guard(mutex)(&st->pga_lock);
+ Blank line.
> + /* Check GPIO control register */
> + ret = regmap_read(st->regmap, AD7768_REG_GPIO_CONTROL, &check_val);
> + if (ret < 0)
Here and elsewhere when it makes sense, drop redundant ' < 0' pieces.
It's even inconsistent in the same patch with how you check other
regmap returned calls.
> + return ret;
> + return 0;
> +}
...
> case IIO_CHAN_INFO_SCALE:
> + if (st->chip->has_pga) {
> + guard(mutex)(&st->pga_lock);
> +
> + *val = st->scale_tbl[st->pga_gain_mode][0];
> + *val2 = st->scale_tbl[st->pga_gain_mode][1];
> + return IIO_VAL_INT_PLUS_NANO;
> + }
> +
> *val = (st->vref_uv * 2) / 1000;
> + if (st->chip->has_variable_aaf)
> + *val = (*val * MILLI) / ad7768_aaf_gains[st->aaf_gain];
This is unreadable. Just use a temporary variable for all calculations
and when it's done, assign *val to it.
> *val2 = scan_type->realbits;
>
> return IIO_VAL_FRACTIONAL_LOG2;
...
> +static int ad7768_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> +
> + return -EINVAL;
What's the point in this return, please?
> +}
...
> static const struct ad7768_chip_info ad7768_chip_info = {
> .channel_spec = ad7768_channels,
> .num_channels = ARRAY_SIZE(ad7768_channels),
> .available_masks = ad7768_channel_masks,
> + .has_vcm_regulator = true
Missing trailing comma. Also same in the below. I believe I have
pointed that out in v1.
> +};
> +
> +static const struct ad7768_chip_info adaq7767_chip_info = {
> + .name = "adaq7767-1",
> + .channel_spec = ad7768_channels,
> + .num_channels = ARRAY_SIZE(ad7768_channels),
> + .available_masks = ad7768_channel_masks,
> + .has_variable_aaf = true
> +};
> +static const struct ad7768_chip_info adaq7769_chip_info = {
> + .name = "adaq7769-1",
> + .channel_spec = adaq776x_channels,
> + .num_channels = ARRAY_SIZE(adaq776x_channels),
> + .available_masks = ad7768_channel_masks,
> + .pga_gains = adaq7769_gains,
> + .default_pga_mode = AD7768_PGA_GAIN_0,
> + .num_pga_modes = ARRAY_SIZE(adaq7769_gains),
> + .pgia_mode2pin_offset = 0,
> + .has_pga = true,
> + .has_variable_aaf = true
> };
...
> init_completion(&st->completion);
> + mutex_init(&st->pga_lock);
Perhaps devm_mutex_init()?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists