diff mbox series

[v2,2/4] dt-bindings: iio: adc: add AD4695 and similar ADCs

Message ID 20240617-iio-adc-ad4695-v2-2-63ef6583f25d@baylibre.com (mailing list archive)
State Changes Requested
Headers show
Series iio: adc: ad4695: new driver for AD4695 and similar ADCs | expand

Commit Message

David Lechner June 17, 2024, 7:53 p.m. UTC
Add device tree bindings for AD4695 and similar ADCs.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

v2 changes:
* Drop *-wlcsp compatible strings
* Don't use fallback compatible strings
* Reword supply descriptions
* Use standard channel properties instead of adi,pin-pairing
* Fix unnecessary | character
* Fix missing blank line
* Add header file with common mode channel macros
---
 .../devicetree/bindings/iio/adc/adi,ad4695.yaml    | 290 +++++++++++++++++++++
 MAINTAINERS                                        |  10 +
 include/dt-bindings/iio/adi,ad4695.h               |   9 +
 3 files changed, 309 insertions(+)

Comments

Rob Herring June 17, 2024, 9:30 p.m. UTC | #1
On Mon, 17 Jun 2024 14:53:13 -0500, David Lechner wrote:
> Add device tree bindings for AD4695 and similar ADCs.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> v2 changes:
> * Drop *-wlcsp compatible strings
> * Don't use fallback compatible strings
> * Reword supply descriptions
> * Use standard channel properties instead of adi,pin-pairing
> * Fix unnecessary | character
> * Fix missing blank line
> * Add header file with common mode channel macros
> ---
>  .../devicetree/bindings/iio/adc/adi,ad4695.yaml    | 290 +++++++++++++++++++++
>  MAINTAINERS                                        |  10 +
>  include/dt-bindings/iio/adi,ad4695.h               |   9 +
>  3 files changed, 309 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml: single-channel: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml: common-mode-channel: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240617-iio-adc-ad4695-v2-2-63ef6583f25d@baylibre.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
David Lechner June 17, 2024, 10:06 p.m. UTC | #2
On 6/17/24 4:30 PM, Rob Herring (Arm) wrote:
> 
> On Mon, 17 Jun 2024 14:53:13 -0500, David Lechner wrote:
>> Add device tree bindings for AD4695 and similar ADCs.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>
>> v2 changes:
>> * Drop *-wlcsp compatible strings
>> * Don't use fallback compatible strings
>> * Reword supply descriptions
>> * Use standard channel properties instead of adi,pin-pairing
>> * Fix unnecessary | character
>> * Fix missing blank line
>> * Add header file with common mode channel macros
>> ---
>>  .../devicetree/bindings/iio/adc/adi,ad4695.yaml    | 290 +++++++++++++++++++++
>>  MAINTAINERS                                        |  10 +
>>  include/dt-bindings/iio/adi,ad4695.h               |   9 +
>>  3 files changed, 309 insertions(+)
>>
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml: single-channel: missing type definition
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml: common-mode-channel: missing type definition
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240617-iio-adc-ad4695-v2-2-63ef6583f25d@baylibre.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 

I think the problem is that I don't have a well-known commit as the
base-commit in my cover letter (oversight on my part).

single-channel and common-mode-channel are recent additions to the
common iio/adc.yaml so the types are defined there.

