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

["Nuno Sá" <noname.nuno@gmail.com>]

Hi Andy,

On Mon, 2025-11-10 at 18:30 +0200, Andy Shevchenko wrote:
> 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.
> 

I think there's more into this. I did not went into much detail but IIUC it's
doing the same as (to some degree) as __iio_str_to_fixpoint() but the part needs
u64 for the integer part. Being this one case not sure if we should rush into doing
something in IIO core. Or the whole thing might end up being done completely
different.

Having said the above We should have a proper comment justifying the function.

> ...
> 
> 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.


Not really sure what happened here. We are trying to make things better (as more people
in ADI from different teams start contributing) and have a process where internal reviews
do happen (mostly even public in github) and once approved it can go upstream. We also have
a fairly good CI in place to catch things like bindings, checkpatch, smatch, cocci and compiler
analyzers.

For this particular part we did had at least one round of reviewing where I actually stated

"...We'll need some iterations on this one and skipped more questionable/complicated things for now.
..."

I guess Rodrigo was eager to send his first patch series but hopefully, lessons learned.

- Nuno Sá