Re: [PATCH v4 7/8] iio: imu: adis16607: Add external clock sync support

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 23 Oct 2026 12:30:24 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Add support for an optional external clock (clocks property),
> selecting between SYNC_DIRECT and SYNC_SCALED modes depending on the
> supplied frequency, with the internal 8 kHz clock used as the default
> when no clock is provided.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> ---

> -/*
> - * No external clock sync support yet: always run on the internal clock.
> - * Default dec=0 after reset, so period = 1 / clk_freq_mhz.
> - */
>  static int adis16607_config_clk(struct adis16607 *st)
>  {
> -	st->clk_freq_mhz = st->info->int_clk_hz * MILLI;
> -	st->sample_period_ns =
> -		DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * MILLI, st->clk_freq_mhz);
> +	struct device *dev = &st->adis.spi->dev;
> +	const struct adis16607_sync *sync_mode_data = NULL;
> +	unsigned long ext_clk_hz = 0;
> +	u16 sync_scale = 0;
> +	u16 dec;
> +	struct clk *clk;
> +	int ret, i;

where there is no other particular reason for ordering, reverse xmas tree
preferred.


> +
> +	clk = devm_clk_get_optional_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	if (clk) {
> +		ext_clk_hz = clk_get_rate(clk);

Hmm. What if it is a controllable clock?

> +
> +		for (i = 1; i < st->info->num_sync; i++) {

		for (unsigned int i = 1; ...

> +			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;
> +	} else {
> +		st->sync_mode = ADIS16607_SYNC_DEFAULT;
> +	}
> +
> +	/*
> +	 * Write the full desired sync state explicitly for every mode,
> +	 * rather than only when enabling non-default sync: USER_SYNC's
> +	 * documented reset default (0x0000, direct mode / scale 0) means
> +	 * this isn't fixing a known reset-state issue, but each mode
> +	 * establishing its own complete state here, instead of only
> +	 * touching bits conditionally, is simpler to reason about and
> +	 * doesn't depend on that default holding true.
> +	 */
> +	ret = adis_update_bits_base(&st->adis, ADIS16607_REG_USER_GPIO_CFG,
> +				    ADIS16607_SYNC_GPIO_MASK,
> +				    FIELD_PREP(ADIS16607_SYNC_GPIO_MASK,
> +					       st->sync_mode != ADIS16607_SYNC_DEFAULT), 2);
> +	if (ret)
> +		return ret;
> +
> +	if (st->sync_mode == ADIS16607_SYNC_SCALED)
> +		sync_scale = DIV_ROUND_CLOSEST(st->info->int_clk_hz, ext_clk_hz);
> +
> +	ret = adis_write_reg_16(&st->adis, ADIS16607_REG_USER_SYNC,
> +				(st->sync_mode == ADIS16607_SYNC_SCALED) ?
> +				ADIS16607_SYNC_MODE_MASK | sync_scale : 0);
> +	if (ret)
> +		return ret;
> +
> +	switch (st->sync_mode) {
> +	case ADIS16607_SYNC_SCALED:
> +		/*
> +		 * sync_scale is a rounded integer, so the hardware's actual
> +		 * effective rate is ext_clk_hz * sync_scale, not necessarily
> +		 * int_clk_hz - use the real value here so sample_period_ns (and
> +		 * therefore FIFO sample timestamps) don't drift from it.
> +		 */
> +		st->clk_freq_mhz = (unsigned long)ext_clk_hz * sync_scale * 1000;
> +		break;
> +	case ADIS16607_SYNC_DIRECT:
> +		/* DIRECT: external clock is the effective internal rate */
> +		st->clk_freq_mhz = ext_clk_hz * 1000;
> +		break;
> +	default:
> +		st->clk_freq_mhz = (unsigned long)st->info->int_clk_hz * 1000;
> +		break;
> +	}
> +
> +	ret = adis_read_reg_16(&st->adis, ADIS16607_REG_DEC_RATE, &dec);
> +	if (ret)
> +		return ret;
> +
> +	st->sample_period_ns = DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * 1000ULL * (dec + 1),
> +						     st->clk_freq_mhz);
> +
>  	return 0;
>  }
>  
>