| 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: <aLsclc2WHsbR1jfa@smile.fi.intel.com>
Date: Fri, 5 Sep 2025 20:23:33 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Jonathan Santos <Jonathan.Santos@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, 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, jonath4nns@...il.com
Subject: Re: [PATCH v3 4/4] iio: adc: ad7768-1: add support for ADAQ776x-1
ADC Family
On Fri, Sep 05, 2025 at 06:49:42AM -0300, 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.
>
> 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 the device tree.
...
> +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]);
Is it usual patter in IIO? Otherwise it can be written as
scan_type = iio_get_current_scan_type(dev, dev->channels);
> + if (scan_type->sign == 's')
> + val2 = scan_type->realbits - 1;
> + else
> + val2 = scan_type->realbits;
Wasn't smatch happy about this check?
> + 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;
> + }
My question also was: Are these overflow u32? If not, the second patch is not
needed. Otherwise, how is the previous version supposed to work with unsigned
long type (if I am not mistaken) on 32-bit platforms?
> + 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);
> + 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_calc_pga_gain(struct ad7768_state *st, int gain_int,
> + int gain_fract, int precision)
> +{
> + u64 gain_nano;
> + u32 tmp;
> +
> + gain_nano = gain_int * NANO + gain_fract;
> + gain_nano = clamp(gain_nano, 0, ADAQ776X_GAIN_MAX_NANO);
> + tmp = DIV_ROUND_CLOSEST_ULL(gain_nano << precision, NANO);
> + gain_nano = DIV_ROUND_CLOSEST(st->vref_uv, tmp);
> + if (st->chip->has_variable_aaf)
> + gain_nano = DIV_ROUND_CLOSEST_ULL(gain_nano * 10000,
One space too many.
> + ad7768_aaf_gains_bp[st->aaf_gain]);
> +
> + return find_closest(gain_nano, st->chip->pga_gains,
> + (int)st->chip->num_pga_modes);
Is casting needed?
> +}
...
> + case IIO_CHAN_INFO_SCALE: {
One space too many.
> + int gain_mode;
> +
> + if (!st->chip->has_pga)
> + return -EOPNOTSUPP;
> +
> + if (scan_type->sign == 's')
> + gain_mode = ad7768_calc_pga_gain(st, val, val2,
> + scan_type->realbits - 1);
> + else
> + gain_mode = ad7768_calc_pga_gain(st, val, val2,
> + scan_type->realbits);
> +
> + return ad7768_set_pga_gain(st, gain_mode);
> + }
...
> +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;
> + ret = device_property_read_u32(dev, "adi,aaf-gain-bp", &val);
> + aaf_gain_provided = (ret == 0);
Hmm... It seems rather incorrect check. You should actually return an error
code in case it's provided but can't be read.
I would expect something like
ret = device_property_read_u32(dev, "adi,aaf-gain-bp", &val);
if (ret == -EINVAL)
_gain_provided = false;
else if (ret)
return dev_err_probe(...);
else
_gain_provided = true;
> + if (!aaf_gain_provided && st->chip->has_variable_aaf) {
> + dev_warn(dev, "AAF gain not specified, using default\n");
> + return 0;
> + }
> +
> + if (aaf_gain_provided && !st->chip->has_variable_aaf) {
> + dev_warn(dev, "AAF gain not supported for %s\n", st->chip->name);
> + return 0;
> + }
We can refactor these to have one level indentation less for the switch-case.
if (aaf_gain_provided && !st->chip->has_variable_aaf) {
dev_warn(dev, "AAF gain not supported for %s\n", st->chip->name);
return 0;
}
if (!aaf_gain_provided) {
if (st->chip->has_variable_aaf)
dev_warn(dev, "AAF gain not specified, using default\n");
return 0;
}
> + if (aaf_gain_provided) {
So this one may be dropped.
> + 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;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists