| Message ID | 20191123051927.5016-1-rodrigorsdc@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] dt-bindings: iio: accel: add binding documentation for ADIS16240 | expand |
On Sat, 23 Nov 2019 02:19:27 -0300 Rodrigo Carvalho <rodrigorsdc@gmail.com> wrote: > This patch add device tree binding documentation for ADIS16240. > > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@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 > --- > 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.ardelean@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>; > + }; > + };
On Sat, 2019-11-23 at 11:41 +0000, Jonathan Cameron wrote: > On Sat, 23 Nov 2019 02:19:27 -0300 > Rodrigo Carvalho <rodrigorsdc@gmail.com> wrote: > > > This patch add device tree binding documentation for ADIS16240. > > > > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@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. > Thanks, > > Jonathan > > > --- > > 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.ardelean@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>; > > + }; > > + };
+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. On Mon, 25 Nov 2019 07:51:30 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Sat, 2019-11-23 at 11:41 +0000, Jonathan Cameron wrote: > > On Sat, 23 Nov 2019 02:19:27 -0300 > > Rodrigo Carvalho <rodrigorsdc@gmail.com> wrote: > > > > > This patch add device tree binding documentation for ADIS16240. > > > > > > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@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. 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 > > > Thanks, > > > > Jonathan > > > > > --- > > > 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.ardelean@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>; > > > + }; > > > + };
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.
On Tue, 3 Dec 2019 16:38:50 +0000 Mark Brown <broonie@kernel.org> wrote: > 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. > 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
On Tue, 2019-12-03 at 16:51 +0000, Jonathan Cameron wrote: > On Tue, 3 Dec 2019 16:38:50 +0000 > Mark Brown <broonie@kernel.org> wrote: > > > 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. > > > > 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. 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 > >
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.
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...
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 > > 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...
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.ardelean@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>; + }; + };
This patch add device tree binding documentation for ADIS16240. Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@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