Re: [PATCH 5/5] dt-bindings: iio: Add adis16475 documentation

Lars-Peter Clausen
2020-03-05T10:34:12Z

On 3/4/20 7:00 PM, Sa, Nuno wrote:
> On Tue, 2020-03-03 at 21:10 +0000, Jonathan Cameron wrote:
>> [External]
>>
>> On Tue, 25 Feb 2020 13:41:52 +0100
>> Nuno Sá <nuno.sa@analog.com> wrote:
>>
>>> Document the ADIS16475 device devicetree bindings.
>>>
>>> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
>> One thing inline on the burst mode stuff.
>>
>> Thanks,
>>
>> Jonathan
>>
>>> ---
>>> .../bindings/iio/imu/adi,adis16475.yaml | 130
>>> ++++++++++++++++++
>>> MAINTAINERS | 1 +
>>> 2 files changed, 131 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
>>> b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
>>> new file mode 100644
>>> index 000000000000..c0f2146e000c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
>>> @@ -0,0 +1,130 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/iio/imu/adi,adis16475.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Analog Devices ADIS16475 and similar IMUs
>>> +
>>> +maintainers:
>>> + - Nuno Sá <nuno.sa@analog.com>
>>> +
>>> +description: |
>>> + Analog Devices ADIS16475 and similar IMUs
>>> +
>>> https://www.analog.com/media/en/technical-documentation/data-sheets/ADIS16475.pdf
>>> +
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - adi,adis16475-1
>>> + - adi,adis16475-2
>>> + - adi,adis16475-3
>>> + - adi,adis16477-1
>>> + - adi,adis16477-2
>>> + - adi,adis16477-3
>>> + - adi,adis16470
>>> + - adi,adis16465-1
>>> + - adi,adis16465-2
>>> + - adi,adis16465-3
>>> + - adi,adis16467-1
>>> + - adi,adis16467-2
>>> + - adi,adis16467-3
>>> + - adi,adis16500
>>> + - adi,adis16505-1
>>> + - adi,adis16505-2
>>> + - adi,adis16505-3
>>> + - adi,adis16507-1
>>> + - adi,adis16507-2
>>> + - adi,adis16507-3
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + spi-cpha: true
>>> +
>>> + spi-cpol: true
>>> +
>>> + spi-max-frequency:
>>> + maximum: 2000000
>>> +
>>> + interrupts:
>>> + maxItems: 1
>>> +
>>> + clocks:
>>> + maxItems: 1
>>> +
>>> + clock-names:
>>> + oneOf:
>>> + - const: sync
>>> + - const: direct-sync
>>> + - const: pulse-sync
>>> + - const: scaled-sync
>>> +
>>> + reset-gpios:
>>> + description:
>>> + Must be the device tree identifier of the RESET pin. If
>>> specified,
>>> + it will be asserted during driver probe. As the line is
>>> active low,
>>> + it should be marked GPIO_ACTIVE_LOW.
>>> + maxItems: 1
>>> +
>>> + 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
>>> + - interrupts
>>> + - spi-cpha
>>> + - spi-cpol
>>> +
>>> +if:
>>> + properties:
>>> + compatible:
>>> + contains:
>>> + enum:
>>> + - adi,adis16500
>>> + - adi,adis16505-1
>>> + - adi,adis16505-2
>>> + - adi,adis16505-3
>>> + - adi,adis16507-1
>>> + - adi,adis16507-2
>>> + - adi,adis16507-3
>>> +
>>> +then:
>>> + properties:
>>> + clock-names:
>>> + oneOf:
>>> + - const: sync
>>> + - const: direct-sync
>>> + - const: scaled-sync
>>> +
>>> + adi,burst32-enable:
>>> + description:
>>> + Enable burst32 mode. In this mode, a burst reading
>>> contains calibrated
>>> + gyroscope and accelerometer data in 32-bit format.
>> Why is this in DT? Is it not a runtime decision
>> (ideally automatically selected)
> So, you mean just have this mode by default on parts that support it?

Maybe lets start with explaining what burst32 mode is, so everybody is
on the same page.

The way registers are usually accessed for this chip is that you first
write the address you want to read on the SPI bus and then read the
selected register. This can be quite slow though if you want to read
multiple registers and is too slow to be able to read all the data at
full data rate. So there is a special burst mode which allows to read
all the data registers in one go.

Now by default the data registers are 16-bit. But there is an internal
decimation filter and the extra bits produced by the decimation filter
go into additional data registers making the data 32-bit wide. The chip
allows to configure whether to read the only 16-bit MSBs or the full
32-bit register.

So the decision whether a user wants to use 32-bit or 16-bit depends on
whether the extra 16-bit are needed or if they are even available. E.g.
if the decimation filter is off there wont be any extra bits.

This means ideally it would be user configurable whether to use 16-bit
or 32-bit burst mode, since it is application specific. The problem is
we don't have an interface for changing the bit width of a buffer
channel. Adding such an interface would require quite a bit of effort
since the assumption currently is that the bit width does not chance.
E.g. libiio assumes this and would stop working if it did change.

Maybe as a compromise for now. Use 32-bit burst when there is actually
meaningful data is the LSBs, i.e. the decimation filter is used and
disable it otherwise. And then think about how to make it configurable
as a follow up action.

In my opinion there is should not be a difference in the userspace
interface for chips that do support 32-bit burst and those that don't.
For devices that don't support 32-bit burst it should be emulated by
reading the LSB bits registers manually.

- Lars