[PATCH 0/4] Fix/Improve sync clock mode handling

[PATCH 0/4] Fix/Improve sync clock mode handling

[Nuno Sá]

The first patch in this series is just a simple helper to lock/unlock
the device. Having these helpers make the code slightly neater (IMHO).

The following patches introduces changes in the sampling frequency
calculation when sync scale/pps modes are used. First, it's important
to understand the purpose of this mode and how it should be used. Let's
say our part has an internal rate of 4250 (e.g adis1649x family) and the
user wants an output rate of 200SPS. Obviously, we can't use this
sampling rate and divide back down to get 200 SPS with decimation on.
Hence, we can use this mode to give an input clock of 1HZ, scale it to
something like 4200 or 4000 SPS and then use the decimation filter to get
the exact desired 200SPS. There are also some limits that should be
taken into account when scaling:

 * For the devices in the adis16475 driver:
     - Input sync frequency range is 1 to 128 Hz
     - Native sample rate: 2 kSPS.  Optimal range: 1900-2100 sps

 * For the adis1649x family (adis16480 driver):
    - Input sync frequency range is 1 to 128 Hz
    - Native sample rate: 4.25 kSPS.  Optimal range: 4000-4250 sps 

I'm not 100% convinced on how to handle the optimal minimum. For now,
I'm just throwing a warning saying we might get into trouble if we get a
value lower than that. I was also tempted to just return -EINVAL or
clamp the value. However, I know that there are ADI customers that
(for some reason) are using a sampling rate lower than the minimum
advised.

That said, the patch for the adis16480 driver is a fix as this mode was
being wrongly handled. There should not be a "separation" between using
the sync_scale and the dec_rate registers. The way things were being done,
we could easily get into a situation where the part could be running with
an internal rate way lower than the optimal minimum.

For the adis16475 drivers, things were not really wrong. They were just
not optimal as we were forcing users to specify the IMU scaled internal
rate once in the devicetree. Calculating things at runtime gives much
more flexibility to choose the output rate.

Nuno Sá (4):
  iio: adis: add helpers for locking
  iio: adis16480: fix pps mode sampling frequency math
  iio: adis16475: improve sync scale mode handling
  dt-bindings: adis16475: remove property

 .../bindings/iio/imu/adi,adis16475.yaml       |   9 --
 drivers/iio/imu/adis16475.c                   | 110 ++++++++++++----
 drivers/iio/imu/adis16480.c                   | 120 +++++++++++++-----
 include/linux/iio/imu/adis.h                  |  10 ++
 4 files changed, 178 insertions(+), 71 deletions(-)

-- 
2.30.0

[PATCH 1/4] iio: adis: add helpers for locking

[Nuno Sá]

Add some helpers to lock and unlock the device.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 include/linux/iio/imu/adis.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 04e96d688ba9..f9b728d490b1 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -428,6 +428,16 @@ static inline int adis_initial_startup(struct adis *adis)
 	return ret;
 }
 
+static inline void adis_dev_lock(struct adis *adis)
+{
+	mutex_lock(&adis->state_lock);
+}
+
+static inline void adis_dev_unlock(struct adis *adis)
+{
+	mutex_unlock(&adis->state_lock);
+}
+
 int adis_single_conversion(struct iio_dev *indio_dev,
 	const struct iio_chan_spec *chan, unsigned int error_mask,
 	int *val);
-- 
2.30.0

[PATCH 2/4] iio: adis16480: fix pps mode sampling frequency math

[Nuno Sá]

When using PPS mode, the input clock needs to be scaled so that we have
an IMU sample rate between (optimally) 4000 and 4250. 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:
 1. lcm of the input clock and the desired output rate.
 2. get the highest multiple of the previous result lower than the adis
    max rate.
 3. The last result becomes the IMU sample rate. Use that to calculate
    SYNC_SCALE and DEC_RATE (to get the user output rate).

Fixes: 326e2357553d3 ("iio: imu: adis16480: Add support for external clock")
Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16480.c | 120 ++++++++++++++++++++++++++----------
 1 file changed, 86 insertions(+), 34 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index dfe86c589325..7620822f3350 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/module.h>
+#include <linux/lcm.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -312,7 +313,8 @@ static int adis16480_debugfs_init(struct iio_dev *indio_dev)
 static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int val2)
 {
 	struct adis16480 *st = iio_priv(indio_dev);
-	unsigned int t, reg;
+	unsigned int t, sample_rate = st->clk_freq;
+	int ret;
 
 	if (val < 0 || val2 < 0)
 		return -EINVAL;
@@ -321,28 +323,63 @@ static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int val2)
 	if (t == 0)
 		return -EINVAL;
 
+	adis_dev_lock(&st->adis);
 	/*
-	 * When using PPS mode, the rate of data collection is equal to the
-	 * product of the external clock frequency and the scale factor in the
-	 * SYNC_SCALE register.
-	 * When using sync mode, or internal clock, the output data rate is
-	 * equal with  the clock frequency divided by DEC_RATE + 1.
+	 * When using PPS mode, the input clock needs to be scaled so that we have an IMU
+	 * sample rate between (optimally) 4000 and 4250. 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:
+	 *	1. lcm of the input clock and the desired output rate.
+	 *	2. get the highest multiple of the previous result lower than the adis max rate.
+	 *	3. The last result becomes the IMU sample rate. Use that to calculate SYNC_SCALE
+	 *	   and DEC_RATE (to get the user output rate)
 	 */
 	if (st->clk_mode == ADIS16480_CLK_PPS) {
-		t = t / st->clk_freq;
-		reg = ADIS16495_REG_SYNC_SCALE;
-	} else {
-		t = st->clk_freq / t;
-		reg = ADIS16480_REG_DEC_RATE;
+		unsigned long scaled_rate = lcm(st->clk_freq, t);
+		int sync_scale;
+		struct device *dev = &st->adis.spi->dev;
+
+		/*
+		 * If lcm is bigger than the IMU maximum sampling rate there's no perfect
+		 * solution. In this case, we get the highest multiple of the input clock
+		 * lower that the IMU max sample rate.
+		 */
+		if (scaled_rate > st->chip_info->int_clk)
+			scaled_rate = st->chip_info->int_clk / st->clk_freq * st->clk_freq;
+		else
+			scaled_rate = st->chip_info->int_clk / 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 4000Hz due to possible undersampling
+		 * issues so we will log a warning here. We could even force the rate
+		 * to 4000 but some users might really want this...
+		 */
+		if (scaled_rate < 4000000)
+			dev_warn(dev, "Possible undersampling issues due to sampling rate=%lu < 4000\n",
+				 scaled_rate / 1000);
+
+		sync_scale = scaled_rate / st->clk_freq;
+		ret = __adis_write_reg_16(&st->adis, ADIS16495_REG_SYNC_SCALE, sync_scale);
+		if (ret)
+			goto error;
+
+		sample_rate = scaled_rate;
 	}
 
+	t = DIV_ROUND_CLOSEST(sample_rate, t);
+	if (t)
+		t--;
+
 	if (t > st->chip_info->max_dec_rate)
 		t = st->chip_info->max_dec_rate;
 
-	if ((t != 0) && (st->clk_mode != ADIS16480_CLK_PPS))
-		t--;
-
-	return adis_write_reg_16(&st->adis, reg, t);
+	ret = __adis_write_reg_16(&st->adis, ADIS16480_REG_DEC_RATE, t);
+error:
+	adis_dev_unlock(&st->adis);
+	return ret;
 }
 
 static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
@@ -350,34 +387,35 @@ static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
 	struct adis16480 *st = iio_priv(indio_dev);
 	uint16_t t;
 	int ret;
-	unsigned int freq;
-	unsigned int reg;
+	unsigned int freq, sample_rate = st->clk_freq;
 
-	if (st->clk_mode == ADIS16480_CLK_PPS)
-		reg = ADIS16495_REG_SYNC_SCALE;
-	else
-		reg = ADIS16480_REG_DEC_RATE;
+	adis_dev_lock(&st->adis);
 
-	ret = adis_read_reg_16(&st->adis, reg, &t);
+	if (st->clk_mode == ADIS16480_CLK_PPS) {
+		u16 sync_scale;
+
+		ret = __adis_read_reg_16(&st->adis, ADIS16495_REG_SYNC_SCALE, &sync_scale);
+		if (ret)
+			goto error;
+
+		sample_rate = st->clk_freq * sync_scale;
+	}
+
+	ret = __adis_read_reg_16(&st->adis, ADIS16480_REG_DEC_RATE, &t);
 	if (ret)
-		return ret;
+		goto error;
 
-	/*
-	 * When using PPS mode, the rate of data collection is equal to the
-	 * product of the external clock frequency and the scale factor in the
-	 * SYNC_SCALE register.
-	 * When using sync mode, or internal clock, the output data rate is
-	 * equal with  the clock frequency divided by DEC_RATE + 1.
-	 */
-	if (st->clk_mode == ADIS16480_CLK_PPS)
-		freq = st->clk_freq * t;
-	else
-		freq = st->clk_freq / (t + 1);
+	adis_dev_unlock(&st->adis);
+
+	freq = DIV_ROUND_CLOSEST(sample_rate, (t + 1));
 
 	*val = freq / 1000;
 	*val2 = (freq % 1000) * 1000;
 
 	return IIO_VAL_INT_PLUS_MICRO;