make dt_binding_check did pass for me locally before sending the series.
Conor Dooley June 18, 2024, 6 p.m. UTC | #3
On Mon, Jun 17, 2024 at 05:06:29PM -0500, David Lechner wrote:
> On 6/17/24 4:30 PM, Rob Herring (Arm) wrote:
> > 
> > On Mon, 17 Jun 2024 14:53:13 -0500, David Lechner wrote:
> >> Add device tree bindings for AD4695 and similar ADCs.
> >>
> >> Signed-off-by: David Lechner <dlechner@baylibre.com>
> >> ---
> >>
> >> v2 changes:
> >> * Drop *-wlcsp compatible strings
> >> * Don't use fallback compatible strings
> >> * Reword supply descriptions
> >> * Use standard channel properties instead of adi,pin-pairing
> >> * Fix unnecessary | character
> >> * Fix missing blank line
> >> * Add header file with common mode channel macros
> >> ---
> >>  .../devicetree/bindings/iio/adc/adi,ad4695.yaml    | 290 +++++++++++++++++++++
> >>  MAINTAINERS                                        |  10 +
> >>  include/dt-bindings/iio/adi,ad4695.h               |   9 +
> >>  3 files changed, 309 insertions(+)
> >>
> > 
> > My bot found errors running 'make dt_binding_check' on your patch:
> > 
> > yamllint warnings/errors:
> > 
> > dtschema/dtc warnings/errors:
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml: single-channel: missing type definition
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml: common-mode-channel: missing type definition
> > 
> > doc reference errors (make refcheckdocs):
> > 
> > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240617-iio-adc-ad4695-v2-2-63ef6583f25d@baylibre.com
> > 
> > The base for the series is generally the latest rc1. A different dependency
> > should be noted in *this* patch.
> > 
> > If you already ran 'make dt_binding_check' and didn't see the above
> > error(s), then make sure 'yamllint' is installed and dt-schema is up to
> > date:
> > 
> > pip3 install dtschema --upgrade
> > 
> > Please check and re-submit after running the above command yourself. Note
> > that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> > your schema. However, it must be unset to test all examples with your schema.
> > 
> 
> I think the problem is that I don't have a well-known commit as the
> base-commit in my cover letter (oversight on my part).

I think for his bot it needs, as written above, to be "in *this* patch".
I'm not sure if that's to allow for manual review, or something the bot
does automagically however.

> single-channel and common-mode-channel are recent additions to the
> common iio/adc.yaml so the types are defined there.
> 
> make dt_binding_check did pass for me locally before sending the series.
David Lechner June 18, 2024, 7:29 p.m. UTC | #4
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@baylibre.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.
   

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

> +
> +      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.
Jonathan Cameron June 23, 2024, 4:39 p.m. UTC | #5
On Tue, 18 Jun 2024 14:29:10 -0500
David Lechner <dlechner@baylibre.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@baylibre.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?
> 

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

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?

> 
> > +
> > +      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
Rob Herring June 24, 2024, 2:35 p.m. UTC | #6
On Tue, Jun 18, 2024 at 07:00:03PM +0100, Conor Dooley wrote:
> On Mon, Jun 17, 2024 at 05:06:29PM -0500, David Lechner wrote:
> > On 6/17/24 4:30 PM, Rob Herring (Arm) wrote:
> > > 
> > > On Mon, 17 Jun 2024 14:53:13 -0500, David Lechner wrote:
> > >> Add device tree bindings for AD4695 and similar ADCs.
> > >>
> > >> Signed-off-by: David Lechner <dlechner@baylibre.com>
> > >> ---
> > >>
> > >> v2 changes:
> > >> * Drop *-wlcsp compatible strings
> > >> * Don't use fallback compatible strings
> > >> * Reword supply descriptions
> > >> * Use standard channel properties instead of adi,pin-pairing
> > >> * Fix unnecessary | character
> > >> * Fix missing blank line
> > >> * Add header file with common mode channel macros
> > >> ---
> > >>  .../devicetree/bindings/iio/adc/adi,ad4695.yaml    | 290 +++++++++++++++++++++
> > >>  MAINTAINERS                                        |  10 +
> > >>  include/dt-bindings/iio/adi,ad4695.h               |   9 +
> > >>  3 files changed, 309 insertions(+)
> > >>
> > > 
> > > My bot found errors running 'make dt_binding_check' on your patch:
> > > 
> > > yamllint warnings/errors:
> > > 
> > > dtschema/dtc warnings/errors:
> > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml: single-channel: missing type definition
> > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml: common-mode-channel: missing type definition
> > > 
> > > doc reference errors (make refcheckdocs):
> > > 
> > > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240617-iio-adc-ad4695-v2-2-63ef6583f25d@baylibre.com
> > > 
> > > The base for the series is generally the latest rc1. A different dependency
> > > should be noted in *this* patch.
> > > 
> > > If you already ran 'make dt_binding_check' and didn't see the above
> > > error(s), then make sure 'yamllint' is installed and dt-schema is up to
> > > date:
> > > 
> > > pip3 install dtschema --upgrade
> > > 
> > > Please check and re-submit after running the above command yourself. Note
> > > that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> > > your schema. However, it must be unset to test all examples with your schema.
> > > 
> > 
> > I think the problem is that I don't have a well-known commit as the
> > base-commit in my cover letter (oversight on my part).
> 
> I think for his bot it needs, as written above, to be "in *this* patch".
> I'm not sure if that's to allow for manual review, or something the bot
> does automagically however.

