Re: [PATCH v13 08/12] iio: frequency: adf41513: driver implementation

[Rodrigo Alencar]

On 26/05/17 10:14AM, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> 
> 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 and
>   reference path configuration;
> - Power management support with suspend/resume callbacks;
> - Lock detect GPIO monitoring.

Sashiko's feed back at:
https://sashiko.dev/#/patchset/20260517-adf41513-iio-driver-v13-0-bb6e134a360f%40analog.com?part=8

...

> +static int adf41513_calc_pfd_frequency(struct adf41513_state *st,
> +				       struct adf41513_pll_settings *result,
> +				       u64 fpfd_limit_uhz)
> +{
> +	result->ref_div2 = st->data.ref_div2_en;
> +	result->ref_doubler = st->data.ref_doubler_en;
> +
> +	if (st->data.ref_doubler_en &&
> +	    st->ref_freq_hz > ADF41513_MAX_REF_FREQ_DOUBLER_HZ) {
> +		result->ref_doubler = 0;
> +		dev_warn(&st->spi->dev, "Disabling ref doubler due to high reference frequency\n");

	Will this cause kernel log spam if an application frequently changes the
	frequency? Since ref_doubler_en and ref_freq_hz are constant after probe,
	could we validate this once during initialization or use dev_warn_once()
	here?

Indeed, will move this check to parse_fw() 

> +	}

...

> +static int adf41513_calc_variable_mod(struct adf41513_state *st,
> +				      struct adf41513_pll_settings *result)
> +{
> +	u64 freq_error_uhz, mod2;
> +	u32 frac1, frac2;
> +	u32 int_val = div64_u64_rem(result->target_frequency_uhz,
> +				    result->pfd_frequency_uhz, &freq_error_uhz);
> +
> +	if (st->chip_info->has_prescaler_8_9 && int_val >= ADF41513_MIN_INT_FRAC_8_9 &&
> +	    int_val <= ADF41513_MAX_INT_8_9)
> +		result->prescaler = 1;
> +	else if (int_val >= ADF41513_MIN_INT_FRAC_4_5 && int_val <= 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);
> +	/* 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_u32_div(result->pfd_frequency_uhz, frac1,
> +					  ADF41513_FIXED_MODULUS);
> +	frac2 = mul_u64_u64_div_u64(freq_error_uhz, mod2 * ADF41513_FIXED_MODULUS,
> +				    result->pfd_frequency_uhz);

	Can this calculation result in a frac2 value that exceeds mod2? 
	Because mul_u64_u32_div() truncates the remainder during the integer
	division, the subtracted value might be smaller than the exact mathematical
	contribution of frac1. This leaves freq_error_uhz artificially larger than
	its theoretical bound. 
	When frac2 is computed by multiplying this enlarged error by
	(mod2 * ADF41513_FIXED_MODULUS), couldn't the result exceed mod2? If
	the hardware requires frac2 < mod2, should we cap frac2 or handle the
	remainder differently?

will replace to:

	/* calculate frac1 and frac2 */
	frac1 = mul_u64_u64_div_u64(freq_error_uhz, ADF41513_FIXED_MODULUS,                                                                                                                                                                           
	            result->pfd_frequency_uhz);                                                                                                                                                                                         
	frac2 = mul_u64_u64_div_u64(freq_error_uhz, mod2 * ADF41513_FIXED_MODULUS,                                                                                                                                                                    
	            result->pfd_frequency_uhz) - (u64)frac1 * mod2;

which would remove the possible error introduced by freq_error_uhz -= mul_u64_u32_div().   

> +
> +	/* integer part */
> +	result->actual_frequency_uhz = (u64)int_val * result->pfd_frequency_uhz;

...

-- 
Kind regards,

Rodrigo Alencar