[v2] Add iio backend compatibility for ad7606

[PATCH v2 01/10] dt-bindings: iio: adc: ad7606: Set the correct polarity

Posted by Guillaume Stols 2 months, 1 week ago

According to the datasheet, "Data is clocked in from SDI on the falling
edge of SCLK, while data is clocked out on DOUTA on the rising edge of
SCLK".
Also, even if not stated textually in the datasheet, it is made clear on
the diagrams that sclk idles at high.

So the documentation is erroneously stating that spi-cpha is required, and
the example is erroneously setting both spi-cpol and spi-cpha.

Fixes: 416f882c3b40 ("dt-bindings: iio: adc: Migrate AD7606 documentation to yaml")
Fixes: 6e33a125df66 ("dt-bindings: iio: adc: Add docs for AD7606 ADC")

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
index 69408cae3db9..75334a033539 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
@@ -29,8 +29,6 @@ properties:
   reg:
     maxItems: 1
 
-  spi-cpha: true
-
   spi-cpol: true
 
   avcc-supply: true
@@ -117,7 +115,7 @@ properties:
 required:
   - compatible
   - reg
-  - spi-cpha
+  - spi-cpol
   - avcc-supply
   - vdrive-supply
   - interrupts
@@ -185,7 +183,6 @@ examples:
             reg = <0>;
             spi-max-frequency = <1000000>;
             spi-cpol;
-            spi-cpha;
 
             avcc-supply = <&adc_vref>;
             vdrive-supply = <&vdd_supply>;

-- 
2.34.1

Re: [PATCH v2 01/10] dt-bindings: iio: adc: ad7606: Set the correct polarity

Posted by Uwe Kleine-König 2 months, 1 week ago

Hello Guillaume,

On Fri, Sep 20, 2024 at 05:33:21PM +0000, Guillaume Stols wrote:
> According to the datasheet, "Data is clocked in from SDI on the falling
> edge of SCLK, while data is clocked out on DOUTA on the rising edge of
> SCLK".
> Also, even if not stated textually in the datasheet, it is made clear on
> the diagrams that sclk idles at high.
> 
> So the documentation is erroneously stating that spi-cpha is required, and
> the example is erroneously setting both spi-cpol and spi-cpha.

I would expect that the communication with the chip is at least
unreliable if not outright broken with the wrong polarity. So maybe add
something like:

	On $MyMachine dropping the spi-cpha property reduces IO errors / fixes
	measurement readout / improves somehow differently.

to the commit log?

> Fixes: 416f882c3b40 ("dt-bindings: iio: adc: Migrate AD7606 documentation to yaml")
> Fixes: 6e33a125df66 ("dt-bindings: iio: adc: Add docs for AD7606 ADC")
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>

The empty line between Fixes and S-o-b is unusual. Assuming you resend
anyway, please drop it.

> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> index 69408cae3db9..75334a033539 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> @@ -29,8 +29,6 @@ properties:
>    reg:
>      maxItems: 1
>  
> -  spi-cpha: true
> -
>    spi-cpol: true
>  
>    avcc-supply: true
> @@ -117,7 +115,7 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - spi-cpha
> +  - spi-cpol

Adding cpol seems unrelated to this patch. (And you remove it again in
patch #2.)

>    - avcc-supply
>    - vdrive-supply
>    - interrupts

Best regards
Uwe

Re: [PATCH v2 01/10] dt-bindings: iio: adc: ad7606: Set the correct polarity

Posted by Jonathan Cameron 2 months ago

On Sat, 21 Sep 2024 11:11:31 +0200
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello Guillaume,
> 
> On Fri, Sep 20, 2024 at 05:33:21PM +0000, Guillaume Stols wrote:
> > According to the datasheet, "Data is clocked in from SDI on the falling
> > edge of SCLK, while data is clocked out on DOUTA on the rising edge of
> > SCLK".
> > Also, even if not stated textually in the datasheet, it is made clear on
> > the diagrams that sclk idles at high.
> > 
> > So the documentation is erroneously stating that spi-cpha is required, and
> > the example is erroneously setting both spi-cpol and spi-cpha.  
> 
> I would expect that the communication with the chip is at least
> unreliable if not outright broken with the wrong polarity. So maybe add
> something like:
> 
> 	On $MyMachine dropping the spi-cpha property reduces IO errors / fixes
> 	measurement readout / improves somehow differently.
> 
> to the commit log?
> 
> > Fixes: 416f882c3b40 ("dt-bindings: iio: adc: Migrate AD7606 documentation to yaml")
> > Fixes: 6e33a125df66 ("dt-bindings: iio: adc: Add docs for AD7606 ADC")
> > 
> > Signed-off-by: Guillaume Stols <gstols@baylibre.com>  
> 
> The empty line between Fixes and S-o-b is unusual. Assuming you resend
> anyway, please drop it.

It's also scanned for in linux-next so to reduce chances I forget to fix this
definitely resend.

> 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > index 69408cae3db9..75334a033539 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > @@ -29,8 +29,6 @@ properties:
> >    reg:
> >      maxItems: 1
> >  
> > -  spi-cpha: true
> > -
> >    spi-cpol: true
> >  
> >    avcc-supply: true
> > @@ -117,7 +115,7 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > -  - spi-cpha
> > +  - spi-cpol  
> 
> Adding cpol seems unrelated to this patch. (And you remove it again in
> patch #2.)
> 
> >    - avcc-supply
> >    - vdrive-supply
> >    - interrupts  
> 
> Best regards
> Uwe