Re: [PATCH v12 08/11] iio: frequency: adf41513: handle LE synchronization feature

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver modifies the cached register state (`st->regs`) before performing SPI transactions, but fails to revert these changes if the SPI transaction fails. This leaves the shadow register state corrupted and violates hardware sequencing constraints on subsequent retries.
--

commit a7ad01199feb60254a514f4aa41d379243946b28
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: frequency: adf41513: handle LE synchronization feature

This commit introduces support for the LE synchronization feature on the
ADF41513. It enables the feature during device setup and handles
enabling/disabling it during resume and suspend.

> diff --git a/drivers/iio/frequency/adf41513.c b/drivers/iio/frequency/adf41513.c
> index b8a8b9fb81d9d..397e859170cc0 100644
> --- a/drivers/iio/frequency/adf41513.c
> +++ b/drivers/iio/frequency/adf41513.c

[ ... ]

> @@ -634,13 +635,27 @@ 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);

If adf41513_sync_config() fails, is it possible for st->regs[] to be left with
these updated values?

If the SPI transaction fails when writing ADF41513_REG12 (which happens before
ADF41513_REG6), the state in st->regs is not reverted. A subsequent frequency
change might then inadvertently power down the device because the dirty
st->regs state is pushed to the hardware during adf41513_sync_config().

>  }
>  
>  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_ALL);
> +	ret = adf41513_sync_config(st, ADF41513_SYNC_ALL);
> +	if (ret)
> +		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);
> +		if (ret)
> +			return ret;

Could this cause a regression on a subsequent retry of adf41513_resume() if
the SPI write fails here?

If adf41513_sync_config(st, ADF41513_SYNC_DIFF) fails, the function returns
an error, but st->regs[ADF41513_REG12] retains the
ADF41513_REG12_LE_SELECT_MSK bit.

On a subsequent retry of adf41513_resume(), the first call to
adf41513_sync_config(st, ADF41513_SYNC_ALL) writes all registers in reverse
order (ADF41513_REG13 down to ADF41513_REG0). Since st->regs[ADF41513_REG12]
already has the ADF41513_REG12_LE_SELECT_MSK bit set from the previous failed
attempt, it will write ADF41513_REG12 with this bit set before writing
ADF41513_REG6 to power up the device.

Does this violate the hardware constraint mentioned in the commit message that
LE sync must be set after powering up?

> +	}
> +
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-adf41513-iio-driver-v12-0-34af2ed2779f@analog.com?part=8