| 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: <aW3dxuelYDM67pqZ@smile.fi.intel.com>
Date: Mon, 19 Jan 2026 09:31:18 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: rodrigo.alencar@...log.com
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
Jonathan Cameron <jic23@...nel.org>,
David Lechner <dlechner@...libre.com>,
Andy Shevchenko <andy@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH v4 3/7] iio: frequency: adf41513: driver implementation
On Fri, Jan 16, 2026 at 02:32:22PM +0000, Rodrigo Alencar via B4 Relay wrote:
> The driver is based on existing PLL drivers in the IIO subsystem and
> implements the following key features:
>
> - Integer-N and fractional-N (fixed/variable modulus) synthesis modes
> - High-resolution frequency calculations using microhertz (µHz) precision
> to handle sub-Hz resolution across multi-GHz frequency ranges
> - IIO debugfs interface for direct register access
> - FW property parsing from devicetree including charge pump settings,
> reference path configuration and muxout options
> - Power management support with suspend/resume callbacks
> - Lock detect GPIO monitoring
>
> The driver uses 64-bit microhertz values throughout PLL calculations to
> maintain precision when working with frequencies that exceed 32-bit Hz
> representation while requiring fractional Hz resolution.
...
> +#define ADF41513_MAX_PFD_FREQ_INT_N_UHZ (250ULL * MEGA * MICROHZ_PER_HZ)
> +#define ADF41513_MAX_PFD_FREQ_FRAC_N_UHZ (125ULL * MEGA * MICROHZ_PER_HZ)
> +#define ADF41513_MAX_FREQ_RESOLUTION_UHZ (100ULL * KILO * MICROHZ_PER_HZ)
Yep, thanks, looks much better now!
...
> +struct adf41513_chip_info {
> + bool has_prescaler_8_9;
> + u64 max_rf_freq_hz;
If you swap them, it might be a 4 bytes gain in some cases. Have you run
`pahole`?
> +};
...
> +struct adf41513_pll_settings {
> + enum adf41513_pll_mode mode;
Sounds to me like a room to improve the layout here,
> + /* reference path parameters */
> + u8 r_counter;
> + u8 ref_doubler;
> + u8 ref_div2;
> + u8 prescaler;
> +
> + /* frequency parameters */
> + u64 target_frequency_uhz;
> + u64 actual_frequency_uhz;
> + u64 pfd_frequency_uhz;
> +
> + /* pll parameters */
> + u16 int_value;
> + u32 frac1;
> + u32 frac2;
> + u32 mod2;
> +};
...
> + * See iio_write_channel_info and __iio_str_to_fixpoint in
Refer to function as func(), i.o.w. mind the parentheses.
> + * drivers/iio/industrialio-core.c
Missing period at the end.
...
> +static int adf41513_parse_uhz(const char *str, u64 *freq_uhz)
> +{
> + u64 uhz = 0;
> + int f_count = ADF41513_HZ_DECIMAL_PRECISION;
> + bool frac_part = false;
> +
> + if (str[0] == '+')
> + str++;
> +
> + while (*str && f_count > 0) {
> + if ('0' <= *str && *str <= '9') {
> + uhz = uhz * 10 + *str - '0';
> + if (frac_part)
> + f_count--;
> + } else if (*str == '\n') {
> + if (*(str + 1) == '\0')
> + break;
> + return -EINVAL;
> + } else if (*str == '.' && !frac_part) {
This can be found by strchr() / strrchr() (depending on the expectations of
the input).
> + frac_part = true;
> + } else {
> + return -EINVAL;
> + }
> + str++;
> + }
With the above the rest becomes just a couple of simple_strtoull() calls with
a couple of int_pow(10) calls (and some validation on top).
> + for (; f_count > 0; f_count--)
> + uhz *= 10;
This is int_pow(10).
> + *freq_uhz = uhz;
> +
> + return 0;
> +}
...
> +static int adf41513_uhz_to_str(u64 freq_uhz, char *buf)
> +{
> + u32 frac_part;
> + u64 int_part = div_u64_rem(freq_uhz, MICRO, &frac_part);
Perhaps MICROHZ_PER_HZ? This will be consistent with the int_value in
_calc_*() below.
> + return sysfs_emit(buf, "%llu.%06u\n", int_part, frac_part);
> +}
...
> + cfg->pfd_frequency_uhz = div_u64(cfg->pfd_frequency_uhz,
> + cfg->r_counter);
Here 81 characters...
> + cfg->actual_frequency_uhz = (u64)cfg->int_value * cfg->pfd_frequency_uhz;
...and here, but in one case the line is wrapped. I would unwrap the first one.
...
> + result->ref_div2 = st->data.ref_div2_en ? 1 : 0;
> + result->ref_doubler = st->data.ref_doubler_en ? 1 : 0;
Seems like "? 1 : 0" just redundant.
...
> +static int adf41513_calc_integer_n(struct adf41513_state *st,
> + struct adf41513_pll_settings *result)
> +{
> + u16 max_int = (st->chip_info->has_prescaler_8_9) ?
Redundant parentheses.
> + ADF41513_MAX_INT_8_9 : ADF41513_MAX_INT_4_5;
> + u64 freq_error_uhz;
> + u16 int_value = div64_u64_rem(result->target_frequency_uhz, result->pfd_frequency_uhz,
> + &freq_error_uhz);
> + /* check if freq error is within a tolerance of 1/2 resolution */
> + if (freq_error_uhz > (result->pfd_frequency_uhz >> 1) && int_value < max_int) {
> + int_value++;
> + freq_error_uhz = result->pfd_frequency_uhz - freq_error_uhz;
> + }
This and below the part for frac check seems very similar, I would consider
adding a helper.
> + if (freq_error_uhz > st->data.freq_resolution_uhz)
> + return -ERANGE;
> +
> + /* set prescaler */
> + if (st->chip_info->has_prescaler_8_9 && int_value >= ADF41513_MIN_INT_8_9 &&
> + int_value <= ADF41513_MAX_INT_8_9)
> + result->prescaler = 1;
> + else if (int_value >= ADF41513_MIN_INT_4_5 && int_value <= ADF41513_MAX_INT_4_5)
> + result->prescaler = 0;
> + else
> + return -ERANGE;
> +
> + result->actual_frequency_uhz = (u64)int_value * result->pfd_frequency_uhz;
> + result->mode = ADF41513_MODE_INTEGER_N;
> + result->int_value = int_value;
> + result->frac1 = 0;
> + result->frac2 = 0;
> + result->mod2 = 0;
> +
> + return 0;
> +}
...
> +static int adf41513_calc_variable_mod(struct adf41513_state *st,
> + struct adf41513_pll_settings *result)
> +{
> + u64 freq_error_uhz;
> + u32 frac1, frac2, mod2;
> + u16 int_value = div64_u64_rem(result->target_frequency_uhz,
> + result->pfd_frequency_uhz,
> + &freq_error_uhz);
> +
> + if (st->chip_info->has_prescaler_8_9 && int_value >= ADF41513_MIN_INT_FRAC_8_9 &&
> + int_value <= ADF41513_MAX_INT_8_9)
> + result->prescaler = 1;
> + else if (int_value >= ADF41513_MIN_INT_FRAC_4_5 && int_value <= ADF41513_MAX_INT_4_5)
> + result->prescaler = 0;
> + else
> + return -ERANGE;
> + /* calculate required mod2 based on target resolution / 2 */
> + mod2 = DIV64_U64_ROUND_CLOSEST(result->pfd_frequency_uhz << 1,
> + st->data.freq_resolution_uhz * ADF41513_FIXED_MODULUS);
This also seems familiar with the above mentioned check (for 50% tolerance).
> + /* ensure mod2 is at least 2 for meaningful operation */
> + mod2 = clamp(mod2, 2, ADF41513_MAX_MOD2);
> +
> + /* calculate frac1 and frac2 */
> + frac1 = mul_u64_u64_div_u64(freq_error_uhz, ADF41513_FIXED_MODULUS,
> + result->pfd_frequency_uhz);
> + freq_error_uhz -= mul_u64_u64_div_u64(frac1, result->pfd_frequency_uhz,
> + ADF41513_FIXED_MODULUS);
> + frac2 = mul_u64_u64_div_u64(freq_error_uhz, (u64)mod2 * ADF41513_FIXED_MODULUS,
I'm wondering why mod2 can't be defined as u64. This will allow to drop castings.
> + result->pfd_frequency_uhz);
> +
> + /* integer part */
> + result->actual_frequency_uhz = (u64)int_value * result->pfd_frequency_uhz;
> + /* fractional part */
> + result->actual_frequency_uhz += mul_u64_u64_div_u64((u64)frac1 * mod2 + frac2,
> + result->pfd_frequency_uhz,
> + (u64)mod2 * ADF41513_FIXED_MODULUS);
> + result->mode = ADF41513_MODE_VARIABLE_MODULUS;
> + result->int_value = int_value;
> + result->frac1 = frac1;
> + result->frac2 = frac2;
> + result->mod2 = mod2;
> +
> + return 0;
> +}
...
> + /* apply computed results to pll settings */
> + memcpy(&st->settings, &result, sizeof(st->settings));
Can't we simply use
st->settings = result;
?
...
> +static ssize_t adf41513_read_powerdown(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + char *buf)
> +{
> + struct adf41513_state *st = iio_priv(indio_dev);
> + u32 val;
> +
> + guard(mutex)(&st->lock);
> +
> + switch ((u32)private) {
Why casting? Ditto for similar cases.
> + case ADF41513_POWER_DOWN:
> + val = FIELD_GET(ADF41513_REG6_POWER_DOWN_MSK,
> + st->regs_hw[ADF41513_REG6]);
> + return sysfs_emit(buf, "%u\n", val);
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static ssize_t adf41513_write_uhz(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len)
> +{
> + struct adf41513_state *st = iio_priv(indio_dev);
> + u64 freq_uhz;
> + int ret;
> +
> + ret = adf41513_parse_uhz(buf, &freq_uhz);
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&st->lock);
> +
> + switch ((u32)private) {
> + case ADF41513_FREQ:
> + ret = adf41513_set_frequency(st, freq_uhz, ADF41513_SYNC_DIFF);
> + break;
> + case ADF41513_FREQ_RESOLUTION:
> + if (freq_uhz == 0 || freq_uhz > ADF41513_MAX_FREQ_RESOLUTION_UHZ)
> + return -EINVAL;
> + st->data.freq_resolution_uhz = freq_uhz;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return ret ? ret : len;
It can be Elvis operator:
return ret ?: len;
Ditto for similar cases.
> +}
...
> +static int adf41513_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long info)
> +{
> + struct adf41513_state *st = iio_priv(indio_dev);
> + u64 phase_urad;
> + u16 phase_val;
> +
> + guard(mutex)(&st->lock);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_PHASE:
> + phase_val = FIELD_GET(ADF41513_REG2_PHASE_VAL_MSK,
> + st->regs_hw[ADF41513_REG2]);
> + phase_urad = (u64)phase_val * ADF41513_MAX_PHASE_MICRORAD;
> + phase_urad >>= 12;
> + *val = (u32)phase_urad / MICRO;
> + *val2 = (u32)phase_urad % MICRO;
This needs a short comment explaining why castings are okay
(i.o.w. why the 64-bit variable will contain 32-bit value).
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int adf41513_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long info)
> +{
> + struct adf41513_state *st = iio_priv(indio_dev);
> + u64 phase_urad;
> + u16 phase_val;
> +
> + guard(mutex)(&st->lock);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_PHASE:
> + phase_urad = (u64)val * MICRO + val2;
> + if (val < 0 || val2 < 0 || phase_urad >= ADF41513_MAX_PHASE_MICRORAD)
> + return -EINVAL;
It's better to check val* before use them.
> + phase_val = DIV_U64_ROUND_CLOSEST(phase_urad << 12,
> + ADF41513_MAX_PHASE_MICRORAD);
> + phase_val = min(phase_val, ADF41513_MAX_PHASE_VAL);
> + st->regs[ADF41513_REG2] |= ADF41513_REG2_PHASE_ADJ_MSK;
> + FIELD_MODIFY(ADF41513_REG2_PHASE_VAL_MSK,
> + &st->regs[ADF41513_REG2], phase_val);
> + return adf41513_sync_config(st, ADF41513_SYNC_REG0);
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static void adf41513_power_down(void *data)
> +{
> + struct adf41513_state *st = data;
> +
> + adf41513_suspend(st);
> + if (st->chip_enable)
Remove this dup check.
> + gpiod_set_value_cansleep(st->chip_enable, 0);
> +}
...
> +static int adf41513_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct adf41513_state *st;
> + struct device *dev = &spi->dev;
> + int ret;
Please, use reversed xmas tree ordering.
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> + st->chip_info = spi_get_device_match_data(spi);
> + if (!st->chip_info)
> + return -EINVAL;
> +
> + spi_set_drvdata(spi, st);
> +
> + st->ref_clk = devm_clk_get_enabled(dev, NULL);
> + if (IS_ERR(st->ref_clk))
> + return PTR_ERR(st->ref_clk);
> +
> + st->ref_freq_hz = clk_get_rate(st->ref_clk);
> + if (st->ref_freq_hz < ADF41513_MIN_REF_FREQ_HZ ||
> + st->ref_freq_hz > ADF41513_MAX_REF_FREQ_HZ)
> + return dev_err_probe(dev, -ERANGE,
> + "reference frequency %u Hz out of range\n",
> + st->ref_freq_hz);
> +
> + ret = adf41513_parse_fw(st);
> + if (ret)
> + return ret;
> +
> + ret = devm_regulator_bulk_get_enable(dev,
> + ARRAY_SIZE(adf41513_power_supplies),
> + adf41513_power_supplies);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to get and enable regulators\n");
> +
> + st->chip_enable = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> + if (IS_ERR(st->chip_enable))
> + return dev_err_probe(dev, PTR_ERR(st->chip_enable),
> + "fail to request chip enable GPIO\n");
> +
> + st->lock_detect = devm_gpiod_get_optional(dev, "lock-detect", GPIOD_IN);
> + if (IS_ERR(st->lock_detect))
> + return dev_err_probe(dev, PTR_ERR(st->lock_detect),
> + "fail to request lock detect GPIO\n");
> +
> + ret = devm_mutex_init(dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->info = &adf41513_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = &adf41513_chan;
> + indio_dev->num_channels = 1;
> +
> + ret = adf41513_setup(st);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed to setup device\n");
> +
> + ret = devm_add_action_or_reset(dev, adf41513_power_down, st);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to add power down action\n");
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists