Re: [PATCH v4 3/7] iio: frequency: adf41513: driver implementation

Andy Shevchenko
2026-01-19T07:31:24Z

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