-----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 [off-list ref]; Peter Meerwald-Stadler
[off-list ref]; Lars-Peter Clausen [off-list ref];
Hennerich, Michael [off-list ref]; Ardelean,
Alexandru [off-list ref]
Subject: Re: [PATCH 0/4] Fix/Improve sync clock mode handling


On Thu, 21 Jan 2021 12:49:50 +0100
Nuno Sá [off-list ref] wrote:

quoted

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).

quoted

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

quoted

say our part has an internal rate of 4250 (e.g adis1649x family) and

the

quoted

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.

quoted

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

quoted

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,

quoted

I'm just throwing a warning saying we might get into trouble if we get

a

quoted

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á

quoted

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

quoted

the sync_scale and the dec_rate registers. The way things were

being done,

quoted

we could easily get into a situation where the part could be running

with

quoted

an internal rate way lower than the optimal minimum.

For the adis16475 drivers, things were not really wrong. They were

just

quoted

not optimal as we were forcing users to specify the IMU scaled

internal

quoted

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(-)