[PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240

0 views
Skip to first unread message

Rodrigo Carvalho

unread,
Nov 23, 2019, 12:20:34 AM11/23/19
to alexandru...@analog.com, Lars-Peter Clausen, Michael Hennerich, Stefan Popa, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, linu...@vger.kernel.org, de...@driverdev.osuosl.org, linux-...@vger.kernel.org, devic...@vger.kernel.org, kerne...@googlegroups.com, Rodrigo Carvalho
This patch add device tree binding documentation for ADIS16240.

Signed-off-by: Rodrigo Ribeiro Carvalho <rodri...@gmail.com>
---
V4:
- Remove spi-cpha and spi-cpol in binding example, since this driver
supports only one timing mode.
.../bindings/iio/accel/adi,adis16240.yaml | 49 +++++++++++++++++++
1 file changed, 49 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml

diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
new file mode 100644
index 000000000000..8e902f7c49e6
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADIS16240 Programmable Impact Sensor and Recorder driver
+
+maintainers:
+ - Alexandru Ardelean <alexandru...@analog.com>
+
+description: |
+ ADIS16240 Programmable Impact Sensor and Recorder driver that supports
+ SPI interface.
+ https://www.analog.com/en/products/adis16240.html
+
+properties:
+ compatible:
+ enum:
+ - adi,adis16240
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ spi0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ /* Example for a SPI device node */
+ accelerometer@0 {
+ compatible = "adi,adis16240";
+ reg = <0>;
+ spi-max-frequency = <2500000>;
+ interrupt-parent = <&gpio0>;
+ interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+ };
+ };
--
2.24.0

Jonathan Cameron

unread,
Nov 23, 2019, 6:41:25 AM11/23/19
to Rodrigo Carvalho, alexandru...@analog.com, Lars-Peter Clausen, Michael Hennerich, Stefan Popa, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, linu...@vger.kernel.org, de...@driverdev.osuosl.org, linux-...@vger.kernel.org, devic...@vger.kernel.org, kerne...@googlegroups.com
On Sat, 23 Nov 2019 02:19:27 -0300
Rodrigo Carvalho <rodri...@gmail.com> wrote:

> This patch add device tree binding documentation for ADIS16240.
>
> Signed-off-by: Rodrigo Ribeiro Carvalho <rodri...@gmail.com>
No problem with this patch, but I definitely want to see an accompanying
one enforcing the SPI mode in the driver.

Right now the driver doesn't set it and so I'm fairly sure not putting
it in the binding will leave you with a non working device.

The right option if only one option is supported is for the driver
to call spi_setup with the relevant options.

Thanks,

Jonathan

Ardelean, Alexandru

unread,
Nov 25, 2019, 2:51:39 AM11/25/19
to ji...@kernel.org, rodri...@gmail.com, Popa, Stefan Serban, kerne...@googlegroups.com, de...@driverdev.osuosl.org, linu...@vger.kernel.org, devic...@vger.kernel.org, gre...@linuxfoundation.org, la...@metafoo.de, pme...@pmeerw.net, Hennerich, Michael, linux-...@vger.kernel.org, knaa...@gmx.de
On Sat, 2019-11-23 at 11:41 +0000, Jonathan Cameron wrote:
> On Sat, 23 Nov 2019 02:19:27 -0300
> Rodrigo Carvalho <rodri...@gmail.com> wrote:
>
> > This patch add device tree binding documentation for ADIS16240.
> >
> > Signed-off-by: Rodrigo Ribeiro Carvalho <rodri...@gmail.com>

My bad for the late timing on this.
I'm slightly more fresh on Mondays.
But I will get overloaded with work in a few hours, so I may not have time
ot respond.

> No problem with this patch, but I definitely want to see an accompanying
> one enforcing the SPI mode in the driver.
>

So, then the binding should probably also define spi-cpol & spi-cpha
as mandatory.
Maybe, the driver would do a check and print a warning.

I'm noticing that this device uses SPI mode 3, but this DT binding defaults
to SPI mode 0

> Right now the driver doesn't set it and so I'm fairly sure not putting
> it in the binding will leave you with a non working device.
>
> The right option if only one option is supported is for the driver
> to call spi_setup with the relevant options.
>

What if the board uses some level inverters [because of some weird reason]
and that messes up with the SPI mode?
It's not common, but it is possible.

Jonathan Cameron

unread,
Dec 1, 2019, 6:40:38 AM12/1/19
to Ardelean, Alexandru, rodri...@gmail.com, Popa, Stefan Serban, kerne...@googlegroups.com, de...@driverdev.osuosl.org, linu...@vger.kernel.org, devic...@vger.kernel.org, gre...@linuxfoundation.org, la...@metafoo.de, pme...@pmeerw.net, Hennerich, Michael, linux-...@vger.kernel.org, knaa...@gmx.de, Mark Brown

