| 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: <aRITLaJir-2IoclU@smile.fi.intel.com>
Date: Mon, 10 Nov 2025 18:30:37 +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 1/3] iio: frequency: adf41513: driver implementation
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
Powered by blists - more mailing lists