Re: [PATCH v4 06/10] drivers: iio: imu: adis16475: generic computation for sample rate

[Nuno Sá]

On Fri, 2024-05-24 at 12:00 +0300, Ramona Gradinariu wrote:
> Currently adis16475 supports a sample rate between 1900 and 2100 Hz.
> This patch changes the setting of sample rate from hardcoded values to
> a generic computation based on the internal clock frequency.
> This is a preparatory patch for adding support for adis1657x family
> devices which allow sample rates between 3900 and 4100 Hz.
> 
> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@xxxxxxxxx>
> ---

Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>

> no changes in v4
>  drivers/iio/imu/adis16475.c | 39 +++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> index ab955efdad92..c589f214259b 100644
> --- a/drivers/iio/imu/adis16475.c
> +++ b/drivers/iio/imu/adis16475.c
> @@ -310,6 +310,9 @@ static int adis16475_set_freq(struct adis16475 *st, const u32
> freq)
>  	u16 dec;
>  	int ret;
>  	u32 sample_rate = st->clk_freq;
> +	/* The optimal sample rate for the supported IMUs is between int_clk - 100
> and int_clk + 100. */
> +	u32 max_sample_rate =  st->info->int_clk * 1000 + 100000;
> +	u32 min_sample_rate =  st->info->int_clk * 1000 - 100000;
> 
>  	if (!freq)
>  		return -EINVAL;
> @@ -317,8 +320,9 @@ static int adis16475_set_freq(struct adis16475 *st, const u32
> freq)
>  	adis_dev_lock(&st->adis);
>  	/*
>  	 * When using sync scaled mode, the input clock needs to be scaled so that
> we have
> -	 * an IMU sample rate between (optimally) 1900 and 2100. After this, we
> can use the
> -	 * decimation filter to lower the sampling rate in order to get what the
> user wants.
> +	 * an IMU sample rate between (optimally) int_clk - 100 and int_clk + 100.
> +	 * After this, we can use the decimation filter to lower the sampling rate
> in order
> +	 * to get what the user wants.
>  	 * Optimally, the user sample rate is a multiple of both the IMU sample
> rate and
>  	 * the input clock. Hence, calculating the sync_scale dynamically gives us
> better
>  	 * chances of achieving a perfect/integer value for DEC_RATE. The math
> here is:
> @@ -336,23 +340,24 @@ static int adis16475_set_freq(struct adis16475 *st, const u32
> freq)
>  		 * solution. In this case, we get the highest multiple of the
> input clock
>  		 * lower than the IMU max sample rate.
>  		 */
> -		if (scaled_rate > 2100000)
> -			scaled_rate = 2100000 / st->clk_freq * st->clk_freq;
> +		if (scaled_rate > max_sample_rate)
> +			scaled_rate = max_sample_rate / st->clk_freq * st-
> >clk_freq;
>  		else
> -			scaled_rate = 2100000 / scaled_rate * scaled_rate;
> +			scaled_rate = max_sample_rate / scaled_rate * scaled_rate;
> 
>  		/*
>  		 * This is not an hard requirement but it's not advised to run the
> IMU
> -		 * with a sample rate lower than 1900Hz due to possible
> undersampling
> -		 * issues. However, there are users that might really want to take
> the risk.
> -		 * Hence, we provide a module parameter for them. If set, we allow
> sample
> -		 * rates lower than 1.9KHz. By default, we won't allow this and we
> just roundup
> -		 * the rate to the next multiple of the input clock bigger than
> 1.9KHz. This
> -		 * is done like this as in some cases (when DEC_RATE is 0) might
> give
> -		 * us the closest value to the one desired by the user...
> +		 * with a sample rate lower than internal clock frequency, due to
> possible
> +		 * undersampling issues. However, there are users that might
> really want
> +		 * to take the risk. Hence, we provide a module parameter for
> them. If set,
> +		 * we allow sample rates lower than internal clock frequency.
> +		 * By default, we won't allow this and we just roundup the rate to
> the next
> +		 *  multiple of the input clock. This is done like this as in some
> cases
> +		 * (when DEC_RATE is 0) might give us the closest value to the one
> desired
> +		 * by the user...
>  		 */
> -		if (scaled_rate < 1900000 && !low_rate_allow)
> -			scaled_rate = roundup(1900000, st->clk_freq);
> +		if (scaled_rate < min_sample_rate && !low_rate_allow)
> +			scaled_rate = roundup(min_sample_rate, st->clk_freq);
> 
>  		sync_scale = scaled_rate / st->clk_freq;
>  		ret = __adis_write_reg_16(&st->adis, ADIS16475_REG_UP_SCALE,
> sync_scale);
> @@ -1359,6 +1364,7 @@ static int adis16475_config_sync_mode(struct adis16475 *st)
>  	struct device *dev = &st->adis.spi->dev;
>  	const struct adis16475_sync *sync;
>  	u32 sync_mode;
> +	u16 max_sample_rate = st->info->int_clk + 100;
> 
>  	/* default to internal clk */
>  	st->clk_freq = st->info->int_clk * 1000;
> @@ -1398,10 +1404,9 @@ static int adis16475_config_sync_mode(struct adis16475 *st)
>  			/*
>  			 * In sync scaled mode, the IMU sample rate is the
> clk_freq * sync_scale.
>  			 * Hence, default the IMU sample rate to the highest
> multiple of the input
> -			 * clock lower than the IMU max sample rate. The optimal
> range is
> -			 * 1900-2100 sps...
> +			 * clock lower than the IMU max sample rate.
>  			 */
> -			up_scale = 2100 / st->clk_freq;
> +			up_scale = max_sample_rate / st->clk_freq;
> 
>  			ret = __adis_write_reg_16(&st->adis,
>  						  ADIS16475_REG_UP_SCALE,
> --
> 2.34.1
>