+CC Mark as we probably need a more general view point on
the question of whether SPI mode should be enforced by binding
or in the driver.
I have wondered the same and have a few boards that do this sort of thing.
My personal feeling is that such level convertors should have explicit
representation rather than being hidden in changes to
the devicetree. Perhaps via a single input single output mux that
would wrap up the actions of any inverters in the path.

As you say, the alternative is to just leave it to the devicetree bindings.

Jonathan

Mark Brown

unread,
Dec 3, 2019, 11:38:59 AM12/3/19
to Jonathan Cameron, Ardelean, Alexandru, rodri...@gmail.com, Popa, Stefan Serban, kerne...@googlegroups.com, de...@driverdev.osuosl.org, linu...@vger.kernel.org, devic...@vger.kernel.org, gre...@linuxfoundation.org, la...@metafoo.de, pme...@pmeerw.net, Hennerich, Michael, linux-...@vger.kernel.org, knaa...@gmx.de
On Sun, Dec 01, 2019 at 11:40:32AM +0000, Jonathan Cameron wrote:

> +CC Mark as we probably need a more general view point on
> the question of whether SPI mode should be enforced by binding
> or in the driver.

Not sure I see the question here, I think I was missing a bit of
the conversation? It's perfectly fine for a driver to specify a
mode, if the hardware always uses some unusual mode then there's
no sense in forcing every single DT to set the same mode. On the
other hand if there's some configuration for the driver that was
handling some board specific configuration that there's already
some generic SPI support for setting then it seems odd to have a
custom driver specific configuration mechanism.
signature.asc

Jonathan Cameron

unread,
Dec 3, 2019, 11:51:59 AM12/3/19
to Mark Brown, Jonathan Cameron, Ardelean, Alexandru, rodri...@gmail.com, Popa, Stefan Serban, kerne...@googlegroups.com, de...@driverdev.osuosl.org, linu...@vger.kernel.org, devic...@vger.kernel.org, gre...@linuxfoundation.org, la...@metafoo.de, pme...@pmeerw.net, Hennerich, Michael, linux-...@vger.kernel.org, knaa...@gmx.de
If the driver picks a mode because that's what it says on the datasheet
it prevents odd board configurations from working. The question
becomes whether it makes sense in general to assume those odd board
conditions don't exist until we actually have one, or to assume that
they might and push the burden on to all DT files.

Traditionally in IIO at least we've mostly taken the view the DT
should be right and complete and had bindings state what normal
parameters must be for it to work (assuming no inverters etc)

If we encode it in the driver, and we later meet such a board we
end up with a custom dance to query the DT parameters again and
only override if present.

We can't rely on the core SPI handling because I don't think
there is any means of specifying a default.

We can adopt the view that in general these weird boards with inverters
are weird and just handle them when they occur. Sounds like that is your
preference, at least for new parts.

For old ones we have no idea if there are boards out there using
them with inverters so easiest is probably to just carry on putting them
in the DT bindings.

Jonathan


Ardelean, Alexandru

unread,
Dec 4, 2019, 2:18:23 AM12/4/19
to Jonathan...@huawei.com, bro...@kernel.org, kerne...@googlegroups.com, de...@driverdev.osuosl.org, linu...@vger.kernel.org, devic...@vger.kernel.org, gre...@linuxfoundation.org, ji...@kernel.org, la...@metafoo.de, Hennerich, Michael, pme...@pmeerw.net, Popa, Stefan Serban, rodri...@gmail.com, linux-...@vger.kernel.org, knaa...@gmx.de
There might be a few other options, which would require some SPI OF change.

One example (for spi-cpha):
if (of_property_read_u32(nc, "spi-cpha", &tmp) == 0) {
spi->mode |= SPI_CPHA_OVERRIDE;
if (tmp)
spi->mode |= SPI_CPHA;
} else
if (of_property_read_bool(nc, "spi-cpha"))
spi->mode |= SPI_CPHA;

