| Message ID | 20240617-iio-adc-ad4695-v2-2-63ef6583f25d@baylibre.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad4695: new driver for AD4695 and similar ADCs | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
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.
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.
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.
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.
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
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
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.
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 --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 */
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(+)