Received-SPF: pass (google.com: domain of linux-kernel+bounces-188629-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <7be63d9ed78848cb1ee43edae0d97720641daf44.camel@gmail.com>
Subject: Re: [PATCH v4 06/10] drivers: iio: imu: adis16475: generic
 computation for sample rate
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Ramona Gradinariu <ramona.bolboaca13@gmail.com>, 
	linux-kernel@vger.kernel.org, jic23@kernel.org, linux-iio@vger.kernel.org, 
	devicetree@vger.kernel.org, conor+dt@kernel.org, 
	krzysztof.kozlowski+dt@linaro.org, robh@kernel.org, nuno.sa@analog.com
Date: Fri, 24 May 2024 12:59:12 +0200
In-Reply-To: <20240524090030.336427-7-ramona.bolboaca13@gmail.com>
References: <20240524090030.336427-1-ramona.bolboaca13@gmail.com>
	 <20240524090030.336427-7-ramona.bolboaca13@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.1 (3.52.1-1.fc40) 
Precedence: bulk
MIME-Version: 1.0

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.
>=20
> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@gmail.com>
> ---

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

> no changes in v4
> =C2=A0drivers/iio/imu/adis16475.c | 39 +++++++++++++++++++++-------------=
---
> =C2=A01 file changed, 22 insertions(+), 17 deletions(-)
>=20
> 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, c=
onst u32
> freq)
> =C2=A0	u16 dec;
> =C2=A0	int ret;
> =C2=A0	u32 sample_rate =3D st->clk_freq;
> +	/* The optimal sample rate for the supported IMUs is between int_clk - =
100
> and int_clk + 100. */
> +	u32 max_sample_rate =3D=C2=A0 st->info->int_clk * 1000 + 100000;
> +	u32 min_sample_rate =3D=C2=A0 st->info->int_clk * 1000 - 100000;
>=20
> =C2=A0	if (!freq)
> =C2=A0		return -EINVAL;
> @@ -317,8 +320,9 @@ static int adis16475_set_freq(struct adis16475 *st, c=
onst u32
> freq)
> =C2=A0	adis_dev_lock(&st->adis);
> =C2=A0	/*
> =C2=A0	 * 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 th=
e
> user wants.
> +	 * an IMU sample rate between (optimally) int_clk - 100 and int_clk + 1=
00.
> +	 * After this, we can use the decimation filter to lower the sampling r=
ate
> in order
> +	 * to get what the user wants.
> =C2=A0	 * Optimally, the user sample rate is a multiple of both the IMU s=
ample
> rate and
> =C2=A0	 * the input clock. Hence, calculating the sync_scale dynamically =
gives us
> better
> =C2=A0	 * 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)
> =C2=A0		 * solution. In this case, we get the highest multiple of the
> input clock
> =C2=A0		 * lower than the IMU max sample rate.
> =C2=A0		 */
> -		if (scaled_rate > 2100000)
> -			scaled_rate =3D 2100000 / st->clk_freq * st->clk_freq;
> +		if (scaled_rate > max_sample_rate)
> +			scaled_rate =3D max_sample_rate / st->clk_freq * st-
> >clk_freq;
> =C2=A0		else
> -			scaled_rate =3D 2100000 / scaled_rate * scaled_rate;
> +			scaled_rate =3D max_sample_rate / scaled_rate * scaled_rate;
>=20
> =C2=A0		/*
> =C2=A0		 * This is not an hard requirement but it's not advised to run th=
e
> 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
> +		 *=C2=A0 multiple of the input clock. This is done like this as in som=
e
> cases
> +		 * (when DEC_RATE is 0) might give us the closest value to the one
> desired
> +		 * by the user...
> =C2=A0		 */
> -		if (scaled_rate < 1900000 && !low_rate_allow)
> -			scaled_rate =3D roundup(1900000, st->clk_freq);
> +		if (scaled_rate < min_sample_rate && !low_rate_allow)
> +			scaled_rate =3D roundup(min_sample_rate, st->clk_freq);
>=20
> =C2=A0		sync_scale =3D scaled_rate / st->clk_freq;
> =C2=A0		ret =3D __adis_write_reg_16(&st->adis, ADIS16475_REG_UP_SCALE,
> sync_scale);
> @@ -1359,6 +1364,7 @@ static int adis16475_config_sync_mode(struct adis16=
475 *st)
> =C2=A0	struct device *dev =3D &st->adis.spi->dev;
> =C2=A0	const struct adis16475_sync *sync;
> =C2=A0	u32 sync_mode;
> +	u16 max_sample_rate =3D st->info->int_clk + 100;
>=20
> =C2=A0	/* default to internal clk */
> =C2=A0	st->clk_freq =3D st->info->int_clk * 1000;
> @@ -1398,10 +1404,9 @@ static int adis16475_config_sync_mode(struct adis1=
6475 *st)
> =C2=A0			/*
> =C2=A0			 * In sync scaled mode, the IMU sample rate is the
> clk_freq * sync_scale.
> =C2=A0			 * 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.
> =C2=A0			 */
> -			up_scale =3D 2100 / st->clk_freq;
> +			up_scale =3D max_sample_rate / st->clk_freq;
>=20
> =C2=A0			ret =3D __adis_write_reg_16(&st->adis,
> =C2=A0						=C2=A0 ADIS16475_REG_UP_SCALE,
> --
> 2.34.1
>=20