| Message ID | 20240819064721.91494-7-aardelean@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7606: add support for AD7606C-{16,18} parts | expand |
On Mon, Aug 19, 2024 at 09:47:16AM +0300, Alexandru Ardelean wrote: > The driver will support the AD7606C-16 and AD7606C-18. > This change adds the compatible strings for these devices. > > The AD7606C-16,18 channels also support these (individually configurable) > types of channels: > - bipolar single-ended > - unipolar single-ended > - bipolar differential > > This DT adds support for 'channel@X' nodes' I don't understand this sentence, suggest to drop it. > > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com> > --- > .../bindings/iio/adc/adi,ad7606.yaml | 83 +++++++++++++++++++ > 1 file changed, 83 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > index 69408cae3db9..f9e177de3f8c 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > @@ -14,6 +14,8 @@ description: | > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7605-4.pdf > https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606_7606-6_7606-4.pdf > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7606B.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7616.pdf > > properties: > @@ -24,6 +26,8 @@ properties: > - adi,ad7606-6 > - adi,ad7606-8 # Referred to as AD7606 (without -8) in the datasheet > - adi,ad7606b > + - adi,ad7606c-16 > + - adi,ad7606c-18 > - adi,ad7616 > > reg: > @@ -114,6 +118,30 @@ properties: > assumed that the pins are hardwired to VDD. > type: boolean > > +patternProperties: > + "^channel@([0-9a-f])$": [0-7] > + type: object > + $ref: adc.yaml > + unevaluatedProperties: false > + > + properties: > + reg: > + description: The channel number. > + minimum: 0 > + maximum: 7 > + > + diff-channel: > + description: Channel is bipolar differential. There is diff-channels property, why do we need one more? > + type: boolean > + > + bipolar: > + description: | > + Channel is bipolar single-ended. If 'diff-channel' is set, then > + the value of this property will be ignored. Then provide here allOf:if:then which makes it false if diff-channel(s) is present. And then drop entire property, because you duplicate what's in adc.yaml. > + type: boolean Blank line. > + required: > + - reg > + > required: > - compatible > - reg > @@ -170,6 +198,21 @@ allOf: > adi,conversion-start-gpios: > maxItems: 1 > > + - if: > + not: > + properties: > + compatible: > + contains: > + enum: > + - adi,ad7606c-16 > + - adi,ad7606c-18 > + then: > + patternProperties: > + "^channel@([0-9a-f])$": > + properties: > + diff-channels: false > + bipolar: true ? Drop, no clue what you want to say here. But more important, you are now adding channels to other variants. Split your commit between new device and new properties for existing devices. Best regards, Krzysztof
On Mon, Aug 19, 2024 at 4:09 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Mon, Aug 19, 2024 at 09:47:16AM +0300, Alexandru Ardelean wrote: > > The driver will support the AD7606C-16 and AD7606C-18. > > This change adds the compatible strings for these devices. > > > > The AD7606C-16,18 channels also support these (individually configurable) > > types of channels: > > - bipolar single-ended > > - unipolar single-ended > > - bipolar differential > > > > This DT adds support for 'channel@X' nodes' > > I don't understand this sentence, suggest to drop it. Huh? I guess I'm developing more ADHD, where I forget to finish sentences(?) > > > > > > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com> > > --- > > .../bindings/iio/adc/adi,ad7606.yaml | 83 +++++++++++++++++++ > > 1 file changed, 83 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > > index 69408cae3db9..f9e177de3f8c 100644 > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > > @@ -14,6 +14,8 @@ description: | > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7605-4.pdf > > https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606_7606-6_7606-4.pdf > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7606B.pdf > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7616.pdf > > > > properties: > > @@ -24,6 +26,8 @@ properties: > > - adi,ad7606-6 > > - adi,ad7606-8 # Referred to as AD7606 (without -8) in the datasheet > > - adi,ad7606b > > + - adi,ad7606c-16 > > + - adi,ad7606c-18 > > - adi,ad7616 > > > > reg: > > @@ -114,6 +118,30 @@ properties: > > assumed that the pins are hardwired to VDD. > > type: boolean > > > > +patternProperties: > > + "^channel@([0-9a-f])$": > > [0-7] > > > + type: object > > + $ref: adc.yaml > > + unevaluatedProperties: false > > + > > + properties: > > + reg: > > + description: The channel number. > > + minimum: 0 > > + maximum: 7 > > + > > + diff-channel: > > + description: Channel is bipolar differential. > > There is diff-channels property, why do we need one more? Yeah, I wanted to use that. Maybe I will try another spin at that. The thing with "diff-channels" is that it requires 2 ints. So, diff-channels = <0 0>. To use it here, a rule would need to be put in place where "reg == diff-channels[0] && reg == diff-channels[1]". That also works from my side. Part of the reason for this patchset, was to also get some feedback (if this is the correct direction). > > > + type: boolean > > + > > + bipolar: > > + description: | > > + Channel is bipolar single-ended. If 'diff-channel' is set, then > > + the value of this property will be ignored. > > Then provide here allOf:if:then which makes it false if diff-channel(s) > is present. And then drop entire property, because you duplicate what's > in adc.yaml. Ack. > > > > + type: boolean > > Blank line. > > > + required: > > + - reg > > + > > required: > > - compatible > > - reg > > @@ -170,6 +198,21 @@ allOf: > > adi,conversion-start-gpios: > > maxItems: 1 > > > > + - if: > > + not: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - adi,ad7606c-16 > > + - adi,ad7606c-18 > > + then: > > + patternProperties: > > + "^channel@([0-9a-f])$": > > + properties: > > + diff-channels: false > > + bipolar: true > > ? Drop, no clue what you want to say here. But more important, you are > now adding channels to other variants. Split your commit between new > device and new properties for existing devices. Ack. Will do that. > > > Best regards, > Krzysztof >
> > > + type: object > > > + $ref: adc.yaml > > > + unevaluatedProperties: false > > > + > > > + properties: > > > + reg: > > > + description: The channel number. > > > + minimum: 0 > > > + maximum: 7 > > > + > > > + diff-channel: > > > + description: Channel is bipolar differential. > > > > There is diff-channels property, why do we need one more? > > Yeah, I wanted to use that. > Maybe I will try another spin at that. > The thing with "diff-channels" is that it requires 2 ints. > So, diff-channels = <0 0>. > To use it here, a rule would need to be put in place where "reg == > diff-channels[0] && reg == diff-channels[1]". > That also works from my side. > Part of the reason for this patchset, was to also get some feedback > (if this is the correct direction). > So I 'think' this is a datasheet matching thing. In many cases, even for strictly differential devices, the pin naming allows for a clear A - B channel description. Here in the non differential modes, the negative pins are effectively not used (from a really quick look at the datasheet) So we 'could' introduce magic channels (give them high numbers) for the negative ends. I think we may want to do that for the userspace ABI (0-0 on the few times it has come up has been a calibration / self check mode not what you have here - it wires the actual inputs together). Alternative is just present them as a simple voltage and don't worry about the differential aspect as it's not hugely different to bipolar (where the zero level is effectively the negative input of a differential ADC. For the binding I'm fine with the binding using A, A as you suggest with an update to adc.yaml to cover this corner. We never (I think) have bindings for the self check case where the input is wired to both sides. It's just a mode that is applied to any inputs that are wired. > > > > > + type: boolean > > > + > > > + bipolar: > > > + description: | > > > + Channel is bipolar single-ended. If 'diff-channel' is set, then > > > + the value of this property will be ignored. > > > > Then provide here allOf:if:then which makes it false if diff-channel(s) > > is present. And then drop entire property, because you duplicate what's > > in adc.yaml. > > Ack.
On 21/08/2024 22:26, Jonathan Cameron wrote: > >>>> + type: object >>>> + $ref: adc.yaml >>>> + unevaluatedProperties: false >>>> + >>>> + properties: >>>> + reg: >>>> + description: The channel number. >>>> + minimum: 0 >>>> + maximum: 7 >>>> + >>>> + diff-channel: >>>> + description: Channel is bipolar differential. >>> >>> There is diff-channels property, why do we need one more? >> >> Yeah, I wanted to use that. >> Maybe I will try another spin at that. >> The thing with "diff-channels" is that it requires 2 ints. >> So, diff-channels = <0 0>. >> To use it here, a rule would need to be put in place where "reg == >> diff-channels[0] && reg == diff-channels[1]". >> That also works from my side. >> Part of the reason for this patchset, was to also get some feedback >> (if this is the correct direction). >> > So I 'think' this is a datasheet matching thing. > In many cases, even for strictly differential devices, the pin > naming allows for a clear A - B channel description. Here > in the non differential modes, the negative pins are effectively > not used (from a really quick look at the datasheet) > > So we 'could' introduce magic channels (give them high numbers) for > the negative ends. I think we may want to do that for the > userspace ABI (0-0 on the few times it has come up has been a > calibration / self check mode not what you have here - it > wires the actual inputs together). Alternative is just present > them as a simple voltage and don't worry about the differential aspect > as it's not hugely different to bipolar (where the zero level is > effectively the negative input of a differential ADC. > > For the binding I'm fine with the binding using A, A as you suggest > with an update to adc.yaml to cover this corner. Yep, let's add it to adc.yaml. > > We never (I think) have bindings for the self check case where the input > is wired to both sides. It's just a mode that is applied to > any inputs that are wired. > Best regards, Krzysztof
On Fri, Aug 23, 2024 at 12:09 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 21/08/2024 22:26, Jonathan Cameron wrote: > > > >>>> + type: object > >>>> + $ref: adc.yaml > >>>> + unevaluatedProperties: false > >>>> + > >>>> + properties: > >>>> + reg: > >>>> + description: The channel number. > >>>> + minimum: 0 > >>>> + maximum: 7 > >>>> + > >>>> + diff-channel: > >>>> + description: Channel is bipolar differential. > >>> > >>> There is diff-channels property, why do we need one more? > >> > >> Yeah, I wanted to use that. > >> Maybe I will try another spin at that. > >> The thing with "diff-channels" is that it requires 2 ints. > >> So, diff-channels = <0 0>. > >> To use it here, a rule would need to be put in place where "reg == > >> diff-channels[0] && reg == diff-channels[1]". > >> That also works from my side. > >> Part of the reason for this patchset, was to also get some feedback > >> (if this is the correct direction). > >> > > So I 'think' this is a datasheet matching thing. > > In many cases, even for strictly differential devices, the pin > > naming allows for a clear A - B channel description. Here > > in the non differential modes, the negative pins are effectively > > not used (from a really quick look at the datasheet) > > > > So we 'could' introduce magic channels (give them high numbers) for > > the negative ends. I think we may want to do that for the > > userspace ABI (0-0 on the few times it has come up has been a > > calibration / self check mode not what you have here - it > > wires the actual inputs together). Alternative is just present > > them as a simple voltage and don't worry about the differential aspect > > as it's not hugely different to bipolar (where the zero level is > > effectively the negative input of a differential ADC. > > > > For the binding I'm fine with the binding using A, A as you suggest > > with an update to adc.yaml to cover this corner. The main difference the "diff-channels" property brings is a change to the available scales. They differ a bit between differential, and single-ended (unipolar and bipolar). I'll update the adc.yaml file then. > > Yep, let's add it to adc.yaml. > > > > > We never (I think) have bindings for the self check case where the input > > is wired to both sides. It's just a mode that is applied to > > any inputs that are wired. > > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml index 69408cae3db9..f9e177de3f8c 100644 --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml @@ -14,6 +14,8 @@ description: | https://www.analog.com/media/en/technical-documentation/data-sheets/AD7605-4.pdf https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606_7606-6_7606-4.pdf https://www.analog.com/media/en/technical-documentation/data-sheets/AD7606B.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf https://www.analog.com/media/en/technical-documentation/data-sheets/AD7616.pdf properties: @@ -24,6 +26,8 @@ properties: - adi,ad7606-6 - adi,ad7606-8 # Referred to as AD7606 (without -8) in the datasheet - adi,ad7606b + - adi,ad7606c-16 + - adi,ad7606c-18 - adi,ad7616 reg: @@ -114,6 +118,30 @@ properties: assumed that the pins are hardwired to VDD. type: boolean +patternProperties: + "^channel@([0-9a-f])$": + type: object + $ref: adc.yaml + unevaluatedProperties: false + + properties: + reg: + description: The channel number. + minimum: 0 + maximum: 7 + + diff-channel: + description: Channel is bipolar differential. + type: boolean + + bipolar: + description: | + Channel is bipolar single-ended. If 'diff-channel' is set, then + the value of this property will be ignored. + type: boolean + required: + - reg + required: - compatible - reg @@ -170,6 +198,21 @@ allOf: adi,conversion-start-gpios: maxItems: 1 + - if: + not: + properties: + compatible: + contains: + enum: + - adi,ad7606c-16 + - adi,ad7606c-18 + then: + patternProperties: + "^channel@([0-9a-f])$": + properties: + diff-channels: false + bipolar: true + unevaluatedProperties: false examples: @@ -202,4 +245,44 @@ examples: standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; }; }; + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad7606-8"; + reg = <0>; + spi-max-frequency = <1000000>; + spi-cpol; + spi-cpha; + + avcc-supply = <&adc_vref>; + vdrive-supply = <&vdd_supply>; + + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; + interrupt-parent = <&gpio>; + + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; + + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>; + adi,first-data-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>; + standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; + + adi,sw-mode; + + channel@1 { + reg = <1>; + diff-channel; + }; + + channel@3 { + reg = <3>; + bipolar; + }; + }; + }; ...
The driver will support the AD7606C-16 and AD7606C-18. This change adds the compatible strings for these devices. The AD7606C-16,18 channels also support these (individually configurable) types of channels: - bipolar single-ended - unipolar single-ended - bipolar differential This DT adds support for 'channel@X' nodes' Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com> --- .../bindings/iio/adc/adi,ad7606.yaml | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+)