Message-ID: <20240624185220.000044d8@Huawei.com>
Date: Mon, 24 Jun 2024 18:52:20 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: David Lechner <dlechner@...libre.com>
CC: Jonathan Cameron <jic23@...nel.org>, 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 Mon, 24 Jun 2024 09:36:43 -0500
David Lechner <dlechner@...libre.com> wrote:

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

Avoid the custom parameters They are not needed as software gets to decide
how these pins are used (if I read you description correctly)

Do it as 3 interrupts of which an combination could be supplied and the
driver gets to figure out what to use them for.

We are describing the wiring here for the interrupts, not what the driver
is doing with them.   So if only GP0 is provided then shared interrupt
it is with either alert or busy.


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

Don't describe the mode, just the pin in this case.
There are other cases of this but normally they are called int1, int2
or something like that rather than gpX.  They can have weird and complex
constraints on what is possible to signal but that's a driver problem
not a dt one.

interrupt-names:
  minItems: 1
  items:
    - const: gp0
    - const: gp3
    - const: gp2

I'm not sure I understand if there are other constraints, but if there
are I think they are on what can be used at the same time, not what can be wired.

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

Fair enough.  Sounds like the right test to me.  I guess
that second mux has a slow tracking buffer or similar.
Good work tracking this down.

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

Jonathan

Powered by blists - more mailing lists