| Message ID | 20200225124152.270914-6-nuno.sa@analog.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Support ADIS16475 and similar IMUs | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success |
On Tue, Feb 25, 2020 at 01:41:52PM +0100, Nuno Sá wrote: > Document the ADIS16475 device devicetree bindings. > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > .../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 According to the datasheet I looked at, the input is called 'sync'. It looks like you are mixing operating mode and clock connection. > + > + 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. > + type: boolean > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adis16475: adis16475-3@0 { > + compatible = "adi,adis16475-3"; > + reg = <0>; > + spi-cpha; > + spi-cpol; > + spi-max-frequency = <2000000>; > + interrupts = <4 IRQ_TYPE_EDGE_RISING>; > + interrupt-parent = <&gpio>; > + }; > + }; > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index f11262f1f3bb..f8ccc92ab378 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1015,6 +1015,7 @@ W: http://ez.analog.com/community/linux-device-drivers > S: Supported > F: drivers/iio/imu/adis16475.c > F: Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475 > +F: Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml > > ANALOG DEVICES INC ADM1177 DRIVER > M: Beniamin Bia <beniamin.bia@analog.com> > -- > 2.25.1 >
On Mon, 2020-03-02 at 16:22 -0600, Rob Herring wrote: > On Tue, Feb 25, 2020 at 01:41:52PM +0100, Nuno Sá wrote: > > Document the ADIS16475 device devicetree bindings. > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > .../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 > > According to the datasheet I looked at, the input is called 'sync'. > It > looks like you are mixing operating mode and clock connection. The sync pin is where the external clock should be connected (when available). I'm kinda of using the clock-name property as a way of selecting the mode the user wants to use as done in other devices ( https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt ). In the end, what we should have in the sync pin is an external clock with the exception of the `sync` mode. I guess this one could be called output-sync and, in this case, the sync pin is actually an output pin pulsing when the internal processor collects data. I'm ok in changing it if there's a better way of doing it... Do you have any suggestion? -Nuno Sá > > + > > + 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. > > + type: boolean > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + spi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + adis16475: adis16475-3@0 { > > + compatible = "adi,adis16475-3"; > > + reg = <0>; > > + spi-cpha; > > + spi-cpol; > > + spi-max-frequency = <2000000>; > > + interrupts = <4 IRQ_TYPE_EDGE_RISING>; > > + interrupt-parent = <&gpio>; > > + }; > > + }; > > +... > > diff --git a/MAINTAINERS b/MAINTAINERS > > index f11262f1f3bb..f8ccc92ab378 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -1015,6 +1015,7 @@ W: > > http://ez.analog.com/community/linux-device-drivers > > S: Supported > > F: drivers/iio/imu/adis16475.c > > F: Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475 > > +F: Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml > > > > ANALOG DEVICES INC ADM1177 DRIVER > > M: Beniamin Bia <beniamin.bia@analog.com> > > -- > > 2.25.1
On Tue, 2020-03-03 at 09:43 +0000, Sa, Nuno wrote: > [External] > > On Mon, 2020-03-02 at 16:22 -0600, Rob Herring wrote: > > On Tue, Feb 25, 2020 at 01:41:52PM +0100, Nuno Sá wrote: > > > Document the ADIS16475 device devicetree bindings. > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > --- > > > .../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 > > > > According to the datasheet I looked at, the input is called 'sync'. > > It > > looks like you are mixing operating mode and clock connection. > > The sync pin is where the external clock should be connected (when > available). I'm kinda of using the clock-name property as a way of > selecting the mode the user wants to use as done in other devices ( > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt > ). In the end, what we should have in the sync pin is an external > clock > with the exception of the `sync` mode. I guess this one could be > called > output-sync and, in this case, the sync pin is actually an output pin > pulsing when the internal processor collects data. > > I'm ok in changing it if there's a better way of doing it... Do you > have any suggestion? > > -Nuno Sá So, you mean having the clock-name only as "sync" (or maybe even removing it?) and having a dedicated property like clock-mode? -Nuno Sá > > > + > > > + 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. > > > + type: boolean > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > + spi { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + adis16475: adis16475-3@0 { > > > + compatible = "adi,adis16475-3"; > > > + reg = <0>; > > > + spi-cpha; > > > + spi-cpol; > > > + spi-max-frequency = <2000000>; > > > + interrupts = <4 IRQ_TYPE_EDGE_RISING>; > > > + interrupt-parent = <&gpio>; > > > + }; > > > + }; > > > +... > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index f11262f1f3bb..f8ccc92ab378 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -1015,6 +1015,7 @@ W: > > > http://ez.analog.com/community/linux-device-drivers > > > S: Supported > > > F: drivers/iio/imu/adis16475.c > > > F: Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475 > > > +F: Documentation/devicetree/bindings/iio/imu/adi,adis16475 > > > .yaml > > > > > > ANALOG DEVICES INC ADM1177 DRIVER > > > M: Beniamin Bia <beniamin.bia@analog.com> > > > -- > > > 2.25.1
On Tue, Mar 3, 2020 at 3:59 AM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > On Tue, 2020-03-03 at 09:43 +0000, Sa, Nuno wrote: > > [External] > > > > On Mon, 2020-03-02 at 16:22 -0600, Rob Herring wrote: > > > On Tue, Feb 25, 2020 at 01:41:52PM +0100, Nuno Sá wrote: > > > > Document the ADIS16475 device devicetree bindings. > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > --- > > > > .../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 > > > > > > According to the datasheet I looked at, the input is called 'sync'. > > > It > > > looks like you are mixing operating mode and clock connection. > > > > The sync pin is where the external clock should be connected (when > > available). I'm kinda of using the clock-name property as a way of > > selecting the mode the user wants to use as done in other devices ( > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt > > ). In the end, what we should have in the sync pin is an external > > clock > > with the exception of the `sync` mode. I guess this one could be > > called > > output-sync and, in this case, the sync pin is actually an output pin > > pulsing when the internal processor collects data. > > > > I'm ok in changing it if there's a better way of doing it... Do you > > have any suggestion? > > > > -Nuno Sá > > So, you mean having the clock-name only as "sync" (or maybe even > removing it?) and having a dedicated property like clock-mode? Yes. Though it needs a vendor prefix: adi,clock-mode. Or perhaps adi,sync-mode? Rob
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) > + type: boolean > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adis16475: adis16475-3@0 { > + compatible = "adi,adis16475-3"; > + reg = <0>; > + spi-cpha; > + spi-cpol; > + spi-max-frequency = <2000000>; > + interrupts = <4 IRQ_TYPE_EDGE_RISING>; > + interrupt-parent = <&gpio>; > + }; > + }; > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index f11262f1f3bb..f8ccc92ab378 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1015,6 +1015,7 @@ W: http://ez.analog.com/community/linux-device-drivers > S: Supported > F: drivers/iio/imu/adis16475.c > F: Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475 > +F: Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml > > ANALOG DEVICES INC ADM1177 DRIVER > M: Beniamin Bia <beniamin.bia@analog.com>
On Tue, 2020-03-03 at 10:34 -0600, Rob Herring wrote: > On Tue, Mar 3, 2020 at 3:59 AM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > On Tue, 2020-03-03 at 09:43 +0000, Sa, Nuno wrote: > > > [External] > > > > > > On Mon, 2020-03-02 at 16:22 -0600, Rob Herring wrote: > > > > On Tue, Feb 25, 2020 at 01:41:52PM +0100, Nuno Sá wrote: > > > > > Document the ADIS16475 device devicetree bindings. > > > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > --- > > > > > .../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.yam > > > > > l > > > > > b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yam > > > > > l > > > > > new file mode 100644 > > > > > index 000000000000..c0f2146e000c > > > > > --- /dev/null > > > > > +++ > > > > > b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yam > > > > > l > > > > > @@ -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 > > > > > > > > According to the datasheet I looked at, the input is called > > > > 'sync'. > > > > It > > > > looks like you are mixing operating mode and clock connection. > > > > > > The sync pin is where the external clock should be connected > > > (when > > > available). I'm kinda of using the clock-name property as a way > > > of > > > selecting the mode the user wants to use as done in other devices > > > ( > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt > > > ). In the end, what we should have in the sync pin is an external > > > clock > > > with the exception of the `sync` mode. I guess this one could be > > > called > > > output-sync and, in this case, the sync pin is actually an output > > > pin > > > pulsing when the internal processor collects data. > > > > > > I'm ok in changing it if there's a better way of doing it... Do > > > you > > > have any suggestion? > > > > > > -Nuno Sá > > > > So, you mean having the clock-name only as "sync" (or maybe even > > removing it?) and having a dedicated property like clock-mode? > > Yes. Though it needs a vendor prefix: adi,clock-mode. Or perhaps > adi,sync-mode? I'm tempted to go with adi,sync-mode to respect the datasheet terminology. Moreover, when using output-sync mode, the sync pin is an output pin and there's no external clock. Hence, in this mode, it actually does not make any sense in giving a clock property. This must also be fixed in the driver. - Nuno Sá > Rob
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? - Nuno Sá > > + type: boolean > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + spi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + adis16475: adis16475-3@0 { > > + compatible = "adi,adis16475-3"; > > + reg = <0>; > > + spi-cpha; > > + spi-cpol; > > + spi-max-frequency = <2000000>; > > + interrupts = <4 IRQ_TYPE_EDGE_RISING>; > > + interrupt-parent = <&gpio>; > > + }; > > + }; > > +... > > diff --git a/MAINTAINERS b/MAINTAINERS > > index f11262f1f3bb..f8ccc92ab378 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -1015,6 +1015,7 @@ W: > > http://ez.analog.com/community/linux-device-drivers > > S: Supported > > F: drivers/iio/imu/adis16475.c > > F: Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475 > > +F: Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml > > > > ANALOG DEVICES INC ADM1177 DRIVER > > M: Beniamin Bia <beniamin.bia@analog.com>
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
On Thu, 2020-03-05 at 11:34 +0100, Lars-Peter Clausen wrote: > On 3/4/20 7:00 PM, Sa, Nuno wrote: > > On Tue, 2020-03-03 at 21:10 +0000, Jonathan Cameron wrote: > > > > > > 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. I do agree with that. Just to add that I think we also need to take into account the FIR filter which can also be responsible for bit growth. I will cache these values and if one of them is != 0 than burst 32 will be used... > 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. Hmm. In terms of interface I think there is no difference. We always report 32bits channels (for accel and gyro). However, what we do right know is just to set the LSB to 0 if burst32 is not supported. So, we can be just ignoring the LSB bits if they are being used... - Nuno Sá > - Lars >
On 3/5/20 1:27 PM, Sa, Nuno wrote: > >> 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. > Hmm. In terms of interface I think there is no difference. We always > report 32bits channels (for accel and gyro). However, what we do right > know is just to set the LSB to 0 if burst32 is not supported. So, we > can be just ignoring the LSB bits if they are being used... What I meant was that somebody might still want to get the full 32-bit values in buffered mode, even if the device does not support burst32. In that case you can first do a 16-bit burst read to get the MSBs and then do manual reads of all the LSB registers and then put both into the buffer. - Lars
On Thu, 2020-03-05 at 13:43 +0100, Lars-Peter Clausen wrote: > On 3/5/20 1:27 PM, Sa, Nuno wrote: > > > 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. > > Hmm. In terms of interface I think there is no difference. We > > always > > report 32bits channels (for accel and gyro). However, what we do > > right > > know is just to set the LSB to 0 if burst32 is not supported. So, > > we > > can be just ignoring the LSB bits if they are being used... > > What I meant was that somebody might still want to get the full 32- > bit > values in buffered mode, even if the device does not support burst32. They are. Just that the LSB part is always set to 0 :). And that, in my opinion, is wrong. As you say, we should do the manual readings if there are any bits on the LSB registers... - Nuno Sá > In > that case you can first do a 16-bit burst read to get the MSBs and > then > do manual reads of all the LSB registers and then put both into the > buffer. > - Lars >
On Thu, 5 Mar 2020 13:04:27 +0000 "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > On Thu, 2020-03-05 at 13:43 +0100, Lars-Peter Clausen wrote: > > On 3/5/20 1:27 PM, Sa, Nuno wrote: > > > > 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. > > > Hmm. In terms of interface I think there is no difference. We > > > always > > > report 32bits channels (for accel and gyro). However, what we do > > > right > > > know is just to set the LSB to 0 if burst32 is not supported. So, > > > we > > > can be just ignoring the LSB bits if they are being used... > > > > What I meant was that somebody might still want to get the full 32- > > bit > > values in buffered mode, even if the device does not support burst32. > > They are. Just that the LSB part is always set to 0 :). And that, in my > opinion, is wrong. As you say, we should do the manual readings if > there are any bits on the LSB registers... > > - Nuno Sá > > In > > that case you can first do a 16-bit burst read to get the MSBs and > > then > > do manual reads of all the LSB registers and then put both into the > > buffer. > > - Lars > > > Thanks Lars and Nuno, I'd not grasped exactly what this was. Hmm. Not allowing for variable bit widths has bitten us a few times in the past. Howwever, it's a really nasty thing to try and add to the core now unfortunately. In some cases we've just padded the smaller bitwidth buffer but that is costly to actually do. You get fast reads from the hardware then loose at least some of the benefit respacing the data. Still it is definitely a policy decision so not DT. It's ugly but if we want to support it and can't do it at runtime, perhaps a module parameter is the best option? Thanks, Jonathan
On 07.03.20 12:33, Jonathan Cameron wrote: > On Thu, 5 Mar 2020 13:04:27 +0000 > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > >> On Thu, 2020-03-05 at 13:43 +0100, Lars-Peter Clausen wrote: >>> On 3/5/20 1:27 PM, Sa, Nuno wrote: >>>>> 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. >>>> Hmm. In terms of interface I think there is no difference. We >>>> always >>>> report 32bits channels (for accel and gyro). However, what we do >>>> right >>>> know is just to set the LSB to 0 if burst32 is not supported. So, >>>> we >>>> can be just ignoring the LSB bits if they are being used... >>> >>> What I meant was that somebody might still want to get the full 32- >>> bit >>> values in buffered mode, even if the device does not support burst32. >> >> They are. Just that the LSB part is always set to 0 :). And that, in my >> opinion, is wrong. As you say, we should do the manual readings if >> there are any bits on the LSB registers... >> >> - Nuno Sá >>> In >>> that case you can first do a 16-bit burst read to get the MSBs and >>> then >>> do manual reads of all the LSB registers and then put both into the >>> buffer. >>> - Lars >>> >> > Thanks Lars and Nuno, I'd not grasped exactly what this was. > > Hmm. Not allowing for variable bit widths has bitten us a few times in the > past. Howwever, it's a really nasty thing to try and add to the core now > unfortunately. > > In some cases we've just padded the smaller bitwidth buffer but that > is costly to actually do. You get fast reads from the hardware then loose > at least some of the benefit respacing the data. > > Still it is definitely a policy decision so not DT. It's ugly but if > we want to support it and can't do it at runtime, perhaps a module parameter > is the best option? > So, we can decide this at runtime. As Lars pointed out, the LSB bits are not used by default (decimation and FIR filters disabled). However, applications can change this by changing the sampling frequency (affects the decimation filter) and the low_pass_filter_3db_freq (affects the FIR filter). If one of these filters is used, then the LSB bits are meaningful and makes sense to use burst32. For parts that do not support burst32, we should manually read the data. I started to prepare the version 2 of this series and Im starting to have mixed feelings. For now, I can see 3 ways of handling this: 1) If we assume that changing from burst32 to burst mode can occur at any time, we need some special handling. We need to realloc the buffer used on the spi transfer and readjust the spi xfer length. I'm not a big fan of the realloc part... 2) Alternatively, we could introduce a `burst_max_len` in the library that could be used in devices with different burst modes with different sizes. Max len would just hold the maximum burst len (as the name implies) and would be used to allocate the buffer to use on the spi tranfer. On the spi xfer we would then use the real burst length. With this we would not need to care about reallocs... 3) More conservative, we would not allow changing burst modes if buffering is ongoing... Changing a filter setting that would lead to burst mode change when buffering would return -EPERM... Either way, I will probably just send the v2 patch with 1) and then everyone can have a better look on how it looks and we can discuss improvements/other mechanism in the v2 thread. - Nuno Sá > Thanks, > > Jonathan > >
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. + type: boolean + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + spi { + #address-cells = <1>; + #size-cells = <0>; + + adis16475: adis16475-3@0 { + compatible = "adi,adis16475-3"; + reg = <0>; + spi-cpha; + spi-cpol; + spi-max-frequency = <2000000>; + interrupts = <4 IRQ_TYPE_EDGE_RISING>; + interrupt-parent = <&gpio>; + }; + }; +... diff --git a/MAINTAINERS b/MAINTAINERS index f11262f1f3bb..f8ccc92ab378 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1015,6 +1015,7 @@ W: http://ez.analog.com/community/linux-device-drivers S: Supported F: drivers/iio/imu/adis16475.c F: Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475 +F: Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml ANALOG DEVICES INC ADM1177 DRIVER M: Beniamin Bia <beniamin.bia@analog.com>
Document the ADIS16475 device devicetree bindings. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- .../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