DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240
@ 2019-11-23  5:19 Rodrigo Carvalho
  2019-11-23 11:41 ` Jonathan Cameron
  0 siblings, 1 reply; 10+ messages in thread
From: Rodrigo Carvalho @ 2019-11-23  5:19 UTC (permalink / raw)
  To: alexandru.ardelean, Lars-Peter Clausen, Michael Hennerich,
	Stefan Popa, Jonathan Cameron, Hartmut Knaack,
	Peter Meerwald-Stadler, Greg Kroah-Hartman
  Cc: devel, devicetree, linux-iio, linux-kernel, Rodrigo Carvalho, kernel-usp

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

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>;
+        };
+    };
-- 
2.24.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240
  2019-11-23  5:19 [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240 Rodrigo Carvalho
@ 2019-11-23 11:41 ` Jonathan Cameron
  2019-11-25  7:51   ` Ardelean, Alexandru
  0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2019-11-23 11:41 UTC (permalink / raw)
  To: Rodrigo Carvalho
  Cc: devel, devicetree, Lars-Peter Clausen, Stefan Popa,
	Michael Hennerich, linux-iio, Greg Kroah-Hartman, linux-kernel,
	kernel-usp, Peter Meerwald-Stadler, Hartmut Knaack,
	alexandru.ardelean

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>;
> +        };
> +    };

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240
  2019-11-23 11:41 ` Jonathan Cameron
@ 2019-11-25  7:51   ` Ardelean, Alexandru
  2019-12-01 11:40     ` Jonathan Cameron
  0 siblings, 1 reply; 10+ messages in thread
From: Ardelean, Alexandru @ 2019-11-25  7:51 UTC (permalink / raw)
  To: jic23, rodrigorsdc
  Cc: devel, devicetree, lars, Hennerich,  Michael, linux-iio, gregkh,
	linux-kernel, kernel-usp, pmeerw, knaack.h, Popa, Stefan Serban

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>;
> > +        };
> > +    };
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240
  2019-11-25  7:51   ` Ardelean, Alexandru
@ 2019-12-01 11:40     ` Jonathan Cameron
  2019-12-03 16:38       ` Mark Brown
  0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2019-12-01 11:40 UTC (permalink / raw)
  To: Ardelean, Alexandru
  Cc: devel, devicetree, lars, Hennerich, Michael, linux-iio, gregkh,
	linux-kernel, rodrigorsdc, kernel-usp, Mark Brown, pmeerw,
	knaack.h, Popa, Stefan Serban


+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>;
> > > +        };
> > > +    };  

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240
  2019-12-01 11:40     ` Jonathan Cameron
@ 2019-12-03 16:38       ` Mark Brown
  2019-12-03 16:51         ` Jonathan Cameron
  0 siblings, 1 reply; 10+ messages in thread
From: Mark Brown @ 2019-12-03 16:38 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: devel, devicetree, lars, Hennerich, Michael, linux-iio, gregkh,
	linux-kernel, rodrigorsdc, kernel-usp, pmeerw, knaack.h,
	Ardelean, Alexandru, Popa, Stefan Serban

[-- Attachment #1.1: Type: text/plain, Size: 716 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240
  2019-12-03 16:38       ` Mark Brown
@ 2019-12-03 16:51         ` Jonathan Cameron
  2019-12-04  7:18           ` Ardelean, Alexandru
  2019-12-04 11:12           ` Mark Brown
  0 siblings, 2 replies; 10+ messages in thread
From: Jonathan Cameron @ 2019-12-03 16:51 UTC (permalink / raw)
  To: Mark Brown
  Cc: devel, devicetree, lars, Hennerich, Michael, linux-iio, gregkh,
	linux-kernel, rodrigorsdc, kernel-usp, pmeerw, knaack.h,
	Ardelean, Alexandru, Popa, Stefan Serban, Jonathan Cameron

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


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240
  2019-12-03 16:51         ` Jonathan Cameron
@ 2019-12-04  7:18           ` Ardelean, Alexandru
  2019-12-04 17:00             ` Mark Brown
  2019-12-04 11:12           ` Mark Brown
  1 sibling, 1 reply; 10+ messages in thread
From: Ardelean, Alexandru @ 2019-12-04  7:18 UTC (permalink / raw)
  To: Jonathan.Cameron, broonie
  Cc: devel, devicetree, lars, Hennerich, Michael, linux-iio, gregkh,
	linux-kernel, rodrigorsdc, kernel-usp, pmeerw, knaack.h, Popa,
	Stefan Serban, jic23

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
> 
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240
  2019-12-03 16:51         ` Jonathan Cameron
  2019-12-04  7:18           ` Ardelean, Alexandru
@ 2019-12-04 11:12           ` Mark Brown
  1 sibling, 0 replies; 10+ messages in thread
From: Mark Brown @ 2019-12-04 11:12 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: devel, devicetree, lars, Hennerich, Michael, linux-iio, gregkh,
	linux-kernel, rodrigorsdc, kernel-usp, pmeerw, knaack.h,
	Ardelean, Alexandru, Popa, Stefan Serban, Jonathan Cameron

[-- Attachment #1.1: Type: text/plain, Size: 634 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240
  2019-12-04  7:18           ` Ardelean, Alexandru
@ 2019-12-04 17:00             ` Mark Brown
  2019-12-05  8:19               ` Ardelean, Alexandru
  0 siblings, 1 reply; 10+ messages in thread
From: Mark Brown @ 2019-12-04 17:00 UTC (permalink / raw)
  To: Ardelean, Alexandru
  Cc: devel, devicetree, lars, Hennerich, Michael, linux-iio, gregkh,
	knaack.h, linux-kernel, rodrigorsdc, kernel-usp, pmeerw,
	Jonathan.Cameron, Popa, Stefan Serban, jic23

[-- Attachment #1.1: Type: text/plain, Size: 1091 bytes --]

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...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240
  2019-12-04 17:00             ` Mark Brown
@ 2019-12-05  8:19               ` Ardelean, Alexandru
  0 siblings, 0 replies; 10+ messages in thread
From: Ardelean, Alexandru @ 2019-12-05  8:19 UTC (permalink / raw)
  To: broonie
  Cc: devel, devicetree, lars, Hennerich, Michael, linux-iio, gregkh,
	Jonathan.Cameron, linux-kernel, rodrigorsdc, kernel-usp, pmeerw,
	knaack.h, Popa, Stefan Serban, jic23

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...
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-23  5:19 [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240 Rodrigo Carvalho
2019-11-23 11:41 ` Jonathan Cameron
2019-11-25  7:51   ` Ardelean, Alexandru
2019-12-01 11:40     ` Jonathan Cameron
2019-12-03 16:38       ` Mark Brown
2019-12-03 16:51         ` Jonathan Cameron
2019-12-04  7:18           ` Ardelean, Alexandru
2019-12-04 17:00             ` Mark Brown
2019-12-05  8:19               ` Ardelean, Alexandru
2019-12-04 11:12           ` Mark Brown

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git