I used to do manual review before sending, but then I added 
co-maintainers that review patches before the bot emails got sent out. 
So now they are sent out as run without review and dependencies aren't 
dealt with at all.

But yes, put something in this patch to say failures are expected 
because of a dependency.

Rob
David Lechner June 24, 2024, 2:36 p.m. UTC | #7
On 6/23/24 11:39 AM, Jonathan Cameron wrote:
> On Tue, 18 Jun 2024 14:29:10 -0500
> David Lechner <dlechner@baylibre.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@baylibre.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.
Jonathan Cameron June 24, 2024, 5:52 p.m. UTC | #8
On Mon, 24 Jun 2024 09:36:43 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 6/23/24 11:39 AM, Jonathan Cameron wrote:
> > On Tue, 18 Jun 2024 14:29:10 -0500
> > David Lechner <dlechner@baylibre.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@baylibre.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
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml
new file mode 100644
index 000000000000..e5dafb1f6acf
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml
@@ -0,0 +1,290 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4695.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices Easy Drive Multiplexed SAR Analog to Digital Converters
+
+maintainers:
+  - Michael Hennerich <Michael.Hennerich@analog.com>
+  - Nuno Sá <nuno.sa@analog.com>
+
+description: |
+  A family of similar multi-channel analog to digital converters with SPI bus.
+
+  * https://www.analog.com/en/products/ad4695.html
+  * https://www.analog.com/en/products/ad4696.html
+  * https://www.analog.com/en/products/ad4697.html
+  * https://www.analog.com/en/products/ad4698.html
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4695
+      - adi,ad4696
+      - adi,ad4697
+      - adi,ad4698
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 80000000
+
+  spi-cpol: true
+  spi-cpha: true
+
+  spi-rx-bus-width:
+    minimum: 1
+    maximum: 4
+
+  avdd-supply:
+    description: Analog power supply.
+
+  vio-supply:
+    description: I/O pin power supply.
+
+  ldo-in-supply:
+    description: Internal LDO Input. Mutually exclusive with vdd-supply.
+
+  vdd-supply:
+    description: Core power supply. Mutually exclusive with ldo-in-supply.
+
+  ref-supply:
+    description:
+      External reference voltage. Mutually exclusive with refin-supply.
+
+  refin-supply:
+    description:
+      Internal reference buffer input. Mutually exclusive with ref-supply.
+
+  com-supply:
+    description: Common voltage supply for pseudo-differential analog inputs.
+
+  adi,no-ref-current-limit:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      When this flag is present, the REF Overvoltage Reduced Current protection
+      is disabled.
+
+  adi,no-ref-high-z:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Enable this flag if the ref-supply requires Reference Input High-Z Mode
+      to be disabled for proper operation.
+
+  cnv-gpios:
+    description: The Convert Input (CNV). If omitted, CNV is tied to SPI CS.
+    maxItems: 1
+
+  reset-gpios:
+    description: The Reset Input (RESET). Should be configured GPIO_ACTIVE_LOW.
+    maxItems: 1
+
+  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
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+    description: |
+      The first cell is the GPn number: 0 to 3.
+      The second cell takes standard GPIO flags.
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+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
+
+      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
+
+      adi,no-high-z:
+        $ref: /schemas/types.yaml#/definitions/flag
+        description:
+          Enable this flag if the input pin requires the Analog Input High-Z
+          Mode to be disabled for proper operation.
+
+    required:
+      - reg
+
+    oneOf:
+      - required:
+          - diff-channels
+      - required:
+          - single-channel
+
+    allOf:
+      # bipolar mode can't be used with REFGND
+      - if:
+          required:
+            - single-channel
+          not:
+            required:
+              - common-mode-channel
+        then:
+          properties:
+            bipolar: false
+      - if:
+          required:
+            - common-mode-channel
+          properties:
+            common-mode-channel:
+              const: 0
+        then:
+          properties:
+            bipolar: false
+
+required:
+  - compatible
+  - reg
+  - avdd-supply
+  - vio-supply
+
+allOf:
+  - oneOf:
+      - required:
+          - ldo-in-supply
+      - required:
+          - vdd-supply
+
+  - oneOf:
+      - required:
+          - ref-supply
+      - required:
+          - refin-supply
+
+  # the internal reference buffer always requires high-z mode
+  - if:
+      required:
+        - refin-supply
+    then:
+      properties:
+        adi,no-ref-high-z: false
+
+  # limit channels for 8-channel chips
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad4697
+              - adi,ad4698
+    then:
+      patternProperties:
+        "^channel@[0-7]$":
+          properties:
+            reg:
+              maximum: 7
+            diff-channels:
+              items:
+                - maximum: 6
+                - maximum: 7
+            single-channel:
+              maximum: 7
+        "^channel@[8-9a-f]$": false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/iio/adi,ad4695.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 = <&power_supply>;
+            ldo-in-supply = <&power_supply>;
+            vio-supply = <&io_supply>;
+            refin-supply = <&supply_5V>;
+            com-supply = <&supply_2V5>;
+            reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            /* Differential channel between IN0 and IN1. */
+            channel@0 {
+                reg = <0>;
+                diff-channels = <0>, <1>;
+                bipolar;
+            };
+
+            /* Single-ended channel between IN2 and REFGND. */
+            channel@1 {
+                reg = <1>;
+                single-channel = <2>;
+            };
+
+            /* Pseudo-differential channel between IN3 and COM. */
+            channel@2 {
+                reg = <2>;
+                single-channel = <3>;
+                common-mode-channel = <AD4695_COMMON_MODE_COM>;
+                bipolar;
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index c870bc6b8d63..19d4bc962c77 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1209,6 +1209,16 @@  F:	Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
 F:	drivers/iio/adc/ad4130.c
 
+ANALOG DEVICES INC AD4695 DRIVER
+M:	Michael Hennerich <michael.hennerich@analog.com>
+M:	Nuno Sá <nuno.sa@analog.com>
+R:	David Lechner <dlechner@baylibre.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad4695.yaml
+F:	include/dt-bindings/iio/adi,ad4695.h
+
 ANALOG DEVICES INC AD7091R DRIVER
 M:	Marcelo Schmitt <marcelo.schmitt@analog.com>
 L:	linux-iio@vger.kernel.org
diff --git a/include/dt-bindings/iio/adi,ad4695.h b/include/dt-bindings/iio/adi,ad4695.h
new file mode 100644
index 000000000000..87a1b94af62c
--- /dev/null
+++ b/include/dt-bindings/iio/adi,ad4695.h
@@ -0,0 +1,9 @@ 
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+
+#ifndef _DT_BINDINGS_ADI_AD4695_H
+#define _DT_BINDINGS_ADI_AD4695_H
+
+#define AD4695_COMMON_MODE_REFGND	0
+#define AD4695_COMMON_MODE_COM		1
+
+#endif /* _DT_BINDINGS_ADI_AD4695_H */