Re: [PATCH v3 3/6] iio: frequency: adf41513: handle LE synchronization feature

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 08 Jan 2026 12:14:52 +0000
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> When LE sync is enabled, it is must be set after powering up and must be
> disabled when powering down. It is recommended when using the PLL as
> a frequency synthesizer, where reference signal will always be present
> while the device is being configured.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
Hi Rodrigo,

A few comments inline.

> ---
>  drivers/iio/frequency/adf41513.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/frequency/adf41513.c b/drivers/iio/frequency/adf41513.c
> index 69dcbbc1f393..0cdf24989c93 100644
> --- a/drivers/iio/frequency/adf41513.c
> +++ b/drivers/iio/frequency/adf41513.c
> @@ -220,6 +220,7 @@ struct adf41513_data {
>  	bool phase_detector_polarity;
>  
>  	bool logic_lvl_1v8_en;
> +	bool le_sync_en;
>  };
>  
>  struct adf41513_pll_settings {
> @@ -697,13 +698,25 @@ static int adf41513_set_frequency(struct adf41513_state *st, u64 freq_uhz, u16 s
>  static int adf41513_suspend(struct adf41513_state *st)
>  {
>  	st->regs[ADF41513_REG6] |= FIELD_PREP(ADF41513_REG6_POWER_DOWN_MSK, 1);
> +	st->regs[ADF41513_REG12] &= ~ADF41513_REG12_LE_SELECT_MSK;
>  	return adf41513_sync_config(st, ADF41513_SYNC_DIFF);
>  }
>  
>  static int adf41513_resume(struct adf41513_state *st)
>  {
> +	int ret;
> +
>  	st->regs[ADF41513_REG6] &= ~ADF41513_REG6_POWER_DOWN_MSK;
> -	return adf41513_sync_config(st, ADF41513_SYNC_DIFF);
> +	ret = adf41513_sync_config(st, ADF41513_SYNC_DIFF);
> +	if (ret < 0)
If you know it is either 0 for good or less than zero, prefer
	if (ret) (for reason that follows)

> +		return ret;
> +
> +	if (st->data.le_sync_en) {
> +		st->regs[ADF41513_REG12] |= ADF41513_REG12_LE_SELECT_MSK;
> +		ret = adf41513_sync_config(st, ADF41513_SYNC_DIFF);
Similar to below - I'd like to see 	
		if (ret)
			return ret;
here to avoid returning stale parameter from above (which might be postive
based on local code).

> +	}
> +
> +	return ret;
>  }
>  
>  static ssize_t adf41513_read_uhz(struct iio_dev *indio_dev,
> @@ -994,6 +1007,8 @@ static int adf41513_parse_fw(struct adf41513_state *st)
>  		st->data.lock_detect_count = tmp;
>  	}
>  
> +	/* load enable sync */
> +	st->data.le_sync_en = device_property_read_bool(dev, "adi,le-sync-enable");
>  	st->data.freq_resolution_uhz = MICROHZ_PER_HZ;
>  
>  	return 0;
> @@ -1001,6 +1016,7 @@ static int adf41513_parse_fw(struct adf41513_state *st)
>  
>  static int adf41513_setup(struct adf41513_state *st)
>  {
> +	int ret;
>  	u32 tmp;
>  
>  	memset(st->regs_hw, 0xFF, sizeof(st->regs_hw));
> @@ -1034,8 +1050,18 @@ static int adf41513_setup(struct adf41513_state *st)
>  					      st->data.logic_lvl_1v8_en ? 0 : 1);
>  
>  	/* perform initialization sequence with power-up frequency */
> -	return adf41513_set_frequency(st, st->data.power_up_frequency_hz * MICROHZ_PER_HZ,
> -				      ADF41513_SYNC_ALL);
> +	ret = adf41513_set_frequency(st,
> +				     st->data.power_up_frequency_hz * MICROHZ_PER_HZ,
> +				     ADF41513_SYNC_ALL);
> +	if (ret < 0)
	if (ret)
		assuming set_frequency never returns a positive.

> +		return ret;
> +
> +	if (st->data.le_sync_en) {
> +		st->regs[ADF41513_REG12] |= ADF41513_REG12_LE_SELECT_MSK;
> +		ret = adf41513_sync_config(st, ADF41513_SYNC_DIFF);
Slightly preference for
		if (ret)
			return ret;
	}

	return 0;

Because otherwise a 'stale' (though it is zero) return value is used and that sort
of code pattern tends to be a little fragile and hard to read.

> +	}
> +
> +	return ret;
>  }
>  
>  static void adf41513_power_down(void *data)
>