+error:
+	adis_dev_unlock(&st->adis);
+	return ret;
 }
 
 enum {
@@ -1278,6 +1316,20 @@ static int adis16480_probe(struct spi_device *spi)
 
 		st->clk_freq = clk_get_rate(st->ext_clk);
 		st->clk_freq *= 1000; /* micro */
+		if (st->clk_mode == ADIS16480_CLK_PPS) {
+			u16 sync_scale;
+
+			/*
+			 * In PPS 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 internal sample rate is the
+			 * max...
+			 */
+			sync_scale = st->chip_info->int_clk / st->clk_freq;
+			ret = __adis_write_reg_16(&st->adis, ADIS16495_REG_SYNC_SCALE, sync_scale);
+			if (ret)
+				return ret;
+		}
 	} else {
 		st->clk_freq = st->chip_info->int_clk;
 	}
-- 
2.30.0

[PATCH 3/4] iio: adis16475: improve sync scale mode handling

[Nuno Sá]

With this patch, we don't force users to define the IMU scaled internal
sampling rate once in the devicetree. Now it's dynamically calculated
at runtime depending on the desired output rate given by users.

Calculating the sync_scale dynamically gives us better chances of
achieving a perfect/integer value for DEC_RATE (thus giving more
flexibility). The math is:
 1. lcm of the input clock and the desired output rate.
 2. get the highest multiple of the previous result lower than the adis
    max rate.
 3. The last result becomes the IMU sample rate. Use that to calculate
    SYNC_SCALE and DEC_RATE (to get the user output rate).

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16475.c | 110 +++++++++++++++++++++++++++---------
 1 file changed, 82 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
index 197d48240991..cd44226d0672 100644
--- a/drivers/iio/imu/adis16475.c
+++ b/drivers/iio/imu/adis16475.c
@@ -17,6 +17,7 @@
 #include <linux/iio/sysfs.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/irq.h>
+#include <linux/lcm.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/property.h>
@@ -101,6 +102,7 @@ struct adis16475 {
 	u32 clk_freq;
 	bool burst32;
 	unsigned long lsb_flag;
+	u16 sync_mode;
 	/* Alignment needed for the timestamp */
 	__be16 data[ADIS16475_MAX_SCAN_DATA] __aligned(8);
 };
