| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20241219140353.787ffccc@jic23-huawei>
Date: Thu, 19 Dec 2024 14:03:53 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <lars@...afoo.de>,
<Michael.Hennerich@...log.com>, <robh@...nel.org>, <krzk+dt@...nel.org>,
<conor+dt@...nel.org>, <ana-maria.cusco@...log.com>,
<marcelo.schmitt1@...il.com>
Subject: Re: [RFC PATCH 2/4] dt-bindings: iio: adc: Add AD4170
On Wed, 18 Dec 2024 11:37:42 -0300
Marcelo Schmitt <marcelo.schmitt@...log.com> wrote:
> Add device tree documentation for AD4170 sigma-delta ADCs.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> ---
> .../bindings/iio/adc/adi,ad4170.yaml | 473 ++++++++++++++++++
> 1 file changed, 473 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
> new file mode 100644
> index 000000000000..8c5defc614ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
> @@ -0,0 +1,473 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4170.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4170 Analog to Digital Converter
> +
> +maintainers:
> + - Marcelo Schmitt <marcelo.schmitt@...log.com>
> +
> +description: |
> + Analog Devices AD4170 Analog to Digital Converter.
> + Specifications can be found at:
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4170-4.pdf
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - adi,ad4170
> +
> + avss-supply:
> + description:
> + Referece voltage supply for AVDD. AVSS can be set below 0V to provide a
> + bipolar power supply to AD4170-4. Must be −2.625V at minimum, 0V maximum.
> + If not specified, this is assumed to be analog ground.
> +
> + avdd-supply:
> + description:
> + A supply of 4.75V to 5.25V relative to AVSS that powers the chip (AVDD).
> +
> + iovdd-supply:
> + description: 1.7V to 5.25V reference supply to the serial interface (IOVDD).
> +
> + refin1p-supply:
> + description: REFIN+ supply that can be used as reference for conversion.
> +
> + refin1n-supply:
> + description: REFIN- supply that can be used as reference for conversion.
> +
> + refin2p-supply:
> + description: REFIN2+ supply that can be used as reference for conversion.
> +
> + refin2n-supply:
> + description: REFIN2- supply that can be used as reference for conversion.
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> + description: |
> + Optional external clock source. Can include one clock source: external
> + clock or external crystal.
> +
> + clock-names:
> + enum:
> + - ext-clk
> + - xtal
> +
> + '#clock-cells':
> + const: 0
> +
> + adi,gpio0-power-down-switch:
> + type: boolean
> + description:
> + Describes whether GPIO0 is used as a switch to disconnect bridge circuits
> + from AVSS. Pin defaults to GPIO if this property is not present.
This is interesting because it's power control for external bridges.
Maybe should be described as a regulator (output)? Any bridge circuit will need to be
an IIO consumer anyway so we can work out the channel scaling so that would then
consume this regulator as it's power supply.
> +
> + adi,gpio1-power-down-switch:
> + type: boolean
> + description:
> + Describes whether GPIO1 is used as a switch to disconnect bridge circuits
> + from AVSS. Pin defaults to GPIO if this property is not present.
> +
> + adi,vbias-pins:
> + description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
> + $ref: /schemas/types.yaml#/definitions/uint32-array
This is described as being relevant if we have say a thermocouple wired up.
For that I'd expect it to both be channel specific and us to need a bunch of other
description.
> + minItems: 1
> + maxItems: 9
> + items:
> + minimum: 0
> + maximum: 8
> +
> + adi,dig-aux1:
If it's a data ready signal, then interrupt/ interrupt-names probably
appropriate.
Sync is messier as you need a binding to describe where that goes but we have
provided a specific property for this in the past. Try to match up with an existing
one. Trickier as there are multiple of these here.
The high impedance is what you do if not an interrupt and the sync out not set.
> + description:
> + Describes whether DIG_AUX1 pin will operate as data ready output,
> + synchronization output signal (SYNC_OUT), or if it will be disabled.
> + A value of 0 indicates DIG_AUX1 pin disabled. High impedance.
> + A value of 1 indicates DIG_AUX1 is configured as ADC data ready output.
> + A value of 1 indicates DIG_AUX1 is configured as SYNC_OUT output.
> + If this property is absent, DIG_AUX1 pin is disabled.
> + $ref: /schemas/types.yaml#/definitions/uint8
> + enum: [0, 1, 2]
> + default: 0
> +
> + adi,dig-aux2:
> + description:
> + Describes whether DIG_AUX2 pin will function as DAC LDAC input,
> + synchronization start input (START), or if it will be disabled.
> + A value of 0 indicates DIG_AUX2 pin is disabled. High impedance.
> + A value of 1 indicates DIG_AUX2 pin is configured as active-low LDAC input
> + for the DAC.
> + A value of 2 indicates DIG_AUX2 pin is configured as START input.
Ah. the other side of sync.
Also DAC toggle pin. If we are using it as a dac toggle, needs a gpio binding.
Start will need a specific property. High impedance is what we do if the others
not set.
> + If this property is absent, DIG_AUX2 pin is disabled.
> + $ref: /schemas/types.yaml#/definitions/uint8
> + enum: [0, 1, 2]
> + default: 0
> +
> + adi,sync-option:
> + description:
> + Describes how ADC conversions are going to be synchronized. A value of 1
> + indicates the SYNC_IN pin will function as a synchronization input that
> + allows the user to control the start of sampling by pulling SYNC_IN high.
> + Use option number 2 to set the alternate synchronization functionality
> + which allows per channel conversion start control when multiple channels
> + are enabled. Option number 0 disables synchronization.
> + A value of 0 indicates no synchronization. SYNC_IN pin disabled.
> + A value of 1 indicates standard synchronization functionality.
> + A value of 2 indicates alternate synchronization functionality.
This is a software choice. I can't see why it belongs in DT unless
the assumption is some external timing circuitry is driving this.
To actually control mode 2 timing would be interesting as doesn't correspond
to triggering etc in iio which is matching standard synchronization assuming
I got right idea from quick read of the datasheet section on this).
> + If this property is absent, no synchronization is performed.
Inconsistent with default.
> + $ref: /schemas/types.yaml#/definitions/uint8
> + enum: [0, 1, 2]
> + default: 1
> +
All the bridge related excitation control etc makes me wonder if we need
a way to describe the bridge rather than the modes of this device.
I can't immediately see how to do it though as I'm not sure this stuff
is that standard across devices.
> + adi,excitation-pin-0:
> + description: |
> + Specifies the pin to apply excitation current 0 (IOUT0). Besides the
> + analog pins 0 to 8, the excitation current can be applied to GPIO pins.
Add some flavour for this. What is it for? Basically make a clear argument
it is all about the external circuitry.
> + 17: Output excitation current IOUT0 to GPIO0.
> + 18: Output excitation current IOUT0 to GPIO1.
> + 19: Output excitation current IOUT0 to GPIO2.
> + 20: Output excitation current IOUT0 to GPIO3.
> + If this property is absent, IOUT0 is not routed to any pin.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
> + default: 0
> +
> + adi,excitation-pin-1:
> + description: |
> + Specifies the pin to apply excitation current 1 (IOUT1). Besides the
> + analog pins 0 to 8, the excitation current can be applied to GPIO pins.
> + 17: Output excitation current IOUT1 to GPIO0.
> + 18: Output excitation current IOUT1 to GPIO1.
> + 19: Output excitation current IOUT1 to GPIO2.
> + 20: Output excitation current IOUT1 to GPIO3.
> + If this property is absent, IOUT1 is not routed to any pin.
Default doesn't make much sense if that is intent. Just drop it.
Maybe see if there is a sensible path to sharing the text for these
given repitition. If they were always there I'd suggest
an array, but we need the 'not in use' value and magic
numbers for that are nasty.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
> + default: 0
> +
> + adi,excitation-pin-2:
> + description: |
> + Specifies the pin to apply excitation current 2 (IOUT2). Besides the
> + analog pins 0 to 8, the excitation current can be applied to GPIO pins.
> + 17: Output excitation current IOUT2 to GPIO0.
> + 18: Output excitation current IOUT2 to GPIO1.
> + 19: Output excitation current IOUT2 to GPIO2.
> + 20: Output excitation current IOUT2 to GPIO3.
> + If this property is absent, IOUT2 is not routed to any pin.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
> + default: 0
> +
> + adi,excitation-pin-3:
> + description: |
> + Specifies the pin to apply excitation current 3 (IOUT3). Besides the
> + analog pins 0 to 8, the excitation current can be applied to GPIO pins.
> + 17: Output excitation current IOUT3 to GPIO0.
> + 18: Output excitation current IOUT3 to GPIO1.
> + 19: Output excitation current IOUT3 to GPIO2.
> + 20: Output excitation current IOUT3 to GPIO3.
> + If this property is absent, IOUT3 is not routed to any pin.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
> + default: 0
> +
> + adi,excitation-current-0-microamp:
> + description: |
> + Excitation current in microamps to be applied to IOUT0 output pin
> + specified in adi,excitation-pin-0.
As above, see if you can combine entries and add a bit of flavour on the why.
> + enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
> + default: 0
> +
> + adi,excitation-current-1-microamp:
> + description: |
> + Excitation current in microamps to be applied to IOUT1 output pin
> + specified in adi,excitation-pin-1.
> + enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
> + default: 0
> +
> + adi,excitation-current-2-microamp:
> + description: |
> + Excitation current in microamps to be applied to IOUT2 output pin
> + specified in adi,excitation-pin-2.
> + enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
> + default: 0
> +
> + adi,excitation-current-3-microamp:
> + description: |
> + Excitation current in microamps to be applied to IOUT3 output pin
> + specified in adi,excitation-pin-3.
> + enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
> + default: 0
> +
> + adi,chop-iexc:
> + description: |
> + Specifies the chopping/swapping functionality for excitation currents.
> + 0: No Chopping of Excitation Currents.
> + 1: Chop/swap IOUT0 and IOUT1 (pair AB) excitation currents.
> + 2: Chop/swap IOUT2 and IOUT3 (pair CD) excitation currents.
> + 3: Chop/swap both pairs (pair AB and pair CD) of excitation currents.
> + If this property is absent, no chopping is performed.
> + There are macros for the above values in dt-bindings/iio/adi,ad4170.h.
> + $ref: /schemas/types.yaml#/definitions/uint8
> + enum: [0, 1, 2, 3]
> + default: 0
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> +patternProperties:
> + "^channel@([0-9]|1[0-5])$":
> + $ref: adc.yaml
> + type: object
> + unevaluatedProperties: false
> + description: |
> + Represents the external channels which are connected to the ADC.
> +
> + properties:
> + reg:
> + description: |
> + The channel number. The device can have up to 16 channels numbered
> + from 0 to 15.
> + items:
> + minimum: 0
> + maximum: 15
> +
> + diff-channels:
> + description: |
> + This property is used for defining the inputs of a differential
> + voltage channel. The first value is the positive input and the second
> + value is the negative input of the channel.
> +
> + Besides the analog input pins AIN0 to AIN8, there are special inputs
> + that can be selected with the following values:
> + 17: Temperature sensor input
I guess we can't stop this being set to silly value (so one side of temperature
against anything else).
> + 18: (AVDD-AVSS)/5
> + 19: (IOVDD-DGND)/5
> + 20: DAC output
> + 21: ALDO
> + 22: DLDO
> + 23: AVSS
> + 24: DGND
> + 25: REFIN+
> + 26: REFIN-
> + 27: REFIN2+
> + 28: REFIN2-
> + 29: REFOUT
Most of these are typically used for calibration. I'd be tempted to drop them
from the channel configuration. Or maybe keep them also provide a way to override
and set these up anyway as needed.
> +
> + There are macros for those values in dt-bindings/iio/adi,ad4170.h.
> +
> + items:
> + minimum: 0
> + maximum: 31
Use an enum. We don't want the reserved values.
> +
> + single-channel: true
> +
> + common-mode-channel: true
> +
> + bipolar: true
> +
> + adi,config-setup-number:
> + description: |
> + Specifies which of the 8 setups are used to configure the channel.
> + A setup comprises of: AFE, FILTER, FILTER_FS, MISC, OFFSET, and GAIN
> + registers. More than one channel can use the same configuration setup
> + number in which case they will share the settings of the above
> + mentioned registers.
We have always done this in the driver previously. It can be complex due to
need to match equivalent configs, but most of this is stuff we want
to control. It doesn't lead to simple code, but it does give a lot of flexibility
as things like filters and gains etc are not characteristics of the external wiring
(though good choices are related to it).
> + items:
> + minimum: 0
> + maximum: 7
> +
> + adi,chop-adc:
> + description: |
> + Specifies the chopping/swapping functionality for a channel setup.
> + Macros for adi,chop-adc values are available in
> + dt-bindings/iio/adi,ad4170.h. When enabled, the analog inputs are
> + continuously swapped and a conversion is generated for each time a
> + swap occurs. The analog input pins are connected in one direction,
> + sampled, swapped, sampled again, and then the conversion results are
> + averaged. The input swap minimizes system offset and offset drift.
> + This property also specifies whether AC excitation using 2 or 4 GPIOs
> + are going to be used.
> + 0: No channel chop.
> + 1: Chop/swap the channel inputs.
> + 2: AC Excitation using 4 GPIOs.
> + 3: AC Excitation using 2 GPIOs.
> + If this property is absent, no chopping is performed.
> + $ref: /schemas/types.yaml#/definitions/uint16
> + enum: [0, 1, 2, 3]
> + default: 0
> +
> + adi,burnout-current-nanoamp:
> + description: |
> + Current in nanoamps to be applied for this channel. Burnout currents
> + are only active when the channel is selected for conversion.
All about open wire detection. So a runtime configuration thing.
> + enum: [0, 100, 2000, 10000]
> + default: 0
> +
> + adi,buffered-negative:
> + description: Enable precharge buffer, full buffer, or skip reference
> + buffering of the negative voltage reference. Because the output
> + impedance of the source driving the voltage reference inputs may be
> + dynamic, RC combinations of those inputs can cause DC gain errors if
> + the reference inputs go unbuffered into the ADC. Enable reference
> + buffering if the provided reference source has dynamic high impedance
> + output.
Say why you would not enable it
> + enum: [0, 1, 2]
Value meanings?
> + default: 0
> +
> + adi,buffered-positive:
Probably put positive first. Feels more natural!
> + description: Enable precharge buffer, full buffer, or skip reference
> + buffering of the positive voltage reference. Because the output
> + impedance of the source driving the voltage reference inputs may be
> + dynamic, RC combinations of those inputs can cause DC gain errors if
> + the reference inputs go unbuffered into the ADC. Enable reference
> + buffering if the provided reference source has dynamic high impedance
> + output.
> + enum: [0, 1, 2]
> + default: 0
> +
> + adi,reference-select:
> + description: |
> + Select the reference source to use when converting on the specific
> + channel. Valid values are:
> + 0: Differential reference voltage REFIN+ - REFIN−.
> + 1: Differential reference voltage REFIN2+ - REFIN2−.
> + 2: Internal 2.5V referece (REFOUT) relative to AVSS.
> + 3: Analog supply voltage (AVDD) relative relative AVSS.
> + If this field is left empty, the internal reference is selected.
There is an argument that this might be something you want to change at runtime
but it's pretty unlikely so I'm fine with this in DT.
Hmm. I'm not a fan of trying to review these AFE + bridge supporting ADCs.
So fiddly and with so many weird options that only make sense if I dig out
my memory of analog circuits courses from years ago :(
Jonathan
> + $ref: /schemas/types.yaml#/definitions/uint8
> + enum: [0, 1, 2, 3]
> + default: 2
> +
> + required:
> + - reg
> + - adi,config-setup-number
> +
> + allOf:
> + - oneOf:
> + - required: [single-channel]
> + properties:
> + diff-channels: false
> + - required: [diff-channels]
> + properties:
> + single-channel: false
> + common-mode-channel: false
> +
> +required:
> + - compatible
> + - reg
> + - avdd-supply
> + - iovdd-supply
> +
> +allOf:
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/iio/adc/adi,ad4170.h>
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + adc@0 {
> + compatible = "adi,ad4170";
> + reg = <0>;
> + avdd-supply = <&avdd>;
> + iovdd-supply = <&iovdd>;
> + spi-max-frequency = <20000000>;
> + interrupt-parent = <&gpio_in>;
> + interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
> + adi,dig-aux1 = /bits/ 8 <1>;
> + adi,dig-aux2 = /bits/ 8 <0>;
> + adi,sync-option = /bits/ 8 <0>;
> + adi,excitation-pin-0 = <19>;
> + adi,excitation-current-0-microamp = <10>;
> + adi,excitation-pin-1 = <20>;
> + adi,excitation-current-1-microamp = <10>;
> + adi,chop-iexc = /bits/ 8 <1>;
> + adi,vbias-pins = <5 6>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + // Sample AIN0 with respect to AIN1 throughout AVDD/AVSS input range
> + // Fully differential. If AVSS < 0V, Fully differential true bipolar
> + channel@0 {
> + reg = <0>;
> + bipolar;
> + diff-channels = <AD4170_MAP_AIN0 AD4170_MAP_AIN1>;
> + adi,config-setup-number = <0>;
> + adi,reference-select = /bits/ 8 <3>;
> + adi,burnout-current-nanoamp = <100>;
> + };
> + // Sample AIN2 with respect to DGND throughout AVDD/DGND input range
> + // Peseudo-differential unipolar (fig. 2a)
> + channel@1 {
> + reg = <1>;
> + single-channel = <AD4170_MAP_AIN2>;
> + common-mode-channel = <AD4170_MAP_DGND>;
> + adi,config-setup-number = <1>;
> + adi,reference-select = /bits/ 8 <3>;
> + };
> + // Sample AIN3 with respect to 2.5V throughout AVDD/AVSS input range
> + // Pseudo-differential bipolar (fig. 2b)
> + channel@2 {
> + reg = <2>;
> + bipolar;
> + single-channel = <AD4170_MAP_AIN3>;
> + common-mode-channel = <AD4170_MAP_REFOUT>;
> + adi,config-setup-number = <2>;
> + adi,reference-select = /bits/ 8 <3>;
> + };
> + // Sample AIN4 with respect to DGND throughout AVDD/AVSS input range
> + // Pseudo-differential true bipolar if AVSS < 0V (fig. 2c)
> + channel@3 {
> + reg = <3>;
> + bipolar;
> + single-channel = <AD4170_MAP_AIN4>;
> + common-mode-channel = <AD4170_MAP_DGND>;
> + adi,config-setup-number = <3>;
> + adi,reference-select = /bits/ 8 <3>;
> + };
> + // Sample AIN5 with respect to 2.5V throughout AVDD/REFOUT input range
> + // Pseudo-differential unipolar (AD4170 datasheet page 46 example)
> + channel@4 {
> + reg = <4>;
> + single-channel = <AD4170_MAP_AIN5>;
> + common-mode-channel = <AD4170_MAP_REFOUT>;
> + adi,config-setup-number = <4>;
> + adi,reference-select = /bits/ 8 <2>;
> + };
> + // Sample AIN6 with respect to REFIN+ throughout AVDD/AVSS input range
> + // Pseudo-differential unipolar
> + channel@5 {
> + reg = <5>;
> + single-channel = <AD4170_MAP_AIN6>;
> + common-mode-channel = <AD4170_MAP_REFIN1_P>;
> + adi,config-setup-number = <4>;
> + adi,reference-select = /bits/ 8 <2>;
> + };
> + // Sample AIN7 with respect to DGND throughout REFIN+/REFIN- input range
> + // Pseudo-differential bipolar
> + channel@6 {
> + reg = <6>;
> + bipolar;
> + diff-channels = <AD4170_MAP_AIN7 AD4170_MAP_DGND>;
> + adi,config-setup-number = <5>;
> + adi,reference-select = /bits/ 8 <0>;
> + };
> + };
> + };
> +...
> +
Powered by blists - more mailing lists