| Message ID | 20220715044948.434149-2-cosmin.tanislav@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | AD4130 | expand |
Hi Cosmin, thanks for your patch! On Fri, Jul 15, 2022 at 6:50 AM Cosmin Tanislav <demonsingur@gmail.com> wrote: > AD4130-8 is an ultra-low power, high precision, measurement solution for > low bandwidth battery operated applications. > > The fully integrated AFE (Analog Front-End) includes a multiplexer for up > to 16 single-ended or 8 differential inputs, PGA (Programmable Gain > Amplifier), 24-bit Sigma-Delta ADC, on-chip reference and oscillator, > selectable filter options, smart sequencer, sensor biasing and excitation > options, diagnostics, and a FIFO buffer. > > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> > Reviewed-by: Rob Herring <robh@kernel.org> (...) This caught my eye: > + adi,int-clk-out: > + description: Specify if the internal clock should be exposed on the CLK pin. > + type: boolean Okay, but would it not make more sense to just imply this if the clock on the CLK pin has any consumers? Like update this setting in hardware when the consumer does clk_prepare() or so on that externally routed clock? > + adi,ext-clk-freq-hz: > + description: Specify the frequency of the external clock. > + enum: [76800, 153600] > + default: 76800 This looks like cheating, i.e just outputting a clock on that pin and ignoring to model the consumer. Shouldn't this rather be a clkout subnode with 2 #clock-cells and the fequency set in a cell in a consumer phandle? Like how I did in commit 7335631fcd5eecfa84555bd57433e6446d06ad21 "dt-bindings: clock: u8500: Add clkout clock bindings" Usually it is the consumer that requests a specific clock and then the producer will respond. Certainly whatever is consuming this clock needs to be in the device tree as well, and then this is the right pattern. (In Linux you will then use the clk framework to manage the clock and callbacks but that is irrelevant for the DT bindings.) > + adi,bipolar: > + description: Specify if the device should be used in bipolar mode. > + type: boolean Can you explain what this means? I don't understand what it would mean for an analog device / AFE to be in bipolar mode. Other than that it looks very nice! Yours, Linus Walleij
On 7/18/22 16:14, Linus Walleij wrote: > Hi Cosmin, > > thanks for your patch! > > On Fri, Jul 15, 2022 at 6:50 AM Cosmin Tanislav <demonsingur@gmail.com> wrote: > >> AD4130-8 is an ultra-low power, high precision, measurement solution for >> low bandwidth battery operated applications. >> >> The fully integrated AFE (Analog Front-End) includes a multiplexer for up >> to 16 single-ended or 8 differential inputs, PGA (Programmable Gain >> Amplifier), 24-bit Sigma-Delta ADC, on-chip reference and oscillator, >> selectable filter options, smart sequencer, sensor biasing and excitation >> options, diagnostics, and a FIFO buffer. >> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> >> Reviewed-by: Rob Herring <robh@kernel.org> > (...) > > This caught my eye: > >> + adi,int-clk-out: >> + description: Specify if the internal clock should be exposed on the CLK pin. >> + type: boolean > > Okay, but would it not make more sense to just imply this if the clock > on the CLK > pin has any consumers? Like update this setting in hardware when the consumer > does clk_prepare() or so on that externally routed clock? > You're right, this is indeed fit for being implemented using the clock framework. >> + adi,ext-clk-freq-hz: >> + description: Specify the frequency of the external clock. >> + enum: [76800, 153600] >> + default: 76800 > > This looks like cheating, i.e just outputting a clock on that pin > and ignoring to model the consumer. You got this wrong. The chip has 4 operating modes regarding clocking. Internal 76.8kHz clock (clkout can be used as an interrupt pin). Internal 76.8kHz clock, available externally on the clkout pin (clkout becomes an output). External 76.8kHz clock (clkout is an input). External 153.6kHz clock, internally divided by two (clkout is an input). This property is used to choose between what frequency to set the external clock up with. Indeed, if the external clock is not present, then exposing the 76.8kHz clock using the clock framework would be fine. Maybe you have a better suggestion about what to do with this? How do I tell the chip what frequency the external clock is, but also tell the clock what frequency to use? It's a bit of a conundrum for me. > > Shouldn't this rather be a clkout subnode with 2 #clock-cells > and the fequency set in a cell in a consumer phandle? > Like how I did in > commit 7335631fcd5eecfa84555bd57433e6446d06ad21 > "dt-bindings: clock: u8500: Add clkout clock bindings" > > Usually it is the consumer that requests a specific clock and then the > producer will respond. > > Certainly whatever is consuming this clock needs to be in the device tree > as well, and then this is the right pattern. > > (In Linux you will then use the clk framework to manage the clock and callbacks > but that is irrelevant for the DT bindings.) > >> + adi,bipolar: >> + description: Specify if the device should be used in bipolar mode. >> + type: boolean > > Can you explain what this means? I don't understand what it would > mean for an analog device / AFE to be in bipolar mode. > Range becomes [-VRef, VRef], as opposed to [0, VRef], resolution is halved. > Other than that it looks very nice! > > Yours, > Linus Walleij
On Thu, Sep 8, 2022 at 9:03 AM Cosmin Tanislav <demonsingur@gmail.com> wrote: > >> + adi,ext-clk-freq-hz: > >> + description: Specify the frequency of the external clock. > >> + enum: [76800, 153600] > >> + default: 76800 > > > > This looks like cheating, i.e just outputting a clock on that pin > > and ignoring to model the consumer. > > You got this wrong. > > The chip has 4 operating modes regarding clocking. > > Internal 76.8kHz clock (clkout can be used as an interrupt pin). > Internal 76.8kHz clock, available externally on the clkout pin (clkout > becomes an output). > External 76.8kHz clock (clkout is an input). > External 153.6kHz clock, internally divided by two (clkout is an input). > > This property is used to choose between what frequency to set the > external clock up with. Indeed, if the external clock is not present, > then exposing the 76.8kHz clock using the clock framework would be fine. > > Maybe you have a better suggestion about what to do with this? > How do I tell the chip what frequency the external clock is, but also > tell the clock what frequency to use? It's a bit of a conundrum for me. I would imagine not specify in the device tree what frequency to use at all. Instead use software for that. The clock provide for the clkout should provide static const struct clk_ops extclk_ops = { .recalc_rate = extclk_recalc_rate, .round_rate = extclk_round_rate, .set_rate = extclk_set_rate, }; This way the consumer driver can issue clk_round_rate(), clk_set_rate() etc to make the clk driver determine which parent to use for the consumer. Likewise .enable or .prepare should then mux out the desired clock. > >> + adi,bipolar: > >> + description: Specify if the device should be used in bipolar mode. > >> + type: boolean > > > > Can you explain what this means? I don't understand what it would > > mean for an analog device / AFE to be in bipolar mode. > > > > Range becomes [-VRef, VRef], as opposed to [0, VRef], resolution is > halved. OK that makes sense, put that in the binding description. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml new file mode 100644 index 000000000000..a3cbb84109c0 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml @@ -0,0 +1,256 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright 2022 Analog Devices Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/adi,ad4130.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD4130 ADC device driver + +maintainers: + - Cosmin Tanislav <cosmin.tanislav@analog.com> + +description: | + Bindings for the Analog Devices AD4130 ADC. Datasheet can be found here: + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4130-8.pdf + +properties: + compatible: + enum: + - adi,ad4130 + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + description: phandle to the master clock (mclk) + + clock-names: + items: + - const: mclk + + interrupts: + maxItems: 1 + + interrupt-names: + description: | + Specify which interrupt pin should be configured as Data Ready / FIFO + interrupt. + Default if not supplied is int. + enum: + - int + - clk + - p2 + - dout + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + + refin1-supply: + description: refin1 supply. Can be used as reference for conversion. + + refin2-supply: + description: refin2 supply. Can be used as reference for conversion. + + avdd-supply: + description: AVDD voltage supply. Can be used as reference for conversion. + + iovdd-supply: + description: IOVDD voltage supply. Used for the chip interface. + + spi-max-frequency: + maximum: 5000000 + + adi,int-clk-out: + description: Specify if the internal clock should be exposed on the CLK pin. + type: boolean + + adi,ext-clk-freq-hz: + description: Specify the frequency of the external clock. + enum: [76800, 153600] + default: 76800 + + adi,bipolar: + description: Specify if the device should be used in bipolar mode. + type: boolean + + adi,vbias-pins: + description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to. + $ref: /schemas/types.yaml#/definitions/uint32-array + maxItems: 16 + items: + minimum: 0 + maximum: 15 + +required: + - compatible + - reg + - interrupts + +patternProperties: + "^channel@([0-9a-f])$": + type: object + $ref: adc.yaml + unevaluatedProperties: false + + properties: + reg: + description: The channel number. + minimum: 0 + maximum: 15 + + diff-channels: + description: | + Besides the analog inputs available, internal inputs can be used. + 16: Internal temperature sensor. + 17: AVSS + 18: Internal reference + 19: DGND + 20: (AVDD − AVSS)/6+ + 21: (AVDD − AVSS)/6- + 22: (IOVDD − DGND)/6+ + 23: (IOVDD − DGND)/6- + 24: (ALDO − AVSS)/6+ + 25: (ALDO − AVSS)/6- + 26: (DLDO − DGND)/6+ + 27: (DLDO − DGND)/6- + 28: V_MV_P + 29: V_MV_M + items: + minimum: 0 + maximum: 29 + + adi,reference-select: + description: | + Select the reference source to use when converting on the + specific channel. Valid values are: + 0: REFIN1(+)/REFIN1(−) + 1: REFIN2(+)/REFIN2(−) + 2: REFOUT/AVSS (Internal reference) + 3: AVDD/AVSS + If not specified, REFIN1 is used. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3] + default: 0 + + adi,excitation-pin-0: + description: | + Analog input to apply excitation current to while the channel + is active. + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 15 + default: 0 + + adi,excitation-pin-1: + description: | + Analog input to apply excitation current to while this channel + is active. + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 15 + default: 0 + + adi,excitation-current-0-nanoamp: + description: | + Excitation current in nanoamps to be applied to pin specified in + adi,excitation-pin-0 while this channel is active. + enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000] + default: 0 + + adi,excitation-current-1-nanoamp: + description: | + Excitation current in nanoamps to be applied to pin specified in + adi,excitation-pin-1 while this channel is active. + enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000] + default: 0 + + adi,burnout-current-nanoamp: + description: | + Burnout current in nanoamps to be applied for this channel. + enum: [0, 500, 2000, 4000] + default: 0 + + adi,buffered-positive: + description: Enable buffered mode for positive input. + type: boolean + + adi,buffered-negative: + description: Enable buffered mode for negative input. + type: boolean + + required: + - reg + - diff-channels + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad4130"; + reg = <0>; + + #address-cells = <1>; + #size-cells = <0>; + + spi-max-frequency = <5000000>; + interrupts = <27 IRQ_TYPE_EDGE_FALLING>; + interrupt-parent = <&gpio>; + + channel@0 { + reg = <0>; + + adi,reference-select = <2>; + + /* AIN8, AIN9 */ + diff-channels = <8 9>; + }; + + channel@1 { + reg = <1>; + + adi,reference-select = <2>; + + /* AIN10, AIN11 */ + diff-channels = <10 11>; + }; + + channel@2 { + reg = <2>; + + adi,reference-select = <2>; + + /* Temperature Sensor, DGND */ + diff-channels = <16 19>; + }; + + channel@3 { + reg = <3>; + + adi,reference-select = <2>; + + /* Internal reference, DGND */ + diff-channels = <18 19>; + }; + + channel@4 { + reg = <4>; + + adi,reference-select = <2>; + + /* DGND, DGND */ + diff-channels = <19 19>; + }; + }; + };