@@ -253,25 +255,90 @@ static int adis16475_get_freq(struct adis16475 *st, u32 *freq)
 {
 	int ret;
 	u16 dec;
+	u32 sample_rate = st->clk_freq;
+
+	adis_dev_lock(&st->adis);
+
+	if (st->sync_mode == ADIS16475_SYNC_SCALED) {
+		u16 sync_scale;
+
+		ret = __adis_read_reg_16(&st->adis, ADIS16475_REG_UP_SCALE, &sync_scale);
+		if (ret)
+			goto error;
+
+		sample_rate = st->clk_freq * sync_scale;
+	}
 
-	ret = adis_read_reg_16(&st->adis, ADIS16475_REG_DEC_RATE, &dec);
+	ret = __adis_read_reg_16(&st->adis, ADIS16475_REG_DEC_RATE, &dec);
 	if (ret)
-		return -EINVAL;
+		goto error;
+
+	adis_dev_unlock(&st->adis);
 
-	*freq = DIV_ROUND_CLOSEST(st->clk_freq, dec + 1);
+	*freq = DIV_ROUND_CLOSEST(sample_rate, dec + 1);
 
 	return 0;
+error:
+	adis_dev_unlock(&st->adis);
+	return ret;
 }
 
 static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
 {
 	u16 dec;
 	int ret;
+	u32 sample_rate = st->clk_freq;
 
 	if (!freq)
 		return -EINVAL;
 
-	dec = DIV_ROUND_CLOSEST(st->clk_freq, 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.
+	 * 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:
+	 *	1. lcm of the input clock and the desired output rate.
+	 *	2. get the highest multiple of the previous result lower than the adis max rate.
+	 *	3. The last result becomes the IMU sample rate. Use that to calculate SYNC_SCALE
+	 *	   and DEC_RATE (to get the user output rate)
+	 */
+	if (st->sync_mode == ADIS16475_SYNC_SCALED) {
+		unsigned long scaled_rate = lcm(st->clk_freq, freq);
+		int sync_scale;
+		struct device *dev = &st->adis.spi->dev;
+
+		/*
+		 * If lcm is bigger than the IMU maximum sampling rate there's no perfect
+		 * solution. In this case, we get the highest multiple of the input clock
+		 * lower that the IMU max sample rate.
+		 */
+		if (scaled_rate > 2100000)
+			scaled_rate = 2100000 / st->clk_freq * st->clk_freq;
+		else
+			scaled_rate = 2100000 / 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 so we will log a warning here. We could even force the rate
+		 * to 1900 but some users might really want this...
+		 */
+		if (scaled_rate < 1900000)
+			dev_warn(dev, "Possible undersampling issues due to sampling rate=%lu < 1900\n",
+				 scaled_rate / 1000);
+
+		sync_scale = scaled_rate / st->clk_freq;
+		ret = __adis_write_reg_16(&st->adis, ADIS16475_REG_UP_SCALE, sync_scale);
+		if (ret)
+			goto error;
+
+		sample_rate = scaled_rate;
+	}
+
+	dec = DIV_ROUND_CLOSEST(sample_rate, freq);
 
 	if (dec)
 		dec--;
@@ -281,7 +348,7 @@ static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
 
 	ret = adis_write_reg_16(&st->adis, ADIS16475_REG_DEC_RATE, dec);
 	if (ret)
-		return ret;
+		goto error;
 
 	/*
 	 * If decimation is used, then gyro and accel data will have meaningful
@@ -290,6 +357,9 @@ static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
 	assign_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag, dec);
 
 	return 0;
+error:
+	adis_dev_unlock(&st->adis);
+	return ret;
 }
 
 /* The values are approximated. */
@@ -1085,6 +1155,7 @@ static int adis16475_config_sync_mode(struct adis16475 *st)
 	}
 
 	sync = &st->info->sync[sync_mode];
+	st->sync_mode = sync->sync_mode;
 
 	/* All the other modes require external input signal */
 	if (sync->sync_mode != ADIS16475_SYNC_OUTPUT) {
@@ -1112,37 +1183,20 @@ static int adis16475_config_sync_mode(struct adis16475 *st)
 
 		if (sync->sync_mode == ADIS16475_SYNC_SCALED) {
 			u16 up_scale;
-			u32 scaled_out_freq = 0;
+
 			/*
-			 * If we are in scaled mode, we must have an up_scale.
-			 * In scaled mode the allowable input clock range is
-			 * 1 Hz to 128 Hz, and the allowable output range is
-			 * 1900 to 2100 Hz. Hence, a scale must be given to
-			 * get the allowable output.
+			 * 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...
 			 */
-			ret = device_property_read_u32(dev,
-						       "adi,scaled-output-hz",
-						       &scaled_out_freq);
-			if (ret) {
-				dev_err(dev, "adi,scaled-output-hz must be given when in scaled sync mode");
-				return -EINVAL;
-			} else if (scaled_out_freq < 1900 ||
-				   scaled_out_freq > 2100) {
-				dev_err(dev, "Invalid value: %u for adi,scaled-output-hz",
-					scaled_out_freq);
-				return -EINVAL;
-			}
-
-			up_scale = DIV_ROUND_CLOSEST(scaled_out_freq,
-						     st->clk_freq);
+			up_scale = 2100 / st->clk_freq;
 
 			ret = __adis_write_reg_16(&st->adis,
 						  ADIS16475_REG_UP_SCALE,
 						  up_scale);
 			if (ret)
 				return ret;
-
-			st->clk_freq = scaled_out_freq;
 		}
 
 		st->clk_freq *= 1000;
-- 
2.30.0

[PATCH 4/4] dt-bindings: adis16475: remove property

[Nuno Sá]

`adi,scaled-output-hz` is no longer used by the driver.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 .../devicetree/bindings/iio/imu/adi,adis16475.yaml       | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
index 79fba1508e89..a7574210175a 100644
--- a/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
+++ b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
@@ -71,15 +71,6 @@ properties:
     minimum: 0
     maximum: 3
 
-  adi,scaled-output-hz:
-    description:
-      This property must be present if the clock mode is scaled-sync through
-      clock-names property. In this mode, the input clock can have a range
-      of 1Hz to 128HZ which must be scaled to originate an allowable sample
-      rate. This property specifies that rate.
-    minimum: 1900
-    maximum: 2100
-
 required:
   - compatible
   - reg
-- 
2.30.0

Re: [PATCH 1/4] iio: adis: add helpers for locking

[Jonathan Cameron]

On Thu, 21 Jan 2021 12:49:51 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> Add some helpers to lock and unlock the device.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Hi Nuno

For a patch like this I'd also expect to see it immediately used in
all relevant places in the driver. I don't want a mixture
going forwards of this vs direct access to the lock.

No need to separate that into two patches for such a simple case
just introduce this and put it to use.  There aren't that
many call sites anyway from a quick grep.

Thanks,

Jonathan



> ---
>  include/linux/iio/imu/adis.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index 04e96d688ba9..f9b728d490b1 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -428,6 +428,16 @@ static inline int adis_initial_startup(struct adis *adis)
>  	return ret;
>  }
>  
> +static inline void adis_dev_lock(struct adis *adis)
> +{
> +	mutex_lock(&adis->state_lock);
> +}
> +
> +static inline void adis_dev_unlock(struct adis *adis)
> +{
> +	mutex_unlock(&adis->state_lock);
> +}
> +
>  int adis_single_conversion(struct iio_dev *indio_dev,
>  	const struct iio_chan_spec *chan, unsigned int error_mask,
>  	int *val);

Re: [PATCH 2/4] iio: adis16480: fix pps mode sampling frequency math

[Jonathan Cameron]

On Thu, 21 Jan 2021 12:49:52 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> When using PPS mode, the input clock needs to be scaled so that we have
> an IMU sample rate between (optimally) 4000 and 4250. 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:
>  1. lcm of the input clock and the desired output rate.
>  2. get the highest multiple of the previous result lower than the adis
>     max rate.
>  3. The last result becomes the IMU sample rate. Use that to calculate
>     SYNC_SCALE and DEC_RATE (to get the user output rate).
> 
> Fixes: 326e2357553d3 ("iio: imu: adis16480: Add support for external clock")
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

As this is a fix, please move the refactor of the lock to after this patch.
We don't really want to need to backport that patch in order to apply this
to older kernels.

I'll reply to the cover letter as to what might make sense to do for
the case where we are potentially running the sensor too slow.

Otherwise, patch looks fine to me.

Jonathan

> ---
>  drivers/iio/imu/adis16480.c | 120 ++++++++++++++++++++++++++----------
>  1 file changed, 86 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index dfe86c589325..7620822f3350 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
>  #include <linux/module.h>
> +#include <linux/lcm.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -312,7 +313,8 @@ static int adis16480_debugfs_init(struct iio_dev *indio_dev)
>  static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int val2)
>  {
>  	struct adis16480 *st = iio_priv(indio_dev);
> -	unsigned int t, reg;
> +	unsigned int t, sample_rate = st->clk_freq;
> +	int ret;
>  
>  	if (val < 0 || val2 < 0)
>  		return -EINVAL;
> @@ -321,28 +323,63 @@ static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int val2)
>  	if (t == 0)
>  		return -EINVAL;
>  
> +	adis_dev_lock(&st->adis);
>  	/*
> -	 * When using PPS mode, the rate of data collection is equal to the
> -	 * product of the external clock frequency and the scale factor in the
> -	 * SYNC_SCALE register.
> -	 * When using sync mode, or internal clock, the output data rate is
> -	 * equal with  the clock frequency divided by DEC_RATE + 1.
> +	 * When using PPS mode, the input clock needs to be scaled so that we have an IMU
> +	 * sample rate between (optimally) 4000 and 4250. 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:
> +	 *	1. lcm of the input clock and the desired output rate.
> +	 *	2. get the highest multiple of the previous result lower than the adis max rate.
> +	 *	3. The last result becomes the IMU sample rate. Use that to calculate SYNC_SCALE
> +	 *	   and DEC_RATE (to get the user output rate)
>  	 */
>  	if (st->clk_mode == ADIS16480_CLK_PPS) {
> -		t = t / st->clk_freq;
> -		reg = ADIS16495_REG_SYNC_SCALE;
> -	} else {
> -		t = st->clk_freq / t;
> -		reg = ADIS16480_REG_DEC_RATE;
> +		unsigned long scaled_rate = lcm(st->clk_freq, t);
> +		int sync_scale;
> +		struct device *dev = &st->adis.spi->dev;
> +
> +		/*
> +		 * If lcm is bigger than the IMU maximum sampling rate there's no perfect
> +		 * solution. In this case, we get the highest multiple of the input clock
> +		 * lower that the IMU max sample rate.
> +		 */
> +		if (scaled_rate > st->chip_info->int_clk)
> +			scaled_rate = st->chip_info->int_clk / st->clk_freq * st->clk_freq;
> +		else
> +			scaled_rate = st->chip_info->int_clk / 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 4000Hz due to possible undersampling
> +		 * issues so we will log a warning here. We could even force the rate
> +		 * to 4000 but some users might really want this...
> +		 */
> +		if (scaled_rate < 4000000)
> +			dev_warn(dev, "Possible undersampling issues due to sampling rate=%lu < 4000\n",
> +				 scaled_rate / 1000);
> +
> +		sync_scale = scaled_rate / st->clk_freq;
> +		ret = __adis_write_reg_16(&st->adis, ADIS16495_REG_SYNC_SCALE, sync_scale);
> +		if (ret)
> +			goto error;
> +
> +		sample_rate = scaled_rate;
>  	}
>  
> +	t = DIV_ROUND_CLOSEST(sample_rate, t);
> +	if (t)
> +		t--;
> +
>  	if (t > st->chip_info->max_dec_rate)
>  		t = st->chip_info->max_dec_rate;
>  
> -	if ((t != 0) && (st->clk_mode != ADIS16480_CLK_PPS))
> -		t--;
> -
> -	return adis_write_reg_16(&st->adis, reg, t);
> +	ret = __adis_write_reg_16(&st->adis, ADIS16480_REG_DEC_RATE, t);
> +error:
> +	adis_dev_unlock(&st->adis);
> +	return ret;
>  }
>  
>  static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
> @@ -350,34 +387,35 @@ static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int *val2)
>  	struct adis16480 *st = iio_priv(indio_dev);
>  	uint16_t t;
>  	int ret;
> -	unsigned int freq;
> -	unsigned int reg;
> +	unsigned int freq, sample_rate = st->clk_freq;
>  
> -	if (st->clk_mode == ADIS16480_CLK_PPS)
> -		reg = ADIS16495_REG_SYNC_SCALE;
> -	else
> -		reg = ADIS16480_REG_DEC_RATE;
> +	adis_dev_lock(&st->adis);
>  
> -	ret = adis_read_reg_16(&st->adis, reg, &t);
> +	if (st->clk_mode == ADIS16480_CLK_PPS) {
> +		u16 sync_scale;
> +
> +		ret = __adis_read_reg_16(&st->adis, ADIS16495_REG_SYNC_SCALE, &sync_scale);
> +		if (ret)
> +			goto error;
> +
> +		sample_rate = st->clk_freq * sync_scale;
> +	}
> +
> +	ret = __adis_read_reg_16(&st->adis, ADIS16480_REG_DEC_RATE, &t);
>  	if (ret)
> -		return ret;
> +		goto error;
>  
> -	/*
> -	 * When using PPS mode, the rate of data collection is equal to the
> -	 * product of the external clock frequency and the scale factor in the
> -	 * SYNC_SCALE register.
> -	 * When using sync mode, or internal clock, the output data rate is
> -	 * equal with  the clock frequency divided by DEC_RATE + 1.
> -	 */
> -	if (st->clk_mode == ADIS16480_CLK_PPS)
> -		freq = st->clk_freq * t;
> -	else
> -		freq = st->clk_freq / (t + 1);
> +	adis_dev_unlock(&st->adis);
> +
> +	freq = DIV_ROUND_CLOSEST(sample_rate, (t + 1));
>  
>  	*val = freq / 1000;
>  	*val2 = (freq % 1000) * 1000;
>  
>  	return IIO_VAL_INT_PLUS_MICRO;
> +error:
> +	adis_dev_unlock(&st->adis);
> +	return ret;
>  }
>  
>  enum {
> @@ -1278,6 +1316,20 @@ static int adis16480_probe(struct spi_device *spi)
>  
>  		st->clk_freq = clk_get_rate(st->ext_clk);
>  		st->clk_freq *= 1000; /* micro */
> +		if (st->clk_mode == ADIS16480_CLK_PPS) {
> +			u16 sync_scale;
> +
> +			/*
> +			 * In PPS 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 internal sample rate is the
> +			 * max...
> +			 */
> +			sync_scale = st->chip_info->int_clk / st->clk_freq;
> +			ret = __adis_write_reg_16(&st->adis, ADIS16495_REG_SYNC_SCALE, sync_scale);
> +			if (ret)
> +				return ret;
> +		}
>  	} else {
>  		st->clk_freq = st->chip_info->int_clk;
>  	}

Re: [PATCH 0/4] Fix/Improve sync clock mode handling

[Jonathan Cameron]

On Thu, 21 Jan 2021 12:49:50 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> The first patch in this series is just a simple helper to lock/unlock
> the device. Having these helpers make the code slightly neater (IMHO).
> 
> The following patches introduces changes in the sampling frequency
> calculation when sync scale/pps modes are used. First, it's important
> to understand the purpose of this mode and how it should be used. Let's
> say our part has an internal rate of 4250 (e.g adis1649x family) and the
> user wants an output rate of 200SPS. Obviously, we can't use this
> sampling rate and divide back down to get 200 SPS with decimation on.
> Hence, we can use this mode to give an input clock of 1HZ, scale it to
> something like 4200 or 4000 SPS and then use the decimation filter to get
> the exact desired 200SPS. There are also some limits that should be
> taken into account when scaling:
> 
>  * For the devices in the adis16475 driver:
>      - Input sync frequency range is 1 to 128 Hz
>      - Native sample rate: 2 kSPS.  Optimal range: 1900-2100 sps
> 
>  * For the adis1649x family (adis16480 driver):
>     - Input sync frequency range is 1 to 128 Hz
>     - Native sample rate: 4.25 kSPS.  Optimal range: 4000-4250 sps 
> 
> I'm not 100% convinced on how to handle the optimal minimum. For now,
> I'm just throwing a warning saying we might get into trouble if we get a
> value lower than that. I was also tempted to just return -EINVAL or
> clamp the value. However, I know that there are ADI customers that
> (for some reason) are using a sampling rate lower than the minimum
> advised.

So the opening question I'd have here is how critical is the actual
userspace sampling rate to your users?   Often they don't mind
getting a little more data than they asked for (say 200.5Hz when asking
for 200) and can always read back the attribute after writing it to
discover this has happened. 

