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

[Jonathan Cameron <jic23@kernel.org>]

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