Re: [PATCH v2 5/6] iio: imu: adis16607: Add external clock sync support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iio: adis16607: hardcoded clk_freq in SCALED mode causes timestamp drift
- [Medium] iio: imu: adis16607: missing USER_SYNC clear leaves stale flash config
--

commit c8cae2338b805eec99b0652c721e93ee18498a36
Author: Radu Sabau <radu.sabau@analog.com>

iio: imu: adis16607: Add external clock sync support

This patch adds support for an optional external clock through the "clocks"
property. It selects between SYNC_DIRECT and SYNC_SCALED modes depending on
the supplied frequency, and defaults to the internal 8 kHz clock when no
clock is provided.

> diff --git a/drivers/iio/imu/adis16607.c b/drivers/iio/imu/adis16607.c
> --- a/drivers/iio/imu/adis16607.c
> +++ b/drivers/iio/imu/adis16607.c

[ ... ]

> @@ -1174,15 +1174,75 @@ static const struct adis16607_chip_info adis16607_3_chip_info = {
>  	.deltang_max_val = IIO_DEGREE_TO_RAD(2000),
>  };
>  
> -/*
> - * No external clock sync support yet: always run on the internal clock.
> - * Default dec=0 after reset, so period = 1 / clk_freq.
> - */
>  static int adis16607_config_clk(struct adis16607 *st)
>  {
> -	st->clk_freq = (unsigned long)st->info->int_clk * 1000;
> +	struct device *dev = &st->adis.spi->dev;
> +	const struct adis16607_sync *sync_mode_data = NULL;
> +	unsigned long ext_clk_hz;
> +	struct clk *clk;
> +	int i;
> +
> +	clk = devm_clk_get_optional_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	if (!clk) {
> +		st->clk_freq = (unsigned long)st->info->int_clk * 1000;
> +		st->sync_mode = ADIS16607_SYNC_DEFAULT;
> +		st->sample_period_ns = DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * 1000ULL,
> +							     st->clk_freq);
> +		return 0;
> +	}

[Severity: Medium]
If a user previously configured SCALED mode and saved it to flash, does
this early return leave stale flash sync settings active in the hardware?

Because soft resets restore non-volatile flash settings instead of clearing
them to hardware zero, returning here without explicitly clearing the sync
configuration might leave the device stuck waiting for an external clock
that no longer exists.

> +
> +	ext_clk_hz = clk_get_rate(clk);
> +
> +	for (i = 1; i < st->info->num_sync; i++) {
> +		if (ext_clk_hz >= st->info->sync_mode[i].min_rate &&
> +		    ext_clk_hz <= st->info->sync_mode[i].max_rate) {
> +			sync_mode_data = &st->info->sync_mode[i];
> +			break;
> +		}
> +	}
> +
> +	if (!sync_mode_data)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Clk rate: %lu not in a valid range\n",
> +				     ext_clk_hz);
> +
> +	st->sync_mode = sync_mode_data->sync_mode;
> +	/* DIRECT: external clock is the effective internal rate */
> +	st->clk_freq = ext_clk_hz * 1000;
> +
> +	/* Configure sync via USER_GPIO_CFG and USER_SYNC registers */
> +	if (st->sync_mode != ADIS16607_SYNC_DEFAULT) {
> +		int ret;
> +
> +		/* Enable sync GPIO function */
> +		ret = adis_update_bits_base(&st->adis, ADIS16607_REG_USER_GPIO_CFG,
> +					    ADIS16607_SYNC_GPIO_MASK,
> +					    FIELD_PREP(ADIS16607_SYNC_GPIO_MASK, 1), 2);

[Severity: Medium]
In DIRECT mode, only the GPIO function is updated here. Does this rely
entirely on the prior soft reset to have cleared ADIS16607_SYNC_MODE_MASK
in the ADIS16607_REG_USER_SYNC register?

If flash settings were restored during the soft reset, the device might
still be configured for SCALED mode and operate at the wrong rate. Should
the driver explicitly overwrite ADIS16607_REG_USER_SYNC for all sync modes?

> +		if (ret)
> +			return ret;
> +
> +		/* Set sync mode in USER_SYNC register */
> +		if (st->sync_mode == ADIS16607_SYNC_SCALED) {
> +			u16 sync_scale;
> +
> +			sync_scale = st->info->int_clk / ext_clk_hz;

[Severity: Medium]
Does this truncating integer division lead to inaccurate hardware sync scales
when ext_clk_hz does not perfectly divide int_clk?

> +			ret = adis_write_reg_16(&st->adis, ADIS16607_REG_USER_SYNC,
> +						ADIS16607_SYNC_MODE_MASK | sync_scale);
> +			if (ret)
> +				return ret;
> +
> +			/* SCALED: effective rate is int_clk (ext_clk × sync_scale ≈ int_clk) */
> +			st->clk_freq = (unsigned long)st->info->int_clk * 1000;

[Severity: Medium]
Does hardcoding the assumed clock frequency instead of dynamically computing
the true scaled frequency (ext_clk_hz * sync_scale * 1000) cause IIO
timestamps to systemically drift?

If ext_clk_hz does not perfectly divide the internal clock (e.g. ext_clk_hz
is 399 instead of 400), actual physical sampling will occur at a slightly
different rate than the hardcoded 8000 Hz.

When the trigger handler later increments timestamps using the miscalculated
st->sample_period_ns, does this introduce a systemic timestamp drift that
worsens over time?

> +		}
> +	}
> +
> +	/* Default dec=0 after reset: period = 1 / clk_freq */
>  	st->sample_period_ns = DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * 1000ULL,
> -						     st->clk_freq);
> +						      st->clk_freq);
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-adis16607-v2-0-7c94df80878a@analog.com?part=5