As such, once you've discovered that value doesn't have an exact
match, perhaps tweak the output data rate until it fits nicely?
A bit of quick investigation suggests (by my wife who happened
to be sat across the room) suggests that you have a hideous set
of local minima so your best bet is to brute force search
(not that bad and we don't expect to change this a lot!)

> 
> That said, the patch for the adis16480 driver is a fix as this mode was
> being wrongly handled. There should not be a "separation" between using
> the sync_scale and the dec_rate registers. The way things were being done,
> we could easily get into a situation where the part could be running with
> an internal rate way lower than the optimal minimum.
> 
> For the adis16475 drivers, things were not really wrong. They were just
> not optimal as we were forcing users to specify the IMU scaled internal
> rate once in the devicetree. Calculating things at runtime gives much
> more flexibility to choose the output rate.
> 
> Nuno Sá (4):
>   iio: adis: add helpers for locking
>   iio: adis16480: fix pps mode sampling frequency math
>   iio: adis16475: improve sync scale mode handling
>   dt-bindings: adis16475: remove property
> 
>  .../bindings/iio/imu/adi,adis16475.yaml       |   9 --
>  drivers/iio/imu/adis16475.c                   | 110 ++++++++++++----
>  drivers/iio/imu/adis16480.c                   | 120 +++++++++++++-----
>  include/linux/iio/imu/adis.h                  |  10 ++
>  4 files changed, 178 insertions(+), 71 deletions(-)
> 

RE: [PATCH 1/4] iio: adis: add helpers for locking

[Sa, Nuno]

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, January 24, 2021 2:30 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree@vger.kernel.org; linux-iio@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Peter Meerwald-Stadler
> <pmeerw@pmeerw.net>; Lars-Peter Clausen <lars@metafoo.de>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>
> Subject: Re: [PATCH 1/4] iio: adis: add helpers for locking
> 
> 
> On Thu, 21 Jan 2021 12:49:51 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > Add some helpers to lock and unlock the device.
> >
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> Hi Nuno
>

Hi Jonathan,

> For a patch like this I'd also expect to see it immediately used in
> all relevant places in the driver. I don't want a mixture
> going forwards of this vs direct access to the lock.

Fully agreed...

> No need to separate that into two patches for such a simple case
> just introduce this and put it to use.  There aren't that
> many call sites anyway from a quick grep.
> 

Ok, will do that in v2.

- Nuno Sá

RE: [PATCH 2/4] iio: adis16480: fix pps mode sampling frequency math

[Sa, Nuno]

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, January 24, 2021 2:43 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree@vger.kernel.org; linux-iio@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Peter Meerwald-Stadler
> <pmeerw@pmeerw.net>; Lars-Peter Clausen <lars@metafoo.de>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>
> Subject: Re: [PATCH 2/4] iio: adis16480: fix pps mode sampling
> frequency math
> 
> [External]
> 
> On Thu, 21 Jan 2021 12:49:52 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > When using PPS mode, the input clock needs to be scaled so that we
> have
> > an IMU sample rate between (optimally) 4000 and 4250. 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:
> >  1. lcm of the input clock and the desired output rate.
> >  2. get the highest multiple of the previous result lower than the adis
> >     max rate.
> >  3. The last result becomes the IMU sample rate. Use that to calculate
> >     SYNC_SCALE and DEC_RATE (to get the user output rate).
> >
> > Fixes: 326e2357553d3 ("iio: imu: adis16480: Add support for external
> clock")
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> As this is a fix, please move the refactor of the lock to after this patch.
> We don't really want to need to backport that patch in order to apply
> this
> to older kernels.

Hmm, completely missed this... will do that.

- Nuno Sá

> I'll reply to the cover letter as to what might make sense to do for
> the case where we are potentially running the sensor too slow.
> 
> Otherwise, patch looks fine to me.
> 
> Jonathan

> > ---
> >  drivers/iio/imu/adis16480.c | 120 ++++++++++++++++++++++++++-
> ---------
> >  1 file changed, 86 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > index dfe86c589325..7620822f3350 100644
> > --- a/drivers/iio/imu/adis16480.c
> > +++ b/drivers/iio/imu/adis16480.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/sysfs.h>
> >  #include <linux/module.h>
> > +#include <linux/lcm.h>
> >
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > @@ -312,7 +313,8 @@ static int adis16480_debugfs_init(struct
> iio_dev *indio_dev)
> >  static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int
> val2)
> >  {
> >  	struct adis16480 *st = iio_priv(indio_dev);
> > -	unsigned int t, reg;
> > +	unsigned int t, sample_rate = st->clk_freq;
> > +	int ret;
> >
> >  	if (val < 0 || val2 < 0)
> >  		return -EINVAL;
> > @@ -321,28 +323,63 @@ static int adis16480_set_freq(struct iio_dev
> *indio_dev, int val, int val2)
> >  	if (t == 0)
> >  		return -EINVAL;
> >
> > +	adis_dev_lock(&st->adis);
> >  	/*
> > -	 * When using PPS mode, the rate of data collection is equal to
> the
> > -	 * product of the external clock frequency and the scale factor
> in the
> > -	 * SYNC_SCALE register.
> > -	 * When using sync mode, or internal clock, the output data
> rate is
> > -	 * equal with  the clock frequency divided by DEC_RATE + 1.
> > +	 * When using PPS mode, the input clock needs to be scaled so
> that we have an IMU
> > +	 * sample rate between (optimally) 4000 and 4250. 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:
> > +	 *	1. lcm of the input clock and the desired output rate.
> > +	 *	2. get the highest multiple of the previous result lower
> than the adis max rate.
> > +	 *	3. The last result becomes the IMU sample rate. Use
> that to calculate SYNC_SCALE
> > +	 *	   and DEC_RATE (to get the user output rate)
> >  	 */
> >  	if (st->clk_mode == ADIS16480_CLK_PPS) {
> > -		t = t / st->clk_freq;
> > -		reg = ADIS16495_REG_SYNC_SCALE;
> > -	} else {
> > -		t = st->clk_freq / t;
> > -		reg = ADIS16480_REG_DEC_RATE;
> > +		unsigned long scaled_rate = lcm(st->clk_freq, t);
> > +		int sync_scale;
> > +		struct device *dev = &st->adis.spi->dev;
> > +
> > +		/*
> > +		 * If lcm is bigger than the IMU maximum sampling rate
> there's no perfect
> > +		 * solution. In this case, we get the highest multiple of
> the input clock
> > +		 * lower that the IMU max sample rate.
> > +		 */
> > +		if (scaled_rate > st->chip_info->int_clk)
> > +			scaled_rate = st->chip_info->int_clk / st-
> >clk_freq * st->clk_freq;
> > +		else
> > +			scaled_rate = st->chip_info->int_clk /
> 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 4000Hz due to possible
> undersampling
> > +		 * issues so we will log a warning here. We could even
> force the rate
> > +		 * to 4000 but some users might really want this...
> > +		 */
> > +		if (scaled_rate < 4000000)
> > +			dev_warn(dev, "Possible undersampling issues
> due to sampling rate=%lu < 4000\n",
> > +				 scaled_rate / 1000);
> > +
> > +		sync_scale = scaled_rate / st->clk_freq;
> > +		ret = __adis_write_reg_16(&st->adis,
> ADIS16495_REG_SYNC_SCALE, sync_scale);
> > +		if (ret)
> > +			goto error;
> > +
> > +		sample_rate = scaled_rate;
> >  	}
> >
> > +	t = DIV_ROUND_CLOSEST(sample_rate, t);
> > +	if (t)
> > +		t--;
> > +
> >  	if (t > st->chip_info->max_dec_rate)
> >  		t = st->chip_info->max_dec_rate;
> >
> > -	if ((t != 0) && (st->clk_mode != ADIS16480_CLK_PPS))
> > -		t--;
> > -
> > -	return adis_write_reg_16(&st->adis, reg, t);
> > +	ret = __adis_write_reg_16(&st->adis,
> ADIS16480_REG_DEC_RATE, t);
> > +error:
> > +	adis_dev_unlock(&st->adis);
> > +	return ret;
> >  }
> >
> >  static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int
> *val2)
> > @@ -350,34 +387,35 @@ static int adis16480_get_freq(struct iio_dev
> *indio_dev, int *val, int *val2)
> >  	struct adis16480 *st = iio_priv(indio_dev);
> >  	uint16_t t;
> >  	int ret;
> > -	unsigned int freq;
> > -	unsigned int reg;
> > +	unsigned int freq, sample_rate = st->clk_freq;
> >
> > -	if (st->clk_mode == ADIS16480_CLK_PPS)
> > -		reg = ADIS16495_REG_SYNC_SCALE;
> > -	else
> > -		reg = ADIS16480_REG_DEC_RATE;
> > +	adis_dev_lock(&st->adis);
> >
> > -	ret = adis_read_reg_16(&st->adis, reg, &t);
> > +	if (st->clk_mode == ADIS16480_CLK_PPS) {
> > +		u16 sync_scale;
> > +
> > +		ret = __adis_read_reg_16(&st->adis,
> ADIS16495_REG_SYNC_SCALE, &sync_scale);
> > +		if (ret)
> > +			goto error;
> > +
> > +		sample_rate = st->clk_freq * sync_scale;
> > +	}
> > +
> > +	ret = __adis_read_reg_16(&st->adis,
> ADIS16480_REG_DEC_RATE, &t);
> >  	if (ret)
> > -		return ret;
> > +		goto error;
> >
> > -	/*
> > -	 * When using PPS mode, the rate of data collection is equal to
> the
> > -	 * product of the external clock frequency and the scale factor
> in the
> > -	 * SYNC_SCALE register.
> > -	 * When using sync mode, or internal clock, the output data
> rate is
> > -	 * equal with  the clock frequency divided by DEC_RATE + 1.
> > -	 */
> > -	if (st->clk_mode == ADIS16480_CLK_PPS)
> > -		freq = st->clk_freq * t;
> > -	else
> > -		freq = st->clk_freq / (t + 1);
> > +	adis_dev_unlock(&st->adis);
> > +
> > +	freq = DIV_ROUND_CLOSEST(sample_rate, (t + 1));
> >
> >  	*val = freq / 1000;
> >  	*val2 = (freq % 1000) * 1000;
> >
> >  	return IIO_VAL_INT_PLUS_MICRO;
> > +error:
> > +	adis_dev_unlock(&st->adis);
> > +	return ret;
> >  }
> >
> >  enum {
> > @@ -1278,6 +1316,20 @@ static int adis16480_probe(struct
> spi_device *spi)
> >
> >  		st->clk_freq = clk_get_rate(st->ext_clk);
> >  		st->clk_freq *= 1000; /* micro */
> > +		if (st->clk_mode == ADIS16480_CLK_PPS) {
> > +			u16 sync_scale;
> > +
> > +			/*
> > +			 * In PPS 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
> internal sample rate is the
> > +			 * max...
> > +			 */
> > +			sync_scale = st->chip_info->int_clk / st-
> >clk_freq;
> > +			ret = __adis_write_reg_16(&st->adis,
> ADIS16495_REG_SYNC_SCALE, sync_scale);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  	} else {
> >  		st->clk_freq = st->chip_info->int_clk;
> >  	}

RE: [PATCH 0/4] Fix/Improve sync clock mode handling

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, January 24, 2021 3:21 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree@vger.kernel.org; linux-iio@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Peter Meerwald-Stadler
> <pmeerw@pmeerw.net>; Lars-Peter Clausen <lars@metafoo.de>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>
> Subject: Re: [PATCH 0/4] Fix/Improve sync clock mode handling
> 
> 
> On Thu, 21 Jan 2021 12:49:50 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > The first patch in this series is just a simple helper to lock/unlock
> > the device. Having these helpers make the code slightly neater
> (IMHO).
> >
> > The following patches introduces changes in the sampling frequency
> > calculation when sync scale/pps modes are used. First, it's important
> > to understand the purpose of this mode and how it should be used.
> Let's
> > say our part has an internal rate of 4250 (e.g adis1649x family) and
> the
> > user wants an output rate of 200SPS. Obviously, we can't use this
> > sampling rate and divide back down to get 200 SPS with decimation
> on.
> > Hence, we can use this mode to give an input clock of 1HZ, scale it to
> > something like 4200 or 4000 SPS and then use the decimation filter to
> get
> > the exact desired 200SPS. There are also some limits that should be
> > taken into account when scaling:
> >
> >  * For the devices in the adis16475 driver:
> >      - Input sync frequency range is 1 to 128 Hz
> >      - Native sample rate: 2 kSPS.  Optimal range: 1900-2100 sps
> >
> >  * For the adis1649x family (adis16480 driver):
> >     - Input sync frequency range is 1 to 128 Hz
> >     - Native sample rate: 4.25 kSPS.  Optimal range: 4000-4250 sps
> >
> > I'm not 100% convinced on how to handle the optimal minimum. For
> now,
> > I'm just throwing a warning saying we might get into trouble if we get
> a
> > value lower than that. I was also tempted to just return -EINVAL or
> > clamp the value. However, I know that there are ADI customers that
> > (for some reason) are using a sampling rate lower than the minimum
> > advised.
> 
> So the opening question I'd have here is how critical is the actual
> userspace sampling rate to your users?   Often they don't mind
> getting a little more data than they asked for (say 200.5Hz when asking
> for 200) and can always read back the attribute after writing it to
> discover this has happened.

Well, honestly I'm not really sure here. I can just say (from the info I got internally) that some
users are really using these parts with a data rate lower than the advised. However, I'd say
that this would depend on the use case. For some critical cases, I would expect that users really
want to have an exact sample rate. Though I'd argue that in those cases, they should know what
they are doing and provide an output rate that fits nicely (multiple of both the input clock and IMU
internal sample rate). And as you said, they can always readback the value to check if they are
getting something that is not really what they expect...

> As such, once you've discovered that value doesn't have an exact
> match, perhaps tweak the output data rate until it fits nicely?

I did thought about this. The reason why I didn't went for it in this first version is because of those
who seems to really want to run the part at lower rates. Let's assume we have an input clock of
1HZ and someone writes an output rate of 3000SPS. The only way to accomplish this is to set
sync_scale at 3000 and let the IMU run at 3000SPS with decimation off (DEC_RATE=0). If we are
going to tweak the output rate to fit nicely, we would have to force it to 4000SPS which is
significantly different from the desired 3000SPS.

> A bit of quick investigation suggests (by my wife who happened
> to be sat across the room) suggests that you have a hideous set
> of local minima so your best bet is to brute force search
> (not that bad and we don't expect to change this a lot!)

Hmm, not sure what do you have in mind here :)?

- Nuno Sá

> >
> > That said, the patch for the adis16480 driver is a fix as this mode was
> > being wrongly handled. There should not be a "separation" between
> using
> > the sync_scale and the dec_rate registers. The way things were
> being done,
> > we could easily get into a situation where the part could be running
> with
> > an internal rate way lower than the optimal minimum.
> >
> > For the adis16475 drivers, things were not really wrong. They were
> just
> > not optimal as we were forcing users to specify the IMU scaled
> internal
> > rate once in the devicetree. Calculating things at runtime gives much
> > more flexibility to choose the output rate.
> >
> > Nuno Sá (4):
> >   iio: adis: add helpers for locking
> >   iio: adis16480: fix pps mode sampling frequency math
> >   iio: adis16475: improve sync scale mode handling
> >   dt-bindings: adis16475: remove property
> >
> >  .../bindings/iio/imu/adi,adis16475.yaml       |   9 --
> >  drivers/iio/imu/adis16475.c                   | 110 ++++++++++++----
> >  drivers/iio/imu/adis16480.c                   | 120 +++++++++++++-----
> >  include/linux/iio/imu/adis.h                  |  10 ++
> >  4 files changed, 178 insertions(+), 71 deletions(-)
> >

RE: [PATCH 2/4] iio: adis16480: fix pps mode sampling frequency math

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, January 24, 2021 2:43 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree@vger.kernel.org; linux-iio@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Peter Meerwald-Stadler
> <pmeerw@pmeerw.net>; Lars-Peter Clausen <lars@metafoo.de>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>
> Subject: Re: [PATCH 2/4] iio: adis16480: fix pps mode sampling
> frequency math
> 
> [External]
> 
> On Thu, 21 Jan 2021 12:49:52 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > When using PPS mode, the input clock needs to be scaled so that we
> have
> > an IMU sample rate between (optimally) 4000 and 4250. 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:
> >  1. lcm of the input clock and the desired output rate.
> >  2. get the highest multiple of the previous result lower than the adis
> >     max rate.
> >  3. The last result becomes the IMU sample rate. Use that to calculate
> >     SYNC_SCALE and DEC_RATE (to get the user output rate).
> >
> > Fixes: 326e2357553d3 ("iio: imu: adis16480: Add support for external
> clock")
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> As this is a fix, please move the refactor of the lock to after this patch.
> We don't really want to need to backport that patch in order to apply
> this
> to older kernels.

Hmm, completely missed this... will do that.

- Nuno Sá

> I'll reply to the cover letter as to what might make sense to do for
> the case where we are potentially running the sensor too slow.
> 
> Otherwise, patch looks fine to me.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/imu/adis16480.c | 120 ++++++++++++++++++++++++++-
> ---------
> >  1 file changed, 86 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > index dfe86c589325..7620822f3350 100644
> > --- a/drivers/iio/imu/adis16480.c
> > +++ b/drivers/iio/imu/adis16480.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/sysfs.h>
> >  #include <linux/module.h>
> > +#include <linux/lcm.h>
> >
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > @@ -312,7 +313,8 @@ static int adis16480_debugfs_init(struct
> iio_dev *indio_dev)
> >  static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int
> val2)
> >  {
> >  	struct adis16480 *st = iio_priv(indio_dev);
> > -	unsigned int t, reg;
> > +	unsigned int t, sample_rate = st->clk_freq;
> > +	int ret;
> >
> >  	if (val < 0 || val2 < 0)
> >  		return -EINVAL;
> > @@ -321,28 +323,63 @@ static int adis16480_set_freq(struct iio_dev
> *indio_dev, int val, int val2)
> >  	if (t == 0)
> >  		return -EINVAL;
> >
> > +	adis_dev_lock(&st->adis);
> >  	/*
> > -	 * When using PPS mode, the rate of data collection is equal to
> the
> > -	 * product of the external clock frequency and the scale factor
> in the
> > -	 * SYNC_SCALE register.
> > -	 * When using sync mode, or internal clock, the output data
> rate is
> > -	 * equal with  the clock frequency divided by DEC_RATE + 1.
> > +	 * When using PPS mode, the input clock needs to be scaled so
> that we have an IMU
> > +	 * sample rate between (optimally) 4000 and 4250. 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:
> > +	 *	1. lcm of the input clock and the desired output rate.
> > +	 *	2. get the highest multiple of the previous result lower
> than the adis max rate.
> > +	 *	3. The last result becomes the IMU sample rate. Use
> that to calculate SYNC_SCALE
> > +	 *	   and DEC_RATE (to get the user output rate)
> >  	 */
> >  	if (st->clk_mode == ADIS16480_CLK_PPS) {
> > -		t = t / st->clk_freq;
> > -		reg = ADIS16495_REG_SYNC_SCALE;
> > -	} else {
> > -		t = st->clk_freq / t;
> > -		reg = ADIS16480_REG_DEC_RATE;
> > +		unsigned long scaled_rate = lcm(st->clk_freq, t);
> > +		int sync_scale;
> > +		struct device *dev = &st->adis.spi->dev;
> > +
> > +		/*
> > +		 * If lcm is bigger than the IMU maximum sampling rate
> there's no perfect
> > +		 * solution. In this case, we get the highest multiple of
> the input clock
> > +		 * lower that the IMU max sample rate.
> > +		 */
> > +		if (scaled_rate > st->chip_info->int_clk)
> > +			scaled_rate = st->chip_info->int_clk / st-
> >clk_freq * st->clk_freq;
> > +		else
> > +			scaled_rate = st->chip_info->int_clk /
> 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 4000Hz due to possible
> undersampling
> > +		 * issues so we will log a warning here. We could even
> force the rate
> > +		 * to 4000 but some users might really want this...
> > +		 */
> > +		if (scaled_rate < 4000000)
> > +			dev_warn(dev, "Possible undersampling issues
> due to sampling rate=%lu < 4000\n",
> > +				 scaled_rate / 1000);
> > +
> > +		sync_scale = scaled_rate / st->clk_freq;
> > +		ret = __adis_write_reg_16(&st->adis,
> ADIS16495_REG_SYNC_SCALE, sync_scale);
> > +		if (ret)
> > +			goto error;
> > +
> > +		sample_rate = scaled_rate;
> >  	}
> >
> > +	t = DIV_ROUND_CLOSEST(sample_rate, t);
> > +	if (t)
> > +		t--;
> > +
> >  	if (t > st->chip_info->max_dec_rate)
> >  		t = st->chip_info->max_dec_rate;
> >
> > -	if ((t != 0) && (st->clk_mode != ADIS16480_CLK_PPS))
> > -		t--;
> > -
> > -	return adis_write_reg_16(&st->adis, reg, t);
> > +	ret = __adis_write_reg_16(&st->adis,
> ADIS16480_REG_DEC_RATE, t);
> > +error:
> > +	adis_dev_unlock(&st->adis);
> > +	return ret;
> >  }
> >
> >  static int adis16480_get_freq(struct iio_dev *indio_dev, int *val, int
> *val2)
> > @@ -350,34 +387,35 @@ static int adis16480_get_freq(struct iio_dev
> *indio_dev, int *val, int *val2)
> >  	struct adis16480 *st = iio_priv(indio_dev);
> >  	uint16_t t;
> >  	int ret;
> > -	unsigned int freq;
> > -	unsigned int reg;
> > +	unsigned int freq, sample_rate = st->clk_freq;
> >
> > -	if (st->clk_mode == ADIS16480_CLK_PPS)
> > -		reg = ADIS16495_REG_SYNC_SCALE;
> > -	else
> > -		reg = ADIS16480_REG_DEC_RATE;
> > +	adis_dev_lock(&st->adis);
> >
> > -	ret = adis_read_reg_16(&st->adis, reg, &t);
> > +	if (st->clk_mode == ADIS16480_CLK_PPS) {
> > +		u16 sync_scale;
> > +
> > +		ret = __adis_read_reg_16(&st->adis,
> ADIS16495_REG_SYNC_SCALE, &sync_scale);
> > +		if (ret)
> > +			goto error;
> > +
> > +		sample_rate = st->clk_freq * sync_scale;
> > +	}
> > +
> > +	ret = __adis_read_reg_16(&st->adis,
> ADIS16480_REG_DEC_RATE, &t);
> >  	if (ret)
> > -		return ret;
> > +		goto error;
> >
> > -	/*
> > -	 * When using PPS mode, the rate of data collection is equal to
> the
> > -	 * product of the external clock frequency and the scale factor
> in the
> > -	 * SYNC_SCALE register.
> > -	 * When using sync mode, or internal clock, the output data
> rate is
> > -	 * equal with  the clock frequency divided by DEC_RATE + 1.
> > -	 */
> > -	if (st->clk_mode == ADIS16480_CLK_PPS)
> > -		freq = st->clk_freq * t;
> > -	else
> > -		freq = st->clk_freq / (t + 1);
> > +	adis_dev_unlock(&st->adis);
> > +
> > +	freq = DIV_ROUND_CLOSEST(sample_rate, (t + 1));
> >
> >  	*val = freq / 1000;
> >  	*val2 = (freq % 1000) * 1000;
> >
> >  	return IIO_VAL_INT_PLUS_MICRO;
> > +error:
> > +	adis_dev_unlock(&st->adis);
> > +	return ret;
> >  }
> >
> >  enum {
> > @@ -1278,6 +1316,20 @@ static int adis16480_probe(struct
> spi_device *spi)
> >
> >  		st->clk_freq = clk_get_rate(st->ext_clk);
> >  		st->clk_freq *= 1000; /* micro */
> > +		if (st->clk_mode == ADIS16480_CLK_PPS) {
> > +			u16 sync_scale;
> > +
> > +			/*
> > +			 * In PPS 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
> internal sample rate is the
> > +			 * max...
> > +			 */
> > +			sync_scale = st->chip_info->int_clk / st-
> >clk_freq;
> > +			ret = __adis_write_reg_16(&st->adis,
> ADIS16495_REG_SYNC_SCALE, sync_scale);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  	} else {
> >  		st->clk_freq = st->chip_info->int_clk;
> >  	}

RE: [PATCH 0/4] Fix/Improve sync clock mode handling

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, January 24, 2021 3:21 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree@vger.kernel.org; linux-iio@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Peter Meerwald-Stadler
> <pmeerw@pmeerw.net>; Lars-Peter Clausen <lars@metafoo.de>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>
> Subject: Re: [PATCH 0/4] Fix/Improve sync clock mode handling
> 
> On Thu, 21 Jan 2021 12:49:50 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > The first patch in this series is just a simple helper to lock/unlock
> > the device. Having these helpers make the code slightly neater
> (IMHO).
> >
> > The following patches introduces changes in the sampling frequency
> > calculation when sync scale/pps modes are used. First, it's important
> > to understand the purpose of this mode and how it should be used.
> Let's
> > say our part has an internal rate of 4250 (e.g adis1649x family) and
> the
> > user wants an output rate of 200SPS. Obviously, we can't use this
> > sampling rate and divide back down to get 200 SPS with decimation
> on.
> > Hence, we can use this mode to give an input clock of 1HZ, scale it to
> > something like 4200 or 4000 SPS and then use the decimation filter to
> get
> > the exact desired 200SPS. There are also some limits that should be
> > taken into account when scaling:
> >
> >  * For the devices in the adis16475 driver:
> >      - Input sync frequency range is 1 to 128 Hz
> >      - Native sample rate: 2 kSPS.  Optimal range: 1900-2100 sps
> >
> >  * For the adis1649x family (adis16480 driver):
> >     - Input sync frequency range is 1 to 128 Hz
> >     - Native sample rate: 4.25 kSPS.  Optimal range: 4000-4250 sps
> >
> > I'm not 100% convinced on how to handle the optimal minimum. For
> now,
> > I'm just throwing a warning saying we might get into trouble if we get
> a
> > value lower than that. I was also tempted to just return -EINVAL or
> > clamp the value. However, I know that there are ADI customers that
> > (for some reason) are using a sampling rate lower than the minimum
> > advised.
> 
> So the opening question I'd have here is how critical is the actual
> userspace sampling rate to your users?   Often they don't mind
> getting a little more data than they asked for (say 200.5Hz when asking
> for 200) and can always read back the attribute after writing it to
> discover this has happened.
> 

Well, honestly I'm not really sure here. I can just say (from the info I got internally) that some
users are really using these parts with a data rate lower than the advised. However, I'd say
that this would depend on the use case. For some critical cases, I would expect that users really
want to have an exact sample rate. Though I'd argue that in those cases, they should know what
they are doing and provide an output rate that fits nicely (multiple of both the input clock and IMU
internal sample rate). And as you said, they can always readback the value to check if they are
getting something that is not really what they expect...

> As such, once you've discovered that value doesn't have an exact
> match, perhaps tweak the output data rate until it fits nicely?

I did thought about this. The reason why I didn't went for it in this first version is because of those
who seems to really want to run the part at lower rates. Let's assume we have an input clock of
1HZ and someone writes an output rate of 3000SPS. The only way to accomplish this is to set
sync_scale at 3000 and let the IMU run at 3000SPS with decimation off (DEC_RATE=0). If we are
going to tweak the output rate to fit nicely, we would have to force it to 4000SPS which is
significantly different from the desired 3000SPS.

> A bit of quick investigation suggests (by my wife who happened
> to be sat across the room) suggests that you have a hideous set
> of local minima so your best bet is to brute force search
> (not that bad and we don't expect to change this a lot!)

Hmm, not sure what do you have in mind here :)?

- Nuno Sa

> 
> >
> > That said, the patch for the adis16480 driver is a fix as this mode was
> > being wrongly handled. There should not be a "separation" between
> using
> > the sync_scale and the dec_rate registers. The way things were
> being done,
> > we could easily get into a situation where the part could be running
> with
> > an internal rate way lower than the optimal minimum.
> >
> > For the adis16475 drivers, things were not really wrong. They were
> just
> > not optimal as we were forcing users to specify the IMU scaled
> internal
> > rate once in the devicetree. Calculating things at runtime gives much
> > more flexibility to choose the output rate.
> >
> > Nuno Sá (4):
> >   iio: adis: add helpers for locking
> >   iio: adis16480: fix pps mode sampling frequency math
> >   iio: adis16475: improve sync scale mode handling
> >   dt-bindings: adis16475: remove property
> >
> >  .../bindings/iio/imu/adi,adis16475.yaml       |   9 --
> >  drivers/iio/imu/adis16475.c                   | 110 ++++++++++++----
> >  drivers/iio/imu/adis16480.c                   | 120 +++++++++++++-----
> >  include/linux/iio/imu/adis.h                  |  10 ++
> >  4 files changed, 178 insertions(+), 71 deletions(-)
> >

RE: [PATCH 0/4] Fix/Improve sync clock mode handling

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, January 31, 2021 12:36 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: devicetree@vger.kernel.org; linux-iio@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Peter Meerwald-Stadler
> <pmeerw@pmeerw.net>; Lars-Peter Clausen <lars@metafoo.de>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>
> Subject: Re: [PATCH 0/4] Fix/Improve sync clock mode handling
> 
> 
> On Mon, 25 Jan 2021 09:16:19 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Sunday, January 24, 2021 3:21 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: devicetree@vger.kernel.org; linux-iio@vger.kernel.org; Rob
> > > Herring <robh+dt@kernel.org>; Peter Meerwald-Stadler
> > > <pmeerw@pmeerw.net>; Lars-Peter Clausen <lars@metafoo.de>;
> > > Hennerich, Michael <Michael.Hennerich@analog.com>; Ardelean,
> > > Alexandru <alexandru.Ardelean@analog.com>
> > > Subject: Re: [PATCH 0/4] Fix/Improve sync clock mode handling
> > >
> > >
> > > On Thu, 21 Jan 2021 12:49:50 +0100
> > > Nuno Sá <nuno.sa@analog.com> wrote:
> > >
> > > > The first patch in this series is just a simple helper to lock/unlock
> > > > the device. Having these helpers make the code slightly neater
> > > (IMHO).
> > > >
> > > > The following patches introduces changes in the sampling
> frequency
> > > > calculation when sync scale/pps modes are used. First, it's
> important
> > > > to understand the purpose of this mode and how it should be
> used.
> > > Let's
> > > > say our part has an internal rate of 4250 (e.g adis1649x family) and
> > > the
> > > > user wants an output rate of 200SPS. Obviously, we can't use this
> > > > sampling rate and divide back down to get 200 SPS with
> decimation
> > > on.
> > > > Hence, we can use this mode to give an input clock of 1HZ, scale it
> to
> > > > something like 4200 or 4000 SPS and then use the decimation
> filter to
> > > get
> > > > the exact desired 200SPS. There are also some limits that should
> be
> > > > taken into account when scaling:
> > > >
> > > >  * For the devices in the adis16475 driver:
> > > >      - Input sync frequency range is 1 to 128 Hz
> > > >      - Native sample rate: 2 kSPS.  Optimal range: 1900-2100 sps
> > > >
> > > >  * For the adis1649x family (adis16480 driver):
> > > >     - Input sync frequency range is 1 to 128 Hz
> > > >     - Native sample rate: 4.25 kSPS.  Optimal range: 4000-4250 sps
> > > >
> > > > I'm not 100% convinced on how to handle the optimal minimum.
> For
> > > now,
> > > > I'm just throwing a warning saying we might get into trouble if we
> get
> > > a
> > > > value lower than that. I was also tempted to just return -EINVAL
> or
> > > > clamp the value. However, I know that there are ADI customers
> that
> > > > (for some reason) are using a sampling rate lower than the
> minimum
> > > > advised.
> > >
> > > So the opening question I'd have here is how critical is the actual
> > > userspace sampling rate to your users?   Often they don't mind
> > > getting a little more data than they asked for (say 200.5Hz when
> asking
> > > for 200) and can always read back the attribute after writing it to
> > > discover this has happened.
> >
> > Well, honestly I'm not really sure here. I can just say (from the info I
> got internally) that some
> > users are really using these parts with a data rate lower than the
> advised. However, I'd say
> > that this would depend on the use case. For some critical cases, I
> would expect that users really
> > want to have an exact sample rate. Though I'd argue that in those
> cases, they should know what
> > they are doing and provide an output rate that fits nicely (multiple of
> both the input clock and IMU
> > internal sample rate). And as you said, they can always readback the
> value to check if they are
> > getting something that is not really what they expect...
> >
> > > As such, once you've discovered that value doesn't have an exact
> > > match, perhaps tweak the output data rate until it fits nicely?
> >
> > I did thought about this. The reason why I didn't went for it in this
> first version is because of those
> > who seems to really want to run the part at lower rates. Let's assume
> we have an input clock of
> > 1HZ and someone writes an output rate of 3000SPS. The only way to
> accomplish this is to set
> > sync_scale at 3000 and let the IMU run at 3000SPS with decimation
> off (DEC_RATE=0). If we are
> > going to tweak the output rate to fit nicely, we would have to force it
> to 4000SPS which is
> > significantly different from the desired 3000SPS.
> 
> Good point. If someone wants to do that there isn't much we can do
> to make it work nicely.  I'd argue they are wrong and move it to
> the nearest within the constraints, but that might be rather
> unexpected
> for them...
> 
> I wonder if we do something ugly like have an extra control to
> say 'really do what I say even if it's horrible'.
> That would mean we'd get what 'most' users expect (I hope) in that
> it's within the documented range, but provide the control to those
> who want to do something horrible..  Not nice, what do you think?

