| 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: <598ec4ca-5b36-4ddf-877c-3617711c3aed@baylibre.com>
Date: Fri, 5 Sep 2025 12:08:54 -0500
From: David Lechner <dlechner@...libre.com>
To: Jonathan Santos <Jonathan.Santos@...log.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: lars@...afoo.de, jic23@...nel.org, nuno.sa@...log.com, andy@...nel.org,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
marcelo.schmitt1@...il.com, jonath4nns@...il.com
Subject: Re: [PATCH v3 4/4] iio: adc: ad7768-1: add support for ADAQ776x-1 ADC
Family
On 9/5/25 4:49 AM, Jonathan Santos 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.
>
...
> +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;
> + struct u64_fract fract;
> + unsigned long n, d;
> + 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 */
> + fract.numerator = st->chip->pga_gains[i];
> + fract.denominator = MILLI;
> + if (st->chip->has_variable_aaf) {
> + fract.numerator *= ad7768_aaf_gains_bp[st->aaf_gain];
> + fract.denominator *= 10000;
> + }
> +
> + rational_best_approximation(fract.numerator, fract.denominator,
> + INT_MAX, INT_MAX, &n, &d);
> +
> + val = mult_frac(st->vref_uv, d, n);
> + /* Would multiply by NANO here, but value is already in milli */
> + tmp2 = shift_right((u64)val * MICRO, val2);
shift_right() is only needed for signed values. Can just use >> since this is
dealing with unsigned.
> + tmp0 = div_u64_rem(tmp2, NANO, &tmp1);
> + 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 ret;
> +
> + guard(mutex)(&st->pga_lock);
> +
> + ret = regmap_write(st->regmap, AD7768_REG_GPIO_CONTROL, AD7768_GPIO_PGIA_EN);
Hiding multiple fields in a macro makes it harder to see what this is doing.
> + if (ret)
> + return ret;
> +
> + /* Write the respective gain values to GPIOs 0, 1, 2 */
> + ret = regmap_write(st->regmap, AD7768_REG_GPIO_WRITE,
> + AD7768_GPIO_WRITE(pgia_pins_value));
Hiding the FIELD_PREP() in a macro makes this a bit harder to understand.
> + if (ret)
> + return ret;
> +
> + st->pga_gain_mode = gain_mode;
> +
> + return 0;
> +}
> +
> static int ad7768_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> {
> struct iio_dev *indio_dev = gpiochip_get_data(chip);
> @@ -778,6 +925,10 @@ static const struct iio_chan_spec ad7768_channels[] = {
> AD7768_CHAN(0, 0),
> };
>
> +static const struct iio_chan_spec adaq776x_channels[] = {
> + AD7768_CHAN(0, BIT(IIO_CHAN_INFO_SCALE)),
> +};
> +
> static int ad7768_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long info)
> @@ -805,7 +956,19 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
> return IIO_VAL_INT;
>
> case IIO_CHAN_INFO_SCALE:
> - *val = (st->vref_uv * 2) / 1000;
> + 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;
> + }
> +
> + temp = (st->vref_uv * 2) / 1000;
> + if (st->chip->has_variable_aaf)
> + temp = (temp * 10000) / ad7768_aaf_gains_bp[st->aaf_gain];
Should we add a BASIS_POINTS macro to units.h since we are calling this a
standard unit?
> +
> + *val = temp;
> *val2 = scan_type->realbits;
>
> return IIO_VAL_FRACTIONAL_LOG2;
...
> +static int ad7768_parse_aaf_gain(struct device *dev, struct ad7768_state *st)
> +{
> + bool aaf_gain_provided;
> + u32 val;
> + int ret;
> +
> + st->aaf_gain = AD7768_AAF_IN1;
No sense in setting this if we return early on -EINVAL.
> + ret = device_property_read_u32(dev, "adi,aaf-gain-bp", &val);
> + aaf_gain_provided = (ret == 0);
Only -EINVAL means the property is not present. Other errors can be returned.
Or add a comment explaining why we don't care about other errors.
> + if (!aaf_gain_provided && st->chip->has_variable_aaf) {
> + dev_warn(dev, "AAF gain not specified, using default\n");
Using the default seems like a normal thing to do, so should not be a warning.
> + return 0;
> + }
> +
> + if (aaf_gain_provided && !st->chip->has_variable_aaf) {
> + dev_warn(dev, "AAF gain not supported for %s\n", st->chip->name);
Returning error here would be senseible. Clearly there is something wrong with
the devicetree. But it could be something else, like the wrong compaible string.
> + return 0;
> + }
> +
> + if (aaf_gain_provided) {
> + switch (val) {
> + case 10000:
> + st->aaf_gain = AD7768_AAF_IN1;
> + break;
> + case 3640:
> + st->aaf_gain = AD7768_AAF_IN2;
> + break;
> + case 1430:
> + st->aaf_gain = AD7768_AAF_IN3;
> + break;
> + default:
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid firmware provided AAF gain\n");
> + }
> + }
> +
> + return 0;
> +}
> +
...
> static int ad7768_probe(struct spi_device *spi)
> @@ -1399,7 +1669,13 @@ static int ad7768_probe(struct spi_device *spi)
> indio_dev->modes = INDIO_DIRECT_MODE;
>
> /* Register VCM output regulator */
> - ret = ad7768_register_regulators(&spi->dev, st, indio_dev);
> + if (st->chip->has_vcm_regulator) {
> + ret = ad7768_register_regulators(&spi->dev, st, indio_dev);
ad7768_register_regulators() now seems mis-named since it only handles VMC
supply.
> + if (ret)
> + return ret;
> + }
> +
> + ret = ad7768_parse_aaf_gain(&spi->dev, st);
> if (ret)
> return ret;
>
Powered by blists - more mailing lists