Date: Mon, 24 Jun 2024 09:36:43 -0500
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
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 v2 2/4] dt-bindings: iio: adc: add AD4695 and similar ADCs

On 6/23/24 11:39 AM, Jonathan Cameron wrote:
> On Tue, 18 Jun 2024 14:29:10 -0500
> David Lechner <dlechner@...libre.com> wrote:
> 
>> On 6/17/24 2:53 PM, David Lechner wrote:
>>> Add device tree bindings for AD4695 and similar ADCs.
>>>
>>> Signed-off-by: David Lechner <dlechner@...libre.com>
>>> ---
>>>   
>> ...
>>
>>> +
>>> +  interrupts:
>>> +    minItems: 1
>>> +    items:
>>> +      - description:
>>> +          Signal coming from the BSY_ALT_GP0 or GP3 pin that indicates a busy
>>> +          condition.
>>> +      - description:
>>> +          Signal coming from the BSY_ALT_GP0 or GP2 pin that indicates an alert
>>> +          condition.
>>> +
>>> +  interrupt-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: busy
>>> +      - const: alert
>>> +  
>>
>> Since the interrupt can come from two different pins, it seems like we would
>> need an extra property to specify this. Is there a standard way to do this?
>>
>> Otherwise I will add something like:
>>
>> adi,busy-on-gp3:
>>   $ref: /schemas/types.yaml#/definitions/flag
>>   description:
>>     When present, the busy interrupt is coming from the GP3 pin, otherwise
>>     the interrupt is coming from the BSY_ALT_GP0 pin.
>>    
>> adi,alert-on-gp2:
>>   $ref: /schemas/types.yaml#/definitions/flag
>>   description:
>>     When present, the alert interrupt is coming from the GP2 pin, otherwise
>>     the interrupt is coming from the BSY_ALT_GP0 pin.
> Cut and paste?  Or it ends up on the same pin as the bsy? In which case that's
> a single interrupt and it's up to software to decide how to use. I'll guess
> it comes on GP1?

This is not a typo. The BSY_ALT_GP0 is a multi-purpose pin. The actual function
of the pin isn't selected explicitly, but rather there is an order of priority
(Table 25 in the datasheet).

Also, there are two packages the chip can come in, LFCSP and WLCSP. The former
only has GP0 and not GP1/2/3.

My thinking was that if both interrupts are provided in the DT and neither
adi,busy-on-gp3 or adi,alert-on-gp2 is given, then the driver would use
a shared interrupt and only allow enabling one function alert or busy
at a time.

>>
> 
> More interrupt names.  We shouldn't restrict someone wiring all 4 if they want
> to - we'll just use 2 that we choose in the driver.
> 
> interrupt-names
>   minItems: 1
>   items:
>     - const: busy-gp0
>     - const: busy-gp1
>     - const: alert-gp2
>     - cosnt: alert-gp1

This would actually need to be:

interrupt-names
  minItems: 1
  items:
    - const: busy-gp0
    - const: busy-gp3
    - const: alert-gp0
    - cosnt: alert-gp2

Or would it need to be this since there are two possible signals on the
same pin rather than trying to mess with a shared interrupt?
(Note: if both alert and busy are enabled at the same time on GP0, we
will only get the alert signal and busy signal is masked).

interrupt-names
  minItems: 1
  items:
    - const: alert-busy-gp0
    - const: busy-gp3
    - cosnt: alert-gp2

> 
> T   
>>
>>> +
>>> +patternProperties:
>>> +  "^channel@[0-9a-f]$":
>>> +    type: object
>>> +    $ref: adc.yaml
>>> +    unevaluatedProperties: false
>>> +    description:
>>> +      Describes each individual channel. In addition the properties defined
>>> +      below, bipolar from adc.yaml is also supported.
>>> +
>>> +    properties:
>>> +      reg:
>>> +        maximum: 15
>>> +
>>> +      diff-channels:
>>> +        description:
>>> +          Describes inputs used for differential channels. The first value must
>>> +          be an even numbered input and the second value must be the next
>>> +          consecutive odd numbered input.
>>> +        items:
>>> +          - minimum: 0
>>> +            maximum: 14
>>> +            multipleOf: 2
>>> +          - minimum: 1
>>> +            maximum: 15
>>> +            not:
>>> +              multipleOf: 2  
>>
>> After some more testing, it turns out that I misunderstood the datasheet and
>> this isn't actually fully differential, but rather pseudo-differential.
>>
>> So when pairing with the next pin, it is similar to pairing with the COM pin
>> where the negative input pin is connected to a constant voltage source.
> 
> Ok. I'm curious, how does it actually differ from a differential channel?
> What was that test?  It doesn't cope with an actual differential pair and needs
> a stable value on the negative?

In my initial testing, since I was only doing a direct read, I was using
constant voltages. But when I started working on buffered reads, then I
saw noisy data when using a fully differential (antiphase) signal.

This chip uses a multiplexer for channel so when an odd number pin is used
as the positive input (paired with REFGND or COM), it goes through one
multiplexer, but when an odd number pin is used as the negative input
it goes through the other multiplexer - the same one as REFGND and COM.

And burred in the middle of a paragraph on page 34 of 110 of the datasheet
is the only place in the entire datasheet where it actually says this is
a pseudo differential chip.

> 
>>
>>> +
>>> +      single-channel:
>>> +        minimum: 0
>>> +        maximum: 15
>>> +
>>> +      common-mode-channel:
>>> +        description:
>>> +          Describes the common mode channel for single channels. 0 is REFGND
>>> +          and 1 is COM. Macros are available for these values in
>>> +          dt-bindings/iio/adi,ad4695.h.
>>> +        minimum: 0
>>> +        maximum: 1
>>> +        default: 0  
>>
>> So I'm thinking the right thing to do here go back to using reg and the INx
>> number and only have common-mode-channel (no diff-channels or single-channel).
>>
>> common-mode-channel will need to be changed to allow INx numbers in addition
>> to COM and REFGND.
>>
>> This means that [PATCH v2 1/4] "dt-bindings: iio: adc: add common-mode-channel
>> dependency" would be wrong since we would be using common-mode-channel without
>> single-channel.
>>
>> It also means we will need an optional in1-supply: true for all odd numbered
>> inputs.
> Ok. I'm not totally sure I see how this comes together but will wait for v3 rather
> than trying to figure it out now.
> 
> Jonathan
> 
> 

It should end up like other pseudo differential chips we have done recently.
I've got it worked out, so you'll be seeing it soon enough anyway.

Powered by blists - more mailing lists