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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 18 Feb 2021 12:40:35 +0100
Nuno Sa <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.
> 
> 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.

Series applied.   We may want to revisit this sometime in the future
but for now, lets get things working reasonably well.

Jonathan

> 
> Changes in v2:
>  * Moved the lock helper patch to the end of the series. 
>  * Changed all the users of the lock to use the helper functions.
>  * Added a module parameter to allow users to run the IMUs at lower
>    rates than the advisable.
> 
> Nuno Sa (3):
>   iio: adis16480: fix pps mode sampling frequency math
>   iio: adis16475: improve sync scale mode handling
>   iio: adis: add helpers for locking
> 
> Nuno Sá (1):
>   dt-bindings: adis16475: remove property
> 
>  .../bindings/iio/imu/adi,adis16475.yaml       |   9 --
>  drivers/iio/imu/adis16400.c                   |  22 ++-
>  drivers/iio/imu/adis16475.c                   | 118 ++++++++++++----
>  drivers/iio/imu/adis16480.c                   | 133 +++++++++++++-----
>  include/linux/iio/imu/adis.h                  |  10 ++
>  5 files changed, 206 insertions(+), 86 deletions(-)
>