Or another option could be:
if (of_property_read_bool(nc, "spi-cpha-override")) {
spi->mode |= SPI_CPHA_OVERRIDE;
if
(of_property_read_bool(nc, "spi-cpha"))
spi->mode |= SPI_CPHA;


Naturally, this would require that spi_setup() checks SPI_CPHA_OVERRIDE and
doesn't set SPI_CPHA if SPI_CPHA_OVERRIDE is set.

Or maybe, a more complete solution would be an "spi-mode-conv" driver.
Similar to the fixed-factor-clock clk driver, which just does a computation
based on values from the DT.

To tell the truth, this would be a great idea, because we have something
like a passive 3-wire-to-4-wire HDL converter. This requires that the
driver be configured in 3-wire mode, the SPI controller in normal 4-wire.
That's because the SPI framework does a validation of the supported modes
(for the SPI controller) and invalidates what the device wants (which is
very reasonable).

An "spi-mode-conv" driver would also handle this 3-wire-4-wire dance, and
the level inversions, and other (similar) things.

Thoughts?
Alex

>
> Jonathan
>
>

Mark Brown

unread,
Dec 4, 2019, 6:12:39 AM12/4/19
to Jonathan Cameron, Jonathan Cameron, Ardelean, Alexandru, rodri...@gmail.com, Popa, Stefan Serban, kerne...@googlegroups.com, de...@driverdev.osuosl.org, linu...@vger.kernel.org, devic...@vger.kernel.org, gre...@linuxfoundation.org, la...@metafoo.de, pme...@pmeerw.net, Hennerich, Michael, linux-...@vger.kernel.org, knaa...@gmx.de
On Tue, Dec 03, 2019 at 04:51:54PM +0000, Jonathan Cameron wrote:

> If the driver picks a mode because that's what it says on the datasheet
> it prevents odd board configurations from working. The question
> becomes whether it makes sense in general to assume those odd board
> conditions don't exist until we actually have one, or to assume that
> they might and push the burden on to all DT files.

The cost should be for the weird boards, not everything. If you
just wire up a device with a normally connected SPI bus without
throwing random inverters or whatever into the system then you
shouldn't need to do anything special.
signature.asc

Mark Brown

unread,
Dec 4, 2019, 12:01:10 PM12/4/19
to Ardelean, Alexandru, Jonathan...@huawei.com, kerne...@googlegroups.com, de...@driverdev.osuosl.org, linu...@vger.kernel.org, devic...@vger.kernel.org, gre...@linuxfoundation.org, ji...@kernel.org, la...@metafoo.de, Hennerich, Michael, pme...@pmeerw.net, Popa, Stefan Serban, rodri...@gmail.com, linux-...@vger.kernel.org, knaa...@gmx.de
On Wed, Dec 04, 2019 at 07:18:15AM +0000, Ardelean, Alexandru wrote:

> One example (for spi-cpha):
> if (of_property_read_u32(nc, "spi-cpha", &tmp) == 0) {
> spi->mode |= SPI_CPHA_OVERRIDE;
> if (tmp)
> spi->mode |= SPI_CPHA;

We could also do this with a separate flag saying that the wire
format is forced from DT rather than having one per setting.

> Or maybe, a more complete solution would be an "spi-mode-conv" driver.
> Similar to the fixed-factor-clock clk driver, which just does a computation
> based on values from the DT.

> To tell the truth, this would be a great idea, because we have something
> like a passive 3-wire-to-4-wire HDL converter. This requires that the
> driver be configured in 3-wire mode, the SPI controller in normal 4-wire.
> That's because the SPI framework does a validation of the supported modes
> (for the SPI controller) and invalidates what the device wants (which is
> very reasonable).

This is harder to achieve here because we don't have drivers for
random bits of the wire format...
signature.asc

Ardelean, Alexandru

unread,
Dec 5, 2019, 3:20:01 AM12/5/19
to bro...@kernel.org, kerne...@googlegroups.com, knaa...@gmx.de, gre...@linuxfoundation.org, de...@driverdev.osuosl.org, Jonathan...@huawei.com, devic...@vger.kernel.org, ji...@kernel.org, la...@metafoo.de, Hennerich, Michael, linu...@vger.kernel.org, Popa, Stefan Serban, rodri...@gmail.com, pme...@pmeerw.net, linux-...@vger.kernel.org
On Wed, 2019-12-04 at 17:00 +0000, Mark Brown wrote:
> On Wed, Dec 04, 2019 at 07:18:15AM +0000, Ardelean, Alexandru wrote:
>
> > One example (for spi-cpha):
> > if (of_property_read_u32(nc, "spi-cpha", &tmp) == 0) {
> > spi->mode |= SPI_CPHA_OVERRIDE;
> > if (tmp)
> > spi->mode |= SPI_CPHA;
>
> We could also do this with a separate flag saying that the wire
> format is forced from DT rather than having one per setting.
>

I will admit that [since you seem to incline towards this approach], I am
also inclined to consider it over the spi-mode-conv driver.
And also since you're saying that the driver would be harder to achieve.

I'll see about an implementation for this flag and come back for a review
[on it].
[Until then] I also owe you some more "delay_usecs" cleanup in other
subsystems.

Thanks
Alex
Reply all
Reply to author
Forward
0 new messages