You mean like a driver parameter? Not sure a runtime control would
make much sense (either you want this or not). Yeah, it is definitely 
not pretty but I seems to be the only way to make everyone 'happy'.

> >
> > > A bit of quick investigation suggests (by my wife who happened
> > > to be sat across the room) suggests that you have a hideous set
> > > of local minima so your best bet is to brute force search
> > > (not that bad and we don't expect to change this a lot!)
> >
> > Hmm, not sure what do you have in mind here :)?
> 
> You have two controllable parameters both of which are integers.  As
> such
> there isn't a nice easy 'right' answer for what combination to use.
> For example, if we have a case where there is no right answer, you
> will have to search for every possible multiplier, find the best divisor
> and check how far that is above the desired frequency.  Of those
> find the smallest one (best option given constraints).

Hmm, I need to give it a thought and see what I can come up to v2. We
continue from there.

But this opens one question... Do we want to tweak the output rate to fit
nicely only for 'if (imu_rate) < 4000' or should we also do it in the other
cases? By other cases I mean 'if (lcm(output_rate, input_ckl) > max_imu_rate)'
and in the generic case where we don't have external clock and the IMU runs
with it's internal rate (pretty much all the time the value does not fit nicely)?
Note that DEC_RATE is an integer parameter and for things to fit nicely, the
rate given by the user should be a multiple of the IMU sampling rate. I'm bringing
this up because if we are going into this much trouble to tweak the user rate to
fit nicely and avoid 'possible' undersampling issues, we might do the same for the
other cases... Otherwise, to avoid the oversampling case we could just force the
IMU sample rate to the maximum multiple of the input clock lower than max_rate
and calculate DEC_RATE with 'DIV_ROUND_CLOSEST()' which is what we are
already doing for the generic case. Naturally with this, the user won't get exactly
what he might expect as DEC_RATE is an integer (but again, that is how things are now anyways)...
We would still need the extra control for those who really want to stick to lower IMU rates.

