* [PATCH v3 0/2] Add support to AD8460 Waveform Generator DAC
@ 2024-09-04 2:30 Mariel Tinaco
2024-09-04 2:30 ` [PATCH v3 1/2] dt-bindings: iio: dac: add docs for ad8460 Mariel Tinaco
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Mariel Tinaco @ 2024-09-04 2:30 UTC (permalink / raw)
To: linux-iio, devicetree, linux-kernel, Jonathan Cameron,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau,
David Lechner, Nuno Sá
Apply comments for adding support to AD8460 Waveform Generator DAC
ad8460:
* Fixed errors detected by test bot
* Applied proper masking of fixed values
* Applied proper wrapping to get close to 80 chars
* Applied proper comment formatting
* Applied proper placement of breaks in switch cases
* Removed channel properties unused by IIO buffer interface
* Simplified property getting on probe function
* Fixed error handlings on probe function
* Fixed setting of overvoltage, overcurrent and overtemperature ranges;
If value provided is invalid, default state of the register will not
be rewritten
Bindings:
* Dropped unnecessary descriptions
* Updated property descriptions to describe functionality properly
* Added multiple selection of values for adi,range-microvolt property
* Fixed formatting errors to follow DTS coding style
* Lifted GPIO naming from gpio-consumer-common yaml
Patch:
* Wrapped patches to 75 chars
Mariel Tinaco (2):
dt-bindings: iio: dac: add docs for ad8460
iio: dac: support the ad8460 Waveform DAC
.../bindings/iio/dac/adi,ad8460.yaml | 154 +++
MAINTAINERS | 8 +
drivers/iio/dac/Kconfig | 13 +
drivers/iio/dac/Makefile | 1 +
drivers/iio/dac/ad8460.c | 932 ++++++++++++++++++
5 files changed, 1108 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
create mode 100644 drivers/iio/dac/ad8460.c
base-commit: c4b43d8336e52dce6d124e428aa3b71703e62647
--
2.34.1
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH v3 1/2] dt-bindings: iio: dac: add docs for ad8460 2024-09-04 2:30 [PATCH v3 0/2] Add support to AD8460 Waveform Generator DAC Mariel Tinaco @ 2024-09-04 2:30 ` Mariel Tinaco 2024-09-04 6:20 ` Krzysztof Kozlowski 2024-09-04 2:30 ` [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco 2024-09-04 6:16 ` [PATCH v3 0/2] Add support to AD8460 Waveform Generator DAC Krzysztof Kozlowski 2 siblings, 1 reply; 20+ messages in thread From: Mariel Tinaco @ 2024-09-04 2:30 UTC (permalink / raw) To: linux-iio, devicetree, linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá 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 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 000000000000..da53bae4efed --- /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 + + clocks: + 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 + + 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 + + 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 + + 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] + + adi,range-microamp: + description: Current output range specified as <minimum, maximum> + oneOf: + - items: + - enum: [-50000, -100000, -300000, -500000, -1000000] + - enum: [50000, 100000, 300000, 500000, 1000000] + - items: + - const: 0 + - enum: [50000, 100000, 300000, 500000, 1000000] + + adi,max-millicelsius: + description: Overtemperature threshold + default: 50000 + minimum: 20000 + maximum: 150000 + + shutdown-reset-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 + +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>; + + dmas = <&tx_dma 0>; + dma-names = "tx"; + + shutdown-reset-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>; + }; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index 417c6751c0dc..e0509c9f5545 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1320,6 +1320,13 @@ F: Documentation/ABI/testing/debugfs-iio-ad9467 F: Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml F: drivers/iio/adc/ad9467.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> -- 2.34.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: dac: add docs for ad8460 2024-09-04 2:30 ` [PATCH v3 1/2] dt-bindings: iio: dac: add docs for ad8460 Mariel Tinaco @ 2024-09-04 6:20 ` Krzysztof Kozlowski 2024-09-07 17:01 ` Jonathan Cameron 0 siblings, 1 reply; 20+ messages in thread From: Krzysztof Kozlowski @ 2024-09-04 6:20 UTC (permalink / raw) To: Mariel Tinaco Cc: linux-iio, devicetree, linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá On Wed, Sep 04, 2024 at 10:30:39AM +0800, Mariel Tinaco wrote: > This adds the bindings documentation for the 14-bit Please do not use "This commit/patch/change", but imperative mood. See longer explanation here: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > 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 > + adi,range-microvolt: > + description: Voltage output range specified as <minimum, maximum> > + oneOf: This oneOf does not make sense. There is only one condition. Drop. > + - items: > + - enum: [0, -10000000, -20000000, -30000000, -40000000, -55000000] > + - enum: [10000000, 20000000, 30000000, 40000000, 55000000] What's the default? It's not a required property. > + > + adi,range-microamp: > + description: Current output range specified as <minimum, maximum> > + oneOf: > + - items: > + - enum: [-50000, -100000, -300000, -500000, -1000000] I don't understand why 0 is not listed here. > + - enum: [50000, 100000, 300000, 500000, 1000000] > + - items: > + - const: 0 > + - enum: [50000, 100000, 300000, 500000, 1000000] > + What's the default? It's not a required property. > + adi,max-millicelsius: > + description: Overtemperature threshold > + default: 50000 > + minimum: 20000 > + maximum: 150000 > + > + shutdown-reset-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 > + > +required: > + - compatible > + - reg > + - clocks Some supplies are for sure required. Devices rarely can operate without power provided. > + > +allOf: > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + > +additionalProperties: false unevaluatedProperties instead. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: dac: add docs for ad8460 2024-09-04 6:20 ` Krzysztof Kozlowski @ 2024-09-07 17:01 ` Jonathan Cameron 2024-09-09 6:22 ` Tinaco, Mariel 0 siblings, 1 reply; 20+ messages in thread From: Jonathan Cameron @ 2024-09-07 17:01 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Mariel Tinaco, linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá On Wed, 4 Sep 2024 08:20:53 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Wed, Sep 04, 2024 at 10:30:39AM +0800, Mariel Tinaco wrote: > > This adds the bindings documentation for the 14-bit > > Please do not use "This commit/patch/change", but imperative mood. See > longer explanation here: > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > > > 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 > > > + adi,range-microvolt: > > + description: Voltage output range specified as <minimum, maximum> > > + oneOf: > > This oneOf does not make sense. There is only one condition. Drop. > > > + - items: > > + - enum: [0, -10000000, -20000000, -30000000, -40000000, -55000000] > > + - enum: [10000000, 20000000, 30000000, 40000000, 55000000] > > What's the default? It's not a required property. > > > + > > + adi,range-microamp: > > + description: Current output range specified as <minimum, maximum> > > + oneOf: > > + - items: > > + - enum: [-50000, -100000, -300000, -500000, -1000000] > > I don't understand why 0 is not listed here. I'm not sure why it is a list at all. Seems like the hardware allows a continuous value so this should just specify max and min. > > > + - enum: [50000, 100000, 300000, 500000, 1000000] > > + - items: > > + - const: 0 > > + - enum: [50000, 100000, 300000, 500000, 1000000] > > + > > What's the default? It's not a required property. > > > + adi,max-millicelsius: > > + description: Overtemperature threshold > > + default: 50000 > > + minimum: 20000 > > + maximum: 150000 > > + > > + shutdown-reset-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 > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > Some supplies are for sure required. Devices rarely can operate without > power provided. > > > + > > +allOf: > > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > + > > +additionalProperties: false > > unevaluatedProperties instead. > > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 1/2] dt-bindings: iio: dac: add docs for ad8460 2024-09-07 17:01 ` Jonathan Cameron @ 2024-09-09 6:22 ` Tinaco, Mariel 2024-09-09 6:41 ` Krzysztof Kozlowski 0 siblings, 1 reply; 20+ messages in thread From: Tinaco, Mariel @ 2024-09-09 6:22 UTC (permalink / raw) To: Jonathan Cameron, Krzysztof Kozlowski Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá > -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, September 8, 2024 1:02 AM > To: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Tinaco, Mariel <Mariel.Tinaco@analog.com>; 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 v3 1/2] dt-bindings: iio: dac: add docs for ad8460 > > [External] > > On Wed, 4 Sep 2024 08:20:53 +0200 > Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > On Wed, Sep 04, 2024 at 10:30:39AM +0800, Mariel Tinaco wrote: > > > This adds the bindings documentation for the 14-bit > > > > Please do not use "This commit/patch/change", but imperative mood. See > > longer explanation here: > > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v5.17.1/s > > ource/Documentation/process/submitting- > patches.rst*L95__;Iw!!A3Ni8CS0y > > 2Y!7lj0hq-U2ClkGNYfHqjR3-k- > ea6TFUFsgEYQokkU95K6TXPHIPU33VxQcl_iH_etJ4k > > pbPEV39dP1oAd$ > > > > > 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 > > > > > + adi,range-microvolt: > > > + description: Voltage output range specified as <minimum, maximum> > > > + oneOf: > > > > This oneOf does not make sense. There is only one condition. Drop. > > > > > + - items: > > > + - enum: [0, -10000000, -20000000, -30000000, -40000000, - > 55000000] > > > + - enum: [10000000, 20000000, 30000000, 40000000, > > > + 55000000] > > > > What's the default? It's not a required property. > > > > > + > > > + adi,range-microamp: > > > + description: Current output range specified as <minimum, maximum> > > > + oneOf: > > > + - items: > > > + - enum: [-50000, -100000, -300000, -500000, -1000000] > > > > I don't understand why 0 is not listed here. > > I'm not sure why it is a list at all. Seems like the hardware allows a continuous > value so this should just specify max and min. > That's right, the values can be flexible but only at a certain range. The first element of the array should only be in the negative range, while The second element of the array should only be in the positive range. Is there a way to do this with the max and min attribute? Items: Item 1 min: -10000 max: 0 item 2 min: 0 max: 10000 > > > > > + - enum: [50000, 100000, 300000, 500000, 1000000] > > > + - items: > > > + - const: 0 > > > + - enum: [50000, 100000, 300000, 500000, 1000000] > > > + > > > > What's the default? It's not a required property. > > > > > + adi,max-millicelsius: > > > + description: Overtemperature threshold > > > + default: 50000 > > > + minimum: 20000 > > > + maximum: 150000 > > > + > > > + shutdown-reset-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 > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - clocks > > > > Some supplies are for sure required. Devices rarely can operate > > without power provided. > > > > > + > > > +allOf: > > > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > > + > > > +additionalProperties: false > > > > unevaluatedProperties instead. > > > > Best regards, > > Krzysztof > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: dac: add docs for ad8460 2024-09-09 6:22 ` Tinaco, Mariel @ 2024-09-09 6:41 ` Krzysztof Kozlowski 0 siblings, 0 replies; 20+ messages in thread From: Krzysztof Kozlowski @ 2024-09-09 6:41 UTC (permalink / raw) To: Tinaco, Mariel, Jonathan Cameron Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá On 09/09/2024 08:22, Tinaco, Mariel wrote: > > >> -----Original Message----- >> From: Jonathan Cameron <jic23@kernel.org> >> Sent: Sunday, September 8, 2024 1:02 AM >> To: Krzysztof Kozlowski <krzk@kernel.org> >> Cc: Tinaco, Mariel <Mariel.Tinaco@analog.com>; 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 v3 1/2] dt-bindings: iio: dac: add docs for ad8460 >> >> [External] >> >> On Wed, 4 Sep 2024 08:20:53 +0200 >> Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >>> On Wed, Sep 04, 2024 at 10:30:39AM +0800, Mariel Tinaco wrote: >>>> This adds the bindings documentation for the 14-bit >>> >>> Please do not use "This commit/patch/change", but imperative mood. See >>> longer explanation here: >>> https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v5.17.1/s >>> ource/Documentation/process/submitting- >> patches.rst*L95__;Iw!!A3Ni8CS0y >>> 2Y!7lj0hq-U2ClkGNYfHqjR3-k- >> ea6TFUFsgEYQokkU95K6TXPHIPU33VxQcl_iH_etJ4k >>> pbPEV39dP1oAd$ >>> >>>> 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 >>> >>>> + adi,range-microvolt: >>>> + description: Voltage output range specified as <minimum, maximum> >>>> + oneOf: >>> >>> This oneOf does not make sense. There is only one condition. Drop. >>> >>>> + - items: >>>> + - enum: [0, -10000000, -20000000, -30000000, -40000000, - >> 55000000] >>>> + - enum: [10000000, 20000000, 30000000, 40000000, >>>> + 55000000] >>> >>> What's the default? It's not a required property. >>> >>>> + >>>> + adi,range-microamp: >>>> + description: Current output range specified as <minimum, maximum> >>>> + oneOf: >>>> + - items: >>>> + - enum: [-50000, -100000, -300000, -500000, -1000000] >>> >>> I don't understand why 0 is not listed here. >> >> I'm not sure why it is a list at all. Seems like the hardware allows a continuous >> value so this should just specify max and min. >> > > That's right, the values can be flexible but only at a certain range. > The first element of the array should only be in the negative range, while > The second element of the array should only be in the positive range. > > Is there a way to do this with the max and min attribute? > > Items: > Item 1 > min: -10000 > max: 0 > item 2 > min: 0 > max: 10000 items: - minimum: -10000 maximum: 0 - minimum: 0 maximum: 10000 should work, try. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-04 2:30 [PATCH v3 0/2] Add support to AD8460 Waveform Generator DAC Mariel Tinaco 2024-09-04 2:30 ` [PATCH v3 1/2] dt-bindings: iio: dac: add docs for ad8460 Mariel Tinaco @ 2024-09-04 2:30 ` Mariel Tinaco 2024-09-04 15:50 ` kernel test robot ` (3 more replies) 2024-09-04 6:16 ` [PATCH v3 0/2] Add support to AD8460 Waveform Generator DAC Krzysztof Kozlowski 2 siblings, 4 replies; 20+ messages in thread From: Mariel Tinaco @ 2024-09-04 2:30 UTC (permalink / raw) To: linux-iio, devicetree, linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá The AD8460 is a “bits in, power out” high voltage, high-power, high-speed driver optimized for large output current (up to ±1 A) and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) into capacitive loads. A digital engine implements user-configurable features: modes for digital input, programmable supply current, and fault monitoring and programmable protection settings for output current, output voltage, and junction temperature. The AD8460 operates on high voltage dual supplies up to ±55 V and a single low voltage supply of 5 V. Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> --- MAINTAINERS | 1 + drivers/iio/dac/Kconfig | 13 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/ad8460.c | 932 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 947 insertions(+) create mode 100644 drivers/iio/dac/ad8460.c diff --git a/MAINTAINERS b/MAINTAINERS index e0509c9f5545..30746a3f0bbd 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1326,6 +1326,7 @@ 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 +F: drivers/iio/dac/ad8460.c ANALOG DEVICES INC AD9739a DRIVER M: Nuno Sa <nuno.sa@analog.com> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig index 1cfd7e2a622f..fa091995d002 100644 --- a/drivers/iio/dac/Kconfig +++ b/drivers/iio/dac/Kconfig @@ -301,6 +301,19 @@ config AD7303 To compile this driver as module choose M here: the module will be called ad7303. +config AD8460 + tristate "Analog Devices AD8460 DAC driver" + depends on SPI + select REGMAP_SPI + select IIO_BUFFER + select IIO_BUFFER_DMAENGINE + help + Say yes here to build support for Analog Devices AD8460 Digital to + Analog Converters (DAC). + + To compile this driver as a module choose M here: the module will be called + ad8460. + config AD8801 tristate "Analog Devices AD8801/AD8803 DAC driver" depends on SPI_MASTER diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile index 2cf148f16306..621d553bd6e3 100644 --- a/drivers/iio/dac/Makefile +++ b/drivers/iio/dac/Makefile @@ -28,6 +28,7 @@ obj-$(CONFIG_AD5686_SPI) += ad5686-spi.o obj-$(CONFIG_AD5696_I2C) += ad5696-i2c.o obj-$(CONFIG_AD7293) += ad7293.o obj-$(CONFIG_AD7303) += ad7303.o +obj-$(CONFIG_AD8460) += ad8460.o obj-$(CONFIG_AD8801) += ad8801.o obj-$(CONFIG_AD9739A) += ad9739a.o obj-$(CONFIG_ADI_AXI_DAC) += adi-axi-dac.o diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c new file mode 100644 index 000000000000..6428bfaf0bd7 --- /dev/null +++ b/drivers/iio/dac/ad8460.c @@ -0,0 +1,932 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * AD8460 Waveform generator DAC Driver + * + * Copyright (C) 2024 Analog Devices, Inc. + */ + +#include <linux/bitfield.h> +#include <linux/cleanup.h> +#include <linux/clk.h> +#include <linux/debugfs.h> +#include <linux/delay.h> +#include <linux/dmaengine.h> +#include <linux/gpio/consumer.h> +#include <linux/iio/buffer.h> +#include <linux/iio/buffer-dma.h> +#include <linux/iio/buffer-dmaengine.h> +#include <linux/iio/consumer.h> +#include <linux/iio/events.h> +#include <linux/iio/iio.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/spi/spi.h> + +#define AD8460_CTRL_REG(x) (x) +#define AD8460_HVDAC_DATA_WORD_LOW(x) (0x60 + (2 * (x))) +#define AD8460_HVDAC_DATA_WORD_HIGH(x) (0x61 + (2 * (x))) + +#define AD8460_HV_RESET_MSK BIT(7) +#define AD8460_HV_SLEEP_MSK BIT(4) +#define AD8460_WAVE_GEN_MODE_MSK BIT(0) + +#define AD8460_HVDAC_SLEEP_MSK BIT(3) + +#define AD8460_FAULT_ARM_MSK BIT(7) +#define AD8460_FAULT_LIMIT_MSK GENMASK(6, 0) + +#define AD8460_APG_MODE_ENABLE_MSK BIT(5) +#define AD8460_PATTERN_DEPTH_MSK GENMASK(3, 0) + +#define AD8460_QUIESCENT_CURRENT_MSK GENMASK(7, 0) + +#define AD8460_SHUTDOWN_FLAG_MSK BIT(7) + +#define AD8460_DATA_BYTE_LOW_MSK GENMASK(7, 0) +#define AD8460_DATA_BYTE_HIGH_MSK GENMASK(5, 0) +#define AD8460_DATA_BYTE_FULL_MSK GENMASK(13, 0) + +#define AD8460_DEFAULT_FAULT_PROTECT 0x00 +#define AD8460_DATA_BYTE_WORD_LENGTH 2 +#define AD8460_NUM_DATA_WORDS 16 +#define AD8460_NOMINAL_VOLTAGE_SPAN 80 +#define AD8460_MIN_EXT_RESISTOR_OHMS 2000 +#define AD8460_MAX_EXT_RESISTOR_OHMS 20000 +#define AD8460_MIN_VREFIO_UV 120000 +#define AD8460_MAX_VREFIO_UV 1200000 +#define AD8460_ABS_MAX_OVERVOLTAGE_UV 55000000 +#define AD8460_ABS_MAX_OVERCURRENT_UA 1000000 +#define AD8460_MAX_OVERTEMPERATURE_MC 150000 +#define AD8460_MIN_OVERTEMPERATURE_MC 20000 +#define AD8460_CURRENT_LIMIT_CONV(x) ((x) / 15625) +#define AD8460_VOLTAGE_LIMIT_CONV(x) ((x) / 1953000) +#define AD8460_TEMP_LIMIT_CONV(x) (((x) + 266640) / 6510) + +enum ad8460_fault_type { + AD8460_OVERCURRENT_SRC, + AD8460_OVERCURRENT_SNK, + AD8460_OVERVOLTAGE_POS, + AD8460_OVERVOLTAGE_NEG, + AD8460_OVERTEMPERATURE, +}; + +struct ad8460_state { + struct spi_device *spi; + struct regmap *regmap; + struct iio_channel *tmp_adc_channel; + struct clk *sync_clk; + /* lock to protect against multiple access to the device and shared data */ + struct mutex lock; + int refio_1p2v_mv; + u32 ext_resistor_ohms; + /* + * DMA (thus cache coherency maintenance) requires the + * transfer buffers to live in their own cache lines. + */ + u8 spi_tx_buf[2] __aligned(IIO_DMA_MINALIGN); +}; + +static int ad8460_hv_reset(struct ad8460_state *state) +{ + int ret; + + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00), + AD8460_HV_RESET_MSK, + FIELD_PREP(AD8460_HV_RESET_MSK, 1)); + if (ret) + return ret; + + fsleep(20); + + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00), + AD8460_HV_RESET_MSK, + FIELD_PREP(AD8460_HV_RESET_MSK, 0)); +} + +static int ad8460_reset(const struct ad8460_state *state) +{ + struct device *dev = &state->spi->dev; + struct gpio_desc *reset; + + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(reset)) + return dev_err_probe(dev, PTR_ERR(reset), + "Failed to get reset gpio"); + if (reset) { + /* minimum duration of 10ns */ + ndelay(10); + gpiod_set_value_cansleep(reset, 1); + return 0; + } + + /* bring all registers to their default state */ + return regmap_write(state->regmap, AD8460_CTRL_REG(0x03), 1); +} + +static int ad8460_enable_apg_mode(struct ad8460_state *state, int val) +{ + int ret; + + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x02), + AD8460_APG_MODE_ENABLE_MSK, + FIELD_PREP(AD8460_APG_MODE_ENABLE_MSK, val)); + if (ret) + return ret; + + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00), + AD8460_WAVE_GEN_MODE_MSK, + FIELD_PREP(AD8460_WAVE_GEN_MODE_MSK, val)); +} + +static int ad8460_read_shutdown_flag(struct ad8460_state *state, u64 *flag) +{ + int ret, val; + + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x0E), &val); + if (ret) + return ret; + + *flag = FIELD_GET(AD8460_SHUTDOWN_FLAG_MSK, val); + return 0; +} + +static int ad8460_get_hvdac_word(struct ad8460_state *state, int index, int *val) +{ + int ret; + + ret = regmap_bulk_read(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index), + &state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH); + if (ret) + return ret; + + *val = get_unaligned_le16(state->spi_tx_buf); + + return ret; +} + +static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val) +{ + put_unaligned_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val), + &state->spi_tx_buf); + + return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index), + state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH); +} + +static ssize_t ad8460_dac_input_read(struct iio_dev *indio_dev, uintptr_t private, + const struct iio_chan_spec *chan, char *buf) +{ + struct ad8460_state *state = iio_priv(indio_dev); + unsigned int reg; + int ret; + + ret = ad8460_get_hvdac_word(state, private, ®); + if (ret) + return ret; + + return sysfs_emit(buf, "%d\n", reg); +} + +static ssize_t ad8460_dac_input_write(struct iio_dev *indio_dev, uintptr_t private, + const struct iio_chan_spec *chan, + const char *buf, size_t len) +{ + struct ad8460_state *state = iio_priv(indio_dev); + unsigned int reg; + int ret; + + ret = kstrtou32(buf, 10, ®); + if (ret) + return ret; + + guard(mutex)(&state->lock); + + return ad8460_set_hvdac_word(state, private, reg); +} + +static ssize_t ad8460_read_symbol(struct iio_dev *indio_dev, uintptr_t private, + const struct iio_chan_spec *chan, char *buf) +{ + struct ad8460_state *state = iio_priv(indio_dev); + unsigned int reg; + int ret; + + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®); + if (ret) + return ret; + + return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_PATTERN_DEPTH_MSK, reg)); +} + +static ssize_t ad8460_write_symbol(struct iio_dev *indio_dev, uintptr_t private, + const struct iio_chan_spec *chan, + const char *buf, size_t len) +{ + struct ad8460_state *state = iio_priv(indio_dev); + uint16_t sym; + int ret; + + ret = kstrtou16(buf, 10, &sym); + if (ret) + return ret; + + guard(mutex)(&state->lock); + + return regmap_update_bits(state->regmap, + AD8460_CTRL_REG(0x02), + AD8460_PATTERN_DEPTH_MSK, + FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, sym)); +} + +static ssize_t ad8460_read_toggle_en(struct iio_dev *indio_dev, uintptr_t private, + const struct iio_chan_spec *chan, char *buf) +{ + struct ad8460_state *state = iio_priv(indio_dev); + unsigned int reg; + int ret; + + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®); + if (ret) + return ret; + + return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_APG_MODE_ENABLE_MSK, reg)); +} + +static ssize_t ad8460_write_toggle_en(struct iio_dev *indio_dev, uintptr_t private, + const struct iio_chan_spec *chan, + const char *buf, size_t len) +{ + struct ad8460_state *state = iio_priv(indio_dev); + bool toggle_en; + int ret; + + ret = kstrtobool(buf, &toggle_en); + if (ret) + return ret; + + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) + return ad8460_enable_apg_mode(state, toggle_en); + unreachable(); +} + +static ssize_t ad8460_read_powerdown(struct iio_dev *indio_dev, uintptr_t private, + const struct iio_chan_spec *chan, char *buf) +{ + struct ad8460_state *state = iio_priv(indio_dev); + unsigned int reg; + int ret; + + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x01), ®); + if (ret) + return ret; + + return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_HVDAC_SLEEP_MSK, reg)); +} + +static ssize_t ad8460_write_powerdown(struct iio_dev *indio_dev, uintptr_t private, + const struct iio_chan_spec *chan, + const char *buf, size_t len) +{ + struct ad8460_state *state = iio_priv(indio_dev); + bool pwr_down; + u64 sdn_flag; + int ret; + + ret = kstrtobool(buf, &pwr_down); + if (ret) + return ret; + + guard(mutex)(&state->lock); + + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), + AD8460_HVDAC_SLEEP_MSK, + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, pwr_down)); + if (ret) + return ret; + + if (!pwr_down) { + ret = ad8460_read_shutdown_flag(state, &sdn_flag); + if (ret) + return ret; + + if (sdn_flag) { + ret = ad8460_hv_reset(state); + if (ret) + return ret; + } + } + + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00), + AD8460_HV_SLEEP_MSK, + FIELD_PREP(AD8460_HV_SLEEP_MSK, !pwr_down)); + if (ret) + return ret; + + return len; +} + +static const char * const ad8460_powerdown_modes[] = { + "three_state", +}; + +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan) +{ + return 0; +} + +static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + unsigned int type) +{ + return 0; +} + +static int ad8460_set_sample(struct ad8460_state *state, int val) +{ + int ret; + + ret = ad8460_enable_apg_mode(state, 1); + if (ret) + return ret; + + guard(mutex)(&state->lock); + ret = ad8460_set_hvdac_word(state, 0, val); + if (ret) + return ret; + + return regmap_update_bits(state->regmap, + AD8460_CTRL_REG(0x02), + AD8460_PATTERN_DEPTH_MSK, + FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0)); +} + +static int ad8460_set_fault_threshold(struct ad8460_state *state, + enum ad8460_fault_type fault, + unsigned int threshold) +{ + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08 + fault), + AD8460_FAULT_LIMIT_MSK, + FIELD_PREP(AD8460_FAULT_LIMIT_MSK, threshold)); +} + +static int ad8460_get_fault_threshold(struct ad8460_state *state, + enum ad8460_fault_type fault, + unsigned int *threshold) +{ + unsigned int val; + int ret; + + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault), &val); + if (ret) + return ret; + + *threshold = FIELD_GET(AD8460_FAULT_LIMIT_MSK, val); + + return ret; +} + +static int ad8460_set_fault_threshold_en(struct ad8460_state *state, + enum ad8460_fault_type fault, bool en) +{ + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08 + fault), + AD8460_FAULT_ARM_MSK, + FIELD_PREP(AD8460_FAULT_ARM_MSK, en)); +} + +static int ad8460_get_fault_threshold_en(struct ad8460_state *state, + enum ad8460_fault_type fault, bool *en) +{ + unsigned int val; + int ret; + + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault), &val); + if (ret) + return ret; + + *en = FIELD_GET(AD8460_FAULT_ARM_MSK, val); + + return 0; +} + +static int ad8460_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, int val2, + long mask) +{ + struct ad8460_state *state = iio_priv(indio_dev); + + switch (mask) { + case IIO_CHAN_INFO_RAW: + switch (chan->type) { + case IIO_VOLTAGE: + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) + return ad8460_set_sample(state, val); + unreachable(); + case IIO_CURRENT: + return regmap_write(state->regmap, AD8460_CTRL_REG(0x04), + FIELD_PREP(AD8460_QUIESCENT_CURRENT_MSK, val)); + default: + return -EINVAL; + } + default: + return -EINVAL; + } +} + +static int ad8460_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct ad8460_state *state = iio_priv(indio_dev); + int data, ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + switch (chan->type) { + case IIO_VOLTAGE: + scoped_guard(mutex, &state->lock) { + ret = ad8460_get_hvdac_word(state, 0, &data); + if (ret) + return ret; + } + *val = data; + return IIO_VAL_INT; + case IIO_CURRENT: + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x04), + &data); + if (ret) + return ret; + *val = data; + return IIO_VAL_INT; + case IIO_TEMP: + ret = iio_read_channel_raw(state->tmp_adc_channel, &data); + if (ret) + return ret; + *val = data; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_SAMP_FREQ: + *val = clk_get_rate(state->sync_clk); + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + /* + * vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V + * vMAX = vNOMINAL_SPAN * (2**14 / 2**14) - 40V + * vMIN = vNOMINAL_SPAN * (0 / 2**14) - 40V + * vADJ = vCONV * (2000 / rSET) * (vREF / 1.2) + * vSPAN = vADJ_MAX - vADJ_MIN + * See datasheet page 49, section FULL-SCALE REDUCTION + */ + *val = AD8460_NOMINAL_VOLTAGE_SPAN * 2000 * state->refio_1p2v_mv; + *val2 = state->ext_resistor_ohms * 1200; + return IIO_VAL_FRACTIONAL; + default: + return -EINVAL; + } +} + +static int ad8460_select_fault_type(int chan_type, enum iio_event_direction dir) +{ + switch (chan_type) { + case IIO_VOLTAGE: + switch (dir) { + case IIO_EV_DIR_RISING: + return AD8460_OVERVOLTAGE_POS; + case IIO_EV_DIR_FALLING: + return AD8460_OVERVOLTAGE_NEG; + default: + return -EINVAL; + } + case IIO_CURRENT: + switch (dir) { + case IIO_EV_DIR_RISING: + return AD8460_OVERCURRENT_SRC; + case IIO_EV_DIR_FALLING: + return AD8460_OVERCURRENT_SNK; + default: + return -EINVAL; + } + case IIO_TEMP: + switch (dir) { + case IIO_EV_DIR_RISING: + return AD8460_OVERTEMPERATURE; + default: + return -EINVAL; + } + default: + return -EINVAL; + } +} + +static int ad8460_write_event_value(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, int val, int val2) +{ + struct ad8460_state *state = iio_priv(indio_dev); + unsigned int fault; + + if (type != IIO_EV_TYPE_THRESH) + return -EINVAL; + + if (info != IIO_EV_INFO_VALUE) + return -EINVAL; + + fault = ad8460_select_fault_type(chan->type, dir); + if (fault < 0) + return fault; + + return ad8460_set_fault_threshold(state, fault, val); +} + +static int ad8460_read_event_value(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, int *val, int *val2) +{ + struct ad8460_state *state = iio_priv(indio_dev); + unsigned int fault; + + if (type != IIO_EV_TYPE_THRESH) + return -EINVAL; + + if (info != IIO_EV_INFO_VALUE) + return -EINVAL; + + fault = ad8460_select_fault_type(chan->type, dir); + if (fault < 0) + return fault; + + return ad8460_get_fault_threshold(state, fault, val); +} + +static int ad8460_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, int val) +{ + struct ad8460_state *state = iio_priv(indio_dev); + unsigned int fault; + + if (type != IIO_EV_TYPE_THRESH) + return -EINVAL; + + fault = ad8460_select_fault_type(chan->type, dir); + if (fault < 0) + return fault; + + return ad8460_set_fault_threshold_en(state, fault, val); +} + +static int ad8460_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + struct ad8460_state *state = iio_priv(indio_dev); + unsigned int fault; + bool en; + int ret; + + if (type != IIO_EV_TYPE_THRESH) + return -EINVAL; + + fault = ad8460_select_fault_type(chan->type, dir); + if (fault < 0) + return fault; + + ret = ad8460_get_fault_threshold_en(state, fault, &en); + if (ret) + return ret; + + return en; +} + +static int ad8460_reg_access(struct iio_dev *indio_dev, unsigned int reg, + unsigned int writeval, unsigned int *readval) +{ + struct ad8460_state *state = iio_priv(indio_dev); + + if (readval) + return regmap_read(state->regmap, reg, readval); + + return regmap_write(state->regmap, reg, writeval); +} + +static int ad8460_buffer_preenable(struct iio_dev *indio_dev) +{ + struct ad8460_state *state = iio_priv(indio_dev); + + return ad8460_enable_apg_mode(state, 0); +} + +static int ad8460_buffer_postdisable(struct iio_dev *indio_dev) +{ + struct ad8460_state *state = iio_priv(indio_dev); + + return ad8460_enable_apg_mode(state, 1); +} + +static const struct iio_buffer_setup_ops ad8460_buffer_setup_ops = { + .preenable = &ad8460_buffer_preenable, + .postdisable = &ad8460_buffer_postdisable, +}; + +static const struct iio_info ad8460_info = { + .read_raw = &ad8460_read_raw, + .write_raw = &ad8460_write_raw, + .write_event_value = &ad8460_write_event_value, + .read_event_value = &ad8460_read_event_value, + .write_event_config = &ad8460_write_event_config, + .read_event_config = &ad8460_read_event_config, + .debugfs_reg_access = &ad8460_reg_access, +}; + +static const struct iio_enum ad8460_powerdown_mode_enum = { + .items = ad8460_powerdown_modes, + .num_items = ARRAY_SIZE(ad8460_powerdown_modes), + .get = ad8460_get_powerdown_mode, + .set = ad8460_set_powerdown_mode, +}; + +#define AD8460_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) { \ + .name = _name, \ + .read = (_read), \ + .write = (_write), \ + .private = (_what), \ + .shared = (_shared), \ +} + +static struct iio_chan_spec_ext_info ad8460_ext_info[] = { + AD8460_CHAN_EXT_INFO("raw0", 0, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw1", 1, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw2", 2, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw3", 3, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw4", 4, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw5", 5, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw6", 6, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw7", 7, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw8", 8, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw9", 9, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw10", 10, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw11", 11, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw12", 12, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw13", 13, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw14", 14, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("raw15", 15, IIO_SEPARATE, ad8460_dac_input_read, + ad8460_dac_input_write), + AD8460_CHAN_EXT_INFO("toggle_en", 0, IIO_SEPARATE, ad8460_read_toggle_en, + ad8460_write_toggle_en), + AD8460_CHAN_EXT_INFO("symbol", 0, IIO_SEPARATE, ad8460_read_symbol, + ad8460_write_symbol), + AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE, ad8460_read_powerdown, + ad8460_write_powerdown), + IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad8460_powerdown_mode_enum), + IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE, + &ad8460_powerdown_mode_enum), + {} +}; + +static const struct iio_event_spec ad8460_events[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_separate = BIT(IIO_EV_INFO_VALUE) | + BIT(IIO_EV_INFO_ENABLE), + }, + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_FALLING, + .mask_separate = BIT(IIO_EV_INFO_VALUE) | + BIT(IIO_EV_INFO_ENABLE), + }, +}; + +#define AD8460_VOLTAGE_CHAN { \ + .type = IIO_VOLTAGE, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ + BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .output = 1, \ + .indexed = 1, \ + .channel = 0, \ + .scan_index = 0, \ + .scan_type = { \ + .sign = 'u', \ + .realbits = 14, \ + .storagebits = 16, \ + .endianness = IIO_LE, \ + }, \ + .ext_info = ad8460_ext_info, \ + .event_spec = ad8460_events, \ + .num_event_specs = ARRAY_SIZE(ad8460_events), \ +} + +#define AD8460_CURRENT_CHAN { \ + .type = IIO_CURRENT, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .output = 0, \ + .indexed = 1, \ + .channel = 0, \ + .scan_index = -1, \ + .event_spec = ad8460_events, \ + .num_event_specs = ARRAY_SIZE(ad8460_events), \ +} + +#define AD8460_TEMP_CHAN { \ + .type = IIO_TEMP, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .output = 1, \ + .indexed = 1, \ + .channel = 0, \ + .scan_index = -1, \ + .event_spec = ad8460_events, \ + .num_event_specs = 1, \ +} + +static const struct iio_chan_spec ad8460_channels[] = { + AD8460_VOLTAGE_CHAN, + AD8460_CURRENT_CHAN, +}; + +static const struct iio_chan_spec ad8460_channels_with_tmp_adc[] = { + AD8460_VOLTAGE_CHAN, + AD8460_CURRENT_CHAN, + AD8460_TEMP_CHAN, +}; + +static const struct regmap_config ad8460_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = 0x7F, +}; + +static int ad8460_probe(struct spi_device *spi) +{ + struct ad8460_state *state; + struct iio_dev *indio_dev; + struct device *dev; + u32 tmp[2], temp; + int ret; + + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state)); + if (!indio_dev) + return -ENOMEM; + + state = iio_priv(indio_dev); + mutex_init(&state->lock); + + indio_dev->name = "ad8460"; + indio_dev->info = &ad8460_info; + + state->spi = spi; + dev = &spi->dev; + + state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config); + if (IS_ERR(state->regmap)) + return dev_err_probe(dev, PTR_ERR(state->regmap), + "Failed to initialize regmap"); + + state->sync_clk = devm_clk_get_enabled(dev, NULL); + if (IS_ERR(state->sync_clk)) + return dev_err_probe(dev, PTR_ERR(state->sync_clk), + "Failed to get sync clk\n"); + + state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460-tmp"); + if (IS_ERR_OR_NULL(state->tmp_adc_channel)) { + indio_dev->channels = ad8460_channels; + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels); + } else { + indio_dev->channels = ad8460_channels_with_tmp_adc; + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels_with_tmp_adc); + } + + ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v"); + if (ret == -ENODEV) + state->refio_1p2v_mv = 1200; + else if (ret >= 0) + state->refio_1p2v_mv = ret / 1000; + else + return dev_err_probe(dev, ret, "Failed to get reference voltage\n"); + + if (!in_range(state->refio_1p2v_mv, AD8460_MIN_VREFIO_UV, + AD8460_MAX_VREFIO_UV)) + return dev_err_probe(dev, -EINVAL, + "Invalid ref voltage range(%u mV) [%u mV, %u mV]\n", + state->refio_1p2v_mv, + AD8460_MIN_VREFIO_UV / 1000, + AD8460_MAX_VREFIO_UV / 1000); + + ret = device_property_read_u32(dev, "adi,external-resistor-ohms", + &state->ext_resistor_ohms); + if (ret) + state->ext_resistor_ohms = 2000; + else if (!in_range(state->ext_resistor_ohms, AD8460_MIN_EXT_RESISTOR_OHMS, + AD8460_MAX_EXT_RESISTOR_OHMS)) + return dev_err_probe(dev, -EINVAL, + "Invalid resistor set range(%u) [%u, %u]\n", + state->ext_resistor_ohms, + AD8460_MIN_EXT_RESISTOR_OHMS, + AD8460_MAX_EXT_RESISTOR_OHMS); + + /* Arm the device by default */ + ret = device_property_read_u32_array(dev, "adi,range-microamp", + tmp, ARRAY_SIZE(tmp)); + if (!ret) { + if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERCURRENT_UA)) + regmap_write(state->regmap, AD8460_CTRL_REG(0x08), + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | + AD8460_CURRENT_LIMIT_CONV(tmp[1])); + + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERCURRENT_UA, 0)) + regmap_write(state->regmap, AD8460_CTRL_REG(0x09), + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | + AD8460_CURRENT_LIMIT_CONV(abs(tmp[0]))); + } + + ret = device_property_read_u32_array(dev, "adi,range-microvolt", + tmp, ARRAY_SIZE(tmp)); + if (!ret) { + if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERVOLTAGE_UV)) + regmap_write(state->regmap, AD8460_CTRL_REG(0x0A), + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | + AD8460_VOLTAGE_LIMIT_CONV(tmp[1])); + + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERVOLTAGE_UV, 0)) + regmap_write(state->regmap, AD8460_CTRL_REG(0x0B), + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | + AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0]))); + } + + ret = device_property_read_u32(dev, "adi,max-millicelsius", &temp); + if (!ret) { + if (in_range(temp, AD8460_MIN_OVERTEMPERATURE_MC, + AD8460_MAX_OVERTEMPERATURE_MC)) + regmap_write(state->regmap, AD8460_CTRL_REG(0x0C), + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | + AD8460_TEMP_LIMIT_CONV(abs(temp))); + } + + ret = ad8460_reset(state); + if (ret) + return ret; + + /* Enables DAC by default */ + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), + AD8460_HVDAC_SLEEP_MSK, + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0)); + if (ret) + return ret; + + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->setup_ops = &ad8460_buffer_setup_ops; + + ret = devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev, "tx", + IIO_BUFFER_DIRECTION_OUT); + if (ret) + return dev_err_probe(dev, ret, + "Failed to get DMA buffer\n"); + + return devm_iio_device_register(dev, indio_dev); +} + +static const struct of_device_id ad8460_of_match[] = { + { .compatible = "adi, ad8460" }, + { } +}; +MODULE_DEVICE_TABLE(of, ad8460_of_match); + +static struct spi_driver ad8460_driver = { + .driver = { + .name = "ad8460", + .of_match_table = ad8460_of_match, + }, + .probe = ad8460_probe, +}; +module_spi_driver(ad8460_driver); + +MODULE_AUTHOR("Mariel Tinaco <mariel.tinaco@analog.com"); +MODULE_DESCRIPTION("AD8460 DAC driver"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(IIO_DMAENGINE_BUFFER); -- 2.34.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-04 2:30 ` [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco @ 2024-09-04 15:50 ` kernel test robot 2024-09-04 17:23 ` kernel test robot ` (2 subsequent siblings) 3 siblings, 0 replies; 20+ messages in thread From: kernel test robot @ 2024-09-04 15:50 UTC (permalink / raw) To: Mariel Tinaco, linux-iio, devicetree, linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá Cc: oe-kbuild-all Hi Mariel, kernel test robot noticed the following build errors: [auto build test ERROR on c4b43d8336e52dce6d124e428aa3b71703e62647] url: https://github.com/intel-lab-lkp/linux/commits/Mariel-Tinaco/dt-bindings-iio-dac-add-docs-for-ad8460/20240904-103413 base: c4b43d8336e52dce6d124e428aa3b71703e62647 patch link: https://lore.kernel.org/r/20240904023040.23352-3-Mariel.Tinaco%40analog.com patch subject: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240904/202409042318.7nwvZc3c-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240904/202409042318.7nwvZc3c-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409042318.7nwvZc3c-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/iio/dac/ad8460.c: In function 'ad8460_get_hvdac_word': >> drivers/iio/dac/ad8460.c:165:16: error: implicit declaration of function 'get_unaligned_le16' [-Wimplicit-function-declaration] 165 | *val = get_unaligned_le16(state->spi_tx_buf); | ^~~~~~~~~~~~~~~~~~ drivers/iio/dac/ad8460.c: In function 'ad8460_set_hvdac_word': >> drivers/iio/dac/ad8460.c:172:9: error: implicit declaration of function 'put_unaligned_le16' [-Wimplicit-function-declaration] 172 | put_unaligned_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val), | ^~~~~~~~~~~~~~~~~~ vim +/get_unaligned_le16 +165 drivers/iio/dac/ad8460.c 155 156 static int ad8460_get_hvdac_word(struct ad8460_state *state, int index, int *val) 157 { 158 int ret; 159 160 ret = regmap_bulk_read(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index), 161 &state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH); 162 if (ret) 163 return ret; 164 > 165 *val = get_unaligned_le16(state->spi_tx_buf); 166 167 return ret; 168 } 169 170 static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val) 171 { > 172 put_unaligned_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val), 173 &state->spi_tx_buf); 174 175 return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index), 176 state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH); 177 } 178 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-04 2:30 ` [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco 2024-09-04 15:50 ` kernel test robot @ 2024-09-04 17:23 ` kernel test robot 2024-09-06 0:50 ` David Lechner 2024-09-07 17:14 ` Jonathan Cameron 3 siblings, 0 replies; 20+ messages in thread From: kernel test robot @ 2024-09-04 17:23 UTC (permalink / raw) To: Mariel Tinaco, linux-iio, devicetree, linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá Cc: llvm, oe-kbuild-all Hi Mariel, kernel test robot noticed the following build errors: [auto build test ERROR on c4b43d8336e52dce6d124e428aa3b71703e62647] url: https://github.com/intel-lab-lkp/linux/commits/Mariel-Tinaco/dt-bindings-iio-dac-add-docs-for-ad8460/20240904-103413 base: c4b43d8336e52dce6d124e428aa3b71703e62647 patch link: https://lore.kernel.org/r/20240904023040.23352-3-Mariel.Tinaco%40analog.com patch subject: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240905/202409050100.CGStaazP-lkp@intel.com/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240905/202409050100.CGStaazP-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409050100.CGStaazP-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/iio/dac/ad8460.c:165:9: error: call to undeclared function 'get_unaligned_le16'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 165 | *val = get_unaligned_le16(state->spi_tx_buf); | ^ >> drivers/iio/dac/ad8460.c:172:2: error: call to undeclared function 'put_unaligned_le16'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 172 | put_unaligned_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val), | ^ 2 errors generated. vim +/get_unaligned_le16 +165 drivers/iio/dac/ad8460.c 155 156 static int ad8460_get_hvdac_word(struct ad8460_state *state, int index, int *val) 157 { 158 int ret; 159 160 ret = regmap_bulk_read(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index), 161 &state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH); 162 if (ret) 163 return ret; 164 > 165 *val = get_unaligned_le16(state->spi_tx_buf); 166 167 return ret; 168 } 169 170 static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val) 171 { > 172 put_unaligned_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val), 173 &state->spi_tx_buf); 174 175 return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index), 176 state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH); 177 } 178 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-04 2:30 ` [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco 2024-09-04 15:50 ` kernel test robot 2024-09-04 17:23 ` kernel test robot @ 2024-09-06 0:50 ` David Lechner 2024-09-09 9:47 ` Tinaco, Mariel 2024-09-10 1:44 ` Tinaco, Mariel 2024-09-07 17:14 ` Jonathan Cameron 3 siblings, 2 replies; 20+ messages in thread From: David Lechner @ 2024-09-06 0:50 UTC (permalink / raw) To: Mariel Tinaco, linux-iio, devicetree, linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, Nuno Sá On 9/3/24 9:30 PM, Mariel Tinaco wrote: > The AD8460 is a “bits in, power out” high voltage, high-power, > high-speed driver optimized for large output current (up to ±1 A) > and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) > into capacitive loads. > > A digital engine implements user-configurable features: modes for > digital input, programmable supply current, and fault monitoring > and programmable protection settings for output current, > output voltage, and junction temperature. The AD8460 operates on > high voltage dual supplies up to ±55 V and a single low voltage > supply of 5 V. > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> > --- > MAINTAINERS | 1 + > drivers/iio/dac/Kconfig | 13 + > drivers/iio/dac/Makefile | 1 + > drivers/iio/dac/ad8460.c | 932 +++++++++++++++++++++++++++++++++++++++ > 4 files changed, 947 insertions(+) > create mode 100644 drivers/iio/dac/ad8460.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index e0509c9f5545..30746a3f0bbd 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1326,6 +1326,7 @@ 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 > +F: drivers/iio/dac/ad8460.c > > ANALOG DEVICES INC AD9739a DRIVER > M: Nuno Sa <nuno.sa@analog.com> > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig > index 1cfd7e2a622f..fa091995d002 100644 > --- a/drivers/iio/dac/Kconfig > +++ b/drivers/iio/dac/Kconfig > @@ -301,6 +301,19 @@ config AD7303 > To compile this driver as module choose M here: the module will be called > ad7303. > > +config AD8460 > + tristate "Analog Devices AD8460 DAC driver" > + depends on SPI > + select REGMAP_SPI > + select IIO_BUFFER > + select IIO_BUFFER_DMAENGINE > + help > + Say yes here to build support for Analog Devices AD8460 Digital to > + Analog Converters (DAC). > + > + To compile this driver as a module choose M here: the module will be called > + ad8460. > + > config AD8801 > tristate "Analog Devices AD8801/AD8803 DAC driver" > depends on SPI_MASTER > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > index 2cf148f16306..621d553bd6e3 100644 > --- a/drivers/iio/dac/Makefile > +++ b/drivers/iio/dac/Makefile > @@ -28,6 +28,7 @@ obj-$(CONFIG_AD5686_SPI) += ad5686-spi.o > obj-$(CONFIG_AD5696_I2C) += ad5696-i2c.o > obj-$(CONFIG_AD7293) += ad7293.o > obj-$(CONFIG_AD7303) += ad7303.o > +obj-$(CONFIG_AD8460) += ad8460.o > obj-$(CONFIG_AD8801) += ad8801.o > obj-$(CONFIG_AD9739A) += ad9739a.o > obj-$(CONFIG_ADI_AXI_DAC) += adi-axi-dac.o > diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c > new file mode 100644 > index 000000000000..6428bfaf0bd7 > --- /dev/null > +++ b/drivers/iio/dac/ad8460.c > @@ -0,0 +1,932 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * AD8460 Waveform generator DAC Driver > + * > + * Copyright (C) 2024 Analog Devices, Inc. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/cleanup.h> > +#include <linux/clk.h> > +#include <linux/debugfs.h> > +#include <linux/delay.h> > +#include <linux/dmaengine.h> > +#include <linux/gpio/consumer.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/buffer-dma.h> > +#include <linux/iio/buffer-dmaengine.h> > +#include <linux/iio/consumer.h> > +#include <linux/iio/events.h> > +#include <linux/iio/iio.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/spi/spi.h> > + > +#define AD8460_CTRL_REG(x) (x) > +#define AD8460_HVDAC_DATA_WORD_LOW(x) (0x60 + (2 * (x))) > +#define AD8460_HVDAC_DATA_WORD_HIGH(x) (0x61 + (2 * (x))) I don't see AD8460_HVDAC_DATA_WORD_HIGH() used anywhere, so we could remove that and rename AD8460_HVDAC_DATA_WORD_LOW() to AD8460_HVDAC_DATA_WORD(). > + > +#define AD8460_HV_RESET_MSK BIT(7) > +#define AD8460_HV_SLEEP_MSK BIT(4) > +#define AD8460_WAVE_GEN_MODE_MSK BIT(0) > + > +#define AD8460_HVDAC_SLEEP_MSK BIT(3) > + > +#define AD8460_FAULT_ARM_MSK BIT(7) > +#define AD8460_FAULT_LIMIT_MSK GENMASK(6, 0) > + > +#define AD8460_APG_MODE_ENABLE_MSK BIT(5) > +#define AD8460_PATTERN_DEPTH_MSK GENMASK(3, 0) > + > +#define AD8460_QUIESCENT_CURRENT_MSK GENMASK(7, 0) > + > +#define AD8460_SHUTDOWN_FLAG_MSK BIT(7) > + > +#define AD8460_DATA_BYTE_LOW_MSK GENMASK(7, 0) > +#define AD8460_DATA_BYTE_HIGH_MSK GENMASK(5, 0) > +#define AD8460_DATA_BYTE_FULL_MSK GENMASK(13, 0) > + > +#define AD8460_DEFAULT_FAULT_PROTECT 0x00 > +#define AD8460_DATA_BYTE_WORD_LENGTH 2 > +#define AD8460_NUM_DATA_WORDS 16 > +#define AD8460_NOMINAL_VOLTAGE_SPAN 80 > +#define AD8460_MIN_EXT_RESISTOR_OHMS 2000 > +#define AD8460_MAX_EXT_RESISTOR_OHMS 20000 > +#define AD8460_MIN_VREFIO_UV 120000 > +#define AD8460_MAX_VREFIO_UV 1200000 > +#define AD8460_ABS_MAX_OVERVOLTAGE_UV 55000000 > +#define AD8460_ABS_MAX_OVERCURRENT_UA 1000000 > +#define AD8460_MAX_OVERTEMPERATURE_MC 150000 > +#define AD8460_MIN_OVERTEMPERATURE_MC 20000 > +#define AD8460_CURRENT_LIMIT_CONV(x) ((x) / 15625) > +#define AD8460_VOLTAGE_LIMIT_CONV(x) ((x) / 1953000) > +#define AD8460_TEMP_LIMIT_CONV(x) (((x) + 266640) / 6510) It looks like there are issues with mixed tabs and spaces in the macros above that should be cleaned up. > + > +enum ad8460_fault_type { > + AD8460_OVERCURRENT_SRC, > + AD8460_OVERCURRENT_SNK, > + AD8460_OVERVOLTAGE_POS, > + AD8460_OVERVOLTAGE_NEG, > + AD8460_OVERTEMPERATURE, > +}; > + > +struct ad8460_state { > + struct spi_device *spi; > + struct regmap *regmap; > + struct iio_channel *tmp_adc_channel; > + struct clk *sync_clk; > + /* lock to protect against multiple access to the device and shared data */ > + struct mutex lock; > + int refio_1p2v_mv; > + u32 ext_resistor_ohms; > + /* > + * DMA (thus cache coherency maintenance) requires the > + * transfer buffers to live in their own cache lines. > + */ > + u8 spi_tx_buf[2] __aligned(IIO_DMA_MINALIGN); If we make this `__le16 spi_tx_buf` instead of `u8 spi_tx_buf[2]`, then we don't need the unaligned access functions. > +}; > + > +static int ad8460_hv_reset(struct ad8460_state *state) > +{ > + int ret; > + > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00), > + AD8460_HV_RESET_MSK, > + FIELD_PREP(AD8460_HV_RESET_MSK, 1)); Can be simplified with regmap_set_bits(). > + if (ret) > + return ret; > + > + fsleep(20); > + > + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00), > + AD8460_HV_RESET_MSK, > + FIELD_PREP(AD8460_HV_RESET_MSK, 0)); and regmap_clear_bits(). > +} > + > +static int ad8460_reset(const struct ad8460_state *state) > +{ > + struct device *dev = &state->spi->dev; > + struct gpio_desc *reset; > + > + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(reset)) > + return dev_err_probe(dev, PTR_ERR(reset), > + "Failed to get reset gpio"); > + if (reset) { > + /* minimum duration of 10ns */ > + ndelay(10); > + gpiod_set_value_cansleep(reset, 1); > + return 0; > + } > + > + /* bring all registers to their default state */ > + return regmap_write(state->regmap, AD8460_CTRL_REG(0x03), 1); > +} > + > +static int ad8460_enable_apg_mode(struct ad8460_state *state, int val) > +{ > + int ret; > + > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x02), > + AD8460_APG_MODE_ENABLE_MSK, > + FIELD_PREP(AD8460_APG_MODE_ENABLE_MSK, val)); > + if (ret) > + return ret; > + > + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00), > + AD8460_WAVE_GEN_MODE_MSK, > + FIELD_PREP(AD8460_WAVE_GEN_MODE_MSK, val)); > +} > + > +static int ad8460_read_shutdown_flag(struct ad8460_state *state, u64 *flag) > +{ > + int ret, val; > + > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x0E), &val); > + if (ret) > + return ret; > + > + *flag = FIELD_GET(AD8460_SHUTDOWN_FLAG_MSK, val); > + return 0; > +} > + > +static int ad8460_get_hvdac_word(struct ad8460_state *state, int index, int *val) > +{ > + int ret; > + > + ret = regmap_bulk_read(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index), > + &state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH); > + if (ret) > + return ret; > + > + *val = get_unaligned_le16(state->spi_tx_buf); With spi_tx_buf changed to __le16, this can use le16_to_cpu() instead of get_unaligned_le16(). > + > + return ret; > +} > + > +static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val) > +{ > + put_unaligned_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val), > + &state->spi_tx_buf); > + > + return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index), > + state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH); > +} > + > +static ssize_t ad8460_dac_input_read(struct iio_dev *indio_dev, uintptr_t private, > + const struct iio_chan_spec *chan, char *buf) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + unsigned int reg; > + int ret; > + > + ret = ad8460_get_hvdac_word(state, private, ®); > + if (ret) > + return ret; > + > + return sysfs_emit(buf, "%d\n", reg); %u instead of %d since reg is unsigned > +} > + > +static ssize_t ad8460_dac_input_write(struct iio_dev *indio_dev, uintptr_t private, > + const struct iio_chan_spec *chan, > + const char *buf, size_t len) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + unsigned int reg; > + int ret; > + > + ret = kstrtou32(buf, 10, ®); > + if (ret) > + return ret; > + > + guard(mutex)(&state->lock); > + > + return ad8460_set_hvdac_word(state, private, reg); > +} > + > +static ssize_t ad8460_read_symbol(struct iio_dev *indio_dev, uintptr_t private, > + const struct iio_chan_spec *chan, char *buf) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + unsigned int reg; > + int ret; > + > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®); > + if (ret) > + return ret; > + > + return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_PATTERN_DEPTH_MSK, reg)); > +} I guess FIELD_GET() makes it long? but probalby should be %lu. > + > +static ssize_t ad8460_write_symbol(struct iio_dev *indio_dev, uintptr_t private, > + const struct iio_chan_spec *chan, > + const char *buf, size_t len) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + uint16_t sym; > + int ret; > + > + ret = kstrtou16(buf, 10, &sym); > + if (ret) > + return ret; > + > + guard(mutex)(&state->lock); > + > + return regmap_update_bits(state->regmap, > + AD8460_CTRL_REG(0x02), > + AD8460_PATTERN_DEPTH_MSK, > + FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, sym)); > +} > + > +static ssize_t ad8460_read_toggle_en(struct iio_dev *indio_dev, uintptr_t private, > + const struct iio_chan_spec *chan, char *buf) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + unsigned int reg; > + int ret; > + > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®); > + if (ret) > + return ret; > + > + return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_APG_MODE_ENABLE_MSK, reg)); > +} > + > +static ssize_t ad8460_write_toggle_en(struct iio_dev *indio_dev, uintptr_t private, > + const struct iio_chan_spec *chan, > + const char *buf, size_t len) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + bool toggle_en; > + int ret; > + > + ret = kstrtobool(buf, &toggle_en); > + if (ret) > + return ret; > + > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) > + return ad8460_enable_apg_mode(state, toggle_en); > + unreachable(); Hmm... do we need to make an unscoped version of iio_device_claim_direct_scoped()? > +} > + > +static ssize_t ad8460_read_powerdown(struct iio_dev *indio_dev, uintptr_t private, > + const struct iio_chan_spec *chan, char *buf) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + unsigned int reg; > + int ret; > + > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x01), ®); > + if (ret) > + return ret; > + > + return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_HVDAC_SLEEP_MSK, reg)); > +} > + > +static ssize_t ad8460_write_powerdown(struct iio_dev *indio_dev, uintptr_t private, > + const struct iio_chan_spec *chan, > + const char *buf, size_t len) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + bool pwr_down; > + u64 sdn_flag; > + int ret; > + > + ret = kstrtobool(buf, &pwr_down); > + if (ret) > + return ret; > + > + guard(mutex)(&state->lock); > + > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), > + AD8460_HVDAC_SLEEP_MSK, > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, pwr_down)); > + if (ret) > + return ret; > + > + if (!pwr_down) { > + ret = ad8460_read_shutdown_flag(state, &sdn_flag); > + if (ret) > + return ret; > + > + if (sdn_flag) { > + ret = ad8460_hv_reset(state); > + if (ret) > + return ret; > + } > + } > + > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00), > + AD8460_HV_SLEEP_MSK, > + FIELD_PREP(AD8460_HV_SLEEP_MSK, !pwr_down)); > + if (ret) > + return ret; Some comments explaining the logic in this function would be helpful. Without reading the datasheet, it looks like this puts it in powerdown mode and takes it right back out before returning. > + > + return len; > +} > + > +static const char * const ad8460_powerdown_modes[] = { > + "three_state", > +}; > + > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan) > +{ > + return 0; > +} > + > +static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + unsigned int type) > +{ > + return 0; > +} > + > +static int ad8460_set_sample(struct ad8460_state *state, int val) > +{ > + int ret; > + > + ret = ad8460_enable_apg_mode(state, 1); > + if (ret) > + return ret; > + > + guard(mutex)(&state->lock); > + ret = ad8460_set_hvdac_word(state, 0, val); > + if (ret) > + return ret; > + > + return regmap_update_bits(state->regmap, > + AD8460_CTRL_REG(0x02), > + AD8460_PATTERN_DEPTH_MSK, > + FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0)); > +} > + > +static int ad8460_set_fault_threshold(struct ad8460_state *state, > + enum ad8460_fault_type fault, > + unsigned int threshold) > +{ > + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08 + fault), > + AD8460_FAULT_LIMIT_MSK, > + FIELD_PREP(AD8460_FAULT_LIMIT_MSK, threshold)); > +} > + > +static int ad8460_get_fault_threshold(struct ad8460_state *state, > + enum ad8460_fault_type fault, > + unsigned int *threshold) > +{ > + unsigned int val; > + int ret; > + > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault), &val); > + if (ret) > + return ret; > + > + *threshold = FIELD_GET(AD8460_FAULT_LIMIT_MSK, val); > + > + return ret; > +} > + > +static int ad8460_set_fault_threshold_en(struct ad8460_state *state, > + enum ad8460_fault_type fault, bool en) > +{ > + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08 + fault), > + AD8460_FAULT_ARM_MSK, > + FIELD_PREP(AD8460_FAULT_ARM_MSK, en)); > +} > + > +static int ad8460_get_fault_threshold_en(struct ad8460_state *state, > + enum ad8460_fault_type fault, bool *en) > +{ > + unsigned int val; > + int ret; > + > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault), &val); > + if (ret) > + return ret; > + > + *en = FIELD_GET(AD8460_FAULT_ARM_MSK, val); > + > + return 0; > +} > + > +static int ad8460_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, int val2, > + long mask) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + switch (chan->type) { > + case IIO_VOLTAGE: > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) > + return ad8460_set_sample(state, val); > + unreachable(); > + case IIO_CURRENT: > + return regmap_write(state->regmap, AD8460_CTRL_REG(0x04), > + FIELD_PREP(AD8460_QUIESCENT_CURRENT_MSK, val)); > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} > + > +static int ad8460_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + int data, ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + switch (chan->type) { > + case IIO_VOLTAGE: > + scoped_guard(mutex, &state->lock) { > + ret = ad8460_get_hvdac_word(state, 0, &data); > + if (ret) > + return ret; > + } > + *val = data; > + return IIO_VAL_INT; > + case IIO_CURRENT: > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x04), > + &data); > + if (ret) > + return ret; > + *val = data; > + return IIO_VAL_INT; > + case IIO_TEMP: > + ret = iio_read_channel_raw(state->tmp_adc_channel, &data); > + if (ret) > + return ret; > + *val = data; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_SAMP_FREQ: > + *val = clk_get_rate(state->sync_clk); > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + /* > + * vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V > + * vMAX = vNOMINAL_SPAN * (2**14 / 2**14) - 40V > + * vMIN = vNOMINAL_SPAN * (0 / 2**14) - 40V > + * vADJ = vCONV * (2000 / rSET) * (vREF / 1.2) > + * vSPAN = vADJ_MAX - vADJ_MIN > + * See datasheet page 49, section FULL-SCALE REDUCTION > + */ > + *val = AD8460_NOMINAL_VOLTAGE_SPAN * 2000 * state->refio_1p2v_mv; > + *val2 = state->ext_resistor_ohms * 1200; > + return IIO_VAL_FRACTIONAL; > + default: > + return -EINVAL; > + } > +} > + > +static int ad8460_select_fault_type(int chan_type, enum iio_event_direction dir) > +{ > + switch (chan_type) { > + case IIO_VOLTAGE: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + return AD8460_OVERVOLTAGE_POS; > + case IIO_EV_DIR_FALLING: > + return AD8460_OVERVOLTAGE_NEG; > + default: > + return -EINVAL; > + } > + case IIO_CURRENT: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + return AD8460_OVERCURRENT_SRC; > + case IIO_EV_DIR_FALLING: > + return AD8460_OVERCURRENT_SNK; > + default: > + return -EINVAL; > + } > + case IIO_TEMP: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + return AD8460_OVERTEMPERATURE; > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} > + > +static int ad8460_write_event_value(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, int val, int val2) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + unsigned int fault; > + > + if (type != IIO_EV_TYPE_THRESH) > + return -EINVAL; > + > + if (info != IIO_EV_INFO_VALUE) > + return -EINVAL; > + > + fault = ad8460_select_fault_type(chan->type, dir); > + if (fault < 0) > + return fault; > + > + return ad8460_set_fault_threshold(state, fault, val); > +} > + > +static int ad8460_read_event_value(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, int *val, int *val2) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + unsigned int fault; > + > + if (type != IIO_EV_TYPE_THRESH) > + return -EINVAL; > + > + if (info != IIO_EV_INFO_VALUE) > + return -EINVAL; > + > + fault = ad8460_select_fault_type(chan->type, dir); > + if (fault < 0) > + return fault; > + > + return ad8460_get_fault_threshold(state, fault, val); > +} > + > +static int ad8460_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, int val) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + unsigned int fault; > + > + if (type != IIO_EV_TYPE_THRESH) > + return -EINVAL; > + > + fault = ad8460_select_fault_type(chan->type, dir); > + if (fault < 0) > + return fault; > + > + return ad8460_set_fault_threshold_en(state, fault, val); > +} > + > +static int ad8460_read_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + unsigned int fault; > + bool en; > + int ret; > + > + if (type != IIO_EV_TYPE_THRESH) > + return -EINVAL; > + > + fault = ad8460_select_fault_type(chan->type, dir); > + if (fault < 0) > + return fault; > + > + ret = ad8460_get_fault_threshold_en(state, fault, &en); > + if (ret) > + return ret; > + > + return en; > +} > + > +static int ad8460_reg_access(struct iio_dev *indio_dev, unsigned int reg, > + unsigned int writeval, unsigned int *readval) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + > + if (readval) > + return regmap_read(state->regmap, reg, readval); > + > + return regmap_write(state->regmap, reg, writeval); > +} > + > +static int ad8460_buffer_preenable(struct iio_dev *indio_dev) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + > + return ad8460_enable_apg_mode(state, 0); > +} > + > +static int ad8460_buffer_postdisable(struct iio_dev *indio_dev) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + > + return ad8460_enable_apg_mode(state, 1); > +} > + > +static const struct iio_buffer_setup_ops ad8460_buffer_setup_ops = { > + .preenable = &ad8460_buffer_preenable, > + .postdisable = &ad8460_buffer_postdisable, > +}; > + > +static const struct iio_info ad8460_info = { > + .read_raw = &ad8460_read_raw, > + .write_raw = &ad8460_write_raw, > + .write_event_value = &ad8460_write_event_value, > + .read_event_value = &ad8460_read_event_value, > + .write_event_config = &ad8460_write_event_config, > + .read_event_config = &ad8460_read_event_config, > + .debugfs_reg_access = &ad8460_reg_access, > +}; > + > +static const struct iio_enum ad8460_powerdown_mode_enum = { > + .items = ad8460_powerdown_modes, > + .num_items = ARRAY_SIZE(ad8460_powerdown_modes), > + .get = ad8460_get_powerdown_mode, > + .set = ad8460_set_powerdown_mode, > +}; > + > +#define AD8460_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) { \ Looks like _shared is always IIO_SEPARATE, so we could omit that parameter. > + .name = _name, \ > + .read = (_read), \ > + .write = (_write), \ > + .private = (_what), \ > + .shared = (_shared), \ > +} > + > +static struct iio_chan_spec_ext_info ad8460_ext_info[] = { > + AD8460_CHAN_EXT_INFO("raw0", 0, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw1", 1, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw2", 2, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw3", 3, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw4", 4, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw5", 5, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw6", 6, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw7", 7, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw8", 8, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw9", 9, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw10", 10, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw11", 11, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw12", 12, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw13", 13, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw14", 14, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("raw15", 15, IIO_SEPARATE, ad8460_dac_input_read, > + ad8460_dac_input_write), > + AD8460_CHAN_EXT_INFO("toggle_en", 0, IIO_SEPARATE, ad8460_read_toggle_en, > + ad8460_write_toggle_en), > + AD8460_CHAN_EXT_INFO("symbol", 0, IIO_SEPARATE, ad8460_read_symbol, > + ad8460_write_symbol), > + AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE, ad8460_read_powerdown, > + ad8460_write_powerdown), > + IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad8460_powerdown_mode_enum), > + IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE, > + &ad8460_powerdown_mode_enum), > + {} > +}; > + > +static const struct iio_event_spec ad8460_events[] = { > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_RISING, > + .mask_separate = BIT(IIO_EV_INFO_VALUE) | > + BIT(IIO_EV_INFO_ENABLE), > + }, > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_FALLING, > + .mask_separate = BIT(IIO_EV_INFO_VALUE) | > + BIT(IIO_EV_INFO_ENABLE), > + }, > +}; > + > +#define AD8460_VOLTAGE_CHAN { \ > + .type = IIO_VOLTAGE, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ > + BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + .output = 1, \ > + .indexed = 1, \ > + .channel = 0, \ > + .scan_index = 0, \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 14, \ > + .storagebits = 16, \ > + .endianness = IIO_LE, \ Data is going over DMA, so should this be IIO_CPU? > + }, \ > + .ext_info = ad8460_ext_info, \ > + .event_spec = ad8460_events, \ > + .num_event_specs = ARRAY_SIZE(ad8460_events), \ > +} > + > +#define AD8460_CURRENT_CHAN { \ > + .type = IIO_CURRENT, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .output = 0, \ Since this is writeable, I assume this is an output? (so = 1, not 0) > + .indexed = 1, \ > + .channel = 0, \ > + .scan_index = -1, \ > + .event_spec = ad8460_events, \ > + .num_event_specs = ARRAY_SIZE(ad8460_events), \ > +} > + > +#define AD8460_TEMP_CHAN { \ > + .type = IIO_TEMP, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .output = 1, \ > + .indexed = 1, \ > + .channel = 0, \ > + .scan_index = -1, \ > + .event_spec = ad8460_events, \ > + .num_event_specs = 1, \ > +} > + > +static const struct iio_chan_spec ad8460_channels[] = { > + AD8460_VOLTAGE_CHAN, > + AD8460_CURRENT_CHAN, > +}; > + > +static const struct iio_chan_spec ad8460_channels_with_tmp_adc[] = { > + AD8460_VOLTAGE_CHAN, > + AD8460_CURRENT_CHAN, > + AD8460_TEMP_CHAN, > +}; > + > +static const struct regmap_config ad8460_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = 0x7F, > +}; > + > +static int ad8460_probe(struct spi_device *spi) > +{ > + struct ad8460_state *state; > + struct iio_dev *indio_dev; > + struct device *dev; > + u32 tmp[2], temp; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state)); > + if (!indio_dev) > + return -ENOMEM; > + > + state = iio_priv(indio_dev); > + mutex_init(&state->lock); > + > + indio_dev->name = "ad8460"; > + indio_dev->info = &ad8460_info; > + > + state->spi = spi; > + dev = &spi->dev; > + > + state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config); > + if (IS_ERR(state->regmap)) > + return dev_err_probe(dev, PTR_ERR(state->regmap), > + "Failed to initialize regmap"); > + > + state->sync_clk = devm_clk_get_enabled(dev, NULL); > + if (IS_ERR(state->sync_clk)) > + return dev_err_probe(dev, PTR_ERR(state->sync_clk), > + "Failed to get sync clk\n"); > + > + state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460-tmp"); Should we check for specific errors here? For example what if we get -EPROBE_DEFER? Also, it doesn't like we could ever get NULL, so IS_ERR() should be sufficient. > + if (IS_ERR_OR_NULL(state->tmp_adc_channel)) { > + indio_dev->channels = ad8460_channels; > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels); > + } else { > + indio_dev->channels = ad8460_channels_with_tmp_adc; > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels_with_tmp_adc); > + } > + > + ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v"); > + if (ret == -ENODEV) > + state->refio_1p2v_mv = 1200; > + else if (ret >= 0) > + state->refio_1p2v_mv = ret / 1000; > + else > + return dev_err_probe(dev, ret, "Failed to get reference voltage\n"); The pattern we have been using in other drivers is: ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v"); if (ret < 0 && ret != -ENODEV) return dev_err_probe(dev, ret, "Failed to get reference voltage\n"); state->refio_1p2v_mv = ret == -ENODEV ? 1200 : ret / 1000; It saves a bit of reading. > + > + if (!in_range(state->refio_1p2v_mv, AD8460_MIN_VREFIO_UV, > + AD8460_MAX_VREFIO_UV)) It looks like millivolts is being compared to microvolts here. > + return dev_err_probe(dev, -EINVAL, > + "Invalid ref voltage range(%u mV) [%u mV, %u mV]\n", > + state->refio_1p2v_mv, > + AD8460_MIN_VREFIO_UV / 1000, > + AD8460_MAX_VREFIO_UV / 1000); > + > + ret = device_property_read_u32(dev, "adi,external-resistor-ohms", > + &state->ext_resistor_ohms); > + if (ret) > + state->ext_resistor_ohms = 2000; > + else if (!in_range(state->ext_resistor_ohms, AD8460_MIN_EXT_RESISTOR_OHMS, > + AD8460_MAX_EXT_RESISTOR_OHMS)) > + return dev_err_probe(dev, -EINVAL, > + "Invalid resistor set range(%u) [%u, %u]\n", > + state->ext_resistor_ohms, > + AD8460_MIN_EXT_RESISTOR_OHMS, > + AD8460_MAX_EXT_RESISTOR_OHMS); > + > + /* Arm the device by default */ Comment doesn't match the code. > + ret = device_property_read_u32_array(dev, "adi,range-microamp", > + tmp, ARRAY_SIZE(tmp)); > + if (!ret) { > + if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERCURRENT_UA)) > + regmap_write(state->regmap, AD8460_CTRL_REG(0x08), > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | > + AD8460_CURRENT_LIMIT_CONV(tmp[1])); > + > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERCURRENT_UA, 0)) > + regmap_write(state->regmap, AD8460_CTRL_REG(0x09), > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | > + AD8460_CURRENT_LIMIT_CONV(abs(tmp[0]))); > + } > + > + ret = device_property_read_u32_array(dev, "adi,range-microvolt", > + tmp, ARRAY_SIZE(tmp)); > + if (!ret) { > + if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERVOLTAGE_UV)) > + regmap_write(state->regmap, AD8460_CTRL_REG(0x0A), > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | > + AD8460_VOLTAGE_LIMIT_CONV(tmp[1])); > + > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERVOLTAGE_UV, 0)) > + regmap_write(state->regmap, AD8460_CTRL_REG(0x0B), > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | > + AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0]))); > + } > + > + ret = device_property_read_u32(dev, "adi,max-millicelsius", &temp); > + if (!ret) { > + if (in_range(temp, AD8460_MIN_OVERTEMPERATURE_MC, > + AD8460_MAX_OVERTEMPERATURE_MC)) > + regmap_write(state->regmap, AD8460_CTRL_REG(0x0C), > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | > + AD8460_TEMP_LIMIT_CONV(abs(temp))); > + } > + > + ret = ad8460_reset(state); > + if (ret) > + return ret; > + > + /* Enables DAC by default */ > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), > + AD8460_HVDAC_SLEEP_MSK, > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0)); > + if (ret) > + return ret; > + > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->setup_ops = &ad8460_buffer_setup_ops; > + > + ret = devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev, "tx", > + IIO_BUFFER_DIRECTION_OUT); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to get DMA buffer\n"); > + > + return devm_iio_device_register(dev, indio_dev); > +} > + > +static const struct of_device_id ad8460_of_match[] = { > + { .compatible = "adi, ad8460" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ad8460_of_match); > + > +static struct spi_driver ad8460_driver = { > + .driver = { > + .name = "ad8460", > + .of_match_table = ad8460_of_match, > + }, > + .probe = ad8460_probe, > +}; > +module_spi_driver(ad8460_driver); > + > +MODULE_AUTHOR("Mariel Tinaco <mariel.tinaco@analog.com"); > +MODULE_DESCRIPTION("AD8460 DAC driver"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS(IIO_DMAENGINE_BUFFER); ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-06 0:50 ` David Lechner @ 2024-09-09 9:47 ` Tinaco, Mariel 2024-09-09 14:51 ` David Lechner 2024-09-10 1:44 ` Tinaco, Mariel 1 sibling, 1 reply; 20+ messages in thread From: Tinaco, Mariel @ 2024-09-09 9:47 UTC (permalink / raw) To: David Lechner, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, Nuno Sá > -----Original Message----- > From: David Lechner <dlechner@baylibre.com> > Sent: Friday, September 6, 2024 8:50 AM > 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>; Nuno Sá > <noname.nuno@gmail.com> > Subject: Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC > > [External] > > On 9/3/24 9:30 PM, Mariel Tinaco wrote: > > The AD8460 is a “bits in, power out” high voltage, high-power, > > high-speed driver optimized for large output current (up to ±1 A) and > > high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) into > > capacitive loads. > > > > A digital engine implements user-configurable features: modes for > > digital input, programmable supply current, and fault monitoring and > > programmable protection settings for output current, output voltage, > > and junction temperature. The AD8460 operates on high voltage dual > > supplies up to ±55 V and a single low voltage supply of 5 V. > > > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> > > --- > > MAINTAINERS | 1 + > > drivers/iio/dac/Kconfig | 13 + > > drivers/iio/dac/Makefile | 1 + > > drivers/iio/dac/ad8460.c | 932 > > +++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 947 insertions(+) > > create mode 100644 drivers/iio/dac/ad8460.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS index > > e0509c9f5545..30746a3f0bbd 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -1326,6 +1326,7 @@ 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 > > +F: drivers/iio/dac/ad8460.c > > > > ANALOG DEVICES INC AD9739a DRIVER > > M: Nuno Sa <nuno.sa@analog.com> > > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig index > > 1cfd7e2a622f..fa091995d002 100644 > > --- a/drivers/iio/dac/Kconfig > > +++ b/drivers/iio/dac/Kconfig > > @@ -301,6 +301,19 @@ config AD7303 > > To compile this driver as module choose M here: the module will be > called > > ad7303. > > > > +config AD8460 > > + tristate "Analog Devices AD8460 DAC driver" > > + depends on SPI > > + select REGMAP_SPI > > + select IIO_BUFFER > > + select IIO_BUFFER_DMAENGINE > > + help > > + Say yes here to build support for Analog Devices AD8460 Digital to > > + Analog Converters (DAC). > > + > > + To compile this driver as a module choose M here: the module will be > called > > + ad8460. > > + > > config AD8801 > > tristate "Analog Devices AD8801/AD8803 DAC driver" > > depends on SPI_MASTER > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile index > > 2cf148f16306..621d553bd6e3 100644 > > --- a/drivers/iio/dac/Makefile > > +++ b/drivers/iio/dac/Makefile > > @@ -28,6 +28,7 @@ obj-$(CONFIG_AD5686_SPI) += ad5686-spi.o > > obj-$(CONFIG_AD5696_I2C) += ad5696-i2c.o > > obj-$(CONFIG_AD7293) += ad7293.o > > obj-$(CONFIG_AD7303) += ad7303.o > > +obj-$(CONFIG_AD8460) += ad8460.o > > obj-$(CONFIG_AD8801) += ad8801.o > > obj-$(CONFIG_AD9739A) += ad9739a.o > > obj-$(CONFIG_ADI_AXI_DAC) += adi-axi-dac.o diff --git > > a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c new file mode > > 100644 index 000000000000..6428bfaf0bd7 > > --- /dev/null > > +++ b/drivers/iio/dac/ad8460.c > > @@ -0,0 +1,932 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * AD8460 Waveform generator DAC Driver > > + * > > + * Copyright (C) 2024 Analog Devices, Inc. > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/cleanup.h> > > +#include <linux/clk.h> > > +#include <linux/debugfs.h> > > +#include <linux/delay.h> > > +#include <linux/dmaengine.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/iio/buffer.h> > > +#include <linux/iio/buffer-dma.h> > > +#include <linux/iio/buffer-dmaengine.h> #include > > +<linux/iio/consumer.h> #include <linux/iio/events.h> #include > > +<linux/iio/iio.h> #include <linux/kernel.h> #include <linux/module.h> > > +#include <linux/mod_devicetable.h> #include <linux/regmap.h> #include > > +<linux/regulator/consumer.h> #include <linux/spi/spi.h> > > + > > +#define AD8460_CTRL_REG(x) (x) > > +#define AD8460_HVDAC_DATA_WORD_LOW(x) (0x60 + (2 * (x))) > > +#define AD8460_HVDAC_DATA_WORD_HIGH(x) (0x61 + (2 * (x))) > > I don't see AD8460_HVDAC_DATA_WORD_HIGH() used anywhere, so we > could remove that and rename AD8460_HVDAC_DATA_WORD_LOW() to > AD8460_HVDAC_DATA_WORD(). > > > + > > +#define AD8460_HV_RESET_MSK BIT(7) > > +#define AD8460_HV_SLEEP_MSK BIT(4) > > +#define AD8460_WAVE_GEN_MODE_MSK BIT(0) > > + > > +#define AD8460_HVDAC_SLEEP_MSK BIT(3) > > + > > +#define AD8460_FAULT_ARM_MSK BIT(7) > > +#define AD8460_FAULT_LIMIT_MSK GENMASK(6, 0) > > + > > +#define AD8460_APG_MODE_ENABLE_MSK BIT(5) > > +#define AD8460_PATTERN_DEPTH_MSK GENMASK(3, 0) > > + > > +#define AD8460_QUIESCENT_CURRENT_MSK GENMASK(7, > 0) > > + > > +#define AD8460_SHUTDOWN_FLAG_MSK BIT(7) > > + > > +#define AD8460_DATA_BYTE_LOW_MSK GENMASK(7, 0) > > +#define AD8460_DATA_BYTE_HIGH_MSK GENMASK(5, 0) > > +#define AD8460_DATA_BYTE_FULL_MSK GENMASK(13, 0) > > + > > +#define AD8460_DEFAULT_FAULT_PROTECT 0x00 > > +#define AD8460_DATA_BYTE_WORD_LENGTH 2 > > +#define AD8460_NUM_DATA_WORDS 16 > > +#define AD8460_NOMINAL_VOLTAGE_SPAN 80 > > +#define AD8460_MIN_EXT_RESISTOR_OHMS 2000 > > +#define AD8460_MAX_EXT_RESISTOR_OHMS 20000 > > +#define AD8460_MIN_VREFIO_UV 120000 > > +#define AD8460_MAX_VREFIO_UV 1200000 > > +#define AD8460_ABS_MAX_OVERVOLTAGE_UV 55000000 > > +#define AD8460_ABS_MAX_OVERCURRENT_UA 1000000 > > +#define AD8460_MAX_OVERTEMPERATURE_MC 150000 > > +#define AD8460_MIN_OVERTEMPERATURE_MC 20000 > > +#define AD8460_CURRENT_LIMIT_CONV(x) ((x) / 15625) > > +#define AD8460_VOLTAGE_LIMIT_CONV(x) ((x) / 1953000) > > +#define AD8460_TEMP_LIMIT_CONV(x) (((x) + 266640) / > 6510) > > It looks like there are issues with mixed tabs and spaces in the macros above > that should be cleaned up. > > > + > > +enum ad8460_fault_type { > > + AD8460_OVERCURRENT_SRC, > > + AD8460_OVERCURRENT_SNK, > > + AD8460_OVERVOLTAGE_POS, > > + AD8460_OVERVOLTAGE_NEG, > > + AD8460_OVERTEMPERATURE, > > +}; > > + > > +struct ad8460_state { > > + struct spi_device *spi; > > + struct regmap *regmap; > > + struct iio_channel *tmp_adc_channel; > > + struct clk *sync_clk; > > + /* lock to protect against multiple access to the device and shared data > */ > > + struct mutex lock; > > + int refio_1p2v_mv; > > + u32 ext_resistor_ohms; > > + /* > > + * DMA (thus cache coherency maintenance) requires the > > + * transfer buffers to live in their own cache lines. > > + */ > > + u8 spi_tx_buf[2] __aligned(IIO_DMA_MINALIGN); > > If we make this `__le16 spi_tx_buf` instead of `u8 spi_tx_buf[2]`, then we > don't need the unaligned access functions. > > > +}; > > + > > +static int ad8460_hv_reset(struct ad8460_state *state) { > > + int ret; > > + > > + ret = regmap_update_bits(state->regmap, > AD8460_CTRL_REG(0x00), > > + AD8460_HV_RESET_MSK, > > + FIELD_PREP(AD8460_HV_RESET_MSK, 1)); > > Can be simplified with regmap_set_bits(). > > > + if (ret) > > + return ret; > > + > > + fsleep(20); > > + > > + return regmap_update_bits(state->regmap, > AD8460_CTRL_REG(0x00), > > + AD8460_HV_RESET_MSK, > > + FIELD_PREP(AD8460_HV_RESET_MSK, 0)); > > and regmap_clear_bits(). > > > +} > > + > > +static int ad8460_reset(const struct ad8460_state *state) { > > + struct device *dev = &state->spi->dev; > > + struct gpio_desc *reset; > > + > > + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > > + if (IS_ERR(reset)) > > + return dev_err_probe(dev, PTR_ERR(reset), > > + "Failed to get reset gpio"); > > + if (reset) { > > + /* minimum duration of 10ns */ > > + ndelay(10); > > + gpiod_set_value_cansleep(reset, 1); > > + return 0; > > + } > > + > > + /* bring all registers to their default state */ > > + return regmap_write(state->regmap, AD8460_CTRL_REG(0x03), 1); } > > + > > +static int ad8460_enable_apg_mode(struct ad8460_state *state, int > > +val) { > > + int ret; > > + > > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x02), > > + AD8460_APG_MODE_ENABLE_MSK, > > + > FIELD_PREP(AD8460_APG_MODE_ENABLE_MSK, val)); > > + if (ret) > > + return ret; > > + > > + return regmap_update_bits(state->regmap, > AD8460_CTRL_REG(0x00), > > + AD8460_WAVE_GEN_MODE_MSK, > > + > FIELD_PREP(AD8460_WAVE_GEN_MODE_MSK, val)); } > > + > > +static int ad8460_read_shutdown_flag(struct ad8460_state *state, u64 > > +*flag) { > > + int ret, val; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x0E), &val); > > + if (ret) > > + return ret; > > + > > + *flag = FIELD_GET(AD8460_SHUTDOWN_FLAG_MSK, val); > > + return 0; > > +} > > + > > +static int ad8460_get_hvdac_word(struct ad8460_state *state, int > > +index, int *val) { > > + int ret; > > + > > + ret = regmap_bulk_read(state->regmap, > AD8460_HVDAC_DATA_WORD_LOW(index), > > + &state->spi_tx_buf, > AD8460_DATA_BYTE_WORD_LENGTH); > > + if (ret) > > + return ret; > > + > > + *val = get_unaligned_le16(state->spi_tx_buf); > > With spi_tx_buf changed to __le16, this can use le16_to_cpu() instead of > get_unaligned_le16(). > I suppose we use the cpu_to_le16 for the set_hvdac_word function? static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val) { state->spi_tx_buf = cpu_to_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val)); return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD(index), &state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH); } > > + > > + return ret; > > +} > > + > > +static int ad8460_set_hvdac_word(struct ad8460_state *state, int > > +index, int val) { > > + put_unaligned_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, > val), > > + &state->spi_tx_buf); > > + > > + return regmap_bulk_write(state->regmap, > AD8460_HVDAC_DATA_WORD_LOW(index), > > + state->spi_tx_buf, > AD8460_DATA_BYTE_WORD_LENGTH); } > > + > > +static ssize_t ad8460_dac_input_read(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, char > *buf) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int reg; > > + int ret; > > + > > + ret = ad8460_get_hvdac_word(state, private, ®); > > + if (ret) > > + return ret; > > + > > + return sysfs_emit(buf, "%d\n", reg); > > %u instead of %d since reg is unsigned > > > +} > > + > > +static ssize_t ad8460_dac_input_write(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, > > + const char *buf, size_t len) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int reg; > > + int ret; > > + > > + ret = kstrtou32(buf, 10, ®); > > + if (ret) > > + return ret; > > + > > + guard(mutex)(&state->lock); > > + > > + return ad8460_set_hvdac_word(state, private, reg); } > > + > > +static ssize_t ad8460_read_symbol(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, char *buf) > { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int reg; > > + int ret; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®); > > + if (ret) > > + return ret; > > + > > + return sysfs_emit(buf, "%ld\n", > FIELD_GET(AD8460_PATTERN_DEPTH_MSK, > > +reg)); } > > I guess FIELD_GET() makes it long? but probalby should be %lu. > > > + > > +static ssize_t ad8460_write_symbol(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, > > + const char *buf, size_t len) > > +{ > > + struct ad8460_state *state = iio_priv(indio_dev); > > + uint16_t sym; > > + int ret; > > + > > + ret = kstrtou16(buf, 10, &sym); > > + if (ret) > > + return ret; > > + > > + guard(mutex)(&state->lock); > > + > > + return regmap_update_bits(state->regmap, > > + AD8460_CTRL_REG(0x02), > > + AD8460_PATTERN_DEPTH_MSK, > > + > FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, sym)); } > > + > > +static ssize_t ad8460_read_toggle_en(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, char > *buf) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int reg; > > + int ret; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®); > > + if (ret) > > + return ret; > > + > > + return sysfs_emit(buf, "%ld\n", > > +FIELD_GET(AD8460_APG_MODE_ENABLE_MSK, reg)); } > > + > > +static ssize_t ad8460_write_toggle_en(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, > > + const char *buf, size_t len) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + bool toggle_en; > > + int ret; > > + > > + ret = kstrtobool(buf, &toggle_en); > > + if (ret) > > + return ret; > > + > > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) > > + return ad8460_enable_apg_mode(state, toggle_en); > > + unreachable(); > > Hmm... do we need to make an unscoped version of > iio_device_claim_direct_scoped()? > So iio_device_claim_direct_scoped is used here because the buffer enable/disable accesses this enable_apg_mode function. Is it also a standard practice to put the kstrobool() util inside the scope? > > +} > > + > > +static ssize_t ad8460_read_powerdown(struct iio_dev *indio_dev, > uintptr_t private, > > + const struct iio_chan_spec *chan, char > *buf) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int reg; > > + int ret; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x01), ®); > > + if (ret) > > + return ret; > > + > > + return sysfs_emit(buf, "%ld\n", > FIELD_GET(AD8460_HVDAC_SLEEP_MSK, > > +reg)); } > > + > > +static ssize_t ad8460_write_powerdown(struct iio_dev *indio_dev, > uintptr_t private, > > + const struct iio_chan_spec *chan, > > + const char *buf, size_t len) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + bool pwr_down; > > + u64 sdn_flag; > > + int ret; > > + > > + ret = kstrtobool(buf, &pwr_down); > > + if (ret) > > + return ret; > > + > > + guard(mutex)(&state->lock); > > + > > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), > > + AD8460_HVDAC_SLEEP_MSK, > > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, > pwr_down)); > > + if (ret) > > + return ret; > > + > > + if (!pwr_down) { > > + ret = ad8460_read_shutdown_flag(state, &sdn_flag); > > + if (ret) > > + return ret; > > + > > + if (sdn_flag) { > > + ret = ad8460_hv_reset(state); > > + if (ret) > > + return ret; > > + } > > + } > > + > > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00), > > + AD8460_HV_SLEEP_MSK, > > + FIELD_PREP(AD8460_HV_SLEEP_MSK, > !pwr_down)); > > + if (ret) > > + return ret; > > Some comments explaining the logic in this function would be helpful. > Without reading the datasheet, it looks like this puts it in powerdown mode > and takes it right back out before returning. > > > + > > + return len; > > +} > > + > > +static const char * const ad8460_powerdown_modes[] = { > > + "three_state", > > +}; > > + > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan) { > > + return 0; > > +} > > + > > +static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + unsigned int type) > > +{ > > + return 0; > > +} > > + > > +static int ad8460_set_sample(struct ad8460_state *state, int val) { > > + int ret; > > + > > + ret = ad8460_enable_apg_mode(state, 1); > > + if (ret) > > + return ret; > > + > > + guard(mutex)(&state->lock); > > + ret = ad8460_set_hvdac_word(state, 0, val); > > + if (ret) > > + return ret; > > + > > + return regmap_update_bits(state->regmap, > > + AD8460_CTRL_REG(0x02), > > + AD8460_PATTERN_DEPTH_MSK, > > + > FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0)); } > > + > > +static int ad8460_set_fault_threshold(struct ad8460_state *state, > > + enum ad8460_fault_type fault, > > + unsigned int threshold) > > +{ > > + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08 > + fault), > > + AD8460_FAULT_LIMIT_MSK, > > + FIELD_PREP(AD8460_FAULT_LIMIT_MSK, > threshold)); } > > + > > +static int ad8460_get_fault_threshold(struct ad8460_state *state, > > + enum ad8460_fault_type fault, > > + unsigned int *threshold) > > +{ > > + unsigned int val; > > + int ret; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault), > &val); > > + if (ret) > > + return ret; > > + > > + *threshold = FIELD_GET(AD8460_FAULT_LIMIT_MSK, val); > > + > > + return ret; > > +} > > + > > +static int ad8460_set_fault_threshold_en(struct ad8460_state *state, > > + enum ad8460_fault_type fault, bool > en) { > > + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08 > + fault), > > + AD8460_FAULT_ARM_MSK, > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, > en)); } > > + > > +static int ad8460_get_fault_threshold_en(struct ad8460_state *state, > > + enum ad8460_fault_type fault, bool > *en) { > > + unsigned int val; > > + int ret; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault), > &val); > > + if (ret) > > + return ret; > > + > > + *en = FIELD_GET(AD8460_FAULT_ARM_MSK, val); > > + > > + return 0; > > +} > > + > > +static int ad8460_write_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, int val, int val2, > > + long mask) > > +{ > > + struct ad8460_state *state = iio_priv(indio_dev); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + switch (chan->type) { > > + case IIO_VOLTAGE: > > + iio_device_claim_direct_scoped(return -EBUSY, > indio_dev) > > + return ad8460_set_sample(state, val); > > + unreachable(); > > + case IIO_CURRENT: > > + return regmap_write(state->regmap, > AD8460_CTRL_REG(0x04), > > + > FIELD_PREP(AD8460_QUIESCENT_CURRENT_MSK, val)); > > + default: > > + return -EINVAL; > > + } > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int ad8460_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec > const *chan, > > + int *val, int *val2, long mask) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + int data, ret; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + switch (chan->type) { > > + case IIO_VOLTAGE: > > + scoped_guard(mutex, &state->lock) { > > + ret = ad8460_get_hvdac_word(state, 0, > &data); > > + if (ret) > > + return ret; > > + } > > + *val = data; > > + return IIO_VAL_INT; > > + case IIO_CURRENT: > > + ret = regmap_read(state->regmap, > AD8460_CTRL_REG(0x04), > > + &data); > > + if (ret) > > + return ret; > > + *val = data; > > + return IIO_VAL_INT; > > + case IIO_TEMP: > > + ret = iio_read_channel_raw(state->tmp_adc_channel, > &data); > > + if (ret) > > + return ret; > > + *val = data; > > + return IIO_VAL_INT; > > + default: > > + return -EINVAL; > > + } > > + case IIO_CHAN_INFO_SAMP_FREQ: > > + *val = clk_get_rate(state->sync_clk); > > + return IIO_VAL_INT; > > + case IIO_CHAN_INFO_SCALE: > > + /* > > + * vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V > > + * vMAX = vNOMINAL_SPAN * (2**14 / 2**14) - 40V > > + * vMIN = vNOMINAL_SPAN * (0 / 2**14) - 40V > > + * vADJ = vCONV * (2000 / rSET) * (vREF / 1.2) > > + * vSPAN = vADJ_MAX - vADJ_MIN > > + * See datasheet page 49, section FULL-SCALE REDUCTION > > + */ > > + *val = AD8460_NOMINAL_VOLTAGE_SPAN * 2000 * state- > >refio_1p2v_mv; > > + *val2 = state->ext_resistor_ohms * 1200; > > + return IIO_VAL_FRACTIONAL; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int ad8460_select_fault_type(int chan_type, enum > > +iio_event_direction dir) { > > + switch (chan_type) { > > + case IIO_VOLTAGE: > > + switch (dir) { > > + case IIO_EV_DIR_RISING: > > + return AD8460_OVERVOLTAGE_POS; > > + case IIO_EV_DIR_FALLING: > > + return AD8460_OVERVOLTAGE_NEG; > > + default: > > + return -EINVAL; > > + } > > + case IIO_CURRENT: > > + switch (dir) { > > + case IIO_EV_DIR_RISING: > > + return AD8460_OVERCURRENT_SRC; > > + case IIO_EV_DIR_FALLING: > > + return AD8460_OVERCURRENT_SNK; > > + default: > > + return -EINVAL; > > + } > > + case IIO_TEMP: > > + switch (dir) { > > + case IIO_EV_DIR_RISING: > > + return AD8460_OVERTEMPERATURE; > > + default: > > + return -EINVAL; > > + } > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int ad8460_write_event_value(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, > > + enum iio_event_info info, int val, int val2) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int fault; > > + > > + if (type != IIO_EV_TYPE_THRESH) > > + return -EINVAL; > > + > > + if (info != IIO_EV_INFO_VALUE) > > + return -EINVAL; > > + > > + fault = ad8460_select_fault_type(chan->type, dir); > > + if (fault < 0) > > + return fault; > > + > > + return ad8460_set_fault_threshold(state, fault, val); } > > + > > +static int ad8460_read_event_value(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, > > + enum iio_event_info info, int *val, int *val2) > { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int fault; > > + > > + if (type != IIO_EV_TYPE_THRESH) > > + return -EINVAL; > > + > > + if (info != IIO_EV_INFO_VALUE) > > + return -EINVAL; > > + > > + fault = ad8460_select_fault_type(chan->type, dir); > > + if (fault < 0) > > + return fault; > > + > > + return ad8460_get_fault_threshold(state, fault, val); } > > + > > +static int ad8460_write_event_config(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, int val) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int fault; > > + > > + if (type != IIO_EV_TYPE_THRESH) > > + return -EINVAL; > > + > > + fault = ad8460_select_fault_type(chan->type, dir); > > + if (fault < 0) > > + return fault; > > + > > + return ad8460_set_fault_threshold_en(state, fault, val); } > > + > > +static int ad8460_read_event_config(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int fault; > > + bool en; > > + int ret; > > + > > + if (type != IIO_EV_TYPE_THRESH) > > + return -EINVAL; > > + > > + fault = ad8460_select_fault_type(chan->type, dir); > > + if (fault < 0) > > + return fault; > > + > > + ret = ad8460_get_fault_threshold_en(state, fault, &en); > > + if (ret) > > + return ret; > > + > > + return en; > > +} > > + > > +static int ad8460_reg_access(struct iio_dev *indio_dev, unsigned int reg, > > + unsigned int writeval, unsigned int *readval) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + > > + if (readval) > > + return regmap_read(state->regmap, reg, readval); > > + > > + return regmap_write(state->regmap, reg, writeval); } > > + > > +static int ad8460_buffer_preenable(struct iio_dev *indio_dev) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + > > + return ad8460_enable_apg_mode(state, 0); } > > + > > +static int ad8460_buffer_postdisable(struct iio_dev *indio_dev) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + > > + return ad8460_enable_apg_mode(state, 1); } > > + > > +static const struct iio_buffer_setup_ops ad8460_buffer_setup_ops = { > > + .preenable = &ad8460_buffer_preenable, > > + .postdisable = &ad8460_buffer_postdisable, }; > > + > > +static const struct iio_info ad8460_info = { > > + .read_raw = &ad8460_read_raw, > > + .write_raw = &ad8460_write_raw, > > + .write_event_value = &ad8460_write_event_value, > > + .read_event_value = &ad8460_read_event_value, > > + .write_event_config = &ad8460_write_event_config, > > + .read_event_config = &ad8460_read_event_config, > > + .debugfs_reg_access = &ad8460_reg_access, }; > > + > > +static const struct iio_enum ad8460_powerdown_mode_enum = { > > + .items = ad8460_powerdown_modes, > > + .num_items = ARRAY_SIZE(ad8460_powerdown_modes), > > + .get = ad8460_get_powerdown_mode, > > + .set = ad8460_set_powerdown_mode, > > +}; > > + > > +#define AD8460_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) { > \ > > Looks like _shared is always IIO_SEPARATE, so we could omit that parameter. > > > + .name = _name, > \ > > + .read = (_read), \ > > + .write = (_write), \ > > + .private = (_what), \ > > + .shared = (_shared), \ > > +} > > + > > +static struct iio_chan_spec_ext_info ad8460_ext_info[] = { > > + AD8460_CHAN_EXT_INFO("raw0", 0, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw1", 1, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw2", 2, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw3", 3, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw4", 4, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw5", 5, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw6", 6, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw7", 7, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw8", 8, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw9", 9, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw10", 10, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw11", 11, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw12", 12, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw13", 13, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw14", 14, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw15", 15, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("toggle_en", 0, IIO_SEPARATE, > ad8460_read_toggle_en, > > + ad8460_write_toggle_en), > > + AD8460_CHAN_EXT_INFO("symbol", 0, IIO_SEPARATE, > ad8460_read_symbol, > > + ad8460_write_symbol), > > + AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE, > ad8460_read_powerdown, > > + ad8460_write_powerdown), > > + IIO_ENUM("powerdown_mode", IIO_SEPARATE, > &ad8460_powerdown_mode_enum), > > + IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE, > > + &ad8460_powerdown_mode_enum), > > + {} > > +}; > > + > > +static const struct iio_event_spec ad8460_events[] = { > > + { > > + .type = IIO_EV_TYPE_THRESH, > > + .dir = IIO_EV_DIR_RISING, > > + .mask_separate = BIT(IIO_EV_INFO_VALUE) | > > + BIT(IIO_EV_INFO_ENABLE), > > + }, > > + { > > + .type = IIO_EV_TYPE_THRESH, > > + .dir = IIO_EV_DIR_FALLING, > > + .mask_separate = BIT(IIO_EV_INFO_VALUE) | > > + BIT(IIO_EV_INFO_ENABLE), > > + }, > > +}; > > + > > +#define AD8460_VOLTAGE_CHAN { \ > > + .type = IIO_VOLTAGE, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ > > + BIT(IIO_CHAN_INFO_RAW) | \ > > + BIT(IIO_CHAN_INFO_SCALE), \ > > + .output = 1, \ > > + .indexed = 1, \ > > + .channel = 0, \ > > + .scan_index = 0, \ > > + .scan_type = { \ > > + .sign = 'u', \ > > + .realbits = 14, \ > > + .storagebits = 16, \ > > + .endianness = IIO_LE, \ > > Data is going over DMA, so should this be IIO_CPU? > > > + }, \ > > + .ext_info = ad8460_ext_info, \ > > + .event_spec = ad8460_events, \ > > + .num_event_specs = ARRAY_SIZE(ad8460_events), \ > > +} > > + > > +#define AD8460_CURRENT_CHAN { \ > > + .type = IIO_CURRENT, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .output = 0, \ > > Since this is writeable, I assume this is an output? (so = 1, not 0) > > > + .indexed = 1, \ > > + .channel = 0, \ > > + .scan_index = -1, \ > > + .event_spec = ad8460_events, \ > > + .num_event_specs = ARRAY_SIZE(ad8460_events), \ > > +} > > + > > +#define AD8460_TEMP_CHAN { \ > > + .type = IIO_TEMP, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .output = 1, \ > > + .indexed = 1, \ > > + .channel = 0, \ > > + .scan_index = -1, \ > > + .event_spec = ad8460_events, \ > > + .num_event_specs = 1, \ > > +} > > + > > +static const struct iio_chan_spec ad8460_channels[] = { > > + AD8460_VOLTAGE_CHAN, > > + AD8460_CURRENT_CHAN, > > +}; > > + > > +static const struct iio_chan_spec ad8460_channels_with_tmp_adc[] = { > > + AD8460_VOLTAGE_CHAN, > > + AD8460_CURRENT_CHAN, > > + AD8460_TEMP_CHAN, > > +}; > > + > > +static const struct regmap_config ad8460_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = 0x7F, > > +}; > > + > > +static int ad8460_probe(struct spi_device *spi) { > > + struct ad8460_state *state; > > + struct iio_dev *indio_dev; > > + struct device *dev; > > + u32 tmp[2], temp; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + state = iio_priv(indio_dev); > > + mutex_init(&state->lock); > > + > > + indio_dev->name = "ad8460"; > > + indio_dev->info = &ad8460_info; > > + > > + state->spi = spi; > > + dev = &spi->dev; > > + > > + state->regmap = devm_regmap_init_spi(spi, > &ad8460_regmap_config); > > + if (IS_ERR(state->regmap)) > > + return dev_err_probe(dev, PTR_ERR(state->regmap), > > + "Failed to initialize regmap"); > > + > > + state->sync_clk = devm_clk_get_enabled(dev, NULL); > > + if (IS_ERR(state->sync_clk)) > > + return dev_err_probe(dev, PTR_ERR(state->sync_clk), > > + "Failed to get sync clk\n"); > > + > > + state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460- > tmp"); > > Should we check for specific errors here? For example what if we get - > EPROBE_DEFER? Also, it doesn't like we could ever get NULL, so IS_ERR() > should be sufficient. > > > + if (IS_ERR_OR_NULL(state->tmp_adc_channel)) { > > + indio_dev->channels = ad8460_channels; > > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels); > > + } else { > > + indio_dev->channels = ad8460_channels_with_tmp_adc; > > + indio_dev->num_channels = > ARRAY_SIZE(ad8460_channels_with_tmp_adc); > > + } > > + > > + ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v"); > > + if (ret == -ENODEV) > > + state->refio_1p2v_mv = 1200; > > + else if (ret >= 0) > > + state->refio_1p2v_mv = ret / 1000; > > + else > > + return dev_err_probe(dev, ret, "Failed to get reference > > +voltage\n"); > > The pattern we have been using in other drivers is: > > ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v"); > if (ret < 0 && ret != -ENODEV) > return dev_err_probe(dev, ret, "Failed to get reference > voltage\n"); > > state->refio_1p2v_mv = ret == -ENODEV ? 1200 : ret / 1000; > > It saves a bit of reading. > > > + > > + if (!in_range(state->refio_1p2v_mv, AD8460_MIN_VREFIO_UV, > > + AD8460_MAX_VREFIO_UV)) > > It looks like millivolts is being compared to microvolts here. > > > + return dev_err_probe(dev, -EINVAL, > > + "Invalid ref voltage range(%u mV) [%u mV, > %u mV]\n", > > + state->refio_1p2v_mv, > > + AD8460_MIN_VREFIO_UV / 1000, > > + AD8460_MAX_VREFIO_UV / 1000); > > + > > + ret = device_property_read_u32(dev, "adi,external-resistor-ohms", > > + &state->ext_resistor_ohms); > > + if (ret) > > + state->ext_resistor_ohms = 2000; > > + else if (!in_range(state->ext_resistor_ohms, > AD8460_MIN_EXT_RESISTOR_OHMS, > > + AD8460_MAX_EXT_RESISTOR_OHMS)) > > + return dev_err_probe(dev, -EINVAL, > > + "Invalid resistor set range(%u) [%u, %u]\n", > > + state->ext_resistor_ohms, > > + AD8460_MIN_EXT_RESISTOR_OHMS, > > + AD8460_MAX_EXT_RESISTOR_OHMS); > > + > > + /* Arm the device by default */ > > Comment doesn't match the code. > > > + ret = device_property_read_u32_array(dev, "adi,range-microamp", > > + tmp, ARRAY_SIZE(tmp)); > > + if (!ret) { > > + if (in_range(tmp[1], 0, > AD8460_ABS_MAX_OVERCURRENT_UA)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x08), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + AD8460_CURRENT_LIMIT_CONV(tmp[1])); > > + > > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERCURRENT_UA, > 0)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x09), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + > AD8460_CURRENT_LIMIT_CONV(abs(tmp[0]))); > > + } > > + > > + ret = device_property_read_u32_array(dev, "adi,range-microvolt", > > + tmp, ARRAY_SIZE(tmp)); > > + if (!ret) { > > + if (in_range(tmp[1], 0, > AD8460_ABS_MAX_OVERVOLTAGE_UV)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x0A), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + AD8460_VOLTAGE_LIMIT_CONV(tmp[1])); > > + > > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERVOLTAGE_UV, > 0)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x0B), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + > AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0]))); > > + } > > + > > + ret = device_property_read_u32(dev, "adi,max-millicelsius", &temp); > > + if (!ret) { > > + if (in_range(temp, AD8460_MIN_OVERTEMPERATURE_MC, > > + AD8460_MAX_OVERTEMPERATURE_MC)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x0C), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + AD8460_TEMP_LIMIT_CONV(abs(temp))); > > + } > > + > > + ret = ad8460_reset(state); > > + if (ret) > > + return ret; > > + > > + /* Enables DAC by default */ > > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), > > + AD8460_HVDAC_SLEEP_MSK, > > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, > 0)); > > + if (ret) > > + return ret; > > + > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->setup_ops = &ad8460_buffer_setup_ops; > > + > > + ret = devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev, "tx", > > + > IIO_BUFFER_DIRECTION_OUT); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Failed to get DMA buffer\n"); > > + > > + return devm_iio_device_register(dev, indio_dev); } > > + > > +static const struct of_device_id ad8460_of_match[] = { > > + { .compatible = "adi, ad8460" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, ad8460_of_match); > > + > > +static struct spi_driver ad8460_driver = { > > + .driver = { > > + .name = "ad8460", > > + .of_match_table = ad8460_of_match, > > + }, > > + .probe = ad8460_probe, > > +}; > > +module_spi_driver(ad8460_driver); > > + > > +MODULE_AUTHOR("Mariel Tinaco <mariel.tinaco@analog.com"); > > +MODULE_DESCRIPTION("AD8460 DAC driver"); MODULE_LICENSE("GPL"); > > +MODULE_IMPORT_NS(IIO_DMAENGINE_BUFFER); ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-09 9:47 ` Tinaco, Mariel @ 2024-09-09 14:51 ` David Lechner 2024-09-09 19:09 ` Jonathan Cameron 0 siblings, 1 reply; 20+ messages in thread From: David Lechner @ 2024-09-09 14:51 UTC (permalink / raw) To: Tinaco, Mariel, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, Nuno Sá On 9/9/24 4:47 AM, Tinaco, Mariel wrote: > > ... >>> + *val = get_unaligned_le16(state->spi_tx_buf); >> >> With spi_tx_buf changed to __le16, this can use le16_to_cpu() instead of >> get_unaligned_le16(). >> > > I suppose we use the cpu_to_le16 for the set_hvdac_word function? > > static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val) > { > state->spi_tx_buf = cpu_to_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val)); > > return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD(index), > &state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH); > } > Yes, that looks correct. >>> +static ssize_t ad8460_write_toggle_en(struct iio_dev *indio_dev, uintptr_t >> private, >>> + const struct iio_chan_spec *chan, >>> + const char *buf, size_t len) { >>> + struct ad8460_state *state = iio_priv(indio_dev); >>> + bool toggle_en; >>> + int ret; >>> + >>> + ret = kstrtobool(buf, &toggle_en); >>> + if (ret) >>> + return ret; >>> + >>> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) >>> + return ad8460_enable_apg_mode(state, toggle_en); >>> + unreachable(); >>> +} >> >> Hmm... do we need to make an unscoped version of >> iio_device_claim_direct_scoped()? >> > > So iio_device_claim_direct_scoped is used here because the buffer enable/disable > accesses this enable_apg_mode function. Is it also a standard practice to put the > kstrobool() util inside the scope? > Since this is at the end of a function with nothing after it, it would be nice if we could avoid the indent and unreachable(); The idea would be to write the last 3 lines like this instead: guard_cond(iio_device_claim_direct, return -EBUSY, indio_dev); But I didn't see a `guard_cond()` analog of `guard()` in linux/cleanup.h. So this is probably fine for now and adding `guard_cond()` (if it is actually a good idea in the first place) can be a job for another time. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-09 14:51 ` David Lechner @ 2024-09-09 19:09 ` Jonathan Cameron 0 siblings, 0 replies; 20+ messages in thread From: Jonathan Cameron @ 2024-09-09 19:09 UTC (permalink / raw) To: David Lechner Cc: Tinaco, Mariel, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, Nuno Sá On Mon, 9 Sep 2024 09:51:35 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 9/9/24 4:47 AM, Tinaco, Mariel wrote: > > > > > > ... > > >>> + *val = get_unaligned_le16(state->spi_tx_buf); > >> > >> With spi_tx_buf changed to __le16, this can use le16_to_cpu() instead of > >> get_unaligned_le16(). > >> > > > > I suppose we use the cpu_to_le16 for the set_hvdac_word function? > > > > static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val) > > { > > state->spi_tx_buf = cpu_to_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val)); > > > > return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD(index), > > &state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH); > > } > > > > Yes, that looks correct. > > > >>> +static ssize_t ad8460_write_toggle_en(struct iio_dev *indio_dev, uintptr_t > >> private, > >>> + const struct iio_chan_spec *chan, > >>> + const char *buf, size_t len) { > >>> + struct ad8460_state *state = iio_priv(indio_dev); > >>> + bool toggle_en; > >>> + int ret; > >>> + > >>> + ret = kstrtobool(buf, &toggle_en); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) > >>> + return ad8460_enable_apg_mode(state, toggle_en); > >>> + unreachable(); > >>> +} > >> > >> Hmm... do we need to make an unscoped version of > >> iio_device_claim_direct_scoped()? > >> > > > > So iio_device_claim_direct_scoped is used here because the buffer enable/disable > > accesses this enable_apg_mode function. Is it also a standard practice to put the > > kstrobool() util inside the scope? > > > > Since this is at the end of a function with nothing after it, it > would be nice if we could avoid the indent and unreachable(); > > The idea would be to write the last 3 lines like this instead: > > guard_cond(iio_device_claim_direct, return -EBUSY, indio_dev); > > But I didn't see a `guard_cond()` analog of `guard()` in > linux/cleanup.h. So this is probably fine for now and adding > `guard_cond()` (if it is actually a good idea in the first > place) can be a job for another time. That's a fun topic if you check the archives. Linus really didn't like the proposal https://lore.kernel.org/all/170905253339.2268463.9376907713092612237.stgit@dwillia2-xfh.jf.intel.com/ Mind you I don't think he much likes scoped_cond_guard() either! Jonathan ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-06 0:50 ` David Lechner 2024-09-09 9:47 ` Tinaco, Mariel @ 2024-09-10 1:44 ` Tinaco, Mariel 2024-09-14 11:45 ` Jonathan Cameron 1 sibling, 1 reply; 20+ messages in thread From: Tinaco, Mariel @ 2024-09-10 1:44 UTC (permalink / raw) To: David Lechner, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, Nuno Sá > -----Original Message----- > From: David Lechner <dlechner@baylibre.com> > Sent: Friday, September 6, 2024 8:50 AM > 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>; Nuno Sá > <noname.nuno@gmail.com> > Subject: Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC > > [External] > > On 9/3/24 9:30 PM, Mariel Tinaco wrote: > > The AD8460 is a “bits in, power out” high voltage, high-power, > > high-speed driver optimized for large output current (up to ±1 A) and > > high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) into > > capacitive loads. > > > > A digital engine implements user-configurable features: modes for > > digital input, programmable supply current, and fault monitoring and > > programmable protection settings for output current, output voltage, > > and junction temperature. The AD8460 operates on high voltage dual > > supplies up to ±55 V and a single low voltage supply of 5 V. > > > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> > > --- > > MAINTAINERS | 1 + > > drivers/iio/dac/Kconfig | 13 + > > drivers/iio/dac/Makefile | 1 + > > drivers/iio/dac/ad8460.c | 932 > > +++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 947 insertions(+) > > create mode 100644 drivers/iio/dac/ad8460.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS index > > e0509c9f5545..30746a3f0bbd 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -1326,6 +1326,7 @@ 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 > > +F: drivers/iio/dac/ad8460.c > > > > ANALOG DEVICES INC AD9739a DRIVER > > M: Nuno Sa <nuno.sa@analog.com> > > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig index > > 1cfd7e2a622f..fa091995d002 100644 > > --- a/drivers/iio/dac/Kconfig > > +++ b/drivers/iio/dac/Kconfig > > @@ -301,6 +301,19 @@ config AD7303 > > To compile this driver as module choose M here: the module will be > called > > ad7303. > > > > +config AD8460 > > + tristate "Analog Devices AD8460 DAC driver" > > + depends on SPI > > + select REGMAP_SPI > > + select IIO_BUFFER > > + select IIO_BUFFER_DMAENGINE > > + help > > + Say yes here to build support for Analog Devices AD8460 Digital to > > + Analog Converters (DAC). > > + > > + To compile this driver as a module choose M here: the module will be > called > > + ad8460. > > + > > config AD8801 > > tristate "Analog Devices AD8801/AD8803 DAC driver" > > depends on SPI_MASTER > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile index > > 2cf148f16306..621d553bd6e3 100644 > > --- a/drivers/iio/dac/Makefile > > +++ b/drivers/iio/dac/Makefile > > @@ -28,6 +28,7 @@ obj-$(CONFIG_AD5686_SPI) += ad5686-spi.o > > obj-$(CONFIG_AD5696_I2C) += ad5696-i2c.o > > obj-$(CONFIG_AD7293) += ad7293.o > > obj-$(CONFIG_AD7303) += ad7303.o > > +obj-$(CONFIG_AD8460) += ad8460.o > > obj-$(CONFIG_AD8801) += ad8801.o > > obj-$(CONFIG_AD9739A) += ad9739a.o > > obj-$(CONFIG_ADI_AXI_DAC) += adi-axi-dac.o diff --git > > a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c new file mode > > 100644 index 000000000000..6428bfaf0bd7 > > --- /dev/null > > +++ b/drivers/iio/dac/ad8460.c > > @@ -0,0 +1,932 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * AD8460 Waveform generator DAC Driver > > + * > > + * Copyright (C) 2024 Analog Devices, Inc. > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/cleanup.h> > > +#include <linux/clk.h> > > +#include <linux/debugfs.h> > > +#include <linux/delay.h> > > +#include <linux/dmaengine.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/iio/buffer.h> > > +#include <linux/iio/buffer-dma.h> > > +#include <linux/iio/buffer-dmaengine.h> #include > > +<linux/iio/consumer.h> #include <linux/iio/events.h> #include > > +<linux/iio/iio.h> #include <linux/kernel.h> #include <linux/module.h> > > +#include <linux/mod_devicetable.h> #include <linux/regmap.h> #include > > +<linux/regulator/consumer.h> #include <linux/spi/spi.h> > > + > > +#define AD8460_CTRL_REG(x) (x) > > +#define AD8460_HVDAC_DATA_WORD_LOW(x) (0x60 + (2 * (x))) > > +#define AD8460_HVDAC_DATA_WORD_HIGH(x) (0x61 + (2 * (x))) > > I don't see AD8460_HVDAC_DATA_WORD_HIGH() used anywhere, so we > could remove that and rename AD8460_HVDAC_DATA_WORD_LOW() to > AD8460_HVDAC_DATA_WORD(). > > > + > > +#define AD8460_HV_RESET_MSK BIT(7) > > +#define AD8460_HV_SLEEP_MSK BIT(4) > > +#define AD8460_WAVE_GEN_MODE_MSK BIT(0) > > + > > +#define AD8460_HVDAC_SLEEP_MSK BIT(3) > > + > > +#define AD8460_FAULT_ARM_MSK BIT(7) > > +#define AD8460_FAULT_LIMIT_MSK GENMASK(6, 0) > > + > > +#define AD8460_APG_MODE_ENABLE_MSK BIT(5) > > +#define AD8460_PATTERN_DEPTH_MSK GENMASK(3, 0) > > + > > +#define AD8460_QUIESCENT_CURRENT_MSK GENMASK(7, > 0) > > + > > +#define AD8460_SHUTDOWN_FLAG_MSK BIT(7) > > + > > +#define AD8460_DATA_BYTE_LOW_MSK GENMASK(7, 0) > > +#define AD8460_DATA_BYTE_HIGH_MSK GENMASK(5, 0) > > +#define AD8460_DATA_BYTE_FULL_MSK GENMASK(13, 0) > > + > > +#define AD8460_DEFAULT_FAULT_PROTECT 0x00 > > +#define AD8460_DATA_BYTE_WORD_LENGTH 2 > > +#define AD8460_NUM_DATA_WORDS 16 > > +#define AD8460_NOMINAL_VOLTAGE_SPAN 80 > > +#define AD8460_MIN_EXT_RESISTOR_OHMS 2000 > > +#define AD8460_MAX_EXT_RESISTOR_OHMS 20000 > > +#define AD8460_MIN_VREFIO_UV 120000 > > +#define AD8460_MAX_VREFIO_UV 1200000 > > +#define AD8460_ABS_MAX_OVERVOLTAGE_UV 55000000 > > +#define AD8460_ABS_MAX_OVERCURRENT_UA 1000000 > > +#define AD8460_MAX_OVERTEMPERATURE_MC 150000 > > +#define AD8460_MIN_OVERTEMPERATURE_MC 20000 > > +#define AD8460_CURRENT_LIMIT_CONV(x) ((x) / 15625) > > +#define AD8460_VOLTAGE_LIMIT_CONV(x) ((x) / 1953000) > > +#define AD8460_TEMP_LIMIT_CONV(x) (((x) + 266640) / > 6510) > > It looks like there are issues with mixed tabs and spaces in the macros above > that should be cleaned up. > > > + > > +enum ad8460_fault_type { > > + AD8460_OVERCURRENT_SRC, > > + AD8460_OVERCURRENT_SNK, > > + AD8460_OVERVOLTAGE_POS, > > + AD8460_OVERVOLTAGE_NEG, > > + AD8460_OVERTEMPERATURE, > > +}; > > + > > +struct ad8460_state { > > + struct spi_device *spi; > > + struct regmap *regmap; > > + struct iio_channel *tmp_adc_channel; > > + struct clk *sync_clk; > > + /* lock to protect against multiple access to the device and shared data > */ > > + struct mutex lock; > > + int refio_1p2v_mv; > > + u32 ext_resistor_ohms; > > + /* > > + * DMA (thus cache coherency maintenance) requires the > > + * transfer buffers to live in their own cache lines. > > + */ > > + u8 spi_tx_buf[2] __aligned(IIO_DMA_MINALIGN); > > If we make this `__le16 spi_tx_buf` instead of `u8 spi_tx_buf[2]`, then we > don't need the unaligned access functions. > > > +}; > > + > > +static int ad8460_hv_reset(struct ad8460_state *state) { > > + int ret; > > + > > + ret = regmap_update_bits(state->regmap, > AD8460_CTRL_REG(0x00), > > + AD8460_HV_RESET_MSK, > > + FIELD_PREP(AD8460_HV_RESET_MSK, 1)); > > Can be simplified with regmap_set_bits(). > > > + if (ret) > > + return ret; > > + > > + fsleep(20); > > + > > + return regmap_update_bits(state->regmap, > AD8460_CTRL_REG(0x00), > > + AD8460_HV_RESET_MSK, > > + FIELD_PREP(AD8460_HV_RESET_MSK, 0)); > > and regmap_clear_bits(). > > > +} > > + > > +static int ad8460_reset(const struct ad8460_state *state) { > > + struct device *dev = &state->spi->dev; > > + struct gpio_desc *reset; > > + > > + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > > + if (IS_ERR(reset)) > > + return dev_err_probe(dev, PTR_ERR(reset), > > + "Failed to get reset gpio"); > > + if (reset) { > > + /* minimum duration of 10ns */ > > + ndelay(10); > > + gpiod_set_value_cansleep(reset, 1); > > + return 0; > > + } > > + > > + /* bring all registers to their default state */ > > + return regmap_write(state->regmap, AD8460_CTRL_REG(0x03), 1); } > > + > > +static int ad8460_enable_apg_mode(struct ad8460_state *state, int > > +val) { > > + int ret; > > + > > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x02), > > + AD8460_APG_MODE_ENABLE_MSK, > > + > FIELD_PREP(AD8460_APG_MODE_ENABLE_MSK, val)); > > + if (ret) > > + return ret; > > + > > + return regmap_update_bits(state->regmap, > AD8460_CTRL_REG(0x00), > > + AD8460_WAVE_GEN_MODE_MSK, > > + > FIELD_PREP(AD8460_WAVE_GEN_MODE_MSK, val)); } > > + > > +static int ad8460_read_shutdown_flag(struct ad8460_state *state, u64 > > +*flag) { > > + int ret, val; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x0E), &val); > > + if (ret) > > + return ret; > > + > > + *flag = FIELD_GET(AD8460_SHUTDOWN_FLAG_MSK, val); > > + return 0; > > +} > > + > > +static int ad8460_get_hvdac_word(struct ad8460_state *state, int > > +index, int *val) { > > + int ret; > > + > > + ret = regmap_bulk_read(state->regmap, > AD8460_HVDAC_DATA_WORD_LOW(index), > > + &state->spi_tx_buf, > AD8460_DATA_BYTE_WORD_LENGTH); > > + if (ret) > > + return ret; > > + > > + *val = get_unaligned_le16(state->spi_tx_buf); > > With spi_tx_buf changed to __le16, this can use le16_to_cpu() instead of > get_unaligned_le16(). > > > + > > + return ret; > > +} > > + > > +static int ad8460_set_hvdac_word(struct ad8460_state *state, int > > +index, int val) { > > + put_unaligned_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, > val), > > + &state->spi_tx_buf); > > + > > + return regmap_bulk_write(state->regmap, > AD8460_HVDAC_DATA_WORD_LOW(index), > > + state->spi_tx_buf, > AD8460_DATA_BYTE_WORD_LENGTH); } > > + > > +static ssize_t ad8460_dac_input_read(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, char > *buf) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int reg; > > + int ret; > > + > > + ret = ad8460_get_hvdac_word(state, private, ®); > > + if (ret) > > + return ret; > > + > > + return sysfs_emit(buf, "%d\n", reg); > > %u instead of %d since reg is unsigned > > > +} > > + > > +static ssize_t ad8460_dac_input_write(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, > > + const char *buf, size_t len) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int reg; > > + int ret; > > + > > + ret = kstrtou32(buf, 10, ®); > > + if (ret) > > + return ret; > > + > > + guard(mutex)(&state->lock); > > + > > + return ad8460_set_hvdac_word(state, private, reg); } > > + > > +static ssize_t ad8460_read_symbol(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, char *buf) > { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int reg; > > + int ret; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®); > > + if (ret) > > + return ret; > > + > > + return sysfs_emit(buf, "%ld\n", > FIELD_GET(AD8460_PATTERN_DEPTH_MSK, > > +reg)); } > > I guess FIELD_GET() makes it long? but probalby should be %lu. > > > + > > +static ssize_t ad8460_write_symbol(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, > > + const char *buf, size_t len) > > +{ > > + struct ad8460_state *state = iio_priv(indio_dev); > > + uint16_t sym; > > + int ret; > > + > > + ret = kstrtou16(buf, 10, &sym); > > + if (ret) > > + return ret; > > + > > + guard(mutex)(&state->lock); > > + > > + return regmap_update_bits(state->regmap, > > + AD8460_CTRL_REG(0x02), > > + AD8460_PATTERN_DEPTH_MSK, > > + > FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, sym)); } > > + > > +static ssize_t ad8460_read_toggle_en(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, char > *buf) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int reg; > > + int ret; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®); > > + if (ret) > > + return ret; > > + > > + return sysfs_emit(buf, "%ld\n", > > +FIELD_GET(AD8460_APG_MODE_ENABLE_MSK, reg)); } > > + > > +static ssize_t ad8460_write_toggle_en(struct iio_dev *indio_dev, uintptr_t > private, > > + const struct iio_chan_spec *chan, > > + const char *buf, size_t len) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + bool toggle_en; > > + int ret; > > + > > + ret = kstrtobool(buf, &toggle_en); > > + if (ret) > > + return ret; > > + > > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) > > + return ad8460_enable_apg_mode(state, toggle_en); > > + unreachable(); > > Hmm... do we need to make an unscoped version of > iio_device_claim_direct_scoped()? > > > +} > > + > > +static ssize_t ad8460_read_powerdown(struct iio_dev *indio_dev, > uintptr_t private, > > + const struct iio_chan_spec *chan, char > *buf) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int reg; > > + int ret; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x01), ®); > > + if (ret) > > + return ret; > > + > > + return sysfs_emit(buf, "%ld\n", > FIELD_GET(AD8460_HVDAC_SLEEP_MSK, > > +reg)); } > > + > > +static ssize_t ad8460_write_powerdown(struct iio_dev *indio_dev, > uintptr_t private, > > + const struct iio_chan_spec *chan, > > + const char *buf, size_t len) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + bool pwr_down; > > + u64 sdn_flag; > > + int ret; > > + > > + ret = kstrtobool(buf, &pwr_down); > > + if (ret) > > + return ret; > > + > > + guard(mutex)(&state->lock); > > + > > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), > > + AD8460_HVDAC_SLEEP_MSK, > > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, > pwr_down)); > > + if (ret) > > + return ret; > > + > > + if (!pwr_down) { > > + ret = ad8460_read_shutdown_flag(state, &sdn_flag); > > + if (ret) > > + return ret; > > + > > + if (sdn_flag) { > > + ret = ad8460_hv_reset(state); > > + if (ret) > > + return ret; > > + } > > + } > > + > > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00), > > + AD8460_HV_SLEEP_MSK, > > + FIELD_PREP(AD8460_HV_SLEEP_MSK, > !pwr_down)); > > + if (ret) > > + return ret; > > Some comments explaining the logic in this function would be helpful. > Without reading the datasheet, it looks like this puts it in powerdown mode > and takes it right back out before returning. > > > + > > + return len; > > +} > > + > > +static const char * const ad8460_powerdown_modes[] = { > > + "three_state", > > +}; > > + > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan) { > > + return 0; > > +} > > + > > +static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + unsigned int type) > > +{ > > + return 0; > > +} > > + > > +static int ad8460_set_sample(struct ad8460_state *state, int val) { > > + int ret; > > + > > + ret = ad8460_enable_apg_mode(state, 1); > > + if (ret) > > + return ret; > > + > > + guard(mutex)(&state->lock); > > + ret = ad8460_set_hvdac_word(state, 0, val); > > + if (ret) > > + return ret; > > + > > + return regmap_update_bits(state->regmap, > > + AD8460_CTRL_REG(0x02), > > + AD8460_PATTERN_DEPTH_MSK, > > + > FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0)); } > > + > > +static int ad8460_set_fault_threshold(struct ad8460_state *state, > > + enum ad8460_fault_type fault, > > + unsigned int threshold) > > +{ > > + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08 > + fault), > > + AD8460_FAULT_LIMIT_MSK, > > + FIELD_PREP(AD8460_FAULT_LIMIT_MSK, > threshold)); } > > + > > +static int ad8460_get_fault_threshold(struct ad8460_state *state, > > + enum ad8460_fault_type fault, > > + unsigned int *threshold) > > +{ > > + unsigned int val; > > + int ret; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault), > &val); > > + if (ret) > > + return ret; > > + > > + *threshold = FIELD_GET(AD8460_FAULT_LIMIT_MSK, val); > > + > > + return ret; > > +} > > + > > +static int ad8460_set_fault_threshold_en(struct ad8460_state *state, > > + enum ad8460_fault_type fault, bool > en) { > > + return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08 > + fault), > > + AD8460_FAULT_ARM_MSK, > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, > en)); } > > + > > +static int ad8460_get_fault_threshold_en(struct ad8460_state *state, > > + enum ad8460_fault_type fault, bool > *en) { > > + unsigned int val; > > + int ret; > > + > > + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault), > &val); > > + if (ret) > > + return ret; > > + > > + *en = FIELD_GET(AD8460_FAULT_ARM_MSK, val); > > + > > + return 0; > > +} > > + > > +static int ad8460_write_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, int val, int val2, > > + long mask) > > +{ > > + struct ad8460_state *state = iio_priv(indio_dev); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + switch (chan->type) { > > + case IIO_VOLTAGE: > > + iio_device_claim_direct_scoped(return -EBUSY, > indio_dev) > > + return ad8460_set_sample(state, val); > > + unreachable(); > > + case IIO_CURRENT: > > + return regmap_write(state->regmap, > AD8460_CTRL_REG(0x04), > > + > FIELD_PREP(AD8460_QUIESCENT_CURRENT_MSK, val)); > > + default: > > + return -EINVAL; > > + } > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int ad8460_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec > const *chan, > > + int *val, int *val2, long mask) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + int data, ret; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + switch (chan->type) { > > + case IIO_VOLTAGE: > > + scoped_guard(mutex, &state->lock) { > > + ret = ad8460_get_hvdac_word(state, 0, > &data); > > + if (ret) > > + return ret; > > + } > > + *val = data; > > + return IIO_VAL_INT; > > + case IIO_CURRENT: > > + ret = regmap_read(state->regmap, > AD8460_CTRL_REG(0x04), > > + &data); > > + if (ret) > > + return ret; > > + *val = data; > > + return IIO_VAL_INT; > > + case IIO_TEMP: > > + ret = iio_read_channel_raw(state->tmp_adc_channel, > &data); > > + if (ret) > > + return ret; > > + *val = data; > > + return IIO_VAL_INT; > > + default: > > + return -EINVAL; > > + } > > + case IIO_CHAN_INFO_SAMP_FREQ: > > + *val = clk_get_rate(state->sync_clk); > > + return IIO_VAL_INT; > > + case IIO_CHAN_INFO_SCALE: > > + /* > > + * vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V > > + * vMAX = vNOMINAL_SPAN * (2**14 / 2**14) - 40V > > + * vMIN = vNOMINAL_SPAN * (0 / 2**14) - 40V > > + * vADJ = vCONV * (2000 / rSET) * (vREF / 1.2) > > + * vSPAN = vADJ_MAX - vADJ_MIN > > + * See datasheet page 49, section FULL-SCALE REDUCTION > > + */ > > + *val = AD8460_NOMINAL_VOLTAGE_SPAN * 2000 * state- > >refio_1p2v_mv; > > + *val2 = state->ext_resistor_ohms * 1200; > > + return IIO_VAL_FRACTIONAL; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int ad8460_select_fault_type(int chan_type, enum > > +iio_event_direction dir) { > > + switch (chan_type) { > > + case IIO_VOLTAGE: > > + switch (dir) { > > + case IIO_EV_DIR_RISING: > > + return AD8460_OVERVOLTAGE_POS; > > + case IIO_EV_DIR_FALLING: > > + return AD8460_OVERVOLTAGE_NEG; > > + default: > > + return -EINVAL; > > + } > > + case IIO_CURRENT: > > + switch (dir) { > > + case IIO_EV_DIR_RISING: > > + return AD8460_OVERCURRENT_SRC; > > + case IIO_EV_DIR_FALLING: > > + return AD8460_OVERCURRENT_SNK; > > + default: > > + return -EINVAL; > > + } > > + case IIO_TEMP: > > + switch (dir) { > > + case IIO_EV_DIR_RISING: > > + return AD8460_OVERTEMPERATURE; > > + default: > > + return -EINVAL; > > + } > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int ad8460_write_event_value(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, > > + enum iio_event_info info, int val, int val2) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int fault; > > + > > + if (type != IIO_EV_TYPE_THRESH) > > + return -EINVAL; > > + > > + if (info != IIO_EV_INFO_VALUE) > > + return -EINVAL; > > + > > + fault = ad8460_select_fault_type(chan->type, dir); > > + if (fault < 0) > > + return fault; > > + > > + return ad8460_set_fault_threshold(state, fault, val); } > > + > > +static int ad8460_read_event_value(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, > > + enum iio_event_info info, int *val, int *val2) > { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int fault; > > + > > + if (type != IIO_EV_TYPE_THRESH) > > + return -EINVAL; > > + > > + if (info != IIO_EV_INFO_VALUE) > > + return -EINVAL; > > + > > + fault = ad8460_select_fault_type(chan->type, dir); > > + if (fault < 0) > > + return fault; > > + > > + return ad8460_get_fault_threshold(state, fault, val); } > > + > > +static int ad8460_write_event_config(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, int val) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int fault; > > + > > + if (type != IIO_EV_TYPE_THRESH) > > + return -EINVAL; > > + > > + fault = ad8460_select_fault_type(chan->type, dir); > > + if (fault < 0) > > + return fault; > > + > > + return ad8460_set_fault_threshold_en(state, fault, val); } > > + > > +static int ad8460_read_event_config(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + unsigned int fault; > > + bool en; > > + int ret; > > + > > + if (type != IIO_EV_TYPE_THRESH) > > + return -EINVAL; > > + > > + fault = ad8460_select_fault_type(chan->type, dir); > > + if (fault < 0) > > + return fault; > > + > > + ret = ad8460_get_fault_threshold_en(state, fault, &en); > > + if (ret) > > + return ret; > > + > > + return en; > > +} > > + > > +static int ad8460_reg_access(struct iio_dev *indio_dev, unsigned int reg, > > + unsigned int writeval, unsigned int *readval) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + > > + if (readval) > > + return regmap_read(state->regmap, reg, readval); > > + > > + return regmap_write(state->regmap, reg, writeval); } > > + > > +static int ad8460_buffer_preenable(struct iio_dev *indio_dev) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + > > + return ad8460_enable_apg_mode(state, 0); } > > + > > +static int ad8460_buffer_postdisable(struct iio_dev *indio_dev) { > > + struct ad8460_state *state = iio_priv(indio_dev); > > + > > + return ad8460_enable_apg_mode(state, 1); } > > + > > +static const struct iio_buffer_setup_ops ad8460_buffer_setup_ops = { > > + .preenable = &ad8460_buffer_preenable, > > + .postdisable = &ad8460_buffer_postdisable, }; > > + > > +static const struct iio_info ad8460_info = { > > + .read_raw = &ad8460_read_raw, > > + .write_raw = &ad8460_write_raw, > > + .write_event_value = &ad8460_write_event_value, > > + .read_event_value = &ad8460_read_event_value, > > + .write_event_config = &ad8460_write_event_config, > > + .read_event_config = &ad8460_read_event_config, > > + .debugfs_reg_access = &ad8460_reg_access, }; > > + > > +static const struct iio_enum ad8460_powerdown_mode_enum = { > > + .items = ad8460_powerdown_modes, > > + .num_items = ARRAY_SIZE(ad8460_powerdown_modes), > > + .get = ad8460_get_powerdown_mode, > > + .set = ad8460_set_powerdown_mode, > > +}; > > + > > +#define AD8460_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) { > \ > > Looks like _shared is always IIO_SEPARATE, so we could omit that parameter. > > > + .name = _name, > \ > > + .read = (_read), \ > > + .write = (_write), \ > > + .private = (_what), \ > > + .shared = (_shared), \ > > +} > > + > > +static struct iio_chan_spec_ext_info ad8460_ext_info[] = { > > + AD8460_CHAN_EXT_INFO("raw0", 0, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw1", 1, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw2", 2, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw3", 3, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw4", 4, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw5", 5, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw6", 6, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw7", 7, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw8", 8, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw9", 9, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw10", 10, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw11", 11, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw12", 12, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw13", 13, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw14", 14, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("raw15", 15, IIO_SEPARATE, > ad8460_dac_input_read, > > + ad8460_dac_input_write), > > + AD8460_CHAN_EXT_INFO("toggle_en", 0, IIO_SEPARATE, > ad8460_read_toggle_en, > > + ad8460_write_toggle_en), > > + AD8460_CHAN_EXT_INFO("symbol", 0, IIO_SEPARATE, > ad8460_read_symbol, > > + ad8460_write_symbol), > > + AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE, > ad8460_read_powerdown, > > + ad8460_write_powerdown), > > + IIO_ENUM("powerdown_mode", IIO_SEPARATE, > &ad8460_powerdown_mode_enum), > > + IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE, > > + &ad8460_powerdown_mode_enum), > > + {} > > +}; > > + > > +static const struct iio_event_spec ad8460_events[] = { > > + { > > + .type = IIO_EV_TYPE_THRESH, > > + .dir = IIO_EV_DIR_RISING, > > + .mask_separate = BIT(IIO_EV_INFO_VALUE) | > > + BIT(IIO_EV_INFO_ENABLE), > > + }, > > + { > > + .type = IIO_EV_TYPE_THRESH, > > + .dir = IIO_EV_DIR_FALLING, > > + .mask_separate = BIT(IIO_EV_INFO_VALUE) | > > + BIT(IIO_EV_INFO_ENABLE), > > + }, > > +}; > > + > > +#define AD8460_VOLTAGE_CHAN { \ > > + .type = IIO_VOLTAGE, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ > > + BIT(IIO_CHAN_INFO_RAW) | \ > > + BIT(IIO_CHAN_INFO_SCALE), \ > > + .output = 1, \ > > + .indexed = 1, \ > > + .channel = 0, \ > > + .scan_index = 0, \ > > + .scan_type = { \ > > + .sign = 'u', \ > > + .realbits = 14, \ > > + .storagebits = 16, \ > > + .endianness = IIO_LE, \ > > Data is going over DMA, so should this be IIO_CPU? > > > + }, \ > > + .ext_info = ad8460_ext_info, \ > > + .event_spec = ad8460_events, \ > > + .num_event_specs = ARRAY_SIZE(ad8460_events), \ > > +} > > + > > +#define AD8460_CURRENT_CHAN { \ > > + .type = IIO_CURRENT, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .output = 0, \ > > Since this is writeable, I assume this is an output? (so = 1, not 0) > > > + .indexed = 1, \ > > + .channel = 0, \ > > + .scan_index = -1, \ > > + .event_spec = ad8460_events, \ > > + .num_event_specs = ARRAY_SIZE(ad8460_events), \ > > +} > > + > > +#define AD8460_TEMP_CHAN { \ > > + .type = IIO_TEMP, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .output = 1, \ > > + .indexed = 1, \ > > + .channel = 0, \ > > + .scan_index = -1, \ > > + .event_spec = ad8460_events, \ > > + .num_event_specs = 1, \ > > +} > > + > > +static const struct iio_chan_spec ad8460_channels[] = { > > + AD8460_VOLTAGE_CHAN, > > + AD8460_CURRENT_CHAN, > > +}; > > + > > +static const struct iio_chan_spec ad8460_channels_with_tmp_adc[] = { > > + AD8460_VOLTAGE_CHAN, > > + AD8460_CURRENT_CHAN, > > + AD8460_TEMP_CHAN, > > +}; > > + > > +static const struct regmap_config ad8460_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = 0x7F, > > +}; > > + > > +static int ad8460_probe(struct spi_device *spi) { > > + struct ad8460_state *state; > > + struct iio_dev *indio_dev; > > + struct device *dev; > > + u32 tmp[2], temp; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + state = iio_priv(indio_dev); > > + mutex_init(&state->lock); > > + > > + indio_dev->name = "ad8460"; > > + indio_dev->info = &ad8460_info; > > + > > + state->spi = spi; > > + dev = &spi->dev; > > + > > + state->regmap = devm_regmap_init_spi(spi, > &ad8460_regmap_config); > > + if (IS_ERR(state->regmap)) > > + return dev_err_probe(dev, PTR_ERR(state->regmap), > > + "Failed to initialize regmap"); > > + > > + state->sync_clk = devm_clk_get_enabled(dev, NULL); > > + if (IS_ERR(state->sync_clk)) > > + return dev_err_probe(dev, PTR_ERR(state->sync_clk), > > + "Failed to get sync clk\n"); > > + > > + state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460- > tmp"); > > Should we check for specific errors here? For example what if we get - > EPROBE_DEFER? Also, it doesn't like we could ever get NULL, so IS_ERR() > should be sufficient. > It says in the docs that, the intended channel might return -EPROBE_DEFER If the driver associated with that channel depends on resources that are not Yet available. For this specific case, should I create a loop that waits for That channel to be available before proceeding with the probe function? How would this be implemented? Originally, this channel was intended to be optional. If any error results from Obtaining it, it will not be included in the channels. > > + if (IS_ERR_OR_NULL(state->tmp_adc_channel)) { > > + indio_dev->channels = ad8460_channels; > > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels); > > + } else { > > + indio_dev->channels = ad8460_channels_with_tmp_adc; > > + indio_dev->num_channels = > ARRAY_SIZE(ad8460_channels_with_tmp_adc); > > + } > > + > > + ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v"); > > + if (ret == -ENODEV) > > + state->refio_1p2v_mv = 1200; > > + else if (ret >= 0) > > + state->refio_1p2v_mv = ret / 1000; > > + else > > + return dev_err_probe(dev, ret, "Failed to get reference > > +voltage\n"); > > The pattern we have been using in other drivers is: > > ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v"); > if (ret < 0 && ret != -ENODEV) > return dev_err_probe(dev, ret, "Failed to get reference > voltage\n"); > > state->refio_1p2v_mv = ret == -ENODEV ? 1200 : ret / 1000; > > It saves a bit of reading. > > > + > > + if (!in_range(state->refio_1p2v_mv, AD8460_MIN_VREFIO_UV, > > + AD8460_MAX_VREFIO_UV)) > > It looks like millivolts is being compared to microvolts here. > > > + return dev_err_probe(dev, -EINVAL, > > + "Invalid ref voltage range(%u mV) [%u mV, > %u mV]\n", > > + state->refio_1p2v_mv, > > + AD8460_MIN_VREFIO_UV / 1000, > > + AD8460_MAX_VREFIO_UV / 1000); > > + > > + ret = device_property_read_u32(dev, "adi,external-resistor-ohms", > > + &state->ext_resistor_ohms); > > + if (ret) > > + state->ext_resistor_ohms = 2000; > > + else if (!in_range(state->ext_resistor_ohms, > AD8460_MIN_EXT_RESISTOR_OHMS, > > + AD8460_MAX_EXT_RESISTOR_OHMS)) > > + return dev_err_probe(dev, -EINVAL, > > + "Invalid resistor set range(%u) [%u, %u]\n", > > + state->ext_resistor_ohms, > > + AD8460_MIN_EXT_RESISTOR_OHMS, > > + AD8460_MAX_EXT_RESISTOR_OHMS); > > + > > + /* Arm the device by default */ > > Comment doesn't match the code. > > > + ret = device_property_read_u32_array(dev, "adi,range-microamp", > > + tmp, ARRAY_SIZE(tmp)); > > + if (!ret) { > > + if (in_range(tmp[1], 0, > AD8460_ABS_MAX_OVERCURRENT_UA)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x08), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + AD8460_CURRENT_LIMIT_CONV(tmp[1])); > > + > > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERCURRENT_UA, > 0)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x09), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + > AD8460_CURRENT_LIMIT_CONV(abs(tmp[0]))); > > + } > > + > > + ret = device_property_read_u32_array(dev, "adi,range-microvolt", > > + tmp, ARRAY_SIZE(tmp)); > > + if (!ret) { > > + if (in_range(tmp[1], 0, > AD8460_ABS_MAX_OVERVOLTAGE_UV)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x0A), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + AD8460_VOLTAGE_LIMIT_CONV(tmp[1])); > > + > > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERVOLTAGE_UV, > 0)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x0B), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + > AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0]))); > > + } > > + > > + ret = device_property_read_u32(dev, "adi,max-millicelsius", &temp); > > + if (!ret) { > > + if (in_range(temp, AD8460_MIN_OVERTEMPERATURE_MC, > > + AD8460_MAX_OVERTEMPERATURE_MC)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x0C), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + AD8460_TEMP_LIMIT_CONV(abs(temp))); > > + } > > + > > + ret = ad8460_reset(state); > > + if (ret) > > + return ret; > > + > > + /* Enables DAC by default */ > > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), > > + AD8460_HVDAC_SLEEP_MSK, > > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, > 0)); > > + if (ret) > > + return ret; > > + > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->setup_ops = &ad8460_buffer_setup_ops; > > + > > + ret = devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev, "tx", > > + > IIO_BUFFER_DIRECTION_OUT); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Failed to get DMA buffer\n"); > > + > > + return devm_iio_device_register(dev, indio_dev); } > > + > > +static const struct of_device_id ad8460_of_match[] = { > > + { .compatible = "adi, ad8460" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, ad8460_of_match); > > + > > +static struct spi_driver ad8460_driver = { > > + .driver = { > > + .name = "ad8460", > > + .of_match_table = ad8460_of_match, > > + }, > > + .probe = ad8460_probe, > > +}; > > +module_spi_driver(ad8460_driver); > > + > > +MODULE_AUTHOR("Mariel Tinaco <mariel.tinaco@analog.com"); > > +MODULE_DESCRIPTION("AD8460 DAC driver"); MODULE_LICENSE("GPL"); > > +MODULE_IMPORT_NS(IIO_DMAENGINE_BUFFER); Warm Regards, Mariel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-10 1:44 ` Tinaco, Mariel @ 2024-09-14 11:45 ` Jonathan Cameron 0 siblings, 0 replies; 20+ messages in thread From: Jonathan Cameron @ 2024-09-14 11:45 UTC (permalink / raw) To: Tinaco, Mariel Cc: David Lechner, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, Nuno Sá Hi Mariel, You get to be the person I moan at today (though not the only person doing it!). Please crop to only the relevant information for continuing the discussion. It is not necessarily easy to find where you reply if you keep too much context. ... > > > +static int ad8460_probe(struct spi_device *spi) { > > > + struct ad8460_state *state; > > > + struct iio_dev *indio_dev; > > > + struct device *dev; > > > + u32 tmp[2], temp; > > > + int ret; > > > + > > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state)); > > > + if (!indio_dev) > > > + return -ENOMEM; > > > + > > > + state = iio_priv(indio_dev); > > > + mutex_init(&state->lock); > > > + > > > + indio_dev->name = "ad8460"; > > > + indio_dev->info = &ad8460_info; > > > + > > > + state->spi = spi; > > > + dev = &spi->dev; > > > + > > > + state->regmap = devm_regmap_init_spi(spi, > > &ad8460_regmap_config); > > > + if (IS_ERR(state->regmap)) > > > + return dev_err_probe(dev, PTR_ERR(state->regmap), > > > + "Failed to initialize regmap"); > > > + > > > + state->sync_clk = devm_clk_get_enabled(dev, NULL); > > > + if (IS_ERR(state->sync_clk)) > > > + return dev_err_probe(dev, PTR_ERR(state->sync_clk), > > > + "Failed to get sync clk\n"); > > > + > > > + state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460- > > tmp"); > > > > Should we check for specific errors here? For example what if we get - > > EPROBE_DEFER? Also, it doesn't like we could ever get NULL, so IS_ERR() > > should be sufficient. > > > > It says in the docs that, the intended channel might return -EPROBE_DEFER > If the driver associated with that channel depends on resources that are not > Yet available. For this specific case, should I create a loop that waits for > That channel to be available before proceeding with the probe function? Normally I'd say fail the probe with EPROBE_DEFER but in this case it's awkward because this is far from a 'required' feature and whilst DT providing the channel would indicate that the board supports using it, that doesn't mean a given system has the driver for the ADC. I don't want to suggest we make this a CONFIG_XXX option but I can't immediately see an alternative that lets people intentionally not build the driver support for the ADC. > > How would this be implemented? > > Originally, this channel was intended to be optional. If any error results from > Obtaining it, it will not be included in the channels. > Given IIO drivers will probe in an unknown order this will fail perhaps half the time. Normally deferral deals with that, because the consumer isn't useful without the channel. Here it is... I'm open to other suggestions on this but right now it looks like only way to definitely handle it is a config option. Maybe for now we don't provide one and see if anyone cares? That is effectively make if required to provide an ADC driver if the DT describes the channel we are getting. If that driver isn't loaded yet -EPROBE_DEFER and wait for it to show up. Jonathan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-04 2:30 ` [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco ` (2 preceding siblings ...) 2024-09-06 0:50 ` David Lechner @ 2024-09-07 17:14 ` Jonathan Cameron 2024-09-09 8:22 ` Tinaco, Mariel 3 siblings, 1 reply; 20+ messages in thread From: Jonathan Cameron @ 2024-09-07 17:14 UTC (permalink / raw) To: Mariel Tinaco Cc: linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá On Wed, 4 Sep 2024 10:30:40 +0800 Mariel Tinaco <Mariel.Tinaco@analog.com> wrote: > The AD8460 is a “bits in, power out” high voltage, high-power, > high-speed driver optimized for large output current (up to ±1 A) > and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) > into capacitive loads. > > A digital engine implements user-configurable features: modes for > digital input, programmable supply current, and fault monitoring > and programmable protection settings for output current, > output voltage, and junction temperature. The AD8460 operates on > high voltage dual supplies up to ±55 V and a single low voltage > supply of 5 V. > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> A few comments inline. Jonathan > obj-$(CONFIG_ADI_AXI_DAC) += adi-axi-dac.o > diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c > new file mode 100644 > index 000000000000..6428bfaf0bd7 > --- /dev/null > +++ b/drivers/iio/dac/ad8460.c > @@ -0,0 +1,932 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * AD8460 Waveform generator DAC Driver > + * > + * Copyright (C) 2024 Analog Devices, Inc. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/cleanup.h> > +#include <linux/clk.h> > +#include <linux/debugfs.h> > +#include <linux/delay.h> > +#include <linux/dmaengine.h> > +#include <linux/gpio/consumer.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/buffer-dma.h> > +#include <linux/iio/buffer-dmaengine.h> > +#include <linux/iio/consumer.h> > +#include <linux/iio/events.h> > +#include <linux/iio/iio.h> Given there are lots of IIO specific includes, probably a case where pulling them out as a separate block after the main includes makes sense. > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/spi/spi.h> > ... > + > + state = iio_priv(indio_dev); > + mutex_init(&state->lock); Trivial but there is no a devm_mutex_init() that deals with the obscure debug case where mutex uninit makes sense. Might as well use it here. > + > + indio_dev->name = "ad8460"; > + indio_dev->info = &ad8460_info; > + > + state->spi = spi; > + dev = &spi->dev; > + > + state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config); > + if (IS_ERR(state->regmap)) > + return dev_err_probe(dev, PTR_ERR(state->regmap), > + "Failed to initialize regmap"); > + > + state->sync_clk = devm_clk_get_enabled(dev, NULL); > + if (IS_ERR(state->sync_clk)) > + return dev_err_probe(dev, PTR_ERR(state->sync_clk), > + "Failed to get sync clk\n"); > + > + state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460-tmp"); > + if (IS_ERR_OR_NULL(state->tmp_adc_channel)) { > + indio_dev->channels = ad8460_channels; > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels); > + } else { > + indio_dev->channels = ad8460_channels_with_tmp_adc; > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels_with_tmp_adc); > + } > + Add and enable the various other supplies. They are probably always on in which case the regulator framework will work it's magic to avoid use having to care that they aren't in the dts. > + ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v"); > + if (ret == -ENODEV) > + state->refio_1p2v_mv = 1200; > + else if (ret >= 0) > + state->refio_1p2v_mv = ret / 1000; > + else > + return dev_err_probe(dev, ret, "Failed to get reference voltage\n"); > + ... > + ret = device_property_read_u32_array(dev, "adi,range-microamp", > + tmp, ARRAY_SIZE(tmp)); > + if (!ret) { > + if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERCURRENT_UA)) > + regmap_write(state->regmap, AD8460_CTRL_REG(0x08), > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | > + AD8460_CURRENT_LIMIT_CONV(tmp[1])); Your binding has a fixed set of values. Yet this is anything in range. Which is correct? Based on datasheet I think it is flexible but that you have listed the example values instead of the limits. > + > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERCURRENT_UA, 0)) > + regmap_write(state->regmap, AD8460_CTRL_REG(0x09), > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | > + AD8460_CURRENT_LIMIT_CONV(abs(tmp[0]))); > + } > + > + ret = device_property_read_u32_array(dev, "adi,range-microvolt", > + tmp, ARRAY_SIZE(tmp)); > + if (!ret) { > + if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERVOLTAGE_UV)) > + regmap_write(state->regmap, AD8460_CTRL_REG(0x0A), > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | > + AD8460_VOLTAGE_LIMIT_CONV(tmp[1])); > + > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERVOLTAGE_UV, 0)) > + regmap_write(state->regmap, AD8460_CTRL_REG(0x0B), > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | > + AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0]))); > + } > + > + ret = device_property_read_u32(dev, "adi,max-millicelsius", &temp); > + if (!ret) { > + if (in_range(temp, AD8460_MIN_OVERTEMPERATURE_MC, > + AD8460_MAX_OVERTEMPERATURE_MC)) > + regmap_write(state->regmap, AD8460_CTRL_REG(0x0C), > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) | > + AD8460_TEMP_LIMIT_CONV(abs(temp))); > + } > + > + ret = ad8460_reset(state); > + if (ret) > + return ret; > + > + /* Enables DAC by default */ > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), > + AD8460_HVDAC_SLEEP_MSK, > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0)); > + if (ret) > + return ret; > + > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->setup_ops = &ad8460_buffer_setup_ops; > + > + ret = devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev, "tx", > + IIO_BUFFER_DIRECTION_OUT); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to get DMA buffer\n"); > + > + return devm_iio_device_register(dev, indio_dev); > +} ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-07 17:14 ` Jonathan Cameron @ 2024-09-09 8:22 ` Tinaco, Mariel 2024-09-09 14:41 ` David Lechner 0 siblings, 1 reply; 20+ messages in thread From: Tinaco, Mariel @ 2024-09-09 8:22 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá > -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, September 8, 2024 1:15 AM > 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 v3 2/2] iio: dac: support the ad8460 Waveform DAC > > [External] > > On Wed, 4 Sep 2024 10:30:40 +0800 > Mariel Tinaco <Mariel.Tinaco@analog.com> wrote: > > > The AD8460 is a “bits in, power out” high voltage, high-power, > > high-speed driver optimized for large output current (up to ±1 A) and > > high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) into > > capacitive loads. > > > > A digital engine implements user-configurable features: modes for > > digital input, programmable supply current, and fault monitoring and > > programmable protection settings for output current, output voltage, > > and junction temperature. The AD8460 operates on high voltage dual > > supplies up to ±55 V and a single low voltage supply of 5 V. > > > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> > A few comments inline. > > Jonathan > > > > obj-$(CONFIG_ADI_AXI_DAC) += adi-axi-dac.o diff --git > > a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c new file mode > > 100644 index 000000000000..6428bfaf0bd7 > > --- /dev/null > > +++ b/drivers/iio/dac/ad8460.c > > @@ -0,0 +1,932 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * AD8460 Waveform generator DAC Driver > > + * > > + * Copyright (C) 2024 Analog Devices, Inc. > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/cleanup.h> > > +#include <linux/clk.h> > > +#include <linux/debugfs.h> > > +#include <linux/delay.h> > > +#include <linux/dmaengine.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/iio/buffer.h> > > +#include <linux/iio/buffer-dma.h> > > +#include <linux/iio/buffer-dmaengine.h> #include > > +<linux/iio/consumer.h> #include <linux/iio/events.h> #include > > +<linux/iio/iio.h> > > Given there are lots of IIO specific includes, probably a case where pulling > them out as a separate block after the main includes makes sense. > > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/regmap.h> > > +#include <linux/regulator/consumer.h> #include <linux/spi/spi.h> > > > ... > > > > + > > + state = iio_priv(indio_dev); > > + mutex_init(&state->lock); > > Trivial but there is no a devm_mutex_init() that deals with the obscure debug > case where mutex uninit makes sense. Might as well use it here. > > > + > > + indio_dev->name = "ad8460"; > > + indio_dev->info = &ad8460_info; > > + > > + state->spi = spi; > > + dev = &spi->dev; > > + > > + state->regmap = devm_regmap_init_spi(spi, > &ad8460_regmap_config); > > + if (IS_ERR(state->regmap)) > > + return dev_err_probe(dev, PTR_ERR(state->regmap), > > + "Failed to initialize regmap"); > > + > > + state->sync_clk = devm_clk_get_enabled(dev, NULL); > > + if (IS_ERR(state->sync_clk)) > > + return dev_err_probe(dev, PTR_ERR(state->sync_clk), > > + "Failed to get sync clk\n"); > > + > > + state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460- > tmp"); > > + if (IS_ERR_OR_NULL(state->tmp_adc_channel)) { > > + indio_dev->channels = ad8460_channels; > > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels); > > + } else { > > + indio_dev->channels = ad8460_channels_with_tmp_adc; > > + indio_dev->num_channels = > ARRAY_SIZE(ad8460_channels_with_tmp_adc); > > + } > > + > Add and enable the various other supplies. They are probably always on in > which case the regulator framework will work it's magic to avoid use having to > care that they aren't in the dts. If the other supplies are added, do they need to be tied up as well to the Private structure just like ref_1p2v? Or do I just apply the devm_regulator_get_enable_read_voltage to it? ret = devm_regulator_get_enable_read_voltage(&client->dev, "vdd"); If (ret < 0 && ret != -ENODEV) return dev_err_probe(ltc2309->dev, ret, "failed to get vref voltage\n"); > > > + ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v"); > > + if (ret == -ENODEV) > > + state->refio_1p2v_mv = 1200; > > + else if (ret >= 0) > > + state->refio_1p2v_mv = ret / 1000; > > + else > > + return dev_err_probe(dev, ret, "Failed to get reference > > +voltage\n"); > > + > ... > > > + ret = device_property_read_u32_array(dev, "adi,range-microamp", > > + tmp, ARRAY_SIZE(tmp)); > > + if (!ret) { > > + if (in_range(tmp[1], 0, > AD8460_ABS_MAX_OVERCURRENT_UA)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x08), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + AD8460_CURRENT_LIMIT_CONV(tmp[1])); > > Your binding has a fixed set of values. Yet this is anything in range. > Which is correct? Based on datasheet I think it is flexible but that you have > listed the example values instead of the limits. > > > > + > > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERCURRENT_UA, > 0)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x09), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + > AD8460_CURRENT_LIMIT_CONV(abs(tmp[0]))); > > + } > > + > > + ret = device_property_read_u32_array(dev, "adi,range-microvolt", > > + tmp, ARRAY_SIZE(tmp)); > > + if (!ret) { > > + if (in_range(tmp[1], 0, > AD8460_ABS_MAX_OVERVOLTAGE_UV)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x0A), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + AD8460_VOLTAGE_LIMIT_CONV(tmp[1])); > > + > > + if (in_range(tmp[0], -AD8460_ABS_MAX_OVERVOLTAGE_UV, > 0)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x0B), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + > AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0]))); > > + } > > + > > + ret = device_property_read_u32(dev, "adi,max-millicelsius", &temp); > > + if (!ret) { > > + if (in_range(temp, AD8460_MIN_OVERTEMPERATURE_MC, > > + AD8460_MAX_OVERTEMPERATURE_MC)) > > + regmap_write(state->regmap, > AD8460_CTRL_REG(0x0C), > > + FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) > | > > + AD8460_TEMP_LIMIT_CONV(abs(temp))); > > + } > > + > > + ret = ad8460_reset(state); > > + if (ret) > > + return ret; > > + > > + /* Enables DAC by default */ > > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), > > + AD8460_HVDAC_SLEEP_MSK, > > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, > 0)); > > + if (ret) > > + return ret; > > + > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->setup_ops = &ad8460_buffer_setup_ops; > > + > > + ret = devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev, "tx", > > + > IIO_BUFFER_DIRECTION_OUT); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Failed to get DMA buffer\n"); > > + > > + return devm_iio_device_register(dev, indio_dev); } ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC 2024-09-09 8:22 ` Tinaco, Mariel @ 2024-09-09 14:41 ` David Lechner 0 siblings, 0 replies; 20+ messages in thread From: David Lechner @ 2024-09-09 14:41 UTC (permalink / raw) To: Tinaco, Mariel, Jonathan Cameron Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, Nuno Sá On 9/9/24 3:22 AM, Tinaco, Mariel wrote: > > ... >>> + if (IS_ERR(state->regmap)) >>> + return dev_err_probe(dev, PTR_ERR(state->regmap), >>> + "Failed to initialize regmap"); >>> + >>> + state->sync_clk = devm_clk_get_enabled(dev, NULL); >>> + if (IS_ERR(state->sync_clk)) >>> + return dev_err_probe(dev, PTR_ERR(state->sync_clk), >>> + "Failed to get sync clk\n"); >>> + >>> + state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460- >> tmp"); >>> + if (IS_ERR_OR_NULL(state->tmp_adc_channel)) { >>> + indio_dev->channels = ad8460_channels; >>> + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels); >>> + } else { >>> + indio_dev->channels = ad8460_channels_with_tmp_adc; >>> + indio_dev->num_channels = >> ARRAY_SIZE(ad8460_channels_with_tmp_adc); >>> + } >>> + >> Add and enable the various other supplies. They are probably always on in >> which case the regulator framework will work it's magic to avoid use having to >> care that they aren't in the dts. > > If the other supplies are added, do they need to be tied up as well to the > Private structure just like ref_1p2v? Or do I just apply the > devm_regulator_get_enable_read_voltage to it? > > ret = devm_regulator_get_enable_read_voltage(&client->dev, "vdd"); > If (ret < 0 && ret != -ENODEV) > return dev_err_probe(ltc2309->dev, ret, > "failed to get vref voltage\n"); > For supplies that just supply power, we usually don't worry about the voltage, so just use devm_regulator_get_enable(). Or if there are lots of supplies, you can use devm_regulator_bulk_get_enable() to do them all at once. They don't need to be added to the private structure. Also, lots of chips have to have supplies turned on in a certain order. This one is no exception, so be sure to have a look at the "POWER SUPPLY SEQUENCING" section in the datasheet to make sure the driver turns things on in the right order. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 0/2] Add support to AD8460 Waveform Generator DAC 2024-09-04 2:30 [PATCH v3 0/2] Add support to AD8460 Waveform Generator DAC Mariel Tinaco 2024-09-04 2:30 ` [PATCH v3 1/2] dt-bindings: iio: dac: add docs for ad8460 Mariel Tinaco 2024-09-04 2:30 ` [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco @ 2024-09-04 6:16 ` Krzysztof Kozlowski 2024-09-05 1:10 ` Tinaco, Mariel 2 siblings, 1 reply; 20+ messages in thread From: Krzysztof Kozlowski @ 2024-09-04 6:16 UTC (permalink / raw) To: Mariel Tinaco Cc: linux-iio, devicetree, linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá On Wed, Sep 04, 2024 at 10:30:38AM +0800, Mariel Tinaco wrote: > Apply comments for adding support to AD8460 Waveform Generator DAC > > ad8460: > * Fixed errors detected by test bot > * Applied proper masking of fixed values > * Applied proper wrapping to get close to 80 chars > * Applied proper comment formatting > * Applied proper placement of breaks in switch cases > * Removed channel properties unused by IIO buffer interface > * Simplified property getting on probe function > * Fixed error handlings on probe function > * Fixed setting of overvoltage, overcurrent and overtemperature ranges; > If value provided is invalid, default state of the register will not > be rewritten > > Bindings: > * Dropped unnecessary descriptions > * Updated property descriptions to describe functionality properly > * Added multiple selection of values for adi,range-microvolt property > * Fixed formatting errors to follow DTS coding style > * Lifted GPIO naming from gpio-consumer-common yaml This all happened in v3? Or v2? Please write accurate changelogs. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 0/2] Add support to AD8460 Waveform Generator DAC 2024-09-04 6:16 ` [PATCH v3 0/2] Add support to AD8460 Waveform Generator DAC Krzysztof Kozlowski @ 2024-09-05 1:10 ` Tinaco, Mariel 0 siblings, 0 replies; 20+ messages in thread From: Tinaco, Mariel @ 2024-09-05 1:10 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá > -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: Wednesday, September 4, 2024 2:16 PM > To: Tinaco, Mariel <Mariel.Tinaco@analog.com> > Cc: 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 v3 0/2] Add support to AD8460 Waveform Generator > DAC > > [External] > > On Wed, Sep 04, 2024 at 10:30:38AM +0800, Mariel Tinaco wrote: > > Apply comments for adding support to AD8460 Waveform Generator DAC > > > > ad8460: > > * Fixed errors detected by test bot > > * Applied proper masking of fixed values > > * Applied proper wrapping to get close to 80 chars > > * Applied proper comment formatting > > * Applied proper placement of breaks in switch cases > > * Removed channel properties unused by IIO buffer interface > > * Simplified property getting on probe function > > * Fixed error handlings on probe function > > * Fixed setting of overvoltage, overcurrent and overtemperature ranges; > > If value provided is invalid, default state of the register will not > > be rewritten > > > > Bindings: > > * Dropped unnecessary descriptions > > * Updated property descriptions to describe functionality properly > > * Added multiple selection of values for adi,range-microvolt property > > * Fixed formatting errors to follow DTS coding style > > * Lifted GPIO naming from gpio-consumer-common yaml > > This all happened in v3? Or v2? Please write accurate changelogs. > > Best regards, > Krzysztof It seems I missed to put the version of the changelogs. These are all comments V2 and the changes were applied to the last release, v3. I'll keep this in mind the next time. Thanks! Best regards, Mariel ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2024-09-14 11:45 UTC | newest] Thread overview: 20+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-04 2:30 [PATCH v3 0/2] Add support to AD8460 Waveform Generator DAC Mariel Tinaco 2024-09-04 2:30 ` [PATCH v3 1/2] dt-bindings: iio: dac: add docs for ad8460 Mariel Tinaco 2024-09-04 6:20 ` Krzysztof Kozlowski 2024-09-07 17:01 ` Jonathan Cameron 2024-09-09 6:22 ` Tinaco, Mariel 2024-09-09 6:41 ` Krzysztof Kozlowski 2024-09-04 2:30 ` [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco 2024-09-04 15:50 ` kernel test robot 2024-09-04 17:23 ` kernel test robot 2024-09-06 0:50 ` David Lechner 2024-09-09 9:47 ` Tinaco, Mariel 2024-09-09 14:51 ` David Lechner 2024-09-09 19:09 ` Jonathan Cameron 2024-09-10 1:44 ` Tinaco, Mariel 2024-09-14 11:45 ` Jonathan Cameron 2024-09-07 17:14 ` Jonathan Cameron 2024-09-09 8:22 ` Tinaco, Mariel 2024-09-09 14:41 ` David Lechner 2024-09-04 6:16 ` [PATCH v3 0/2] Add support to AD8460 Waveform Generator DAC Krzysztof Kozlowski 2024-09-05 1:10 ` Tinaco, Mariel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox