Re: [PATCH v11 09/11] iio: frequency: adf41513: features on frequency change

[Rodrigo Alencar]

On 26/05/06 03:08PM, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> 
> Set Bleed current when PFD frequency changes (bleed enabled when in
> fractional mode). Set lock detector window size, handling bias and
> precision. Add phase resync support, setting clock dividers when
> PFD frequency changes.

Sashiko's feedback:
https://sashiko.dev/#/patchset/20260506-adf41513-iio-driver-v11-0-2b7e99cfe8f2%40analog.com?part=9

...

> +static void adf41513_set_phase_resync(struct adf41513_state *st)
> +{
> +	u32 total_div, clk1_div, clk2_div;
> +
> +	if (!st->data.phase_resync_period_ns)
> +		return;
> +
> +	/* assuming both clock dividers hold similar values */
> +	total_div = mul_u64_u64_div_u64(st->settings.pfd_frequency_uhz,
> +					st->data.phase_resync_period_ns,
> +					1ULL * MICROHZ_PER_HZ * NSEC_PER_SEC);
> +	clk1_div = clamp(int_sqrt(total_div), 1,
> +			 ADF41513_MAX_CLK_DIVIDER);
> +	clk2_div = clamp(DIV_ROUND_CLOSEST(total_div, clk1_div), 1,
> +			 ADF41513_MAX_CLK_DIVIDER);
> +
> +	FIELD_MODIFY(ADF41513_REG5_CLK1_DIV_MSK, &st->regs[ADF41513_REG5],
> +		     clk1_div);
> +	FIELD_MODIFY(ADF41513_REG7_CLK2_DIV_MSK, &st->regs[ADF41513_REG7],
> +		     clk2_div);
> +
> +	/* enable phase resync */
> +	st->regs[ADF41513_REG7] |= ADF41513_REG7_CLK_DIV_MODE_MSK;

	Does this accidentally write a reserved hardware state by forcing all bits
	in the mask to 1? 
	If ADF41513_REG7_CLK_DIV_MODE_MSK is a multi-bit field, using the bitwise
	OR operator will set all bits in the field to 1 (e.g., 0b11). Typically,
	phase resync is 0b10, fast lock is 0b01, and 0b11 is reserved.
	Should this use FIELD_MODIFY() with a specific phase resync mode value
	instead of directly OR-ing the mask?

Indeed, it should be using FIELD_MODIFY() here:

	FIELD_MODIFY(ADF41513_REG7_CLK_DIV_MODE_MSK, &st->regs[ADF41513_REG7], 2);

-- 
Kind regards,

Rodrigo Alencar