- Nuno Sá

> Jonathan
> 
> >
> > - Nuno Sá
> >
> > > >
> > > > That said, the patch for the adis16480 driver is a fix as this mode
> was
> > > > being wrongly handled. There should not be a "separation"
> between
> > > using
> > > > the sync_scale and the dec_rate registers. The way things were
> > > being done,
> > > > we could easily get into a situation where the part could be
> running
> > > with
> > > > an internal rate way lower than the optimal minimum.
> > > >
> > > > For the adis16475 drivers, things were not really wrong. They
> were
> > > just
> > > > not optimal as we were forcing users to specify the IMU scaled
> > > internal
> > > > rate once in the devicetree. Calculating things at runtime gives
> much
> > > > more flexibility to choose the output rate.
> > > >
> > > > Nuno Sá (4):
> > > >   iio: adis: add helpers for locking
> > > >   iio: adis16480: fix pps mode sampling frequency math
> > > >   iio: adis16475: improve sync scale mode handling
> > > >   dt-bindings: adis16475: remove property
> > > >
> > > >  .../bindings/iio/imu/adi,adis16475.yaml       |   9 --
> > > >  drivers/iio/imu/adis16475.c                   | 110 ++++++++++++----
> > > >  drivers/iio/imu/adis16480.c                   | 120 +++++++++++++-----
> > > >  include/linux/iio/imu/adis.h                  |  10 ++
> > > >  4 files changed, 178 insertions(+), 71 deletions(-)
> > > >
> >

