linux-kernel - Re: [PATCH 1/3] dt-bindings: iio: adc: add AD4695 and similar ADCs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240615185516.61346425@jic23-huawei>
Date: Sat, 15 Jun 2024 18:55:16 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Michael Hennerich
 <michael.hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>,
 Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH 1/3] dt-bindings: iio: adc: add AD4695 and similar ADCs


> >> +
> >> +      adi,pin-pairing:
> >> +        description: |
> >> +          The input pin pairing for the negative input. This can be:
> >> +          - REFGND, normally 0V (single-ended)
> >> +          - COM, normally V_REF/2, see com-supply (pseudo-differential)
> >> +          - For even numbered pins, the next odd numbered pin (differential)
> >> +        $ref: /schemas/types.yaml#/definitions/string
> >> +        enum: [refgnd, com, next]  
> > 
> > Next is full on differential, just provide both channels via
> > diff-channels. You can constrain the particular combinations in the binding.
> > 
> > Refcnd is normal single ended.  Probably want to use the new single-channel
> > property for that as we are mixing differential and single ended channels
> > so reg is pretty much just an index.
> > 
> > Hmm. For comm we haven't had done a recent binding for a chip with the option
> > of pseudo differential that is per channel, they've been whole device only.
> > That feels like it will be common enough we need to support it cleanly
> > with a 'general' scheme.  
> 
> I think we have. :-)
> 
> https://lore.kernel.org/linux-iio/adc6cba9-2e79-475f-9c24-039fe9d3345d@baylibre.com/T/#mcbc1ce3a2541db502bf7870b7ea8574626a46312
> 

My goldfish like memory strikes again. Had completely forgotten that :)

> > 
> > Problem is I know someone will have a chip with 2 vincom pins and selecting
> > between them, so we can't just have pseudo-differential as a boolean and adc.yaml
> > 
> > There are horrible solutions like a magic channel number that changes the
> > meaning of diff-channels but that's ugly.
> > Maybe pseudo-differential for now and we have to later we add
> > pseudo-differential-comm  = <0> etc?
> >   
> 
> I was trying to keep things simple with 1 property instead of 3, but we
> can drop adi,pin-pairing and use the standard diff-channels, single-channel
> and common-mode-channel properties.
Sounds good.

> 
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/gpio/gpio.h>
> >> +
> >> +    spi {
> >> +        #address-cells = <1>;
> >> +        #size-cells = <0>;
> >> +
> >> +        adc@0 {
> >> +            compatible = "adi,ad4695";
> >> +            reg = <0>;
> >> +            spi-cpol;
> >> +            spi-cpha;
> >> +            spi-max-frequency = <80000000>;
> >> +            avdd-supply = <&supply_2_5V>;
> >> +            vdd-supply = <&supply_1_8V>;
> >> +            vio-supply = <&supply_1_2V>;
> >> +            ref-supply = <&supply_5V>;
> >> +            reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>;
> >> +
> >> +            #address-cells = <1>;
> >> +            #size-cells = <0>;
> >> +  
> 
> Using the standard adc.yaml properties, these would now be:
> 
> >> +            /* Differential channel between IN0 and IN1. */
> >> +            channel@0 {
> >> +                reg = <0>;  
> 
>                     diff-channels = <0>, <1>;
> 
> >> +                bipolar;
> >> +            };
> >> +
> >> +            /* Single-ended channel between IN2 and REFGND. */
> >> +            channel@2 {
> >> +                reg = <2>;  
> 
>                     single-channel = <2>;
> 		    common-mode-channel = <0>;

I wonder if we count ground ad default and so don't necessarily
specify this? 

I don't really mind either way though.  Maybe just default to 0 works.


> 
> >> +            };
> >> +
> >> +            /* Pseudo-differential channel between IN3 and COM. */
> >> +            channel@f {
> >> +                reg = <3>;  
> 
>                     single-channel = <3>;
>                     common-mode-channel = <1>;
> 
> >> +                bipolar;
> >> +            };
> >> +        };
> >> +    };  
> >   
> 
> And I will add a header file with macros for the common mode
> channel values.

Great

Jonathan

>