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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Nov 10, 2025 at 03:44:44PM +0000, Rodrigo Alencar via B4 Relay wrote:
> 
> - ADF41513: 1 GHz to 26.5 GHz frequency range
> - ADF41510: 1 GHz to 10 GHz frequency range
> - Integer-N and fractional-N operation modes
> - Ultra-low phase noise (-235 dBc/Hz integer-N, -231 dBc/Hz fractional-N)
> - High maximum PFD frequency (250 MHz integer-N, 125 MHz fractional-N)
> - 25-bit fixed modulus or 49-bit variable modulus fractional modes
> - Programmable charge pump currents with 16x range
> - Digital lock detect functionality
> - Phase resync capability for consistent output phase
> - Clock framework integration for system clock generation

It is like a list from the marketing material. Please
1) make sure you are writing the commit message;
2) implement minimum basic functionality and split features to the next
patches, 1.5kLoCs is hard to review.

...

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>

At least types.h is missing. Follow IWYU. Have you passed internal review? I
believe we need to start asking Analog Devices to provide a Rb tag of known
developers on the submitted code to make sure it was passed the internal
review.

...

> +/* Specifications */
> +#define ADF41513_MIN_RF_FREQ			1000000000ULL	/* 1 GHz */
> +#define ADF41510_MAX_RF_FREQ			10000000000ULL	/* 10 GHz */
> +#define ADF41513_MAX_RF_FREQ			26500000000ULL	/* 26.5 GHz */

We have HZ_PER_MHZ, also you can move HZ_PER_GHZ to the units.h and use it here.

> +
> +#define ADF41513_MIN_REF_FREQ			10000000U	/* 10 MHz */
> +#define ADF41513_MAX_REF_FREQ			800000000U	/* 800 MHz */
> +#define ADF41513_MAX_REF_FREQ_DOUBLER		225000000U	/* 225 MHz */
> +
> +#define ADF41513_MAX_PFD_FREQ_INT_N_HZ		250000000U		/* 250 MHz */
> +#define ADF41513_MAX_PFD_FREQ_FRAC_N_HZ		125000000U		/* 125 MHz */
> +#define ADF41513_MAX_PFD_FREQ_INT_N_UHZ		250000000000000ULL	/* 250 MHz */
> +#define ADF41513_MAX_PFD_FREQ_FRAC_N_UHZ	125000000000000ULL	/* 125 MHz */

Ditto.

...

> +#define ADF41513_MIN_CP_VOLTAGE_MV		810
> +#define ADF41513_MAX_CP_VOLTAGE_MV		12960

_mV

...

> +#define ADF41513_MAX_LD_BIAS_UA			40
> +#define ADF41513_LD_BIAS_STEP_UA		10

_uA


...

> +#define ADF41513_MAX_MOD2			((1 << 24) - 1)	/* 2^24 - 1 */

Why not BIT()?

...

> +/* Frequency conversion constants */
> +#define ADF41513_HZ_TO_UHZ			1000000ULL	/* Convert Hz to uHz */

Put it to units.h.

...

> +enum {
> +	ADF41513_FREQ,
> +	ADF41513_POWER_DOWN,
> +	ADF41513_FREQ_RESOLUTION,
> +	ADF41513_FREQ_REFIN

Doesn't sound like a terminator to me, add a comma.

> +};
> +
> +enum adf41513_pll_mode {
> +	ADF41513_MODE_INTEGER_N,
> +	ADF41513_MODE_FIXED_MODULUS,
> +	ADF41513_MODE_VARIABLE_MODULUS,
> +	ADF41513_MODE_INVALID

Ditto.

> +};

...

> +struct adf41513_data {

Run `pahole` and act accordingly.

> +	u64 power_up_frequency;
> +
> +	u8 ref_div_factor;
> +	bool ref_doubler_en;
> +	bool ref_div2_en;
> +
> +	u32 charge_pump_voltage_mv;
> +	bool phase_detector_polarity;
> +
> +	u8 muxout_select;
> +	bool muxout_1v8_en;
> +
> +	u8 lock_detect_precision;
> +	u8 lock_detect_count;
> +	u8 lock_detect_bias;
> +	bool fast_lock_en;
> +
> +	u16 phase_resync_clk_div[2];
> +	bool phase_resync_en;
> +	bool load_enable_sync;
> +
> +	u64 freq_resolution_uhz;
> +};
> +
> +struct adf41513_pll_settings {
> +	enum adf41513_pll_mode mode;
> +
> +	u64 target_frequency_uhz;
> +	u64 actual_frequency_uhz;
> +	u64 pfd_frequency_uhz;
> +
> +	/* pll parameters */
> +	u16 int_value;
> +	u32 frac1;
> +	u32 frac2;
> +	u32 mod2;
> +
> +	/* reference path parameters */
> +	u8 r_counter;
> +	u8 ref_doubler;
> +	u8 ref_div2;
> +	u8 prescaler;
> +};

...

> +static const u32 adf41513_cp_voltage_mv[] = {
> +	810, 1620, 2430, 3240, 4050, 4860, 5670, 6480, 7290, 8100,
> +	8910, 9720, 10530, 11340, 12150, 12960

Make it power-of-two items per line, even with the comments to show
the indexing, like

	810, 1620, 2430, 3240, 4050, 4860, 5670, 6480,	/* 0 - 7 */

> +};

...

> +static int adf41513_parse_uhz(const char *str, u64 *freq_uhz)

My gosh, please, try to check what kernel already has. We try hard to avoid Yet
Another Best Parser in the World to happen, really.

...

In any case, I stopped my review here, you have more than enough to fix.
Please, come next time with a tag from one whose name is in the MAINTAINERS.
From now on it will be my requirement as a reviewer of IIO subsystem.

-- 
With Best Regards,
Andy Shevchenko