Re: [PATCH 0/4] Fix/Improve sync clock mode handling

[Jonathan Cameron]

On Tue, 2 Feb 2021 09:45:45 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, January 31, 2021 12:36 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: devicetree@vger.kernel.org; linux-iio@vger.kernel.org; Rob
> > Herring <robh+dt@kernel.org>; Peter Meerwald-Stadler
> > <pmeerw@pmeerw.net>; Lars-Peter Clausen <lars@metafoo.de>;
> > Hennerich, Michael <Michael.Hennerich@analog.com>; Ardelean,
> > Alexandru <alexandru.Ardelean@analog.com>
> > Subject: Re: [PATCH 0/4] Fix/Improve sync clock mode handling
> > 
> > 
> > On Mon, 25 Jan 2021 09:16:19 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Sunday, January 24, 2021 3:21 PM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: devicetree@vger.kernel.org; linux-iio@vger.kernel.org; Rob
> > > > Herring <robh+dt@kernel.org>; Peter Meerwald-Stadler
> > > > <pmeerw@pmeerw.net>; Lars-Peter Clausen <lars@metafoo.de>;
> > > > Hennerich, Michael <Michael.Hennerich@analog.com>; Ardelean,
> > > > Alexandru <alexandru.Ardelean@analog.com>
> > > > Subject: Re: [PATCH 0/4] Fix/Improve sync clock mode handling
> > > >
> > > >
> > > > On Thu, 21 Jan 2021 12:49:50 +0100
> > > > Nuno Sá <nuno.sa@analog.com> wrote:
> > > >  
> > > > > The first patch in this series is just a simple helper to lock/unlock
> > > > > the device. Having these helpers make the code slightly neater  
> > > > (IMHO).  
> > > > >
> > > > > The following patches introduces changes in the sampling  
> > frequency  
> > > > > calculation when sync scale/pps modes are used. First, it's  
> > important  
> > > > > to understand the purpose of this mode and how it should be  
> > used.  
> > > > Let's  
> > > > > say our part has an internal rate of 4250 (e.g adis1649x family) and  
> > > > the  
> > > > > user wants an output rate of 200SPS. Obviously, we can't use this
> > > > > sampling rate and divide back down to get 200 SPS with  
> > decimation  
> > > > on.  
> > > > > Hence, we can use this mode to give an input clock of 1HZ, scale it  
> > to  
> > > > > something like 4200 or 4000 SPS and then use the decimation  
> > filter to  
> > > > get  
> > > > > the exact desired 200SPS. There are also some limits that should  
> > be  
> > > > > taken into account when scaling:
> > > > >
> > > > >  * For the devices in the adis16475 driver:
> > > > >      - Input sync frequency range is 1 to 128 Hz
> > > > >      - Native sample rate: 2 kSPS.  Optimal range: 1900-2100 sps
> > > > >
> > > > >  * For the adis1649x family (adis16480 driver):
> > > > >     - Input sync frequency range is 1 to 128 Hz
> > > > >     - Native sample rate: 4.25 kSPS.  Optimal range: 4000-4250 sps
> > > > >
> > > > > I'm not 100% convinced on how to handle the optimal minimum.  
> > For  
> > > > now,  
> > > > > I'm just throwing a warning saying we might get into trouble if we  
> > get  
> > > > a  
> > > > > value lower than that. I was also tempted to just return -EINVAL  
> > or  
> > > > > clamp the value. However, I know that there are ADI customers  
> > that  
> > > > > (for some reason) are using a sampling rate lower than the  
> > minimum  
> > > > > advised.  
> > > >
> > > > So the opening question I'd have here is how critical is the actual
> > > > userspace sampling rate to your users?   Often they don't mind
> > > > getting a little more data than they asked for (say 200.5Hz when  
> > asking  
> > > > for 200) and can always read back the attribute after writing it to
> > > > discover this has happened.  
> > >
> > > Well, honestly I'm not really sure here. I can just say (from the info I  
> > got internally) that some  
> > > users are really using these parts with a data rate lower than the  
> > advised. However, I'd say  
> > > that this would depend on the use case. For some critical cases, I  
> > would expect that users really  
> > > want to have an exact sample rate. Though I'd argue that in those  
> > cases, they should know what  
> > > they are doing and provide an output rate that fits nicely (multiple of  
> > both the input clock and IMU  
> > > internal sample rate). And as you said, they can always readback the  
> > value to check if they are  
> > > getting something that is not really what they expect...
> > >  
> > > > As such, once you've discovered that value doesn't have an exact
> > > > match, perhaps tweak the output data rate until it fits nicely?  
> > >
> > > I did thought about this. The reason why I didn't went for it in this  
> > first version is because of those  
> > > who seems to really want to run the part at lower rates. Let's assume  
> > we have an input clock of  
> > > 1HZ and someone writes an output rate of 3000SPS. The only way to  
> > accomplish this is to set  
> > > sync_scale at 3000 and let the IMU run at 3000SPS with decimation  
> > off (DEC_RATE=0). If we are  
> > > going to tweak the output rate to fit nicely, we would have to force it  
> > to 4000SPS which is  
> > > significantly different from the desired 3000SPS.  
> > 
> > Good point. If someone wants to do that there isn't much we can do
> > to make it work nicely.  I'd argue they are wrong and move it to
> > the nearest within the constraints, but that might be rather
> > unexpected
> > for them...
> > 
> > I wonder if we do something ugly like have an extra control to
> > say 'really do what I say even if it's horrible'.
> > That would mean we'd get what 'most' users expect (I hope) in that
> > it's within the documented range, but provide the control to those
> > who want to do something horrible..  Not nice, what do you think?  
> 
> You mean like a driver parameter? Not sure a runtime control would
> make much sense (either you want this or not). Yeah, it is definitely 
> not pretty but I seems to be the only way to make everyone 'happy'.

