| Message ID | 20240730030509.57834-2-Mariel.Tinaco@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | add AD8460 DAC driver | expand |
On 30/07/2024 05:05, Mariel Tinaco wrote: > This adds the bindings documentation for the 14-bit > High Voltage, High Current, Waveform Generator > Digital-to-Analog converter. > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> > + > + refio-1p2v-supply: > + description: Drive voltage in the range of 1.2V maximum to as low as > + low as 0.12V through the REF_IO pin to adjust full scale output span > + > + clocks: maxItems: 1 and drop description (or use items: - description, but then do not repeat redundant parts) > + description: The clock for the DAC. This is the sync clock > + > + adi,rset-ohms: > + description: Specify value of external resistor connected to FS_ADJ pin > + to establish internal HVDAC's reference current I_REF > + default: 2000 > + minimum: 2000 > + maximum: 20000 > + > + adi,range-microvolt: > + description: | > + Voltage output range specified as <minimum, maximum> > + oneOf: Not an oneOf. > + - items: > + - const: -40000000 > + - const: 40000000 Why do you need this property if this cannot be anything else? Drop. > + > + adi,range-microamp: > + description: | Do not need '|' unless you need to preserve formatting. > + Current output range specified as <minimum, maximum> > + oneOf: > + - items: > + - const: 0 > + - const: 50000 > + - items: > + - const: -50000 > + - const: 50000 > + > + adi,temp-max-millicelsius: > + description: Overtemperature threshold > + default: 50000 > + minimum: 20000 > + maximum: 150000 > + > + sdn-reset-gpios: reset-gpios or shutdown-gpios or anything from gpio-consumer-common which is not deprecated. > + description: GPIO spec for the SHUTDOWN RESET pin. As the line is active high, Do not repeat the obvious or redundant parts. There is no point in saying that "GPIO spec is a GPIO spec for ...". It cannot be anything else than GPIO spec. Instead say something useful. It's confusing to have two reset pins, so explain what is the purpose of this pin. > + it should be marked GPIO_ACTIVE_HIGH. Drop last part "it should be marked", because it is clearly incorrect. Different board designs can have it differently. > + maxItems: 1 > + > + reset-gpios: > + description: GPIO spec for the RESET pin. As the line is active low, it > + should be marked GPIO_ACTIVE_LOW. Again, marking it always as active low is not correct. It is enough to say that line is active low. > + maxItems: 1 > + > + sdn-io-gpios: > + description: GPIO spec for the SHUTDOWN INPUT/OUTPUT pin. As the line is > + active high, it should be marked GPIO_ACTIVE_HIGH. What's the purpose? > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - clocks > + > +allOf: > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + dac@0 { > + compatible = "adi,ad8460"; > + reg = <0>; > + spi-max-frequency = <8000000>; > + adi,rset-ohms = <2000>; > + adi,range-microvolt = <(-40000000) 40000000>; > + adi,range-microamp = <0 50000>; > + adi,temp-max-millicelsius = <50000>; Custom properties go to the end. See DTS coding style. > + > + dmas = <&tx_dma 0>; > + dma-names = "tx"; Best regards, Krzysztof
On Tue, 30 Jul 2024 11:05:08 +0800 Mariel Tinaco <Mariel.Tinaco@analog.com> wrote: > This adds the bindings documentation for the 14-bit > High Voltage, High Current, Waveform Generator > Digital-to-Analog converter. > A few additions to Krzysztof's much more detailed review. Wrap patch descriptions to 75 chars. not sub 55. > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> > + > + adi,rset-ohms: Please rename this as rset sounds like reset to me. Not sure what a good name is however! > + description: Specify value of external resistor connected to FS_ADJ pin > + to establish internal HVDAC's reference current I_REF > + default: 2000 > + minimum: 2000 > + maximum: 20000 > +
> -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: Tuesday, July 30, 2024 2:15 PM > To: Tinaco, Mariel <Mariel.Tinaco@analog.com>; linux-iio@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Jonathan Cameron > <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Hennerich, > Michael <Michael.Hennerich@analog.com>; Conor Dooley > <conor+dt@kernel.org>; Marcelo Schmitt <marcelo.schmitt1@gmail.com>; > Dimitri Fedrau <dima.fedrau@gmail.com>; David Lechner > <dlechner@baylibre.com>; Nuno Sá <noname.nuno@gmail.com> > Subject: Re: [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460 > > [External] > > On 30/07/2024 05:05, Mariel Tinaco wrote: > > This adds the bindings documentation for the 14-bit > > High Voltage, High Current, Waveform Generator > > Digital-to-Analog converter. > > > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> > > > + > > + refio-1p2v-supply: > > + description: Drive voltage in the range of 1.2V maximum to as low as > > + low as 0.12V through the REF_IO pin to adjust full scale output span > > + > > + clocks: > > maxItems: 1 > and drop description (or use items: - description, but then do not > repeat redundant parts) > Simplified the description for this item refio-1p2v-supply: description: Reference voltage to adjust full scale output span maxItems: 1 Should I put, "maxItems: 1" in the other vrefs as well? > > + description: The clock for the DAC. This is the sync clock > > + > > + adi,rset-ohms: > > + description: Specify value of external resistor connected to FS_ADJ pin > > + to establish internal HVDAC's reference current I_REF > > + default: 2000 > > + minimum: 2000 > > + maximum: 20000 > > + > > + adi,range-microvolt: > > + description: | > > + Voltage output range specified as <minimum, maximum> > > + oneOf: > > Not an oneOf. > You're right. I should have put all the possible values. I populated it with common Values found in the datasheet adi,range-microvolt: description: Voltage output range specified as <minimum, maximum> oneOf: - items: - enum: [0, -10000000, -20000000, -30000000, -40000000, -55000000] - enum: [10000000, 20000000, 30000000, 40000000, 55000000] > > + - items: > > + - const: -40000000 > > + - const: 40000000 > > Why do you need this property if this cannot be anything else? Drop. > > > + > > + adi,range-microamp: > > + description: | > > Do not need '|' unless you need to preserve formatting. > > > + Current output range specified as <minimum, maximum> > > + oneOf: > > + - items: > > + - const: 0 > > + - const: 50000 > > + - items: > > + - const: -50000 > > + - const: 50000 > > + > > + adi,temp-max-millicelsius: > > + description: Overtemperature threshold > > + default: 50000 > > + minimum: 20000 > > + maximum: 150000 > > + > > + sdn-reset-gpios: > > reset-gpios or shutdown-gpios or anything from gpio-consumer-common > which is not deprecated. > Lifted from gpio-consumer-common yaml. Mapped to corresponding pins wakeup-gpios: description: Corresponds to SDN_RESET pin. To exit shutdown or sleep mode, pulse SDN_RESET HIGH, then leave LOW. maxItems: 1 reset-gpios: description: Manual Power On Reset (POR). Pull this GPIO pin LOW and then HIGH to reset all digital registers to default maxItems: 1 shutdown-gpios: description: Corresponds to SDN_IO pin. Shutdown may be initiated by the user, by pulsing SDN_IO high. To exit shutdown, pulse SDN_IO low, then float. maxItems: 1 Replaced description based on the datasheet > > + description: GPIO spec for the SHUTDOWN RESET pin. As the line is active > high, > > Do not repeat the obvious or redundant parts. There is no point in > saying that "GPIO spec is a GPIO spec for ...". It cannot be anything > else than GPIO spec. Instead say something useful. It's confusing to > have two reset pins, so explain what is the purpose of this pin. > > > + it should be marked GPIO_ACTIVE_HIGH. > > Drop last part "it should be marked", because it is clearly incorrect. > Different board designs can have it differently. > > > > + maxItems: 1 > > + > > + reset-gpios: > > + description: GPIO spec for the RESET pin. As the line is active low, it > > + should be marked GPIO_ACTIVE_LOW. > > Again, marking it always as active low is not correct. It is enough to > say that line is active low. > > > + maxItems: 1 > > + > > + sdn-io-gpios: > > + description: GPIO spec for the SHUTDOWN INPUT/OUTPUT pin. As the > line is > > + active high, it should be marked GPIO_ACTIVE_HIGH. > > What's the purpose? > > > + maxItems: 1 > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + > > +allOf: > > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + > > + spi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + dac@0 { > > + compatible = "adi,ad8460"; > > + reg = <0>; > > + spi-max-frequency = <8000000>; > > + adi,rset-ohms = <2000>; > > + adi,range-microvolt = <(-40000000) 40000000>; > > + adi,range-microamp = <0 50000>; > > + adi,temp-max-millicelsius = <50000>; > > Custom properties go to the end. See DTS coding style. > Moved custom attributes in the end examples: - | #include <dt-bindings/gpio/gpio.h> spi { #address-cells = <1>; #size-cells = <0>; dac@0 { compatible = "adi,ad8460"; reg = <0>; spi-max-frequency = <8000000>; dmas = <&tx_dma 0>; dma-names = "tx"; wakeup-gpios = <&gpio 86 GPIO_ACTIVE_HIGH>; reset-gpios = <&gpio 91 GPIO_ACTIVE_LOW>; shutdown-gpios = <&gpio 88 GPIO_ACTIVE_HIGH>; clocks = <&sync_ext_clk>; hvcc-supply = <&hvcc>; hvee-supply = <&hvee>; vcc-5v-supply = <&vcc_5>; vref-5v-supply = <&vref_5>; dvdd-3p3v-supply = <&dvdd_3_3>; avdd-3p3v-supply = <&avdd_3_3>; refio-1p2v-supply = <&refio_1_2>; adi,external-resistor-ohms = <2000>; adi,range-microvolt = <(-40000000) 40000000>; adi,range-microamp = <0 50000>; adi,max-millicelsius = <50000>; }; }; > > + > > + dmas = <&tx_dma 0>; > > + dma-names = "tx"; > > > Best regards, > Krzysztof
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Saturday, August 3, 2024 6:06 PM > To: Tinaco, Mariel <Mariel.Tinaco@analog.com> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Hennerich, > Michael <Michael.Hennerich@analog.com>; Conor Dooley > <conor+dt@kernel.org>; Marcelo Schmitt <marcelo.schmitt1@gmail.com>; > Dimitri Fedrau <dima.fedrau@gmail.com>; David Lechner > <dlechner@baylibre.com>; Nuno Sá <noname.nuno@gmail.com> > Subject: Re: [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460 > > [External] > > On Tue, 30 Jul 2024 11:05:08 +0800 > Mariel Tinaco <Mariel.Tinaco@analog.com> wrote: > > > This adds the bindings documentation for the 14-bit High Voltage, High > > Current, Waveform Generator Digital-to-Analog converter. > > > A few additions to Krzysztof's much more detailed review. > > Wrap patch descriptions to 75 chars. not sub 55. > > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> > > + > > + adi,rset-ohms: > > Please rename this as rset sounds like reset to me. Not sure what a good name > is however! > > > > + description: Specify value of external resistor connected to FS_ADJ pin > > + to establish internal HVDAC's reference current I_REF > > + default: 2000 > > + minimum: 2000 > > + maximum: 20000 > > + Replaced the name adi,external-resistor-ohms: description: Specify value of external resistor connected to FS_ADJ pin to establish internal HVDAC's reference current I_REF default: 2000 minimum: 2000 maximum: 20000 Regards, Mariel
On 17/08/2024 13:18, Tinaco, Mariel wrote: >>> + clocks: >> >> maxItems: 1 >> and drop description (or use items: - description, but then do not >> repeat redundant parts) >> > > Simplified the description for this item > > refio-1p2v-supply: > description: Reference voltage to adjust full scale output span > maxItems: 1 > > Should I put, "maxItems: 1" in the other vrefs as well? No, why do you change supply? My comment was under clocks. Comments are always under specific part of code which is being discussed. > >>> + description: The clock for the DAC. This is the sync clock >>> + >>> + adi,rset-ohms: >>> + description: Specify value of external resistor connected to FS_ADJ pin >>> + to establish internal HVDAC's reference current I_REF >>> + default: 2000 >>> + minimum: 2000 >>> + maximum: 20000 >>> + >>> + adi,range-microvolt: >>> + description: | >>> + Voltage output range specified as <minimum, maximum> >>> + oneOf: >> >> Not an oneOf. >> > > You're right. I should have put all the possible values. I populated it with common > Values found in the datasheet > > adi,range-microvolt: > description: Voltage output range specified as <minimum, maximum> > oneOf: > - items: > - enum: [0, -10000000, -20000000, -30000000, -40000000, -55000000] > - enum: [10000000, 20000000, 30000000, 40000000, 55000000] > >>> + - items: >>> + - const: -40000000 >>> + - const: 40000000 >> >> Why do you need this property if this cannot be anything else? Drop. >> >>> + >>> + adi,range-microamp: >>> + description: | >> >> Do not need '|' unless you need to preserve formatting. >> >>> + Current output range specified as <minimum, maximum> >>> + oneOf: >>> + - items: >>> + - const: 0 >>> + - const: 50000 >>> + - items: >>> + - const: -50000 >>> + - const: 50000 >>> + >>> + adi,temp-max-millicelsius: >>> + description: Overtemperature threshold >>> + default: 50000 >>> + minimum: 20000 >>> + maximum: 150000 >>> + >>> + sdn-reset-gpios: >> >> reset-gpios or shutdown-gpios or anything from gpio-consumer-common >> which is not deprecated. >> > > Lifted from gpio-consumer-common yaml. Mapped to corresponding pins > > wakeup-gpios: > description: Corresponds to SDN_RESET pin. To exit shutdown > or sleep mode, pulse SDN_RESET HIGH, then leave LOW. That's not a wakeup-gpio. I think my comment was imprecise. You have something like three reset/shutdown GPIOs, so pick from gpio-consumer-common the reset and shutdown. Remaining one could stay as in your original code. > maxItems: 1 > > reset-gpios: > description: Manual Power On Reset (POR). Pull this GPIO pin > LOW and then HIGH to reset all digital registers to default > maxItems: 1 > > shutdown-gpios: > description: Corresponds to SDN_IO pin. Shutdown may be > initiated by the user, by pulsing SDN_IO high. To exit shutdown, > pulse SDN_IO low, then float. > maxItems: 1 > Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml new file mode 100644 index 000000000..6a7031d0d --- /dev/null +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml @@ -0,0 +1,154 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright 2024 Analog Devices Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/dac/adi,ad8460.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD8460 DAC + +maintainers: + - Mariel Tinaco <mariel.tinaco@analog.com> + +description: | + Analog Devices AD8460 110 V High Voltage, 1 A High Current, + Arbitrary Waveform Generator with Integrated 14-Bit High Speed DAC + https://www.analog.com/media/en/technical-documentation/data-sheets/ad8460.pdf + +properties: + compatible: + enum: + - adi,ad8460 + + reg: + maxItems: 1 + + dmas: + maxItems: 1 + + dma-names: + items: + - const: tx + + spi-max-frequency: + maximum: 20000000 + + hvcc-supply: + description: Positive high voltage power supply line + + hvee-supply: + description: Negative high voltage power supply line + + vcc-5v-supply: + description: Low voltage power supply + + vref-5v-supply: + description: Reference voltage for analog low voltage and protection threshold DACs. + + dvdd-3p3v-supply: + description: Digital supply bypass + + avdd-3p3v-supply: + description: Analog supply bypass + + refio-1p2v-supply: + description: Drive voltage in the range of 1.2V maximum to as low as + low as 0.12V through the REF_IO pin to adjust full scale output span + + clocks: + description: The clock for the DAC. This is the sync clock + + adi,rset-ohms: + description: Specify value of external resistor connected to FS_ADJ pin + to establish internal HVDAC's reference current I_REF + default: 2000 + minimum: 2000 + maximum: 20000 + + adi,range-microvolt: + description: | + Voltage output range specified as <minimum, maximum> + oneOf: + - items: + - const: -40000000 + - const: 40000000 + + adi,range-microamp: + description: | + Current output range specified as <minimum, maximum> + oneOf: + - items: + - const: 0 + - const: 50000 + - items: + - const: -50000 + - const: 50000 + + adi,temp-max-millicelsius: + description: Overtemperature threshold + default: 50000 + minimum: 20000 + maximum: 150000 + + sdn-reset-gpios: + description: GPIO spec for the SHUTDOWN RESET pin. As the line is active high, + it should be marked GPIO_ACTIVE_HIGH. + maxItems: 1 + + reset-gpios: + description: GPIO spec for the RESET pin. As the line is active low, it + should be marked GPIO_ACTIVE_LOW. + maxItems: 1 + + sdn-io-gpios: + description: GPIO spec for the SHUTDOWN INPUT/OUTPUT pin. As the line is + active high, it should be marked GPIO_ACTIVE_HIGH. + maxItems: 1 + +required: + - compatible + - reg + - clocks + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + spi { + #address-cells = <1>; + #size-cells = <0>; + + dac@0 { + compatible = "adi,ad8460"; + reg = <0>; + spi-max-frequency = <8000000>; + adi,rset-ohms = <2000>; + adi,range-microvolt = <(-40000000) 40000000>; + adi,range-microamp = <0 50000>; + adi,temp-max-millicelsius = <50000>; + + dmas = <&tx_dma 0>; + dma-names = "tx"; + + sdn-reset-gpios = <&gpio 86 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio 91 GPIO_ACTIVE_LOW>; + sdn-io-gpios = <&gpio 88 GPIO_ACTIVE_HIGH>; + + clocks = <&sync_ext_clk>; + + hvcc-supply = <&hvcc>; + hvee-supply = <&hvee>; + vcc-5v-supply = <&vcc_5>; + vref-5v-supply = <&vref_5>; + dvdd-3p3v-supply = <&dvdd_3_3>; + avdd-3p3v-supply = <&avdd_3_3>; + refio-1p2v-supply = <&refio_1_2>; + }; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index 758c202ec..dae93df2a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1234,6 +1234,13 @@ W: https://ez.analog.com/linux-software-drivers F: Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml F: drivers/iio/adc/ad7780.c +ANALOG DEVICES INC AD8460 DRIVER +M: Mariel Tinaco <Mariel.Tinaco@analog.com> +L: linux-iio@vger.kernel.org +S: Supported +W: https://ez.analog.com/linux-software-drivers +F: Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml + ANALOG DEVICES INC AD9739a DRIVER M: Nuno Sa <nuno.sa@analog.com> M: Dragos Bogdan <dragos.bogdan@analog.com>
This adds the bindings documentation for the 14-bit High Voltage, High Current, Waveform Generator Digital-to-Analog converter. Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> --- .../bindings/iio/dac/adi,ad8460.yaml | 154 ++++++++++++++++++ MAINTAINERS | 7 + 2 files changed, 161 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml