| Message ID | 20240430162946.589423-6-alisa.roman@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7192: Add AD7194 support | expand |
On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote: > + diff-channels: > + description: > + Both inputs can be connected to pins AIN1 to AIN16 by choosing the > + appropriate value from 1 to 16. > + items: > + minimum: 1 > + maximum: 16 > + > + single-channel: > + description: > + Positive input can be connected to pins AIN1 to AIN16 by choosing the > + appropriate value from 1 to 16. Negative input is connected to AINCOM. > + items: > + minimum: 1 > + maximum: 16 Up to 16 differential channels and 16 single-ended channels, but only 16 pins? Would the number of differential channels not max out at 8?
On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote: > From: Alisa-Dariana Roman <alisadariana@gmail.com> > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> btw, the whole series has a from != signoff email problem. My git send-email handles that for me without me having to do anything by inserting a From: header in the patches, like so: https://lore.kernel.org/all/20240424-tabby-plural-5f1d9fe44f47@spud/ I am not sure what option you're missing to suggest, but it may be as setting the `--from` arg (or sendemail.from in your gitconfig). Thanks, Conor.
On Tue, 30 Apr 2024 18:21:01 +0100 Conor Dooley <conor@kernel.org> wrote: > On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote: > > + diff-channels: > > + description: > > + Both inputs can be connected to pins AIN1 to AIN16 by choosing the > > + appropriate value from 1 to 16. > > + items: > > + minimum: 1 > > + maximum: 16 > > + > > + single-channel: > > + description: > > + Positive input can be connected to pins AIN1 to AIN16 by choosing the > > + appropriate value from 1 to 16. Negative input is connected to AINCOM. > > + items: > > + minimum: 1 > > + maximum: 16 > > Up to 16 differential channels and 16 single-ended channels, but only 16 > pins? Would the number of differential channels not max out at 8? May not really be limited to 16 differential. Many chips use general purpose muxes on both sides so you can do all combinations. In practice that's normally pointless. A more useful case is to do all but one channel as positive inputs and the remaining channel as the negative for those 15 differential channels. This is effectively the same as doing pseudo differential channels, but on more flexible hardware. This is in contrast to a device that only supports pseudo differential where there is a special pin for the negative (this device has that as well as full muxes on the other 16 lines). Having said all that. The ad7194 datasheet says 8 differential channels.. I have no idea why though... Maybe something to do with the mux switching? Or maybe assumption is that if you want to do pseudo differential you'll use the pseudo differential mode rather than wasting hardware? Jonathan
On Sun, May 05, 2024 at 08:46:02PM +0100, Jonathan Cameron wrote: > On Tue, 30 Apr 2024 18:21:01 +0100 > Conor Dooley <conor@kernel.org> wrote: > > > On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote: > > > + diff-channels: > > > + description: > > > + Both inputs can be connected to pins AIN1 to AIN16 by choosing the > > > + appropriate value from 1 to 16. > > > + items: > > > + minimum: 1 > > > + maximum: 16 > > > + > > > + single-channel: > > > + description: > > > + Positive input can be connected to pins AIN1 to AIN16 by choosing the > > > + appropriate value from 1 to 16. Negative input is connected to AINCOM. > > > + items: > > > + minimum: 1 > > > + maximum: 16 > > > > Up to 16 differential channels and 16 single-ended channels, but only 16 > > pins? Would the number of differential channels not max out at 8? > > May not really be limited to 16 differential. Many chips use general purpose > muxes on both sides so you can do all combinations. In practice that's normally > pointless. > > A more useful case is to do all but one channel as positive inputs and the remaining > channel as the negative for those 15 differential channels. Yah, 15 is what I had in my head as the highest reasonable number given the information given about the AIN# pins. > This is effectively the same as doing pseudo differential channels, but > on more flexible hardware. This is in contrast to a device that only supports > pseudo differential where there is a special pin for the negative > (this device has that as well as full muxes on the other 16 lines). > > Having said all that. The ad7194 datasheet says 8 differential channels.. > I have no idea why though... Maybe something to do with the mux switching? > Or maybe assumption is that if you want to do pseudo differential you'll use > the pseudo differential mode rather than wasting hardware? I didn't look at the datasheet tbf, I was just asking given the description didn't make sense to me and looking for an explanation from the author.
On 30.04.2024 20:21, Conor Dooley wrote: > On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote: >> + diff-channels: >> + description: >> + Both inputs can be connected to pins AIN1 to AIN16 by choosing the >> + appropriate value from 1 to 16. >> + items: >> + minimum: 1 >> + maximum: 16 >> + >> + single-channel: >> + description: >> + Positive input can be connected to pins AIN1 to AIN16 by choosing the >> + appropriate value from 1 to 16. Negative input is connected to AINCOM. >> + items: >> + minimum: 1 >> + maximum: 16 > > Up to 16 differential channels and 16 single-ended channels, but only 16 > pins? Would the number of differential channels not max out at 8? Hello, Conor! I really appreciate the feedback! The way I thought about it, the only thing constraining the number of channels is the reg number (minimum: 0, maximum: 271). 272 channels cover all possible combinations (16*16 differential and 16 single ended) and I thought there is no need for anything stricter. I added items: minimum:1 maximum:16 to make sure the numbers are from 1 to 16, corresponding to AIN1-AIN16. Please let me know what should be improved! Kind regards, Alisa-Dariana Roman.
On 5/10/24 5:05 AM, Alisa-Dariana Roman wrote: > On 30.04.2024 20:21, Conor Dooley wrote: >> On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote: >>> + diff-channels: >>> + description: >>> + Both inputs can be connected to pins AIN1 to AIN16 by choosing the >>> + appropriate value from 1 to 16. >>> + items: >>> + minimum: 1 >>> + maximum: 16 >>> + >>> + single-channel: >>> + description: >>> + Positive input can be connected to pins AIN1 to AIN16 by choosing the >>> + appropriate value from 1 to 16. Negative input is connected to AINCOM. >>> + items: >>> + minimum: 1 >>> + maximum: 16 >> >> Up to 16 differential channels and 16 single-ended channels, but only 16 >> pins? Would the number of differential channels not max out at 8? > > Hello, Conor! I really appreciate the feedback! > > The way I thought about it, the only thing constraining the number of channels is the reg number (minimum: 0, maximum: 271). 272 channels cover all possible combinations (16*16 differential and 16 single ended) and I thought there is no need for anything stricter. I added items: minimum:1 maximum:16 to make sure the numbers are from 1 to 16, corresponding to AIN1-AIN16. > > Please let me know what should be improved! > > Kind regards, > Alisa-Dariana Roman. > Having looked at the datasheet for this and other similar chips, I agree that this reasoning makes sense. Some of the similar chips that have fixed channel assignments still have, e.g. a channel where + and - are both AIN2 (I assume for diagnostics). So I think it makes sense to allow for doing something similar here even if the most common use cases will probably have at most 16 channels defined in the .dts.
On Fri, May 10, 2024 at 09:21:37AM -0500, David Lechner wrote: > On 5/10/24 5:05 AM, Alisa-Dariana Roman wrote: > > On 30.04.2024 20:21, Conor Dooley wrote: > >> On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote: > >>> + diff-channels: > >>> + description: > >>> + Both inputs can be connected to pins AIN1 to AIN16 by choosing the > >>> + appropriate value from 1 to 16. > >>> + items: > >>> + minimum: 1 > >>> + maximum: 16 > >>> + > >>> + single-channel: > >>> + description: > >>> + Positive input can be connected to pins AIN1 to AIN16 by choosing the > >>> + appropriate value from 1 to 16. Negative input is connected to AINCOM. > >>> + items: > >>> + minimum: 1 > >>> + maximum: 16 > >> > >> Up to 16 differential channels and 16 single-ended channels, but only 16 > >> pins? Would the number of differential channels not max out at 8? > > > > Hello, Conor! I really appreciate the feedback! > > > > The way I thought about it, the only thing constraining the number of channels is the reg number (minimum: 0, maximum: 271). 272 channels cover all possible combinations (16*16 differential and 16 single ended) and I thought there is no need for anything stricter. I added items: minimum:1 maximum:16 to make sure the numbers are from 1 to 16, corresponding to AIN1-AIN16. > > > > Please let me know what should be improved! > > > > Kind regards, > > Alisa-Dariana Roman. > > > > Having looked at the datasheet for this and other similar chips, I agree > that this reasoning makes sense. Some of the similar chips that have fixed > channel assignments still have, e.g. a channel where + and - are both > AIN2 (I assume for diagnostics). So I think it makes sense to allow for > doing something similar here even if the most common use cases will > probably have at most 16 channels defined in the .dts. Actually, I think there were a bunch of whiffs on this one by either misreading the property in question (me) or not realising that I had done that and trying to explain what the possible combinations are. Looking at it now, I dunno wtf I was smoking because there's no way that this would be a functional binding if the min/max in the quote above constraining the number of channels. I can hardly blame y'all for that though, I am supposed to know how bindings work after all...
On Fri, 10 May 2024 22:26:22 +0100 Conor Dooley <conor@kernel.org> wrote: > On Fri, May 10, 2024 at 09:21:37AM -0500, David Lechner wrote: > > On 5/10/24 5:05 AM, Alisa-Dariana Roman wrote: > > > On 30.04.2024 20:21, Conor Dooley wrote: > > >> On Tue, Apr 30, 2024 at 07:29:45PM +0300, Alisa-Dariana Roman wrote: > > >>> + diff-channels: > > >>> + description: > > >>> + Both inputs can be connected to pins AIN1 to AIN16 by choosing the > > >>> + appropriate value from 1 to 16. > > >>> + items: > > >>> + minimum: 1 > > >>> + maximum: 16 > > >>> + > > >>> + single-channel: > > >>> + description: > > >>> + Positive input can be connected to pins AIN1 to AIN16 by choosing the > > >>> + appropriate value from 1 to 16. Negative input is connected to AINCOM. > > >>> + items: > > >>> + minimum: 1 > > >>> + maximum: 16 > > >> > > >> Up to 16 differential channels and 16 single-ended channels, but only 16 > > >> pins? Would the number of differential channels not max out at 8? > > > > > > Hello, Conor! I really appreciate the feedback! > > > > > > The way I thought about it, the only thing constraining the number of channels is the reg number (minimum: 0, maximum: 271). 272 channels cover all possible combinations (16*16 differential and 16 single ended) and I thought there is no need for anything stricter. I added items: minimum:1 maximum:16 to make sure the numbers are from 1 to 16, corresponding to AIN1-AIN16. > > > > > > Please let me know what should be improved! > > > > > > Kind regards, > > > Alisa-Dariana Roman. > > > > > > > Having looked at the datasheet for this and other similar chips, I agree > > that this reasoning makes sense. Some of the similar chips that have fixed > > channel assignments still have, e.g. a channel where + and - are both > > AIN2 (I assume for diagnostics). So I think it makes sense to allow for > > doing something similar here even if the most common use cases will > > probably have at most 16 channels defined in the .dts. > > Actually, I think there were a bunch of whiffs on this one by either > misreading the property in question (me) or not realising that I had done > that and trying to explain what the possible combinations are. > Looking at it now, I dunno wtf I was smoking because there's no way that > this would be a functional binding if the min/max in the quote above > constraining the number of channels. I can hardly blame y'all for that > though, I am supposed to know how bindings work after all... Me too :( I also failed to register this doesn't constrain channel counts at all. Anyhow, all's well that ends well! J
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml index cf5c568f140a..a03da9489ed9 100644 --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml @@ -21,8 +21,15 @@ properties: - adi,ad7190 - adi,ad7192 - adi,ad7193 + - adi,ad7194 - adi,ad7195 + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + reg: maxItems: 1 @@ -89,6 +96,42 @@ properties: description: see Documentation/devicetree/bindings/iio/adc/adc.yaml type: boolean +patternProperties: + "^channel@[0-9a-f]+$": + type: object + $ref: adc.yaml + unevaluatedProperties: false + + properties: + reg: + description: The channel index. + minimum: 0 + maximum: 271 + + diff-channels: + description: + Both inputs can be connected to pins AIN1 to AIN16 by choosing the + appropriate value from 1 to 16. + items: + minimum: 1 + maximum: 16 + + single-channel: + description: + Positive input can be connected to pins AIN1 to AIN16 by choosing the + appropriate value from 1 to 16. Negative input is connected to AINCOM. + items: + minimum: 1 + maximum: 16 + + oneOf: + - required: + - reg + - diff-channels + - required: + - reg + - single-channel + required: - compatible - reg @@ -103,6 +146,17 @@ required: allOf: - $ref: /schemas/spi/spi-peripheral-props.yaml# + - if: + properties: + compatible: + enum: + - adi,ad7190 + - adi,ad7192 + - adi,ad7193 + - adi,ad7195 + then: + patternProperties: + "^channel@[0-9a-f]+$": false unevaluatedProperties: false @@ -133,3 +187,38 @@ examples: adi,burnout-currents-enable; }; }; + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad7194"; + reg = <0>; + + #address-cells = <1>; + #size-cells = <0>; + + spi-max-frequency = <1000000>; + spi-cpol; + spi-cpha; + clocks = <&ad7192_mclk>; + clock-names = "mclk"; + interrupts = <25 0x2>; + interrupt-parent = <&gpio>; + aincom-supply = <&aincom>; + dvdd-supply = <&dvdd>; + avdd-supply = <&avdd>; + vref-supply = <&vref>; + + channel@0 { + reg = <0>; + diff-channels = <1 6>; + }; + + channel@1 { + reg = <1>; + single-channel = <1>; + }; + }; + };
Unlike the other AD719Xs, AD7194 has configurable channels. The user can dynamically configure them in the devicetree. Also add an example for AD7194 devicetree. Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> --- .../bindings/iio/adc/adi,ad7192.yaml | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+)