Agreed. Would make more sense as a driver parameter than anything else.

> 
> > >  
> > > > A bit of quick investigation suggests (by my wife who happened
> > > > to be sat across the room) suggests that you have a hideous set
> > > > of local minima so your best bet is to brute force search
> > > > (not that bad and we don't expect to change this a lot!)  
> > >
> > > Hmm, not sure what do you have in mind here :)?  
> > 
> > You have two controllable parameters both of which are integers.  As
> > such
> > there isn't a nice easy 'right' answer for what combination to use.
> > For example, if we have a case where there is no right answer, you
> > will have to search for every possible multiplier, find the best divisor
> > and check how far that is above the desired frequency.  Of those
> > find the smallest one (best option given constraints).  
> 
> Hmm, I need to give it a thought and see what I can come up to v2. We
> continue from there.
> 
> But this opens one question... Do we want to tweak the output rate to fit
> nicely only for 'if (imu_rate) < 4000' or should we also do it in the other
> cases? By other cases I mean 'if (lcm(output_rate, input_ckl) > max_imu_rate)'

For that case, there will still be a 'best' value I think that gets us close
to output rate (from above) as possible.

> and in the generic case where we don't have external clock and the IMU runs
> with it's internal rate (pretty much all the time the value does not fit nicely)?

If it's running with internal rate then we will always have to do a best
aproximation from above.  Often people care about 'at least this rate and not
"too much" more' where too much is very fuzzy as it's more a question of the
system being able to cope with the extra load than any absolute rule.

If we are already doing closest as you say below, then it's probably close enough
that no one has ever moaned about it and we should stick to that.

> Note that DEC_RATE is an integer parameter and for things to fit nicely, the
> rate given by the user should be a multiple of the IMU sampling rate. I'm bringing
> this up because if we are going into this much trouble to tweak the user rate to
> fit nicely and avoid 'possible' undersampling issues, we might do the same for the
> other cases... Otherwise, to avoid the oversampling case we could just force the
> IMU sample rate to the maximum multiple of the input clock lower than max_rate
> and calculate DEC_RATE with 'DIV_ROUND_CLOSEST()' which is what we are
> already doing for the generic case. Naturally with this, the user won't get exactly
> what he might expect as DEC_RATE is an integer (but again, that is how things are now anyways)...
> We would still need the extra control for those who really want to stick to lower IMU rates.

Agreed, it makes sense to do the same whether it's an external clock or internal
clock.

Jonathan


> 
> - Nuno Sá
> 
> > Jonathan
> >   
> > >
> > > - Nuno Sá
> > >  
> > > > >
> > > > > That said, the patch for the adis16480 driver is a fix as this mode  
> > was  
> > > > > being wrongly handled. There should not be a "separation"  
> > between  
> > > > using  
> > > > > the sync_scale and the dec_rate registers. The way things were  
> > > > being done,  
> > > > > we could easily get into a situation where the part could be  
> > running  
> > > > with  
> > > > > an internal rate way lower than the optimal minimum.
> > > > >
> > > > > For the adis16475 drivers, things were not really wrong. They  
> > were  
> > > > just  
> > > > > not optimal as we were forcing users to specify the IMU scaled  
> > > > internal  
> > > > > rate once in the devicetree. Calculating things at runtime gives  
> > much  
> > > > > more flexibility to choose the output rate.
> > > > >
> > > > > Nuno Sá (4):
> > > > >   iio: adis: add helpers for locking
> > > > >   iio: adis16480: fix pps mode sampling frequency math
> > > > >   iio: adis16475: improve sync scale mode handling
> > > > >   dt-bindings: adis16475: remove property
> > > > >
> > > > >  .../bindings/iio/imu/adi,adis16475.yaml       |   9 --
> > > > >  drivers/iio/imu/adis16475.c                   | 110 ++++++++++++----
> > > > >  drivers/iio/imu/adis16480.c                   | 120 +++++++++++++-----
> > > > >  include/linux/iio/imu/adis.h                  |  10 ++
> > > > >  4 files changed, 178 insertions(+), 71 deletions(-)
> > > > >  
> > >  
> 

Re: [PATCH 4/4] dt-bindings: adis16475: remove property

[Rob Herring]

On Thu, 21 Jan 2021 12:49:54 +0100, Nuno Sá wrote:
> `adi,scaled-output-hz` is no longer used by the driver.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  .../devicetree/bindings/iio/imu/adi,adis16475.yaml       | 9 ---------
>  1 file changed, 9 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>