* [PATCH 0/4] Add driver for LTC2664 and LTC2672
@ 2024-04-12 3:20 Kim Seer Paller
2024-04-12 3:20 ` [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml Kim Seer Paller
` (3 more replies)
0 siblings, 4 replies; 24+ messages in thread
From: Kim Seer Paller @ 2024-04-12 3:20 UTC (permalink / raw
To: linux-iio, devicetree, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown,
David Lechner, Michael Hennerich
The LTC2664 is a 4 Channel, Voltage Output SoftSpan DAC while LTC2672 is a 5
channel, Current Output Softspan DAC. The ABI defined for the driver:
LTC2664 - toggle mode channels:
* out_voltageY_toggle_en
* out_voltageY_raw0
* out_voltageY_raw1
* out_voltageY_symbol
LTC2672 - toggle mode channels:
* out_currentY_toggle_en
* out_currentY_raw0
* out_currentY_raw1
* out_currentY_symbol
Default channels won't have any of the above ABIs.
Kim Seer Paller (4):
dt-bindings: iio: dac: Add adi,ltc2664.yaml
iio: ABI: add ABI file for the LTC2664 DAC
iio: ABI: add ABI file for the LTC2672 DAC
iio: dac: ltc2664: Add driver for LTC2664 and LTC2672
.../ABI/testing/sysfs-bus-iio-dac-ltc2664 | 30 +
.../ABI/testing/sysfs-bus-iio-dac-ltc2672 | 30 +
.../bindings/iio/dac/adi,ltc2664.yaml | 230 +++++
MAINTAINERS | 11 +
drivers/iio/dac/Kconfig | 11 +
drivers/iio/dac/Makefile | 1 +
drivers/iio/dac/ltc2664.c | 785 ++++++++++++++++++
7 files changed, 1098 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672
create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml
create mode 100644 drivers/iio/dac/ltc2664.c
base-commit: 27eea4778db8268cd6dc80a5b853c599bd3099f1
--
2.34.1
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml 2024-04-12 3:20 [PATCH 0/4] Add driver for LTC2664 and LTC2672 Kim Seer Paller @ 2024-04-12 3:20 ` Kim Seer Paller 2024-04-12 5:50 ` Krzysztof Kozlowski 2024-04-12 21:23 ` David Lechner 2024-04-12 3:21 ` [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC Kim Seer Paller ` (2 subsequent siblings) 3 siblings, 2 replies; 24+ messages in thread From: Kim Seer Paller @ 2024-04-12 3:20 UTC (permalink / raw To: linux-iio, devicetree, linux-kernel Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, David Lechner, Michael Hennerich Add documentation for ltc2664 and ltc2672. Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> --- .../bindings/iio/dac/adi,ltc2664.yaml | 230 ++++++++++++++++++ MAINTAINERS | 8 + 2 files changed, 238 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml new file mode 100644 index 000000000..2f581a9e5 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml @@ -0,0 +1,230 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/dac/adi,ltc2664.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices LTC2664 and LTC2672 DAC + +maintainers: + - Michael Hennerich <michael.hennerich@analog.com> + - Kim Seer Paller <kimseer.paller@analog.com> + +description: | + Analog Devices LTC2664 4 channel, 16 bit, +-10V DAC + Analog Devices LTC2672 5 channel, 16 bit, 300mA DAC + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2664.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2672.pdf + +$defs: + toggle-operation: + type: object + description: Toggle mode channel setting. + + properties: + reg: + description: Channel number. + minimum: 0 + maximum: 4 + + adi,toggle-mode: + description: + Set the channel as a toggle enabled channel. Toggle operation enables + fast switching of a DAC output between two different DAC codes without + any SPI transaction. + type: boolean + +patternProperties: + "^channel@[0-4]$": + type: object + +properties: + compatible: + enum: + - adi,ltc2664 + - adi,ltc2672 + + reg: + maxItems: 1 + + spi-max-frequency: + maximum: 50000000 + + vcc-supply: + description: Analog Supply Voltage Input. + + iovcc-supply: + description: Digital Input/Output Supply Voltage. + + vref-supply: + description: + Reference Input/Output. The voltage at the REF pin sets the full-scale + range of all channels. If not provided the internal reference is used and + also provided on the VREF pin. + + clr-gpios: + description: + If specified, it will be asserted during driver probe. As the line is + active low, it should be marked GPIO_ACTIVE_LOW. + maxItems: 1 + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + +allOf: + - if: + properties: + compatible: + contains: + const: adi,ltc2664 + then: + properties: + adi,manual-span-operation-config: + description: + This property must mimic the MSPAN pin configurations. + By tying the MSPAN pins (MSP2, MSP1 and MSP0) to GND + and/or VCC, any output range can be hardware-configured + with different mid-scale or zero-scale reset options. + The hardware configuration is latched during power on reset + for proper operation. + 0 - MPS2=GND, MPS1=GND, MSP0=GND + 1 - MPS2=GND, MPS1=GND, MSP0=VCC + 2 - MPS2=GND, MPS1=VCC, MSP0=GND + 3 - MPS2=GND, MPS1=VCC, MSP0=VCC + 4 - MPS2=VCC, MPS1=GND, MSP0=GND + 5 - MPS2=VCC, MPS1=GND, MSP0=VCC + 6 - MPS2=VCC, MPS1=VCC, MSP0=GND + 7 - MPS2=VCC, MPS1=VCC, MSP0=VCC (enables SoftSpan feature) + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3, 4, 5, 6, 7] + default: 7 + + patternProperties: + "^channel@([0-3])$": + $ref: '#/$defs/toggle-operation' + unevaluatedProperties: false + + description: Channel in toggle functionality. + + properties: + adi,output-range-microvolt: + description: Specify the channel output full scale range. + oneOf: + - items: + - const: 0 + - enum: [5000000, 10000000] + - items: + - const: -5000000 + - const: 5000000 + - items: + - const: -10000000 + - const: 10000000 + - items: + - const: -2500000 + - const: 2500000 + + required: + - adi,output-range-microvolt + + - if: + properties: + compatible: + contains: + const: adi,ltc2672 + then: + properties: + adi,rfsadj-ohms: + description: If FSADJ is tied to VCC, an internal RFSADJ (20 kΩ) is + selected, which results in nominal output ranges. When an external + resistor of 19 kΩ to 41 kΩ can be used instead by connecting the + resistor between FSADJ and GND it controls the scaling of the + ranges, and the internal resistor is automatically disconnected. + minimum: 19000 + maximum: 41000 + default: 20000 + + patternProperties: + "^channel@([0-4])$": + $ref: '#/$defs/toggle-operation' + unevaluatedProperties: false + + description: Configuration properties for a channel in toggle mode + + properties: + adi,output-range-microamp: + description: Specify the channel output full scale range. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [3125000, 6250000, 12500000, 25000000, 50000000, 100000000, + 200000000, 300000000] + + required: + - adi,output-range-microamp + +required: + - compatible + - reg + - spi-max-frequency + - vcc-supply + - iovcc-supply + +additionalProperties: false + +examples: + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + dac@0 { + compatible = "adi,ltc2664"; + reg = <0>; + spi-max-frequency = <10000000>; + + vcc-supply = <&vcc>; + iovcc-supply = <&vcc>; + vref-supply = <&vref>; + + #address-cells = <1>; + #size-cells = <0>; + channel@0 { + reg = <0>; + adi,toggle-mode; + adi,output-range-microvolt = <(-10000000) 10000000>; + }; + + channel@1 { + reg = <1>; + adi,output-range-microvolt = <0 10000000>; + }; + }; + }; + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + dac@0 { + compatible = "adi,ltc2672"; + reg = <0>; + spi-max-frequency = <10000000>; + + vcc-supply = <&vcc>; + iovcc-supply = <&vcc>; + vref-supply = <&vref>; + + #address-cells = <1>; + #size-cells = <0>; + channel@0 { + reg = <0>; + adi,toggle-mode; + adi,output-range-microamp = <3125000>; + }; + + channel@1 { + reg = <1>; + adi,output-range-microamp = <6250000>; + }; + }; + }; +... diff --git a/MAINTAINERS b/MAINTAINERS index a7287cf44..bd8645f6e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12836,6 +12836,14 @@ S: Maintained F: Documentation/devicetree/bindings/iio/dac/lltc,ltc1660.yaml F: drivers/iio/dac/ltc1660.c +LTC2664 IIO DAC DRIVER +M: Michael Hennerich <michael.hennerich@analog.com> +M: Kim Seer Paller <kimseer.paller@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,ltc2664.yaml + LTC2688 IIO DAC DRIVER M: Nuno Sá <nuno.sa@analog.com> L: linux-iio@vger.kernel.org -- 2.34.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml 2024-04-12 3:20 ` [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml Kim Seer Paller @ 2024-04-12 5:50 ` Krzysztof Kozlowski 2024-04-13 14:54 ` Jonathan Cameron 2024-04-12 21:23 ` David Lechner 1 sibling, 1 reply; 24+ messages in thread From: Krzysztof Kozlowski @ 2024-04-12 5:50 UTC (permalink / raw To: Kim Seer Paller, linux-iio, devicetree, linux-kernel Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, David Lechner, Michael Hennerich On 12/04/2024 05:20, Kim Seer Paller wrote: > Add documentation for ltc2664 and ltc2672. > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > --- > .../bindings/iio/dac/adi,ltc2664.yaml | 230 ++++++++++++++++++ > MAINTAINERS | 8 + > 2 files changed, 238 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > new file mode 100644 > index 000000000..2f581a9e5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > @@ -0,0 +1,230 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/dac/adi,ltc2664.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices LTC2664 and LTC2672 DAC > + > +maintainers: > + - Michael Hennerich <michael.hennerich@analog.com> > + - Kim Seer Paller <kimseer.paller@analog.com> > + > +description: | > + Analog Devices LTC2664 4 channel, 16 bit, +-10V DAC > + Analog Devices LTC2672 5 channel, 16 bit, 300mA DAC > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2664.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2672.pdf > + > +$defs: > + toggle-operation: > + type: object > + description: Toggle mode channel setting. > + > + properties: > + reg: > + description: Channel number. > + minimum: 0 > + maximum: 4 > + > + adi,toggle-mode: > + description: > + Set the channel as a toggle enabled channel. Toggle operation enables > + fast switching of a DAC output between two different DAC codes without > + any SPI transaction. > + type: boolean > + > +patternProperties: > + "^channel@[0-4]$": > + type: object patternProps go after properties. You miss additionalProperties: false and actual properties defined in top-level part of the binding. I wouldn't call your schema easiest to read. You have two quite different devices. > + > +properties: > + compatible: > + enum: > + - adi,ltc2664 > + - adi,ltc2672 > + > + reg: > + maxItems: 1 > + > + spi-max-frequency: > + maximum: 50000000 Missing definition of this property, so ref to spi-peripheral-props. Look at other bindings having this property. > + > + vcc-supply: > + description: Analog Supply Voltage Input. > + > + iovcc-supply: > + description: Digital Input/Output Supply Voltage. > + > + vref-supply: > + description: > + Reference Input/Output. The voltage at the REF pin sets the full-scale > + range of all channels. If not provided the internal reference is used and > + also provided on the VREF pin. > + > + clr-gpios: > + description: > + If specified, it will be asserted during driver probe. As the line is > + active low, it should be marked GPIO_ACTIVE_LOW. First sentence tells nothing about hardware. Do not describe driver behavior, but hardware. Second sentence is 90% redundant - just say, when describing the hardware, that the line for foo bar is active low. > + maxItems: 1 > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: adi,ltc2664 > + then: > + properties: > + adi,manual-span-operation-config: > + description: > + This property must mimic the MSPAN pin configurations. > + By tying the MSPAN pins (MSP2, MSP1 and MSP0) to GND > + and/or VCC, any output range can be hardware-configured > + with different mid-scale or zero-scale reset options. > + The hardware configuration is latched during power on reset > + for proper operation. > + 0 - MPS2=GND, MPS1=GND, MSP0=GND > + 1 - MPS2=GND, MPS1=GND, MSP0=VCC > + 2 - MPS2=GND, MPS1=VCC, MSP0=GND > + 3 - MPS2=GND, MPS1=VCC, MSP0=VCC > + 4 - MPS2=VCC, MPS1=GND, MSP0=GND > + 5 - MPS2=VCC, MPS1=GND, MSP0=VCC > + 6 - MPS2=VCC, MPS1=VCC, MSP0=GND > + 7 - MPS2=VCC, MPS1=VCC, MSP0=VCC (enables SoftSpan feature) > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1, 2, 3, 4, 5, 6, 7] > + default: 7 > + > + patternProperties: > + "^channel@([0-3])$": and the channel@4 is? You allow anything. > + $ref: '#/$defs/toggle-operation' > + unevaluatedProperties: false > + > + description: Channel in toggle functionality. > + > + properties: > + adi,output-range-microvolt: > + description: Specify the channel output full scale range. > + oneOf: > + - items: > + - const: 0 > + - enum: [5000000, 10000000] > + - items: > + - const: -5000000 > + - const: 5000000 > + - items: > + - const: -10000000 > + - const: 10000000 > + - items: > + - const: -2500000 > + - const: 2500000 > + > + required: > + - adi,output-range-microvolt > + > + - if: > + properties: > + compatible: > + contains: > + const: adi,ltc2672 > + then: > + properties: > + adi,rfsadj-ohms: > + description: If FSADJ is tied to VCC, an internal RFSADJ (20 kΩ) is > + selected, which results in nominal output ranges. When an external > + resistor of 19 kΩ to 41 kΩ can be used instead by connecting the > + resistor between FSADJ and GND it controls the scaling of the > + ranges, and the internal resistor is automatically disconnected. > + minimum: 19000 > + maximum: 41000 > + default: 20000 > + > + patternProperties: > + "^channel@([0-4])$": > + $ref: '#/$defs/toggle-operation' > + unevaluatedProperties: false > + > + description: Configuration properties for a channel in toggle mode > + > + properties: > + adi,output-range-microamp: > + description: Specify the channel output full scale range. > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [3125000, 6250000, 12500000, 25000000, 50000000, 100000000, > + 200000000, 300000000] > + > + required: > + - adi,output-range-microamp > + > +required: > + - compatible > + - reg > + - spi-max-frequency > + - vcc-supply > + - iovcc-supply > + > +additionalProperties: false > + > +examples: > + - | > + spi { > + #address-cells = <1>; Use 4 spaces for example indentation. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml 2024-04-12 5:50 ` Krzysztof Kozlowski @ 2024-04-13 14:54 ` Jonathan Cameron 2024-04-16 14:16 ` Paller, Kim Seer 0 siblings, 1 reply; 24+ messages in thread From: Jonathan Cameron @ 2024-04-13 14:54 UTC (permalink / raw To: Krzysztof Kozlowski Cc: Kim Seer Paller, linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, David Lechner, Michael Hennerich On Fri, 12 Apr 2024 07:50:17 +0200 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 12/04/2024 05:20, Kim Seer Paller wrote: > > Add documentation for ltc2664 and ltc2672. > > > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > > --- > > .../bindings/iio/dac/adi,ltc2664.yaml | 230 ++++++++++++++++++ > > MAINTAINERS | 8 + > > 2 files changed, 238 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > new file mode 100644 > > index 000000000..2f581a9e5 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > @@ -0,0 +1,230 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iio/dac/adi,ltc2664.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Analog Devices LTC2664 and LTC2672 DAC > > + > > +maintainers: > > + - Michael Hennerich <michael.hennerich@analog.com> > > + - Kim Seer Paller <kimseer.paller@analog.com> > > + > > +description: | > > + Analog Devices LTC2664 4 channel, 16 bit, +-10V DAC > > + Analog Devices LTC2672 5 channel, 16 bit, 300mA DAC > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2664.pdf > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2672.pdf > > + > > +$defs: > > + toggle-operation: > > + type: object > > + description: Toggle mode channel setting. > > + > > + properties: > > + reg: > > + description: Channel number. > > + minimum: 0 > > + maximum: 4 > > + > > + adi,toggle-mode: > > + description: > > + Set the channel as a toggle enabled channel. Toggle operation enables > > + fast switching of a DAC output between two different DAC codes without > > + any SPI transaction. > > + type: boolean > > + > > +patternProperties: > > + "^channel@[0-4]$": > > + type: object > > patternProps go after properties. You miss additionalProperties: false > and actual properties defined in top-level part of the binding. > > I wouldn't call your schema easiest to read. You have two quite > different devices. I agree entirely. I think it might be simpler to have 2 bindings. If you haven't already tried that give it a go. Note that we can have 2 bindings that in Linux are handled by one driver (examples already exist max1363 and max1238 IIRC) as well as the other way around where we have one binding and 2 drivers. Jonathan ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml 2024-04-13 14:54 ` Jonathan Cameron @ 2024-04-16 14:16 ` Paller, Kim Seer 0 siblings, 0 replies; 24+ messages in thread From: Paller, Kim Seer @ 2024-04-16 14:16 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, Conor Dooley, Liam Girdwood, Mark Brown, David Lechner, Hennerich, Michael > -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Saturday, April 13, 2024 10:55 PM > To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Cc: Paller, Kim Seer <KimSeer.Paller@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 > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > David Lechner <dlechner@baylibre.com>; Hennerich, Michael > <Michael.Hennerich@analog.com> > Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml > > [External] > > On Fri, 12 Apr 2024 07:50:17 +0200 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > On 12/04/2024 05:20, Kim Seer Paller wrote: > > > Add documentation for ltc2664 and ltc2672. > > > > > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > > > --- > > > .../bindings/iio/dac/adi,ltc2664.yaml | 230 ++++++++++++++++++ > > > MAINTAINERS | 8 + > > > 2 files changed, 238 insertions(+) > > > create mode 100644 > Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > > new file mode 100644 > > > index 000000000..2f581a9e5 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > > @@ -0,0 +1,230 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: > https://urldefense.com/v3/__http://devicetree.org/schemas/iio/dac/adi,ltc266 > 4.yaml*__;Iw!!A3Ni8CS0y2Y!8SuDinm690wjc8X5Et94jlV57PAZ79hvsp- > HRohSvdY5z62lyNXyu4M3R3-BB2PFIqkKsHeFoEJPZzJTgQ$ > > > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta- > schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!8SuDinm690wjc8X5Et94jlV57PAZ79h > vsp-HRohSvdY5z62lyNXyu4M3R3-BB2PFIqkKsHeFoEIk1jpsTw$ > > > + > > > +title: Analog Devices LTC2664 and LTC2672 DAC > > > + > > > +maintainers: > > > + - Michael Hennerich <michael.hennerich@analog.com> > > > + - Kim Seer Paller <kimseer.paller@analog.com> > > > + > > > +description: | > > > + Analog Devices LTC2664 4 channel, 16 bit, +-10V DAC > > > + Analog Devices LTC2672 5 channel, 16 bit, 300mA DAC > > > + https://www.analog.com/media/en/technical-documentation/data- > sheets/ltc2664.pdf > > > + https://www.analog.com/media/en/technical-documentation/data- > sheets/ltc2672.pdf > > > + > > > +$defs: > > > + toggle-operation: > > > + type: object > > > + description: Toggle mode channel setting. > > > + > > > + properties: > > > + reg: > > > + description: Channel number. > > > + minimum: 0 > > > + maximum: 4 > > > + > > > + adi,toggle-mode: > > > + description: > > > + Set the channel as a toggle enabled channel. Toggle operation > enables > > > + fast switching of a DAC output between two different DAC codes > without > > > + any SPI transaction. > > > + type: boolean > > > + > > > +patternProperties: > > > + "^channel@[0-4]$": > > > + type: object > > > > patternProps go after properties. You miss additionalProperties: false > > and actual properties defined in top-level part of the binding. > > > > I wouldn't call your schema easiest to read. You have two quite > > different devices. > > I agree entirely. I think it might be simpler to have 2 bindings. > If you haven't already tried that give it a go. > > Note that we can have 2 bindings that in Linux are handled by one > driver (examples already exist max1363 and max1238 IIRC) as well as > the other way around where we have one binding and 2 drivers. Having 2 separate bindings would simplify the implementation. I will proceed with trying out this approach Thanks, Kim ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml 2024-04-12 3:20 ` [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml Kim Seer Paller 2024-04-12 5:50 ` Krzysztof Kozlowski @ 2024-04-12 21:23 ` David Lechner 2024-04-13 15:06 ` Jonathan Cameron 1 sibling, 1 reply; 24+ messages in thread From: David Lechner @ 2024-04-12 21:23 UTC (permalink / raw To: Kim Seer Paller Cc: linux-iio, devicetree, linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Michael Hennerich On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller <kimseer.paller@analog.com> wrote: > > Add documentation for ltc2664 and ltc2672. > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > --- > .../bindings/iio/dac/adi,ltc2664.yaml | 230 ++++++++++++++++++ > MAINTAINERS | 8 + > 2 files changed, 238 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > new file mode 100644 > index 000000000..2f581a9e5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > @@ -0,0 +1,230 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/dac/adi,ltc2664.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices LTC2664 and LTC2672 DAC > + > +maintainers: > + - Michael Hennerich <michael.hennerich@analog.com> > + - Kim Seer Paller <kimseer.paller@analog.com> > + > +description: | > + Analog Devices LTC2664 4 channel, 16 bit, +-10V DAC > + Analog Devices LTC2672 5 channel, 16 bit, 300mA DAC > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2664.pdf This link gives a 404 error. Is there a typo? > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2672.pdf > + > +$defs: > + toggle-operation: > + type: object > + description: Toggle mode channel setting. > + > + properties: > + reg: > + description: Channel number. > + minimum: 0 > + maximum: 4 > + > + adi,toggle-mode: > + description: > + Set the channel as a toggle enabled channel. Toggle operation enables > + fast switching of a DAC output between two different DAC codes without > + any SPI transaction. > + type: boolean I'm not convinced that this belongs in the devicetree. It seems like everything related to toggling can and should be left to runtime configuration. > + > +patternProperties: > + "^channel@[0-4]$": > + type: object > + > +properties: > + compatible: > + enum: > + - adi,ltc2664 > + - adi,ltc2672 > + > + reg: > + maxItems: 1 > + > + spi-max-frequency: > + maximum: 50000000 > + > + vcc-supply: > + description: Analog Supply Voltage Input. > + There is also an input supply for each output channel on ltc2672, so I think we also need vdd0-supply, vdd1-supply, etc. On ltc2664, there is V+ instead so it needs v-pos-supply. And there is V~ on both which can be between -5.5V/-15.75V and GND, so optional v-neg-supply seems appropriate. > + iovcc-supply: > + description: Digital Input/Output Supply Voltage. > + > + vref-supply: > + description: > + Reference Input/Output. The voltage at the REF pin sets the full-scale > + range of all channels. If not provided the internal reference is used and > + also provided on the VREF pin. There is no VREF pin, so it looks like there is a typo. And it would make more sense to call this ref-supply as well. > + > + clr-gpios: > + description: > + If specified, it will be asserted during driver probe. As the line is > + active low, it should be marked GPIO_ACTIVE_LOW. > + maxItems: 1 Some other potentially properties for complete bindings that would be trivial to add now: * (ltc2672) There is a FAULT output pin, so it would make sense to have an interrupts property for that signal. * (both) I haven't done any DACs myself yet, but I see ldac-gpios on a few other bindings. I assume this is the typical way for handling the LDAC signal on most DACs? * (both) I see these have daisy chain capabilities, so an optional #daisy-chained-devices could be appropriate. Maybe not so trivial: * (both) The MUX/MUXOUT pins look like we have an embedded pin mux, so it could mean we need #pinctrl-cells. ltc2664 would also need muxin-gpios for this. > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: adi,ltc2664 > + then: > + properties: > + adi,manual-span-operation-config: > + description: > + This property must mimic the MSPAN pin configurations. > + By tying the MSPAN pins (MSP2, MSP1 and MSP0) to GND > + and/or VCC, any output range can be hardware-configured > + with different mid-scale or zero-scale reset options. > + The hardware configuration is latched during power on reset > + for proper operation. > + 0 - MPS2=GND, MPS1=GND, MSP0=GND > + 1 - MPS2=GND, MPS1=GND, MSP0=VCC > + 2 - MPS2=GND, MPS1=VCC, MSP0=GND > + 3 - MPS2=GND, MPS1=VCC, MSP0=VCC > + 4 - MPS2=VCC, MPS1=GND, MSP0=GND > + 5 - MPS2=VCC, MPS1=GND, MSP0=VCC > + 6 - MPS2=VCC, MPS1=VCC, MSP0=GND > + 7 - MPS2=VCC, MPS1=VCC, MSP0=VCC (enables SoftSpan feature) > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1, 2, 3, 4, 5, 6, 7] > + default: 7 Are these always hard-wired or could they be connected to gpios and made configurable at runtime? > + > + patternProperties: > + "^channel@([0-3])$": > + $ref: '#/$defs/toggle-operation' > + unevaluatedProperties: false > + > + description: Channel in toggle functionality. > + > + properties: > + adi,output-range-microvolt: > + description: Specify the channel output full scale range. How would someone writing a .dts know what values to select for this property? Or is this something that should be configured at runtime instead of in the devicetree? Or should this info come from the missing voltage supplies I mentioned? > + oneOf: > + - items: > + - const: 0 > + - enum: [5000000, 10000000] > + - items: > + - const: -5000000 > + - const: 5000000 > + - items: > + - const: -10000000 > + - const: 10000000 > + - items: > + - const: -2500000 > + - const: 2500000 > + > + required: > + - adi,output-range-microvolt > + > + - if: > + properties: > + compatible: > + contains: > + const: adi,ltc2672 > + then: > + properties: > + adi,rfsadj-ohms: > + description: If FSADJ is tied to VCC, an internal RFSADJ (20 kΩ) is > + selected, which results in nominal output ranges. When an external > + resistor of 19 kΩ to 41 kΩ can be used instead by connecting the > + resistor between FSADJ and GND it controls the scaling of the > + ranges, and the internal resistor is automatically disconnected. > + minimum: 19000 > + maximum: 41000 > + default: 20000 This is the kind of description that would be helpful on some of the other properties. It does a good job of explaining what value to select based on what is connected to the chip. > + > + patternProperties: > + "^channel@([0-4])$": > + $ref: '#/$defs/toggle-operation' > + unevaluatedProperties: false > + > + description: Configuration properties for a channel in toggle mode > + > + properties: > + adi,output-range-microamp: > + description: Specify the channel output full scale range. > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [3125000, 6250000, 12500000, 25000000, 50000000, 100000000, > + 200000000, 300000000] > + Same comments as adi,output-range-microvolt apply here. > + required: > + - adi,output-range-microamp > + > +required: > + - compatible > + - reg > + - spi-max-frequency > + - vcc-supply > + - iovcc-supply > + > +additionalProperties: false > + ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml 2024-04-12 21:23 ` David Lechner @ 2024-04-13 15:06 ` Jonathan Cameron 2024-04-13 15:11 ` Jonathan Cameron 2024-04-13 16:21 ` David Lechner 0 siblings, 2 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-04-13 15:06 UTC (permalink / raw To: David Lechner Cc: Kim Seer Paller, linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Michael Hennerich On Fri, 12 Apr 2024 16:23:00 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > <kimseer.paller@analog.com> wrote: > > > > Add documentation for ltc2664 and ltc2672. > > > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> A few follow up comments inline as David and Krzysztof already gave good feedback. > > --- > > .../bindings/iio/dac/adi,ltc2664.yaml | 230 ++++++++++++++++++ > > MAINTAINERS | 8 + > > 2 files changed, 238 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > new file mode 100644 > > index 000000000..2f581a9e5 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > @@ -0,0 +1,230 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iio/dac/adi,ltc2664.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Analog Devices LTC2664 and LTC2672 DAC > > + > > +maintainers: > > + - Michael Hennerich <michael.hennerich@analog.com> > > + - Kim Seer Paller <kimseer.paller@analog.com> > > + > > +description: | > > + Analog Devices LTC2664 4 channel, 16 bit, +-10V DAC > > + Analog Devices LTC2672 5 channel, 16 bit, 300mA DAC > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2664.pdf > > This link gives a 404 error. Is there a typo? > > > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2672.pdf > > + > > +$defs: > > + toggle-operation: > > + type: object > > + description: Toggle mode channel setting. > > + > > + properties: > > + reg: > > + description: Channel number. > > + minimum: 0 > > + maximum: 4 > > + > > > + adi,toggle-mode: > > + description: > > + Set the channel as a toggle enabled channel. Toggle operation enables > > + fast switching of a DAC output between two different DAC codes without > > + any SPI transaction. > > + type: boolean > > I'm not convinced that this belongs in the devicetree. It seems like > everything related to toggling can and should be left to runtime > configuration. Agreed - probably on a fifo basis, so each time you switch to the other toggle value, but if you happen to already have the value stored already you can elide the SPI transfer. I think we already have a device doing this, though I can't remember which driver it is. Perhaps search for toggle. > > > + > > +patternProperties: > > + "^channel@[0-4]$": > > + type: object > > + > > +properties: > > + compatible: > > + enum: > > + - adi,ltc2664 > > + - adi,ltc2672 > > + > > + reg: > > + maxItems: 1 > > + > > + spi-max-frequency: > > + maximum: 50000000 > > + > > + vcc-supply: > > + description: Analog Supply Voltage Input. > > + > > There is also an input supply for each output channel on ltc2672, so I > think we also need vdd0-supply, vdd1-supply, etc. > > On ltc2664, there is V+ instead so it needs v-pos-supply. > > And there is V~ on both which can be between -5.5V/-15.75V and GND, so > optional v-neg-supply seems appropriate. Only make it optional in the binding if the settings of the device change depending on whether it is there or not. Looks like there is an internal reference, so maybe it really is optional. > > > + iovcc-supply: > > + description: Digital Input/Output Supply Voltage. > > + > > + vref-supply: > > + description: > > + Reference Input/Output. The voltage at the REF pin sets the full-scale > > + range of all channels. If not provided the internal reference is used and > > + also provided on the VREF pin. > > There is no VREF pin, so it looks like there is a typo. And it would > make more sense to call this ref-supply as well. > > > + > > + clr-gpios: > > + description: > > + If specified, it will be asserted during driver probe. As the line is > > + active low, it should be marked GPIO_ACTIVE_LOW. > > + maxItems: 1 > > Some other potentially properties for complete bindings that would be > trivial to add now: > > * (ltc2672) There is a FAULT output pin, so it would make sense to > have an interrupts property for that signal. > * (both) I haven't done any DACs myself yet, but I see ldac-gpios on a > few other bindings. I assume this is the typical way for handling the > LDAC signal on most DACs? > * (both) I see these have daisy chain capabilities, so an optional > #daisy-chained-devices could be appropriate. > > Maybe not so trivial: > > * (both) The MUX/MUXOUT pins look like we have an embedded pin mux, so > it could mean we need #pinctrl-cells. ltc2664 would also need > muxin-gpios for this. Not convinced that's the right approach - looks more like a channel selector than a conventional mux or pin control. Sure that's a mux, but we want a clean userspace control to let us choose a signal to measure at runtime If you wanted to support this I'd have the binding describe optional stuff to act as a consumer of an ADC channel on another device. The IIO driver would then provide a bunch of input channels to allow measurement of each of the signals. Look at io-channels etc in existing bindings for how to do that. > > > > + > > + '#address-cells': > > + const: 1 > > + > > + '#size-cells': > > + const: 0 > > + > > +allOf: > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: adi,ltc2664 > > + then: > > + properties: > > + adi,manual-span-operation-config: > > + description: > > + This property must mimic the MSPAN pin configurations. > > + By tying the MSPAN pins (MSP2, MSP1 and MSP0) to GND > > + and/or VCC, any output range can be hardware-configured > > + with different mid-scale or zero-scale reset options. > > + The hardware configuration is latched during power on reset > > + for proper operation. > > + 0 - MPS2=GND, MPS1=GND, MSP0=GND > > + 1 - MPS2=GND, MPS1=GND, MSP0=VCC > > + 2 - MPS2=GND, MPS1=VCC, MSP0=GND > > + 3 - MPS2=GND, MPS1=VCC, MSP0=VCC > > + 4 - MPS2=VCC, MPS1=GND, MSP0=GND > > + 5 - MPS2=VCC, MPS1=GND, MSP0=VCC > > + 6 - MPS2=VCC, MPS1=VCC, MSP0=GND > > + 7 - MPS2=VCC, MPS1=VCC, MSP0=VCC (enables SoftSpan feature) > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [0, 1, 2, 3, 4, 5, 6, 7] > > + default: 7 > > Are these always hard-wired or could they be connected to gpios and > made configurable at runtime? This is always a fun gap for GPIOs. It would be nice to have a generic binding that said there was a fixed GPIO value that we could then query but not set. I'm not aware of a general way to do this so we end up with optional GPIOs and some vendor specific property for when it's fixed. > > > + > > + patternProperties: > > + "^channel@([0-3])$": > > + $ref: '#/$defs/toggle-operation' > > + unevaluatedProperties: false > > + > > + description: Channel in toggle functionality. > > + > > + properties: > > + adi,output-range-microvolt: > > + description: Specify the channel output full scale range. > > How would someone writing a .dts know what values to select for this > property? Or is this something that should be configured at runtime > instead of in the devicetree? Or should this info come from the > missing voltage supplies I mentioned? Sometimes this one is a wiring related choice. Sometimes to the extent that picking the wrong one from any userspace control can cause damage or is at least nonsense. You look to be right though that the possible values here aren' fine if the internal reference is used, but not the external. However, it's keyed off MPS pins so you can't control it if they aren't tied to all high. So I'd imagine if the board can be damaged it will be hard wired. Hence these could be controlled form userspace. It's a bit fiddly though as combines scale and offset controls and you can end trying to set things to an invalid combination. E.g. scale set to cover 20V range and offset set to 0V To get around that you have to clamp one parameter to nearest possible when the other is changed. > > > + oneOf: > > + - items: > > + - const: 0 > > + - enum: [5000000, 10000000] > > + - items: > > + - const: -5000000 > > + - const: 5000000 > > + - items: > > + - const: -10000000 > > + - const: 10000000 > > + - items: > > + - const: -2500000 > > + - const: 2500000 > > + > > + required: > > + - adi,output-range-microvolt > > + > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: adi,ltc2672 > > + then: > > + properties: > > + adi,rfsadj-ohms: > > + description: If FSADJ is tied to VCC, an internal RFSADJ (20 kΩ) is > > + selected, which results in nominal output ranges. When an external > > + resistor of 19 kΩ to 41 kΩ can be used instead by connecting the > > + resistor between FSADJ and GND it controls the scaling of the > > + ranges, and the internal resistor is automatically disconnected. > > + minimum: 19000 > > + maximum: 41000 > > + default: 20000 > > This is the kind of description that would be helpful on some of the > other properties. It does a good job of explaining what value to > select based on what is connected to the chip. > > > + > > + patternProperties: > > + "^channel@([0-4])$": > > + $ref: '#/$defs/toggle-operation' > > + unevaluatedProperties: false > > + > > + description: Configuration properties for a channel in toggle mode > > + > > + properties: > > + adi,output-range-microamp: > > + description: Specify the channel output full scale range. > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [3125000, 6250000, 12500000, 25000000, 50000000, 100000000, > > + 200000000, 300000000] > > + > > Same comments as adi,output-range-microvolt apply here. > > > + required: > > + - adi,output-range-microamp > > + > > +required: > > + - compatible > > + - reg > > + - spi-max-frequency > > + - vcc-supply > > + - iovcc-supply > > + > > +additionalProperties: false > > + ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml 2024-04-13 15:06 ` Jonathan Cameron @ 2024-04-13 15:11 ` Jonathan Cameron 2024-04-13 16:21 ` David Lechner 1 sibling, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-04-13 15:11 UTC (permalink / raw To: David Lechner Cc: Kim Seer Paller, linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Michael Hennerich On Sat, 13 Apr 2024 16:06:10 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 12 Apr 2024 16:23:00 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > > <kimseer.paller@analog.com> wrote: > > > > > > Add documentation for ltc2664 and ltc2672. > > > > > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > > A few follow up comments inline as David and Krzysztof already gave > good feedback. > > > > --- > > > .../bindings/iio/dac/adi,ltc2664.yaml | 230 ++++++++++++++++++ > > > MAINTAINERS | 8 + > > > 2 files changed, 238 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > > new file mode 100644 > > > index 000000000..2f581a9e5 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > > @@ -0,0 +1,230 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/iio/dac/adi,ltc2664.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Analog Devices LTC2664 and LTC2672 DAC > > > + > > > +maintainers: > > > + - Michael Hennerich <michael.hennerich@analog.com> > > > + - Kim Seer Paller <kimseer.paller@analog.com> > > > + > > > +description: | > > > + Analog Devices LTC2664 4 channel, 16 bit, +-10V DAC > > > + Analog Devices LTC2672 5 channel, 16 bit, 300mA DAC > > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2664.pdf > > > > This link gives a 404 error. Is there a typo? > > > > > > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2672.pdf > > > + > > > +$defs: > > > + toggle-operation: > > > + type: object > > > + description: Toggle mode channel setting. > > > + > > > + properties: > > > + reg: > > > + description: Channel number. > > > + minimum: 0 > > > + maximum: 4 > > > + > > > > > + adi,toggle-mode: > > > + description: > > > + Set the channel as a toggle enabled channel. Toggle operation enables > > > + fast switching of a DAC output between two different DAC codes without > > > + any SPI transaction. > > > + type: boolean > > > > I'm not convinced that this belongs in the devicetree. It seems like > > everything related to toggling can and should be left to runtime > > configuration. > > Agreed - probably on a fifo basis, so each time you switch to the other > toggle value, but if you happen to already have the value stored already > you can elide the SPI transfer. > > I think we already have a device doing this, though I can't remember which > driver it is. Perhaps search for toggle. Ah. Looking at the ABI, this is like the FSK cases (or I think we have something similar for DACs used in power control) in that the toggle pin is out of our control. We are just setting up what happens when it is tweaked by some external entity. Arguably that's yet another mode. > > > > > > + > > > +patternProperties: > > > + "^channel@[0-4]$": > > > + type: object > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - adi,ltc2664 > > > + - adi,ltc2672 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + spi-max-frequency: > > > + maximum: 50000000 > > > + > > > + vcc-supply: > > > + description: Analog Supply Voltage Input. > > > + > > > > There is also an input supply for each output channel on ltc2672, so I > > think we also need vdd0-supply, vdd1-supply, etc. > > > > On ltc2664, there is V+ instead so it needs v-pos-supply. > > > > And there is V~ on both which can be between -5.5V/-15.75V and GND, so > > optional v-neg-supply seems appropriate. > > Only make it optional in the binding if the settings of the device change > depending on whether it is there or not. Looks like there is an internal > reference, so maybe it really is optional. > > > > > > + iovcc-supply: > > > + description: Digital Input/Output Supply Voltage. > > > + > > > + vref-supply: > > > + description: > > > + Reference Input/Output. The voltage at the REF pin sets the full-scale > > > + range of all channels. If not provided the internal reference is used and > > > + also provided on the VREF pin. > > > > There is no VREF pin, so it looks like there is a typo. And it would > > make more sense to call this ref-supply as well. > > > > > + > > > + clr-gpios: > > > + description: > > > + If specified, it will be asserted during driver probe. As the line is > > > + active low, it should be marked GPIO_ACTIVE_LOW. > > > + maxItems: 1 > > > > Some other potentially properties for complete bindings that would be > > trivial to add now: > > > > * (ltc2672) There is a FAULT output pin, so it would make sense to > > have an interrupts property for that signal. > > * (both) I haven't done any DACs myself yet, but I see ldac-gpios on a > > few other bindings. I assume this is the typical way for handling the > > LDAC signal on most DACs? > > * (both) I see these have daisy chain capabilities, so an optional > > #daisy-chained-devices could be appropriate. > > > > Maybe not so trivial: > > > > * (both) The MUX/MUXOUT pins look like we have an embedded pin mux, so > > it could mean we need #pinctrl-cells. ltc2664 would also need > > muxin-gpios for this. > Not convinced that's the right approach - looks more like a channel > selector than a conventional mux or pin control. Sure that's a mux, but > we want a clean userspace control to let us choose a signal to measure > at runtime > > If you wanted to support this I'd have the binding describe optional > stuff to act as a consumer of an ADC channel on another device. > The IIO driver would then provide a bunch of input channels to allow > measurement of each of the signals. > > Look at io-channels etc in existing bindings for how to do that. > > > > > > > > + > > > + '#address-cells': > > > + const: 1 > > > + > > > + '#size-cells': > > > + const: 0 > > > + > > > +allOf: > > > + - if: > > > + properties: > > > + compatible: > > > + contains: > > > + const: adi,ltc2664 > > > + then: > > > + properties: > > > + adi,manual-span-operation-config: > > > + description: > > > + This property must mimic the MSPAN pin configurations. > > > + By tying the MSPAN pins (MSP2, MSP1 and MSP0) to GND > > > + and/or VCC, any output range can be hardware-configured > > > + with different mid-scale or zero-scale reset options. > > > + The hardware configuration is latched during power on reset > > > + for proper operation. > > > + 0 - MPS2=GND, MPS1=GND, MSP0=GND > > > + 1 - MPS2=GND, MPS1=GND, MSP0=VCC > > > + 2 - MPS2=GND, MPS1=VCC, MSP0=GND > > > + 3 - MPS2=GND, MPS1=VCC, MSP0=VCC > > > + 4 - MPS2=VCC, MPS1=GND, MSP0=GND > > > + 5 - MPS2=VCC, MPS1=GND, MSP0=VCC > > > + 6 - MPS2=VCC, MPS1=VCC, MSP0=GND > > > + 7 - MPS2=VCC, MPS1=VCC, MSP0=VCC (enables SoftSpan feature) > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + enum: [0, 1, 2, 3, 4, 5, 6, 7] > > > + default: 7 > > > > Are these always hard-wired or could they be connected to gpios and > > made configurable at runtime? > > This is always a fun gap for GPIOs. It would be nice to have a generic > binding that said there was a fixed GPIO value that we could then query > but not set. I'm not aware of a general way to do this so we end up with > optional GPIOs and some vendor specific property for when it's fixed. > > > > > > + > > > + patternProperties: > > > + "^channel@([0-3])$": > > > + $ref: '#/$defs/toggle-operation' > > > + unevaluatedProperties: false > > > + > > > + description: Channel in toggle functionality. > > > + > > > + properties: > > > + adi,output-range-microvolt: > > > + description: Specify the channel output full scale range. > > > > How would someone writing a .dts know what values to select for this > > property? Or is this something that should be configured at runtime > > instead of in the devicetree? Or should this info come from the > > missing voltage supplies I mentioned? > > Sometimes this one is a wiring related choice. Sometimes to the extent > that picking the wrong one from any userspace control can cause damage > or is at least nonsense. > > You look to be right though that the possible values here aren' fine > if the internal reference is used, but not the external. > > However, it's keyed off MPS pins so you can't control it if they aren't > tied to all high. So I'd imagine if the board can be damaged it will > be hard wired. Hence these could be controlled form userspace. > It's a bit fiddly though as combines scale and offset controls and > you can end trying to set things to an invalid combination. > E.g. scale set to cover 20V range and offset set to 0V > To get around that you have to clamp one parameter to nearest > possible when the other is changed. > > > > > > + oneOf: > > > + - items: > > > + - const: 0 > > > + - enum: [5000000, 10000000] > > > + - items: > > > + - const: -5000000 > > > + - const: 5000000 > > > + - items: > > > + - const: -10000000 > > > + - const: 10000000 > > > + - items: > > > + - const: -2500000 > > > + - const: 2500000 > > > + > > > + required: > > > + - adi,output-range-microvolt > > > + > > > + - if: > > > + properties: > > > + compatible: > > > + contains: > > > + const: adi,ltc2672 > > > + then: > > > + properties: > > > + adi,rfsadj-ohms: > > > + description: If FSADJ is tied to VCC, an internal RFSADJ (20 kΩ) is > > > + selected, which results in nominal output ranges. When an external > > > + resistor of 19 kΩ to 41 kΩ can be used instead by connecting the > > > + resistor between FSADJ and GND it controls the scaling of the > > > + ranges, and the internal resistor is automatically disconnected. > > > + minimum: 19000 > > > + maximum: 41000 > > > + default: 20000 > > > > This is the kind of description that would be helpful on some of the > > other properties. It does a good job of explaining what value to > > select based on what is connected to the chip. > > > > > + > > > + patternProperties: > > > + "^channel@([0-4])$": > > > + $ref: '#/$defs/toggle-operation' > > > + unevaluatedProperties: false > > > + > > > + description: Configuration properties for a channel in toggle mode > > > + > > > + properties: > > > + adi,output-range-microamp: > > > + description: Specify the channel output full scale range. > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + enum: [3125000, 6250000, 12500000, 25000000, 50000000, 100000000, > > > + 200000000, 300000000] > > > + > > > > Same comments as adi,output-range-microvolt apply here. > > > > > + required: > > > + - adi,output-range-microamp > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - spi-max-frequency > > > + - vcc-supply > > > + - iovcc-supply > > > + > > > +additionalProperties: false > > > + > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml 2024-04-13 15:06 ` Jonathan Cameron 2024-04-13 15:11 ` Jonathan Cameron @ 2024-04-13 16:21 ` David Lechner 2024-04-13 17:10 ` Jonathan Cameron 1 sibling, 1 reply; 24+ messages in thread From: David Lechner @ 2024-04-13 16:21 UTC (permalink / raw To: Jonathan Cameron Cc: Kim Seer Paller, linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Michael Hennerich On Sat, Apr 13, 2024 at 10:06 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 12 Apr 2024 16:23:00 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > > <kimseer.paller@analog.com> wrote: > > > ... > > > > And there is V~ on both which can be between -5.5V/-15.75V and GND, so > > optional v-neg-supply seems appropriate. > > Only make it optional in the binding if the settings of the device change > depending on whether it is there or not. Looks like there is an internal > reference, so maybe it really is optional. I suggested optional with the thinking that if the pin is tied to GND, then the property would be omitted. ... > > > > * (both) The MUX/MUXOUT pins look like we have an embedded pin mux, so > > it could mean we need #pinctrl-cells. ltc2664 would also need > > muxin-gpios for this. > Not convinced that's the right approach - looks more like a channel > selector than a conventional mux or pin control. Sure that's a mux, but > we want a clean userspace control to let us choose a signal to measure > at runtime > > If you wanted to support this I'd have the binding describe optional > stuff to act as a consumer of an ADC channel on another device. > The IIO driver would then provide a bunch of input channels to allow > measurement of each of the signals. > > Look at io-channels etc in existing bindings for how to do that. > Right. I was thinking that this pin might be connected to something else external rather than the signal coming back to the SoC (or whatever has the SPI controller). But it makes more sense that we would want it as extra channels being read back by the SoC for diagnostics. ... > > > > > + > > > + patternProperties: > > > + "^channel@([0-3])$": > > > + $ref: '#/$defs/toggle-operation' > > > + unevaluatedProperties: false > > > + > > > + description: Channel in toggle functionality. > > > + > > > + properties: > > > + adi,output-range-microvolt: > > > + description: Specify the channel output full scale range. > > > > How would someone writing a .dts know what values to select for this > > property? Or is this something that should be configured at runtime > > instead of in the devicetree? Or should this info come from the > > missing voltage supplies I mentioned? > > Sometimes this one is a wiring related choice. Sometimes to the extent > that picking the wrong one from any userspace control can cause damage > or is at least nonsense. > > You look to be right though that the possible values here aren' fine > if the internal reference is used, but not the external. > > However, it's keyed off MPS pins so you can't control it if they aren't > tied to all high. So I'd imagine if the board can be damaged it will > be hard wired. Hence these could be controlled form userspace. > It's a bit fiddly though as combines scale and offset controls and > you can end trying to set things to an invalid combination. > E.g. scale set to cover 20V range and offset set to 0V > To get around that you have to clamp one parameter to nearest > possible when the other is changed. > Thanks for the explanation. It sounds like I missed something in the datasheet that would be helpful to call out in the description for this property. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml 2024-04-13 16:21 ` David Lechner @ 2024-04-13 17:10 ` Jonathan Cameron 2024-04-16 14:40 ` Paller, Kim Seer 0 siblings, 1 reply; 24+ messages in thread From: Jonathan Cameron @ 2024-04-13 17:10 UTC (permalink / raw To: David Lechner Cc: Kim Seer Paller, linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Michael Hennerich On Sat, 13 Apr 2024 11:21:55 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Sat, Apr 13, 2024 at 10:06 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 12 Apr 2024 16:23:00 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > > > <kimseer.paller@analog.com> wrote: > > > > > > ... > > > > > > > And there is V~ on both which can be between -5.5V/-15.75V and GND, so > > > optional v-neg-supply seems appropriate. > > > > Only make it optional in the binding if the settings of the device change > > depending on whether it is there or not. Looks like there is an internal > > reference, so maybe it really is optional. > > I suggested optional with the thinking that if the pin is tied to GND, > then the property would be omitted. We could but given VND isn't really that special in this case I think I'd prefer a fixed voltage reg of 0V if someone does wire it like that. (I think that works, though not sure I've tried a 0V supply ;) > > > ... > > > > > > > > * (both) The MUX/MUXOUT pins look like we have an embedded pin mux, so > > > it could mean we need #pinctrl-cells. ltc2664 would also need > > > muxin-gpios for this. > > Not convinced that's the right approach - looks more like a channel > > selector than a conventional mux or pin control. Sure that's a mux, but > > we want a clean userspace control to let us choose a signal to measure > > at runtime > > > > If you wanted to support this I'd have the binding describe optional > > stuff to act as a consumer of an ADC channel on another device. > > The IIO driver would then provide a bunch of input channels to allow > > measurement of each of the signals. > > > > Look at io-channels etc in existing bindings for how to do that. > > > > Right. I was thinking that this pin might be connected to something > else external rather than the signal coming back to the SoC (or > whatever has the SPI controller). But it makes more sense that we > would want it as extra channels being read back by the SoC for > diagnostics. It might indeed. But I think that's an exercise for the future if it matters. Might be a debugfs control only perhaps. > > ... > > > > > > > > + > > > > + patternProperties: > > > > + "^channel@([0-3])$": > > > > + $ref: '#/$defs/toggle-operation' > > > > + unevaluatedProperties: false > > > > + > > > > + description: Channel in toggle functionality. > > > > + > > > > + properties: > > > > + adi,output-range-microvolt: > > > > + description: Specify the channel output full scale range. > > > > > > How would someone writing a .dts know what values to select for this > > > property? Or is this something that should be configured at runtime > > > instead of in the devicetree? Or should this info come from the > > > missing voltage supplies I mentioned? > > > > Sometimes this one is a wiring related choice. Sometimes to the extent > > that picking the wrong one from any userspace control can cause damage > > or is at least nonsense. > > > > You look to be right though that the possible values here aren' fine > > if the internal reference is used, but not the external. > > > > However, it's keyed off MPS pins so you can't control it if they aren't > > tied to all high. So I'd imagine if the board can be damaged it will > > be hard wired. Hence these could be controlled form userspace. > > It's a bit fiddly though as combines scale and offset controls and > > you can end trying to set things to an invalid combination. > > E.g. scale set to cover 20V range and offset set to 0V > > To get around that you have to clamp one parameter to nearest > > possible when the other is changed. > > > > Thanks for the explanation. It sounds like I missed something in the > datasheet that would be helpful to call out in the description for > this property. Agreed - it needs more detail. Jonathan ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml 2024-04-13 17:10 ` Jonathan Cameron @ 2024-04-16 14:40 ` Paller, Kim Seer 2024-04-20 10:13 ` Jonathan Cameron 0 siblings, 1 reply; 24+ messages in thread From: Paller, Kim Seer @ 2024-04-16 14:40 UTC (permalink / raw To: Jonathan Cameron, David Lechner Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Hennerich, Michael > -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, April 14, 2024 1:10 AM > To: David Lechner <dlechner@baylibre.com> > Cc: Paller, Kim Seer <KimSeer.Paller@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 > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > Hennerich, Michael <Michael.Hennerich@analog.com> > Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml > > [External] > > On Sat, 13 Apr 2024 11:21:55 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On Sat, Apr 13, 2024 at 10:06 AM Jonathan Cameron <jic23@kernel.org> > wrote: > > > > > > On Fri, 12 Apr 2024 16:23:00 -0500 > > > David Lechner <dlechner@baylibre.com> wrote: > > > > > > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > > > > <kimseer.paller@analog.com> wrote: > > > > > > > > > ... > > > > > > > > > > And there is V~ on both which can be between -5.5V/-15.75V and GND, so > > > > optional v-neg-supply seems appropriate. > > > > > > Only make it optional in the binding if the settings of the device change > > > depending on whether it is there or not. Looks like there is an internal > > > reference, so maybe it really is optional. > > > > I suggested optional with the thinking that if the pin is tied to GND, > > then the property would be omitted. > > We could but given VND isn't really that special in this case I think > I'd prefer a fixed voltage reg of 0V if someone does wire it like that. > (I think that works, though not sure I've tried a 0V supply ;) To clarify, does this mean we should still add an optional property for it in the binding? > > ... > > > > > > > > > > > > * (both) The MUX/MUXOUT pins look like we have an embedded pin mux, > so > > > > it could mean we need #pinctrl-cells. ltc2664 would also need > > > > muxin-gpios for this. > > > Not convinced that's the right approach - looks more like a channel > > > selector than a conventional mux or pin control. Sure that's a mux, but > > > we want a clean userspace control to let us choose a signal to measure > > > at runtime > > > > > > If you wanted to support this I'd have the binding describe optional > > > stuff to act as a consumer of an ADC channel on another device. > > > The IIO driver would then provide a bunch of input channels to allow > > > measurement of each of the signals. > > > > > > Look at io-channels etc in existing bindings for how to do that. > > > > > > > Right. I was thinking that this pin might be connected to something > > else external rather than the signal coming back to the SoC (or > > whatever has the SPI controller). But it makes more sense that we > > would want it as extra channels being read back by the SoC for > > diagnostics. > > It might indeed. But I think that's an exercise for the future if > it matters. Might be a debugfs control only perhaps. We can consider potential future use cases as they become relevant. For now, we might not to include or support this functionality. > > > > ... > > > > > > > > > > > + > > > > > + patternProperties: > > > > > + "^channel@([0-3])$": > > > > > + $ref: '#/$defs/toggle-operation' > > > > > + unevaluatedProperties: false > > > > > + > > > > > + description: Channel in toggle functionality. > > > > > + > > > > > + properties: > > > > > + adi,output-range-microvolt: > > > > > + description: Specify the channel output full scale range. > > > > > > > > How would someone writing a .dts know what values to select for this > > > > property? Or is this something that should be configured at runtime > > > > instead of in the devicetree? Or should this info come from the > > > > missing voltage supplies I mentioned? > > > > > > Sometimes this one is a wiring related choice. Sometimes to the extent > > > that picking the wrong one from any userspace control can cause damage > > > or is at least nonsense. > > > > > > You look to be right though that the possible values here aren' fine > > > if the internal reference is used, but not the external. > > > > > > However, it's keyed off MPS pins so you can't control it if they aren't > > > tied to all high. So I'd imagine if the board can be damaged it will > > > be hard wired. Hence these could be controlled form userspace. > > > It's a bit fiddly though as combines scale and offset controls and > > > you can end trying to set things to an invalid combination. > > > E.g. scale set to cover 20V range and offset set to 0V > > > To get around that you have to clamp one parameter to nearest > > > possible when the other is changed. > > > > > > > Thanks for the explanation. It sounds like I missed something in the > > datasheet that would be helpful to call out in the description for > > this property. > Agreed - it needs more detail. > > Jonathan > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml 2024-04-16 14:40 ` Paller, Kim Seer @ 2024-04-20 10:13 ` Jonathan Cameron 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-04-20 10:13 UTC (permalink / raw To: Paller, Kim Seer Cc: David Lechner, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Hennerich, Michael On Tue, 16 Apr 2024 14:40:56 +0000 "Paller, Kim Seer" <KimSeer.Paller@analog.com> wrote: > > -----Original Message----- > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Sunday, April 14, 2024 1:10 AM > > To: David Lechner <dlechner@baylibre.com> > > Cc: Paller, Kim Seer <KimSeer.Paller@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 > > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; > > Hennerich, Michael <Michael.Hennerich@analog.com> > > Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml > > > > [External] > > > > On Sat, 13 Apr 2024 11:21:55 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > On Sat, Apr 13, 2024 at 10:06 AM Jonathan Cameron <jic23@kernel.org> > > wrote: > > > > > > > > On Fri, 12 Apr 2024 16:23:00 -0500 > > > > David Lechner <dlechner@baylibre.com> wrote: > > > > > > > > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > > > > > <kimseer.paller@analog.com> wrote: > > > > > > > > > > > > ... > > > > > > > > > > > > > And there is V~ on both which can be between -5.5V/-15.75V and GND, so > > > > > optional v-neg-supply seems appropriate. > > > > > > > > Only make it optional in the binding if the settings of the device change > > > > depending on whether it is there or not. Looks like there is an internal > > > > reference, so maybe it really is optional. > > > > > > I suggested optional with the thinking that if the pin is tied to GND, > > > then the property would be omitted. > > > > We could but given VND isn't really that special in this case I think > > I'd prefer a fixed voltage reg of 0V if someone does wire it like that. > > (I think that works, though not sure I've tried a 0V supply ;) > > To clarify, does this mean we should still add an optional property for it in the binding? I think it should not be optional. Should be fine providing a 0V fixed regulator via DT if it is wired to 0. Given that is a little unusual, check it works! > > > > ... > > > > > > > > > > > > > > > > * (both) The MUX/MUXOUT pins look like we have an embedded pin mux, > > so > > > > > it could mean we need #pinctrl-cells. ltc2664 would also need > > > > > muxin-gpios for this. > > > > Not convinced that's the right approach - looks more like a channel > > > > selector than a conventional mux or pin control. Sure that's a mux, but > > > > we want a clean userspace control to let us choose a signal to measure > > > > at runtime > > > > > > > > If you wanted to support this I'd have the binding describe optional > > > > stuff to act as a consumer of an ADC channel on another device. > > > > The IIO driver would then provide a bunch of input channels to allow > > > > measurement of each of the signals. > > > > > > > > Look at io-channels etc in existing bindings for how to do that. > > > > > > > > > > Right. I was thinking that this pin might be connected to something > > > else external rather than the signal coming back to the SoC (or > > > whatever has the SPI controller). But it makes more sense that we > > > would want it as extra channels being read back by the SoC for > > > diagnostics. > > > > It might indeed. But I think that's an exercise for the future if > > it matters. Might be a debugfs control only perhaps. > > We can consider potential future use cases as they become relevant. > For now, we might not to include or support this functionality. The pins should be in the DT binding, though the driver support can of course be a future exercise. Make them optional. > > > > > > > ... > > > > > > > > > > > > > > + > > > > > > + patternProperties: > > > > > > + "^channel@([0-3])$": > > > > > > + $ref: '#/$defs/toggle-operation' > > > > > > + unevaluatedProperties: false > > > > > > + > > > > > > + description: Channel in toggle functionality. > > > > > > + > > > > > > + properties: > > > > > > + adi,output-range-microvolt: > > > > > > + description: Specify the channel output full scale range. > > > > > > > > > > How would someone writing a .dts know what values to select for this > > > > > property? Or is this something that should be configured at runtime > > > > > instead of in the devicetree? Or should this info come from the > > > > > missing voltage supplies I mentioned? > > > > > > > > Sometimes this one is a wiring related choice. Sometimes to the extent > > > > that picking the wrong one from any userspace control can cause damage > > > > or is at least nonsense. > > > > > > > > You look to be right though that the possible values here aren' fine > > > > if the internal reference is used, but not the external. > > > > > > > > However, it's keyed off MPS pins so you can't control it if they aren't > > > > tied to all high. So I'd imagine if the board can be damaged it will > > > > be hard wired. Hence these could be controlled form userspace. > > > > It's a bit fiddly though as combines scale and offset controls and > > > > you can end trying to set things to an invalid combination. > > > > E.g. scale set to cover 20V range and offset set to 0V > > > > To get around that you have to clamp one parameter to nearest > > > > possible when the other is changed. > > > > > > > > > > Thanks for the explanation. It sounds like I missed something in the > > > datasheet that would be helpful to call out in the description for > > > this property. > > Agreed - it needs more detail. > > > > Jonathan > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC 2024-04-12 3:20 [PATCH 0/4] Add driver for LTC2664 and LTC2672 Kim Seer Paller 2024-04-12 3:20 ` [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml Kim Seer Paller @ 2024-04-12 3:21 ` Kim Seer Paller 2024-04-12 21:26 ` David Lechner 2024-04-12 3:21 ` [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC Kim Seer Paller 2024-04-12 3:21 ` [PATCH 4/4] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672 Kim Seer Paller 3 siblings, 1 reply; 24+ messages in thread From: Kim Seer Paller @ 2024-04-12 3:21 UTC (permalink / raw To: linux-iio, devicetree, linux-kernel Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, David Lechner, Michael Hennerich Define the sysfs interface for toggle capable channels. Toggle enabled channels will have: * out_voltageY_toggle_en * out_voltageY_raw0 * out_voltageY_raw1 * out_voltageY_symbol The common interface present in all channels is: * out_voltageY_raw (not present in toggle enabled channels) * out_voltageY_raw_available * out_voltageY_powerdown * out_voltageY_scale * out_voltageY_offset Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> --- .../ABI/testing/sysfs-bus-iio-dac-ltc2664 | 30 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 31 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 new file mode 100644 index 000000000..4b656b7af --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 @@ -0,0 +1,30 @@ +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_toggle_en +KernelVersion: 5.18 +Contact: linux-iio@vger.kernel.org +Description: + Toggle enable. Write 1 to enable toggle or 0 to disable it. This is + useful when one wants to change the DAC output codes. The way it should + be done is: + + - disable toggle operation; + - change out_voltageY_raw0 and out_voltageY_raw1; + - enable toggle operation. + +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_raw0 +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_raw1 +KernelVersion: 5.18 +Contact: linux-iio@vger.kernel.org +Description: + It has the same meaning as out_voltageY_raw. This attribute is + specific to toggle enabled channels and refers to the DAC output + code in INPUT_A (_raw0) and INPUT_B (_raw1). The same scale and offset + as in out_voltageY_raw applies. + +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_symbol +KernelVersion: 5.18 +Contact: linux-iio@vger.kernel.org +Description: + Performs a SW toggle. This attribute is specific to toggle + enabled channels and allows to toggle between out_voltageY_raw0 + and out_voltageY_raw1 through software. Writing 0 will select + out_voltageY_raw0 while 1 selects out_voltageY_raw1. diff --git a/MAINTAINERS b/MAINTAINERS index bd8645f6e..9ed00b364 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12842,6 +12842,7 @@ M: Kim Seer Paller <kimseer.paller@analog.com> L: linux-iio@vger.kernel.org S: Supported W: https://ez.analog.com/linux-software-drivers +F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 F: Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml LTC2688 IIO DAC DRIVER -- 2.34.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC 2024-04-12 3:21 ` [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC Kim Seer Paller @ 2024-04-12 21:26 ` David Lechner 2024-04-13 15:25 ` Jonathan Cameron 0 siblings, 1 reply; 24+ messages in thread From: David Lechner @ 2024-04-12 21:26 UTC (permalink / raw To: Kim Seer Paller Cc: linux-iio, devicetree, linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Michael Hennerich On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller <kimseer.paller@analog.com> wrote: > > Define the sysfs interface for toggle capable channels. > > Toggle enabled channels will have: > > * out_voltageY_toggle_en It looks like there are 3 toggle modes. Two involve the notion of "enabled" outputs that I assume this attribute is for: 1. Toggling all enabled pins at the same time using a software trigger (global toggle bit) 2. Toggling all enabled pins at the same time using a hardware trigger (TGP pin) and toggling pins The third mode though looks like it uses the same toggle select register for selecting A or B for each channel instead of enabling or disabling each channel. 3. Toggling all pins to A or B based on the toggle select register. No notion of enabled pins here. I haven't looked at the driver implementation, but it sounds like out_voltageY_toggle_en and out_voltageY_symbol would be writing to the same register in conflicting ways. So maybe we need yet another custom attribute to select the currently active toggle mode? In any case, it would be helpful if the documentation below explained a bit better the intention and conditions required to use each attribute (or add a .rst documentation file for these chips to explain how to use it in more detail since this is rather complex feature). > * out_voltageY_raw0 > * out_voltageY_raw1 I guess there is no enum iio_modifier that fits this. It seems like we could still have out_voltageY_raw for register A so that users that don't need to do any toggling can use standard ABI. And maybe out_voltageY_raw_toggled for register B (question for Jonathan)? Or just have 8 channels instead of 4 where even channels are register A and odd channels are register B? > * out_voltageY_symbol "symbol" is a confusing name. It sounds like this just supports toggling one channel individually so _toggle_select would make more sense to me. Are there plans for supporting toggling multiple channels at the same time using a software trigger as well? > > The common interface present in all channels is: > > * out_voltageY_raw (not present in toggle enabled channels) As mentioned above, I don't think we need to have to make a distinction between toggle enabled channels and not enabled channels. > * out_voltageY_raw_available > * out_voltageY_powerdown Is _powerdown a standard attribute? I don't see it documented anywhere. Perhaps you meant _en (via IIO_CHAN_INFO_ENABLE)? > * out_voltageY_scale > * out_voltageY_offset > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > --- > .../ABI/testing/sysfs-bus-iio-dac-ltc2664 | 30 +++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 31 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 > new file mode 100644 > index 000000000..4b656b7af > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 > @@ -0,0 +1,30 @@ > +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_toggle_en > +KernelVersion: 5.18 > +Contact: linux-iio@vger.kernel.org > +Description: > + Toggle enable. Write 1 to enable toggle or 0 to disable it. This is > + useful when one wants to change the DAC output codes. The way it should > + be done is: > + > + - disable toggle operation; > + - change out_voltageY_raw0 and out_voltageY_raw1; > + - enable toggle operation. > + > +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_raw0 > +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_raw1 > +KernelVersion: 5.18 > +Contact: linux-iio@vger.kernel.org > +Description: > + It has the same meaning as out_voltageY_raw. This attribute is > + specific to toggle enabled channels and refers to the DAC output > + code in INPUT_A (_raw0) and INPUT_B (_raw1). The same scale and offset > + as in out_voltageY_raw applies. > + > +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_symbol > +KernelVersion: 5.18 > +Contact: linux-iio@vger.kernel.org > +Description: > + Performs a SW toggle. This attribute is specific to toggle > + enabled channels and allows to toggle between out_voltageY_raw0 > + and out_voltageY_raw1 through software. Writing 0 will select > + out_voltageY_raw0 while 1 selects out_voltageY_raw1. > diff --git a/MAINTAINERS b/MAINTAINERS > index bd8645f6e..9ed00b364 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12842,6 +12842,7 @@ M: Kim Seer Paller <kimseer.paller@analog.com> > L: linux-iio@vger.kernel.org > S: Supported > W: https://ez.analog.com/linux-software-drivers > +F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 > F: Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > LTC2688 IIO DAC DRIVER > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC 2024-04-12 21:26 ` David Lechner @ 2024-04-13 15:25 ` Jonathan Cameron 2024-04-13 20:38 ` David Lechner 2024-04-15 12:45 ` Nuno Sá 0 siblings, 2 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-04-13 15:25 UTC (permalink / raw To: David Lechner Cc: Kim Seer Paller, linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Michael Hennerich On Fri, 12 Apr 2024 16:26:17 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > <kimseer.paller@analog.com> wrote: > > > > Define the sysfs interface for toggle capable channels. > > > > Toggle enabled channels will have: > > > > * out_voltageY_toggle_en The big missing thing in this ABI is a reference to existing precedence. You aren't actually defining anything new, it just hasn't yet been generalized beyond 1 device (unless you include PSK / FSK DDS drivers that are 'still' after 13+ years in staging!) This patch needs to be generalizing that documentation from the ltc2688. Probably in sysfs-bus-iio-dac > > It looks like there are 3 toggle modes. > > Two involve the notion of "enabled" outputs that I assume this attribute is for: > > 1. Toggling all enabled pins at the same time using a software trigger > (global toggle bit) > 2. Toggling all enabled pins at the same time using a hardware trigger > (TGP pin) and toggling pins > This is presumably the tricky one as that hardware toggle may not be in control of the host CPU. > The third mode though looks like it uses the same toggle select > register for selecting A or B for each channel instead of enabling or > disabling each channel. > > 3. Toggling all pins to A or B based on the toggle select register. No > notion of enabled pins here. > > I haven't looked at the driver implementation, but it sounds like > out_voltageY_toggle_en and out_voltageY_symbol would be writing to the > same register in conflicting ways. So maybe we need yet another custom > attribute to select the currently active toggle mode? This one feels like it could be handled as a software optimisation over just changing the DAC value directly. > > In any case, it would be helpful if the documentation below explained > a bit better the intention and conditions required to use each > attribute (or add a .rst documentation file for these chips to explain > how to use it in more detail since this is rather complex feature). > > > * out_voltageY_raw0 > > * out_voltageY_raw1 > > I guess there is no enum iio_modifier that fits this. It seems like we > could still have out_voltageY_raw for register A so that users that > don't need to do any toggling can use standard ABI. And maybe > out_voltageY_raw_toggled for register B (question for Jonathan)? There is precedence for doing it like this (ltc2688) Note that we should only see these attribute for changes specifically configured for 'hardware' triggered toggling. Note that we cannot have duplicate documentation so we need to create a common docs file covering this and existing ltc2688 ABI that overlaps. That may need some generalising to cover both parts. > > Or just have 8 channels instead of 4 where even channels are register > A and odd channels are register B? > > > * out_voltageY_symbol > > "symbol" is a confusing name. It sounds like this just supports > toggling one channel individually so _toggle_select would make more > sense to me. Are there plans for supporting toggling multiple channels > at the same time using a software trigger as well? Again, precedence should have been called out. It's not great ABI but it corresponds to earlier work on Frequency Shift Keying DDS devices (and I think Phase Shift Keying as well) in which this is call symbol. Hence the name. > > > > > The common interface present in all channels is: > > > > * out_voltageY_raw (not present in toggle enabled channels) > > As mentioned above, I don't think we need to have to make a > distinction between toggle enabled channels and not enabled channels. Was a while back but I think that last time this turned up we concluded we did need a different interface because the current 'toggle value' is not in our control. Hence you are programming one channel that does different things - think of it as setting the Max and Min values for a generated waveform - perhaps the toggle pin is connected to a PWM for example. > > > * out_voltageY_raw_available > > * out_voltageY_powerdown > > Is _powerdown a standard attribute? I don't see it documented > anywhere. Perhaps you meant _en (via IIO_CHAN_INFO_ENABLE)? It's there in Documentation/ABI/testing/sysfs-bus-iio Different form simple enable (which came much later as ABI) because it means entering a powerdown state in which a particular thing happens on the output pin. It is defined alongside powerdown_mode which defines what happens. (often a choice between different impedance / High Z etc) > > > > * out_voltageY_scale > > * out_voltageY_offset > > > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > > --- > > .../ABI/testing/sysfs-bus-iio-dac-ltc2664 | 30 +++++++++++++++++++ > > MAINTAINERS | 1 + > > 2 files changed, 31 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 > > new file mode 100644 > > index 000000000..4b656b7af > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 > > @@ -0,0 +1,30 @@ > > +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_toggle_en > > +KernelVersion: 5.18 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + Toggle enable. Write 1 to enable toggle or 0 to disable it. This is > > + useful when one wants to change the DAC output codes. The way it should > > + be done is: > > + > > + - disable toggle operation; > > + - change out_voltageY_raw0 and out_voltageY_raw1; > > + - enable toggle operation. > > + > > +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_raw0 > > +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_raw1 > > +KernelVersion: 5.18 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + It has the same meaning as out_voltageY_raw. This attribute is > > + specific to toggle enabled channels and refers to the DAC output > > + code in INPUT_A (_raw0) and INPUT_B (_raw1). The same scale and offset > > + as in out_voltageY_raw applies. > > + > > +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_symbol > > +KernelVersion: 5.18 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + Performs a SW toggle. This attribute is specific to toggle > > + enabled channels and allows to toggle between out_voltageY_raw0 > > + and out_voltageY_raw1 through software. Writing 0 will select > > + out_voltageY_raw0 while 1 selects out_voltageY_raw1. > > diff --git a/MAINTAINERS b/MAINTAINERS > > index bd8645f6e..9ed00b364 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -12842,6 +12842,7 @@ M: Kim Seer Paller <kimseer.paller@analog.com> > > L: linux-iio@vger.kernel.org > > S: Supported > > W: https://ez.analog.com/linux-software-drivers > > +F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 > > F: Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > > > LTC2688 IIO DAC DRIVER > > -- > > 2.34.1 > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC 2024-04-13 15:25 ` Jonathan Cameron @ 2024-04-13 20:38 ` David Lechner 2024-04-15 12:45 ` Nuno Sá 1 sibling, 0 replies; 24+ messages in thread From: David Lechner @ 2024-04-13 20:38 UTC (permalink / raw To: Jonathan Cameron Cc: Kim Seer Paller, linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Michael Hennerich On 4/13/24 10:25 AM, Jonathan Cameron wrote: > On Fri, 12 Apr 2024 16:26:17 -0500 > David Lechner <dlechner@baylibre.com> wrote: > >> On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller >> <kimseer.paller@analog.com> wrote: >>> >>> Define the sysfs interface for toggle capable channels. >>> >>> Toggle enabled channels will have: >>> >>> * out_voltageY_toggle_en > The big missing thing in this ABI is a reference to existing precedence. > You aren't actually defining anything new, it just hasn't yet been generalized > beyond 1 device (unless you include PSK / FSK DDS drivers that are 'still' after > 13+ years in staging!) > > This patch needs to be generalizing that documentation from the ltc2688. > > Probably in sysfs-bus-iio-dac > >> >> It looks like there are 3 toggle modes. >> >> Two involve the notion of "enabled" outputs that I assume this attribute is for: >> >> 1. Toggling all enabled pins at the same time using a software trigger >> (global toggle bit) >> 2. Toggling all enabled pins at the same time using a hardware trigger >> (TGP pin) and toggling pins >> > > This is presumably the tricky one as that hardware toggle may not be in > control of the host CPU. > >> The third mode though looks like it uses the same toggle select >> register for selecting A or B for each channel instead of enabling or >> disabling each channel. >> >> 3. Toggling all pins to A or B based on the toggle select register. No >> notion of enabled pins here. >> >> I haven't looked at the driver implementation, but it sounds like >> out_voltageY_toggle_en and out_voltageY_symbol would be writing to the >> same register in conflicting ways. So maybe we need yet another custom >> attribute to select the currently active toggle mode? > > This one feels like it could be handled as a software optimisation over > just changing the DAC value directly. > >> >> In any case, it would be helpful if the documentation below explained >> a bit better the intention and conditions required to use each >> attribute (or add a .rst documentation file for these chips to explain >> how to use it in more detail since this is rather complex feature). >> >>> * out_voltageY_raw0 >>> * out_voltageY_raw1 >> >> I guess there is no enum iio_modifier that fits this. It seems like we >> could still have out_voltageY_raw for register A so that users that >> don't need to do any toggling can use standard ABI. And maybe >> out_voltageY_raw_toggled for register B (question for Jonathan)? > > There is precedence for doing it like this (ltc2688) > Note that we should only see these attribute for changes specifically > configured for 'hardware' triggered toggling. > > Note that we cannot have duplicate documentation so we need to create > a common docs file covering this and existing ltc2688 ABI that overlaps. > That may need some generalising to cover both parts. > >> >> Or just have 8 channels instead of 4 where even channels are register >> A and odd channels are register B? >> >>> * out_voltageY_symbol >> >> "symbol" is a confusing name. It sounds like this just supports >> toggling one channel individually so _toggle_select would make more >> sense to me. Are there plans for supporting toggling multiple channels >> at the same time using a software trigger as well? > > Again, precedence should have been called out. It's not great ABI > but it corresponds to earlier work on Frequency Shift Keying DDS devices > (and I think Phase Shift Keying as well) in which this is call symbol. > Hence the name. > >> >>> >>> The common interface present in all channels is: >>> >>> * out_voltageY_raw (not present in toggle enabled channels) >> >> As mentioned above, I don't think we need to have to make a >> distinction between toggle enabled channels and not enabled channels. > > Was a while back but I think that last time this turned up we concluded > we did need a different interface because the current 'toggle value' > is not in our control. Hence you are programming one channel that > does different things - think of it as setting the Max and Min values > for a generated waveform - perhaps the toggle pin is connected to a PWM > for example. > >> >>> * out_voltageY_raw_available >>> * out_voltageY_powerdown >> >> Is _powerdown a standard attribute? I don't see it documented >> anywhere. Perhaps you meant _en (via IIO_CHAN_INFO_ENABLE)? > > It's there in Documentation/ABI/testing/sysfs-bus-iio > Different form simple enable (which came much later as ABI) because > it means entering a powerdown state in which a particular thing happens > on the output pin. It is defined alongside powerdown_mode which > defines what happens. (often a choice between different impedance / High Z etc) > > >> >> >>> * out_voltageY_scale >>> * out_voltageY_offset >>> >>> Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> >>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> >>> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> >>> --- >>> .../ABI/testing/sysfs-bus-iio-dac-ltc2664 | 30 +++++++++++++++++++ >>> MAINTAINERS | 1 + >>> 2 files changed, 31 insertions(+) >>> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 >>> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 >>> new file mode 100644 >>> index 000000000..4b656b7af >>> --- /dev/null >>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 >>> @@ -0,0 +1,30 @@ >>> +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_toggle_en >>> +KernelVersion: 5.18 >>> +Contact: linux-iio@vger.kernel.org >>> +Description: >>> + Toggle enable. Write 1 to enable toggle or 0 to disable it. This is >>> + useful when one wants to change the DAC output codes. The way it should >>> + be done is: >>> + >>> + - disable toggle operation; >>> + - change out_voltageY_raw0 and out_voltageY_raw1; >>> + - enable toggle operation. >>> + >>> +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_raw0 >>> +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_raw1 >>> +KernelVersion: 5.18 >>> +Contact: linux-iio@vger.kernel.org >>> +Description: >>> + It has the same meaning as out_voltageY_raw. This attribute is >>> + specific to toggle enabled channels and refers to the DAC output >>> + code in INPUT_A (_raw0) and INPUT_B (_raw1). The same scale and offset >>> + as in out_voltageY_raw applies. >>> + >>> +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_symbol >>> +KernelVersion: 5.18 >>> +Contact: linux-iio@vger.kernel.org >>> +Description: >>> + Performs a SW toggle. This attribute is specific to toggle >>> + enabled channels and allows to toggle between out_voltageY_raw0 >>> + and out_voltageY_raw1 through software. Writing 0 will select >>> + out_voltageY_raw0 while 1 selects out_voltageY_raw1. >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index bd8645f6e..9ed00b364 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -12842,6 +12842,7 @@ M: Kim Seer Paller <kimseer.paller@analog.com> >>> L: linux-iio@vger.kernel.org >>> S: Supported >>> W: https://ez.analog.com/linux-software-drivers >>> +F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 >>> F: Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml >>> >>> LTC2688 IIO DAC DRIVER >>> -- >>> 2.34.1 >>> > Clearly I have a lot to learn on this one! Thanks for all of the info. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC 2024-04-13 15:25 ` Jonathan Cameron 2024-04-13 20:38 ` David Lechner @ 2024-04-15 12:45 ` Nuno Sá 2024-04-20 10:22 ` Jonathan Cameron 1 sibling, 1 reply; 24+ messages in thread From: Nuno Sá @ 2024-04-15 12:45 UTC (permalink / raw To: Jonathan Cameron, David Lechner Cc: Kim Seer Paller, linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Michael Hennerich On Sat, 2024-04-13 at 16:25 +0100, Jonathan Cameron wrote: > On Fri, 12 Apr 2024 16:26:17 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > > <kimseer.paller@analog.com> wrote: > > > > > > Define the sysfs interface for toggle capable channels. > > > > > > Toggle enabled channels will have: > > > > > > * out_voltageY_toggle_en > The big missing thing in this ABI is a reference to existing precedence. > You aren't actually defining anything new, it just hasn't yet been generalized > beyond 1 device (unless you include PSK / FSK DDS drivers that are 'still' > after > 13+ years in staging!) > > This patch needs to be generalizing that documentation from the ltc2688. > > Probably in sysfs-bus-iio-dac > > > > > It looks like there are 3 toggle modes. > > > > Two involve the notion of "enabled" outputs that I assume this attribute is > > for: > > > > 1. Toggling all enabled pins at the same time using a software trigger > > (global toggle bit) > > 2. Toggling all enabled pins at the same time using a hardware trigger > > (TGP pin) and toggling pins > > > > This is presumably the tricky one as that hardware toggle may not be in > control of the host CPU. > > > The third mode though looks like it uses the same toggle select > > register for selecting A or B for each channel instead of enabling or > > disabling each channel. > > > > 3. Toggling all pins to A or B based on the toggle select register. No > > notion of enabled pins here. > > > > I haven't looked at the driver implementation, but it sounds like > > out_voltageY_toggle_en and out_voltageY_symbol would be writing to the > > same register in conflicting ways. So maybe we need yet another custom > > attribute to select the currently active toggle mode? > > This one feels like it could be handled as a software optimisation over > just changing the DAC value directly. Things may be slightly different in these devices. But for ltc2688 and AFAIR, the symbol attribute is about toggling between A and B through SW (not really enabling the mode). That interface will only pop up if there's no HW (PWM for example) toggle present. > > > > > In any case, it would be helpful if the documentation below explained > > a bit better the intention and conditions required to use each > > attribute (or add a .rst documentation file for these chips to explain > > how to use it in more detail since this is rather complex feature). > > > > > * out_voltageY_raw0 > > > * out_voltageY_raw1 > > > > I guess there is no enum iio_modifier that fits this. It seems like we > > could still have out_voltageY_raw for register A so that users that > > don't need to do any toggling can use standard ABI. And maybe > > out_voltageY_raw_toggled for register B (question for Jonathan)? > > There is precedence for doing it like this (ltc2688) > Note that we should only see these attribute for changes specifically > configured for 'hardware' triggered toggling. > > Note that we cannot have duplicate documentation so we need to create > a common docs file covering this and existing ltc2688 ABI that overlaps. > That may need some generalising to cover both parts. > > > > > Or just have 8 channels instead of 4 where even channels are register > > A and odd channels are register B? > > > > > * out_voltageY_symbol > > > > "symbol" is a confusing name. It sounds like this just supports > > toggling one channel individually so _toggle_select would make more > > sense to me. Are there plans for supporting toggling multiple channels > > at the same time using a software trigger as well? > > Again, precedence should have been called out. It's not great ABI > but it corresponds to earlier work on Frequency Shift Keying DDS devices > (and I think Phase Shift Keying as well) in which this is call symbol. > Hence the name. > > > > > > > > > The common interface present in all channels is: > > > > > > * out_voltageY_raw (not present in toggle enabled channels) > > > > As mentioned above, I don't think we need to have to make a > > distinction between toggle enabled channels and not enabled channels. > > Was a while back but I think that last time this turned up we concluded > we did need a different interface because the current 'toggle value' > is not in our control. Hence you are programming one channel that > does different things - think of it as setting the Max and Min values > for a generated waveform - perhaps the toggle pin is connected to a PWM > for example. > Yeah and for the ltc2688 we also had the dither mode that has it's own unique set of interfaces. Trying to deal with it all at runtime could be more confusing than beneficial I guess. - Nuno Sá > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC 2024-04-15 12:45 ` Nuno Sá @ 2024-04-20 10:22 ` Jonathan Cameron 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-04-20 10:22 UTC (permalink / raw To: Nuno Sá Cc: David Lechner, Kim Seer Paller, linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Michael Hennerich On Mon, 15 Apr 2024 14:45:52 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Sat, 2024-04-13 at 16:25 +0100, Jonathan Cameron wrote: > > On Fri, 12 Apr 2024 16:26:17 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > > > <kimseer.paller@analog.com> wrote: > > > > > > > > Define the sysfs interface for toggle capable channels. > > > > > > > > Toggle enabled channels will have: > > > > > > > > * out_voltageY_toggle_en > > The big missing thing in this ABI is a reference to existing precedence. > > You aren't actually defining anything new, it just hasn't yet been generalized > > beyond 1 device (unless you include PSK / FSK DDS drivers that are 'still' > > after > > 13+ years in staging!) > > > > This patch needs to be generalizing that documentation from the ltc2688. > > > > Probably in sysfs-bus-iio-dac > > > > > > > > It looks like there are 3 toggle modes. > > > > > > Two involve the notion of "enabled" outputs that I assume this attribute is > > > for: > > > > > > 1. Toggling all enabled pins at the same time using a software trigger > > > (global toggle bit) > > > 2. Toggling all enabled pins at the same time using a hardware trigger > > > (TGP pin) and toggling pins > > > > > > > This is presumably the tricky one as that hardware toggle may not be in > > control of the host CPU. > > > > > The third mode though looks like it uses the same toggle select > > > register for selecting A or B for each channel instead of enabling or > > > disabling each channel. > > > > > > 3. Toggling all pins to A or B based on the toggle select register. No > > > notion of enabled pins here. > > > > > > I haven't looked at the driver implementation, but it sounds like > > > out_voltageY_toggle_en and out_voltageY_symbol would be writing to the > > > same register in conflicting ways. So maybe we need yet another custom > > > attribute to select the currently active toggle mode? > > > > This one feels like it could be handled as a software optimisation over > > just changing the DAC value directly. > > Things may be slightly different in these devices. But for ltc2688 and AFAIR, > the symbol attribute is about toggling between A and B through SW (not really > enabling the mode). That interface will only pop up if there's no HW (PWM for > example) toggle present. I can't remember if we discussed it at the time of that driver, but from a userspace interface point of view, for a single channel there would be little point in this. I guess the key is it simultaneously switches a bunch of channels. Perhaps we can make that clearer in the ABI docs (if it isn't already clear enough!) So a software interface does seem appropriate. There is a fun question of whether the toggle select is useful to software. That is picking which of A or B each output uses for next toggle. At first glance I don't think so, but I'm open to people suggesting why that might need a userspace interface. Superficially feels like anything that can be done with that interface can also be done keeping all channels toggling to A or all to B at one time and potentially a few more register writes. Jonathan ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC 2024-04-12 3:20 [PATCH 0/4] Add driver for LTC2664 and LTC2672 Kim Seer Paller 2024-04-12 3:20 ` [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml Kim Seer Paller 2024-04-12 3:21 ` [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC Kim Seer Paller @ 2024-04-12 3:21 ` Kim Seer Paller 2024-04-13 15:26 ` Jonathan Cameron 2024-04-12 3:21 ` [PATCH 4/4] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672 Kim Seer Paller 3 siblings, 1 reply; 24+ messages in thread From: Kim Seer Paller @ 2024-04-12 3:21 UTC (permalink / raw To: linux-iio, devicetree, linux-kernel Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, David Lechner, Michael Hennerich Define the sysfs interface for toggle capable channels. Toggle enabled channels will have: * out_currentY_toggle_en * out_currentY_raw0 * out_currentY_raw1 * out_currentY_symbol The common interface present in all channels is: * out_currentY_raw (not present in toggle enabled channels) * out_currentY_raw_available * out_currentY_powerdown * out_currentY_scale * out_currentY_offset Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> --- .../ABI/testing/sysfs-bus-iio-dac-ltc2672 | 30 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 31 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 new file mode 100644 index 000000000..b984d92f7 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 @@ -0,0 +1,30 @@ +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_toggle_en +KernelVersion: 5.18 +Contact: linux-iio@vger.kernel.org +Description: + Toggle enable. Write 1 to enable toggle or 0 to disable it. This is + useful when one wants to change the DAC output codes. The way it should + be done is: + + - disable toggle operation; + - change out_currentY_raw0 and out_currentY_raw1; + - enable toggle operation. + +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_raw0 +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_raw1 +KernelVersion: 5.18 +Contact: linux-iio@vger.kernel.org +Description: + It has the same meaning as out_currentY_raw. This attribute is + specific to toggle enabled channels and refers to the DAC output + code in INPUT_A (_raw0) and INPUT_B (_raw1). The same scale and offset + as in out_currentY_raw applies. + +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_symbol +KernelVersion: 5.18 +Contact: linux-iio@vger.kernel.org +Description: + Performs a SW toggle. This attribute is specific to toggle + enabled channels and allows to toggle between out_currentY_raw0 + and out_currentY_raw1 through software. Writing 0 will select + out_currentY_raw0 while 1 selects out_currentY_raw1. diff --git a/MAINTAINERS b/MAINTAINERS index 9ed00b364..fba8bacc0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12843,6 +12843,7 @@ L: linux-iio@vger.kernel.org S: Supported W: https://ez.analog.com/linux-software-drivers F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 +F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 F: Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml LTC2688 IIO DAC DRIVER -- 2.34.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC 2024-04-12 3:21 ` [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC Kim Seer Paller @ 2024-04-13 15:26 ` Jonathan Cameron 2024-04-16 14:18 ` Paller, Kim Seer 0 siblings, 1 reply; 24+ messages in thread From: Jonathan Cameron @ 2024-04-13 15:26 UTC (permalink / raw To: Kim Seer Paller Cc: linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, David Lechner, Michael Hennerich On Fri, 12 Apr 2024 11:21:01 +0800 Kim Seer Paller <kimseer.paller@analog.com> wrote: > Define the sysfs interface for toggle capable channels. > > Toggle enabled channels will have: > > * out_currentY_toggle_en > * out_currentY_raw0 > * out_currentY_raw1 > * out_currentY_symbol > > The common interface present in all channels is: > > * out_currentY_raw (not present in toggle enabled channels) > * out_currentY_raw_available > * out_currentY_powerdown > * out_currentY_scale > * out_currentY_offset > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > --- > .../ABI/testing/sysfs-bus-iio-dac-ltc2672 | 30 +++++++++++++++++++ You can only have per device ABI defined if that is the only user of the ABI. That may actually be true here but given I've asked you to generalize the voltage equivalent, I think we've shown this is general enough that the current version should also be raised to sysfs-bus-iio-dac > MAINTAINERS | 1 + > 2 files changed, 31 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 > new file mode 100644 > index 000000000..b984d92f7 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 > @@ -0,0 +1,30 @@ > +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_toggle_en > +KernelVersion: 5.18 > +Contact: linux-iio@vger.kernel.org > +Description: > + Toggle enable. Write 1 to enable toggle or 0 to disable it. This is > + useful when one wants to change the DAC output codes. The way it should > + be done is: > + > + - disable toggle operation; > + - change out_currentY_raw0 and out_currentY_raw1; > + - enable toggle operation. > + > +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_raw0 > +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_raw1 > +KernelVersion: 5.18 > +Contact: linux-iio@vger.kernel.org > +Description: > + It has the same meaning as out_currentY_raw. This attribute is > + specific to toggle enabled channels and refers to the DAC output > + code in INPUT_A (_raw0) and INPUT_B (_raw1). The same scale and offset > + as in out_currentY_raw applies. > + > +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_symbol > +KernelVersion: 5.18 > +Contact: linux-iio@vger.kernel.org > +Description: > + Performs a SW toggle. This attribute is specific to toggle > + enabled channels and allows to toggle between out_currentY_raw0 > + and out_currentY_raw1 through software. Writing 0 will select > + out_currentY_raw0 while 1 selects out_currentY_raw1. > diff --git a/MAINTAINERS b/MAINTAINERS > index 9ed00b364..fba8bacc0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12843,6 +12843,7 @@ L: linux-iio@vger.kernel.org > S: Supported > W: https://ez.analog.com/linux-software-drivers > F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 > +F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 > F: Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > LTC2688 IIO DAC DRIVER ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC 2024-04-13 15:26 ` Jonathan Cameron @ 2024-04-16 14:18 ` Paller, Kim Seer 2024-04-20 10:23 ` Jonathan Cameron 0 siblings, 1 reply; 24+ messages in thread From: Paller, Kim Seer @ 2024-04-16 14:18 UTC (permalink / raw To: Jonathan Cameron, David Lechner Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Hennerich, Michael > -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Saturday, April 13, 2024 11:27 PM > To: Paller, Kim Seer <KimSeer.Paller@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 <krzysztof.kozlowski+dt@linaro.org>; > Conor Dooley <conor+dt@kernel.org>; Liam Girdwood > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; David Lechner > <dlechner@baylibre.com>; Hennerich, Michael > <Michael.Hennerich@analog.com> > Subject: Re: [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC > > [External] > > On Fri, 12 Apr 2024 11:21:01 +0800 > Kim Seer Paller <kimseer.paller@analog.com> wrote: > > > Define the sysfs interface for toggle capable channels. > > > > Toggle enabled channels will have: > > > > * out_currentY_toggle_en > > * out_currentY_raw0 > > * out_currentY_raw1 > > * out_currentY_symbol > > > > The common interface present in all channels is: > > > > * out_currentY_raw (not present in toggle enabled channels) > > * out_currentY_raw_available > > * out_currentY_powerdown > > * out_currentY_scale > > * out_currentY_offset > > > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > > --- > > .../ABI/testing/sysfs-bus-iio-dac-ltc2672 | 30 +++++++++++++++++++ > > You can only have per device ABI defined if that is the only user > of the ABI. That may actually be true here but given I've asked you to > generalize > the voltage equivalent, I think we've shown this is general enough that the > current > version should also be raised to sysfs-bus-iio-dac I'm still getting familiar with ABI documentation. If I understand correctly, generalizing the documentation to cover both parts would also mean we remove the overlapping sections from the ltc2688 ABI. Is that the correct approach? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC 2024-04-16 14:18 ` Paller, Kim Seer @ 2024-04-20 10:23 ` Jonathan Cameron 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-04-20 10:23 UTC (permalink / raw To: Paller, Kim Seer Cc: David Lechner, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, Hennerich, Michael On Tue, 16 Apr 2024 14:18:23 +0000 "Paller, Kim Seer" <KimSeer.Paller@analog.com> wrote: > > -----Original Message----- > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Saturday, April 13, 2024 11:27 PM > > To: Paller, Kim Seer <KimSeer.Paller@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 <krzysztof.kozlowski+dt@linaro.org>; > > Conor Dooley <conor+dt@kernel.org>; Liam Girdwood > > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; David Lechner > > <dlechner@baylibre.com>; Hennerich, Michael > > <Michael.Hennerich@analog.com> > > Subject: Re: [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC > > > > [External] > > > > On Fri, 12 Apr 2024 11:21:01 +0800 > > Kim Seer Paller <kimseer.paller@analog.com> wrote: > > > > > Define the sysfs interface for toggle capable channels. > > > > > > Toggle enabled channels will have: > > > > > > * out_currentY_toggle_en > > > * out_currentY_raw0 > > > * out_currentY_raw1 > > > * out_currentY_symbol > > > > > > The common interface present in all channels is: > > > > > > * out_currentY_raw (not present in toggle enabled channels) > > > * out_currentY_raw_available > > > * out_currentY_powerdown > > > * out_currentY_scale > > > * out_currentY_offset > > > > > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > > > --- > > > .../ABI/testing/sysfs-bus-iio-dac-ltc2672 | 30 +++++++++++++++++++ > > > > You can only have per device ABI defined if that is the only user > > of the ABI. That may actually be true here but given I've asked you to > > generalize > > the voltage equivalent, I think we've shown this is general enough that the > > current > > version should also be raised to sysfs-bus-iio-dac > > I'm still getting familiar with ABI documentation. If I understand correctly, > generalizing the documentation to cover both parts would also mean we remove > the overlapping sections from the ltc2688 ABI. Is that the correct approach? > > Yes. To test this build the html docs. IIRC that will complain about duplicate ABI definitions. I'm sure there is a way to test just ABI docs build but I've never really looked into it. Jonathan ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672 2024-04-12 3:20 [PATCH 0/4] Add driver for LTC2664 and LTC2672 Kim Seer Paller ` (2 preceding siblings ...) 2024-04-12 3:21 ` [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC Kim Seer Paller @ 2024-04-12 3:21 ` Kim Seer Paller 2024-04-13 15:55 ` Jonathan Cameron 3 siblings, 1 reply; 24+ messages in thread From: Kim Seer Paller @ 2024-04-12 3:21 UTC (permalink / raw To: linux-iio, devicetree, linux-kernel Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, David Lechner, Michael Hennerich LTC2664 4 channel, 16 bit Voltage Output SoftSpan DAC LTC2672 5 channel, 16 bit Current Output Softspan DAC Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> --- MAINTAINERS | 1 + drivers/iio/dac/Kconfig | 11 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/ltc2664.c | 785 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 798 insertions(+) create mode 100644 drivers/iio/dac/ltc2664.c diff --git a/MAINTAINERS b/MAINTAINERS index fba8bacc0..9b5c3d6d1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12845,6 +12845,7 @@ W: https://ez.analog.com/linux-software-drivers F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 F: Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml +F: drivers/iio/dac/ltc2664.c LTC2688 IIO DAC DRIVER M: Nuno Sá <nuno.sa@analog.com> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig index 34eb40bb9..79b7a547e 100644 --- a/drivers/iio/dac/Kconfig +++ b/drivers/iio/dac/Kconfig @@ -333,6 +333,17 @@ config LTC2632 To compile this driver as a module, choose M here: the module will be called ltc2632. +config LTC2664 + tristate "Analog Devices LTC2664 and LTC2672 DAC SPI driver" + depends on SPI + select REGMAP + help + Say yes here to build support for Analog Devices + LTC2664 and LTC2672 converters (DAC). + + To compile this driver as a module, choose M here: the + module will be called ltc2664. + config M62332 tristate "Mitsubishi M62332 DAC driver" depends on I2C diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile index 55bf89739..62df8d7e4 100644 --- a/drivers/iio/dac/Makefile +++ b/drivers/iio/dac/Makefile @@ -35,6 +35,7 @@ obj-$(CONFIG_DS4424) += ds4424.o obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o obj-$(CONFIG_LTC1660) += ltc1660.o obj-$(CONFIG_LTC2632) += ltc2632.o +obj-$(CONFIG_LTC2664) += ltc2664.o obj-$(CONFIG_LTC2688) += ltc2688.o obj-$(CONFIG_M62332) += m62332.o obj-$(CONFIG_MAX517) += max517.o diff --git a/drivers/iio/dac/ltc2664.c b/drivers/iio/dac/ltc2664.c new file mode 100644 index 000000000..70c43fe17 --- /dev/null +++ b/drivers/iio/dac/ltc2664.c @@ -0,0 +1,785 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * LTC2664 4 channel, 16 bit Voltage Output SoftSpan DAC driver + * LTC2672 5 channel, 16 bit Current Output Softspan DAC driver + * + * Copyright 2024 Analog Devices Inc. + */ + +#include <linux/bitfield.h> +#include <linux/cleanup.h> +#include <linux/device.h> +#include <linux/gpio/consumer.h> +#include <linux/iio/iio.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/mutex.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/spi/spi.h> + +#define LTC2664_CMD_WRITE_N(n) (0x00 + (n)) +#define LTC2664_CMD_UPDATE_N(n) (0x10 + (n)) +#define LTC2664_CMD_WRITE_N_UPDATE_ALL 0x20 +#define LTC2664_CMD_WRITE_N_UPDATE_N(n) (0x30 + (n)) +#define LTC2664_CMD_POWER_DOWN_N(n) (0x40 + (n)) +#define LTC2664_CMD_POWER_DOWN_ALL 0x50 +#define LTC2664_CMD_SPAN_N(n) (0x60 + (n)) +#define LTC2664_CMD_CONFIG 0x70 +#define LTC2664_CMD_MUX 0xB0 +#define LTC2664_CMD_TOGGLE_SEL 0xC0 +#define LTC2664_CMD_GLOBAL_TOGGLE 0xD0 +#define LTC2664_CMD_NO_OPERATION 0xF0 +#define LTC2664_REF_DISABLE 0x0001 +#define LTC2664_MSPAN_SOFTSPAN 7 + +#define LTC2672_MAX_CHANNEL 5 +#define LTC2672_MAX_SPAN 7 +#define LTC2672_OFFSET_CODE 384 +#define LTC2672_SCALE_MULTIPLIER(n) (50 * BIT(n)) + +enum ltc2664_ids { + LTC2664, + LTC2672, +}; + +enum { + LTC2664_SPAN_RANGE_0V_5V, + LTC2664_SPAN_RANGE_0V_10V, + LTC2664_SPAN_RANGE_M5V_5V, + LTC2664_SPAN_RANGE_M10V_10V, + LTC2664_SPAN_RANGE_M2V5_2V5, +}; + +enum { + LTC2664_INPUT_A, + LTC2664_INPUT_B, + LTC2664_INPUT_B_AVAIL, + LTC2664_POWERDOWN, + LTC2664_TOGGLE_EN, + LTC2664_GLOBAL_TOGGLE, +}; + +static const u16 ltc2664_mspan_lut[8][2] = { + {LTC2664_SPAN_RANGE_M10V_10V, 32768}, /* MPS2=0, MPS1=0, MSP0=0 (0)*/ + {LTC2664_SPAN_RANGE_M5V_5V, 32768}, /* MPS2=0, MPS1=0, MSP0=1 (1)*/ + {LTC2664_SPAN_RANGE_M2V5_2V5, 32768}, /* MPS2=0, MPS1=1, MSP0=0 (2)*/ + {LTC2664_SPAN_RANGE_0V_10V, 0}, /* MPS2=0, MPS1=1, MSP0=1 (3)*/ + {LTC2664_SPAN_RANGE_0V_10V, 32768}, /* MPS2=1, MPS1=0, MSP0=0 (4)*/ + {LTC2664_SPAN_RANGE_0V_5V, 0}, /* MPS2=1, MPS1=0, MSP0=1 (5)*/ + {LTC2664_SPAN_RANGE_0V_5V, 32768}, /* MPS2=1, MPS1=1, MSP0=0 (6)*/ + {LTC2664_SPAN_RANGE_0V_5V, 0} /* MPS2=1, MPS1=1, MSP0=1 (7)*/ +}; + +struct ltc2664_chip_info { + enum ltc2664_ids id; + const char *name; + unsigned int num_channels; + const struct iio_chan_spec *iio_chan; + const int (*span_helper)[2]; + unsigned int num_span; +}; + +struct ltc2664_chan { + bool toggle_chan; + bool powerdown; + u8 span; + u16 raw[2]; /* A/B */ +}; + +struct ltc2664_state { + struct spi_device *spi; + struct regmap *regmap; + struct ltc2664_chan channels[LTC2672_MAX_CHANNEL]; + /* lock to protect against multiple access to the device and shared data */ + struct mutex lock; + const struct ltc2664_chip_info *chip_info; + struct iio_chan_spec *iio_channels; + int vref; + u32 toggle_sel; + u32 global_toggle; + u32 rfsadj; +}; + +static const int ltc2664_span_helper[][2] = { + {0, 5000}, {0, 10000}, {-5000, 5000}, {-10000, 10000}, {-2500, 2500}, +}; + +static const int ltc2672_span_helper[][2] = { + {0, 3125}, {0, 6250}, {0, 12500}, {0, 25000}, {0, 50000}, {0, 100000}, + {0, 200000}, {0, 300000}, +}; + +static int ltc2664_scale_get(const struct ltc2664_state *st, int c, int *val) +{ + const struct ltc2664_chan *chan = &st->channels[c]; + const int (*span_helper)[2] = st->chip_info->span_helper; + int span, fs; + + span = chan->span; + if (span < 0) + return span; + + switch (st->chip_info->id) { + case LTC2664: + fs = span_helper[span][1] - span_helper[span][0]; + + if (st->vref) + *val = (fs / 2500) * st->vref; + else + *val = fs; + + return 0; + case LTC2672: + if (span == LTC2672_MAX_SPAN) + *val = 4800 * (1000 * st->vref / st->rfsadj); + else + *val = LTC2672_SCALE_MULTIPLIER(span) * + (1000 * st->vref / st->rfsadj); + + return 0; + default: + return -EINVAL; + } +} + +static int ltc2664_offset_get(const struct ltc2664_state *st, int c, int *val) +{ + const struct ltc2664_chan *chan = &st->channels[c]; + int span; + + span = chan->span; + if (span < 0) + return span; + + if (st->chip_info->span_helper[span][0] < 0) + *val = -32768; + else if (chan->raw[0] >= LTC2672_OFFSET_CODE || + chan->raw[1] >= LTC2672_OFFSET_CODE) + *val = st->chip_info->span_helper[1][span] / 250; + else + *val = 0; + + return 0; +} + +static int ltc2664_dac_code_write(struct ltc2664_state *st, u32 chan, u32 input, + u16 code) +{ + struct ltc2664_chan *c = &st->channels[chan]; + int ret, reg; + + guard(mutex)(&st->lock); + /* select the correct input register to write to */ + if (c->toggle_chan) { + ret = regmap_write(st->regmap, LTC2664_CMD_TOGGLE_SEL, + input << chan); + if (ret) + return ret; + } + /* + * If in toggle mode the dac should be updated by an + * external signal (or sw toggle) and not here. + */ + if (st->toggle_sel & BIT(chan)) + reg = LTC2664_CMD_WRITE_N(chan); + else + reg = LTC2664_CMD_WRITE_N_UPDATE_N(chan); + + ret = regmap_write(st->regmap, reg, code); + if (ret) + return ret; + + c->raw[input] = code; + + if (c->toggle_chan) { + ret = regmap_write(st->regmap, LTC2664_CMD_TOGGLE_SEL, + st->toggle_sel); + if (ret) + return ret; + } + + return ret; +} + +static int ltc2664_dac_code_read(struct ltc2664_state *st, u32 chan, u32 input, + u32 *code) +{ + guard(mutex)(&st->lock); + *code = st->channels[chan].raw[input]; + + return 0; +} + +static const int ltc2664_raw_range[] = {0, 1, U16_MAX}; + +static int ltc2664_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int **vals, int *type, int *length, + long info) +{ + switch (info) { + case IIO_CHAN_INFO_RAW: + *vals = ltc2664_raw_range; + *type = IIO_VAL_INT; + + return IIO_AVAIL_RANGE; + default: + return -EINVAL; + } +} + +static int ltc2664_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long info) +{ + struct ltc2664_state *st = iio_priv(indio_dev); + int ret; + + switch (info) { + case IIO_CHAN_INFO_RAW: + ret = ltc2664_dac_code_read(st, chan->channel, + LTC2664_INPUT_A, val); + if (ret) + return ret; + + return IIO_VAL_INT; + case IIO_CHAN_INFO_OFFSET: + ret = ltc2664_offset_get(st, chan->channel, val); + if (ret) + return ret; + + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + ret = ltc2664_scale_get(st, chan->channel, val); + if (ret) + return ret; + + *val2 = 16; + + return IIO_VAL_FRACTIONAL_LOG2; + default: + return -EINVAL; + } +} + +static int ltc2664_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, + int val2, long info) +{ + struct ltc2664_state *st = iio_priv(indio_dev); + + switch (info) { + case IIO_CHAN_INFO_RAW: + if (val > U16_MAX || val < 0) + return -EINVAL; + + return ltc2664_dac_code_write(st, chan->channel, + LTC2664_INPUT_A, val); + default: + return -EINVAL; + } +} + +static ssize_t ltc2664_reg_bool_get(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + char *buf) +{ + struct ltc2664_state *st = iio_priv(indio_dev); + u32 val; + + guard(mutex)(&st->lock); + switch (private) { + case LTC2664_POWERDOWN: + val = st->channels[chan->channel].powerdown; + + return sysfs_emit(buf, "%u\n", val); + case LTC2664_TOGGLE_EN: + val = !!(st->toggle_sel & BIT(chan->channel)); + + return sysfs_emit(buf, "%u\n", val); + case LTC2664_GLOBAL_TOGGLE: + val = st->global_toggle; + + return sysfs_emit(buf, "%u\n", val); + default: + return -EINVAL; + } +} + +static ssize_t ltc2664_reg_bool_set(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + const char *buf, size_t len) +{ + struct ltc2664_state *st = iio_priv(indio_dev); + int ret; + bool en; + + ret = kstrtobool(buf, &en); + if (ret) + return ret; + + guard(mutex)(&st->lock); + switch (private) { + case LTC2664_POWERDOWN: + ret = regmap_write(st->regmap, + en ? LTC2664_CMD_POWER_DOWN_N(chan->channel) : + LTC2664_CMD_UPDATE_N(chan->channel), en); + if (ret) + return ret; + + st->channels[chan->channel].powerdown = en; + + return len; + case LTC2664_TOGGLE_EN: + if (en) + st->toggle_sel |= BIT(chan->channel); + else + st->toggle_sel &= ~BIT(chan->channel); + + ret = regmap_write(st->regmap, LTC2664_CMD_TOGGLE_SEL, + st->toggle_sel); + if (ret) + return ret; + + return len; + case LTC2664_GLOBAL_TOGGLE: + ret = regmap_write(st->regmap, LTC2664_CMD_GLOBAL_TOGGLE, en); + if (ret) + return ret; + + st->global_toggle = en; + + return len; + default: + return -EINVAL; + } +} + +static ssize_t ltc2664_dac_input_read(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + char *buf) +{ + struct ltc2664_state *st = iio_priv(indio_dev); + int ret; + u32 val; + + if (private == LTC2664_INPUT_B_AVAIL) + return sysfs_emit(buf, "[%u %u %u]\n", ltc2664_raw_range[0], + ltc2664_raw_range[1], + ltc2664_raw_range[2] / 4); + + ret = ltc2664_dac_code_read(st, chan->channel, private, &val); + if (ret) + return ret; + + return sysfs_emit(buf, "%u\n", val); +} + +static ssize_t ltc2664_dac_input_write(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + const char *buf, size_t len) +{ + struct ltc2664_state *st = iio_priv(indio_dev); + int ret; + u16 val; + + if (private == LTC2664_INPUT_B_AVAIL) + return -EINVAL; + + ret = kstrtou16(buf, 10, &val); + if (ret) + return ret; + + ret = ltc2664_dac_code_write(st, chan->channel, private, val); + if (ret) + return ret; + + return len; +} + +static int ltc2664_reg_access(struct iio_dev *indio_dev, + unsigned int reg, + unsigned int writeval, + unsigned int *readval) +{ + struct ltc2664_state *st = iio_priv(indio_dev); + + if (readval) + return -EOPNOTSUPP; + + return regmap_write(st->regmap, reg, writeval); +} + +#define LTC2664_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) { \ + .name = _name, \ + .read = (_read), \ + .write = (_write), \ + .private = (_what), \ + .shared = (_shared), \ +} + +/* + * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is + * not provided in dts. + */ +static const struct iio_chan_spec_ext_info ltc2664_toggle_sym_ext_info[] = { + LTC2664_CHAN_EXT_INFO("raw0", LTC2664_INPUT_A, IIO_SEPARATE, + ltc2664_dac_input_read, ltc2664_dac_input_write), + LTC2664_CHAN_EXT_INFO("raw1", LTC2664_INPUT_B, IIO_SEPARATE, + ltc2664_dac_input_read, ltc2664_dac_input_write), + LTC2664_CHAN_EXT_INFO("powerdown", LTC2664_POWERDOWN, IIO_SEPARATE, + ltc2664_reg_bool_get, ltc2664_reg_bool_set), + LTC2664_CHAN_EXT_INFO("symbol", LTC2664_GLOBAL_TOGGLE, IIO_SEPARATE, + ltc2664_reg_bool_get, ltc2664_reg_bool_set), + LTC2664_CHAN_EXT_INFO("toggle_en", LTC2664_TOGGLE_EN, + IIO_SEPARATE, ltc2664_reg_bool_get, + ltc2664_reg_bool_set), + {} +}; + +static const struct iio_chan_spec_ext_info ltc2664_ext_info[] = { + LTC2664_CHAN_EXT_INFO("powerdown", LTC2664_POWERDOWN, IIO_SEPARATE, + ltc2664_reg_bool_get, ltc2664_reg_bool_set), + {} +}; + +#define LTC2664_CHANNEL(_chan) { \ + .indexed = 1, \ + .output = 1, \ + .channel = (_chan), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET) | BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW), \ + .ext_info = ltc2664_ext_info, \ +} + +static const struct iio_chan_spec ltc2664_channels[] = { + LTC2664_CHANNEL(0), + LTC2664_CHANNEL(1), + LTC2664_CHANNEL(2), + LTC2664_CHANNEL(3), +}; + +static const struct iio_chan_spec ltc2672_channels[] = { + LTC2664_CHANNEL(0), + LTC2664_CHANNEL(1), + LTC2664_CHANNEL(2), + LTC2664_CHANNEL(3), + LTC2664_CHANNEL(4), +}; + +static const struct ltc2664_chip_info ltc2664_chip = { + .id = LTC2664, + .name = "ltc2664", + .num_channels = ARRAY_SIZE(ltc2664_channels), + .iio_chan = ltc2664_channels, + .span_helper = ltc2664_span_helper, + .num_span = ARRAY_SIZE(ltc2664_span_helper), +}; + +static const struct ltc2664_chip_info ltc2672_chip = { + .id = LTC2672, + .name = "ltc2672", + .num_channels = ARRAY_SIZE(ltc2672_channels), + .iio_chan = ltc2672_channels, + .span_helper = ltc2672_span_helper, + .num_span = ARRAY_SIZE(ltc2672_span_helper), +}; + +static int ltc2664_span_lookup(const struct ltc2664_state *st, int min, int max) +{ + const int (*span_helper)[2] = st->chip_info->span_helper; + int span; + + for (span = 0; span < st->chip_info->num_span; span++) { + if (min == span_helper[span][0] && max == span_helper[span][1]) + return span; + } + + return -EINVAL; +} + +static int ltc2664_channel_config(struct ltc2664_state *st) +{ + const struct ltc2664_chip_info *chip_info = st->chip_info; + struct device *dev = &st->spi->dev; + u32 reg, tmp[2], mspan; + int ret, span; + + mspan = LTC2664_MSPAN_SOFTSPAN; + ret = device_property_read_u32(dev, "adi,manual-span-operation-config", + &mspan); + if (!ret) { + if (chip_info->id != LTC2664) + return dev_err_probe(dev, -EINVAL, + "adi,manual-span-operation-config not supported\n"); + + if (mspan > ARRAY_SIZE(ltc2664_mspan_lut)) + return dev_err_probe(dev, -EINVAL, + "adi,manual-span-operation-config not in range\n"); + } + + st->rfsadj = 20000; + ret = device_property_read_u32(dev, "adi,rfsadj-ohms", &st->rfsadj); + if (!ret) { + if (chip_info->id != LTC2672) + return dev_err_probe(dev, -EINVAL, + "adi,rfsadj-ohms not supported\n"); + + if ((st->rfsadj < 19000 || st->rfsadj > 41000) || + chip_info->id != LTC2672) + return dev_err_probe(dev, -EINVAL, + "adi,rfsadj-ohms not in range\n"); + } + + device_for_each_child_node_scoped(dev, child) { + struct ltc2664_chan *chan; + + ret = fwnode_property_read_u32(child, "reg", ®); + if (ret) + return dev_err_probe(dev, ret, + "Failed to get reg property\n"); + + if (reg >= chip_info->num_channels) + return dev_err_probe(dev, -EINVAL, + "reg bigger than: %d\n", + chip_info->num_channels); + + chan = &st->channels[reg]; + + if (fwnode_property_read_bool(child, "adi,toggle-mode")) { + chan->toggle_chan = true; + /* assume sw toggle ABI */ + st->iio_channels[reg].ext_info = ltc2664_toggle_sym_ext_info; + /* + * Clear IIO_CHAN_INFO_RAW bit as toggle channels expose + * out_voltage/current_raw{0|1} files. + */ + __clear_bit(IIO_CHAN_INFO_RAW, + &st->iio_channels[reg].info_mask_separate); + } + + chan->raw[0] = ltc2664_mspan_lut[mspan][1]; + chan->raw[1] = ltc2664_mspan_lut[mspan][1]; + + chan->span = ltc2664_mspan_lut[mspan][0]; + + ret = fwnode_property_read_u32_array(child, "adi,output-range-microvolt", + tmp, ARRAY_SIZE(tmp)); + if (!ret && mspan == LTC2664_MSPAN_SOFTSPAN) { + /* voltage type measurement */ + st->iio_channels[reg].type = IIO_VOLTAGE; + + span = ltc2664_span_lookup(st, (int)tmp[0] / 1000, + tmp[1] / 1000); + if (span < 0) + return dev_err_probe(dev, -EINVAL, + "output range not valid"); + + ret = regmap_write(st->regmap, + LTC2664_CMD_SPAN_N(reg), + span); + if (ret) + return dev_err_probe(dev, -EINVAL, + "failed to set chan settings\n"); + + chan->span = span; + } + + ret = fwnode_property_read_u32(child, + "adi,output-range-microamp", + &tmp[0]); + if (!ret) { + /* current type measurement */ + st->iio_channels[reg].type = IIO_CURRENT; + + span = ltc2664_span_lookup(st, 0, + (int)tmp[0] / 1000); + if (span < 0) + return dev_err_probe(dev, -EINVAL, + "output range not valid"); + + ret = regmap_write(st->regmap, + LTC2664_CMD_SPAN_N(reg), + span + 1); + if (ret) + return dev_err_probe(dev, -EINVAL, + "failed to set chan settings\n"); + + chan->span = span; + } + } + + return 0; +} + +static int ltc2664_setup(struct ltc2664_state *st, struct regulator *vref) +{ + const struct ltc2664_chip_info *chip_info = st->chip_info; + struct gpio_desc *gpio; + int ret; + + /* + * If we have a clr/reset pin, use that to reset the chip. + */ + gpio = devm_gpiod_get_optional(&st->spi->dev, "clr", GPIOD_OUT_HIGH); + if (IS_ERR(gpio)) + return dev_err_probe(&st->spi->dev, PTR_ERR(gpio), + "Failed to get reset gpio"); + if (gpio) { + usleep_range(1000, 1200); + /* bring device out of reset */ + gpiod_set_value_cansleep(gpio, 0); + } + + /* + * Duplicate the default channel configuration as it can change during + * @ltc2664_channel_config() + */ + st->iio_channels = devm_kmemdup(&st->spi->dev, chip_info->iio_chan, + chip_info->num_channels * + sizeof(*chip_info->iio_chan), + GFP_KERNEL); + + ret = ltc2664_channel_config(st); + if (ret) + return ret; + + if (!vref) + return 0; + + return regmap_set_bits(st->regmap, LTC2664_CMD_CONFIG, LTC2664_REF_DISABLE); +} + +static void ltc2664_disable_regulator(void *regulator) +{ + regulator_disable(regulator); +} + +static const struct regmap_config ltc2664_regmap_config = { + .reg_bits = 8, + .val_bits = 16, + .max_register = LTC2664_CMD_NO_OPERATION, +}; + +static const struct iio_info ltc2664_info = { + .write_raw = ltc2664_write_raw, + .read_raw = ltc2664_read_raw, + .read_avail = ltc2664_read_avail, + .debugfs_reg_access = ltc2664_reg_access, +}; + +static int ltc2664_probe(struct spi_device *spi) +{ + static const char * const regulators[] = { "vcc", "iovcc" }; + const struct ltc2664_chip_info *chip_info; + struct device *dev = &spi->dev; + struct regulator *vref_reg; + struct iio_dev *indio_dev; + struct ltc2664_state *st; + int ret; + + indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); + if (!indio_dev) + return -ENOMEM; + + st = iio_priv(indio_dev); + st->spi = spi; + + chip_info = spi_get_device_match_data(spi); + if (!chip_info) + return -ENOMEM; + + st->chip_info = chip_info; + + mutex_init(&st->lock); + + st->regmap = devm_regmap_init_spi(spi, <c2664_regmap_config); + if (IS_ERR(st->regmap)) + return dev_err_probe(dev, PTR_ERR(st->regmap), + "Failed to init regmap"); + + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators), + regulators); + if (ret) + return dev_err_probe(dev, ret, "Failed to enable regulators\n"); + + vref_reg = devm_regulator_get_optional(dev, "vref"); + if (IS_ERR(vref_reg)) { + if (PTR_ERR(vref_reg) != -ENODEV) + return dev_err_probe(dev, PTR_ERR(vref_reg), + "Failed to get vref regulator"); + + vref_reg = NULL; + + /* internal reference */ + if (chip_info->id == LTC2664) + st->vref = 2500; + else + st->vref = 1250; + } else { + ret = regulator_enable(vref_reg); + if (ret) + return dev_err_probe(dev, ret, + "Failed to enable vref regulators\n"); + + ret = devm_add_action_or_reset(dev, ltc2664_disable_regulator, + vref_reg); + if (ret) + return ret; + + ret = regulator_get_voltage(vref_reg); + if (ret < 0) + return dev_err_probe(dev, ret, "Failed to get vref\n"); + + st->vref = ret / 1000; + } + + ret = ltc2664_setup(st, vref_reg); + if (ret) + return ret; + + indio_dev->name = chip_info->name; + indio_dev->info = <c2664_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = st->iio_channels; + indio_dev->num_channels = chip_info->num_channels; + + return devm_iio_device_register(dev, indio_dev); +} + +static const struct spi_device_id ltc2664_id[] = { + { "ltc2664", (kernel_ulong_t)<c2664_chip }, + { "ltc2672", (kernel_ulong_t)<c2672_chip }, + { }, +}; +MODULE_DEVICE_TABLE(spi, ltc2664_id); + +static const struct of_device_id ltc2664_of_id[] = { + { .compatible = "adi,ltc2664", .data = <c2664_chip }, + { .compatible = "adi,ltc2672", .data = <c2672_chip }, + { }, +}; +MODULE_DEVICE_TABLE(of, ltc2664_of_id); + +static struct spi_driver ltc2664_driver = { + .driver = { + .name = "ltc2664", + .of_match_table = ltc2664_of_id, + }, + .probe = ltc2664_probe, + .id_table = ltc2664_id, +}; +module_spi_driver(ltc2664_driver); + +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); +MODULE_AUTHOR("Kim Seer Paller <kimseer.paller@analog.com>"); +MODULE_DESCRIPTION("Analog Devices LTC2664 and LTC2672 DAC"); +MODULE_LICENSE("GPL"); -- 2.34.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672 2024-04-12 3:21 ` [PATCH 4/4] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672 Kim Seer Paller @ 2024-04-13 15:55 ` Jonathan Cameron 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-04-13 15:55 UTC (permalink / raw To: Kim Seer Paller Cc: linux-iio, devicetree, linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, David Lechner, Michael Hennerich On Fri, 12 Apr 2024 11:21:02 +0800 Kim Seer Paller <kimseer.paller@analog.com> wrote: > LTC2664 4 channel, 16 bit Voltage Output SoftSpan DAC > LTC2672 5 channel, 16 bit Current Output Softspan DAC > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> Hi Most of the questions are in the earlier patch reviews in this series, so just minor stuff in here. Jonathan > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > index 55bf89739..62df8d7e4 100644 > --- a/drivers/iio/dac/Makefile > +++ b/drivers/iio/dac/Makefile > @@ -35,6 +35,7 @@ obj-$(CONFIG_DS4424) += ds4424.o > obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o > obj-$(CONFIG_LTC1660) += ltc1660.o > obj-$(CONFIG_LTC2632) += ltc2632.o > +obj-$(CONFIG_LTC2664) += ltc2664.o > obj-$(CONFIG_LTC2688) += ltc2688.o > obj-$(CONFIG_M62332) += m62332.o > obj-$(CONFIG_MAX517) += max517.o > diff --git a/drivers/iio/dac/ltc2664.c b/drivers/iio/dac/ltc2664.c > new file mode 100644 > index 000000000..70c43fe17 > --- /dev/null > +++ b/drivers/iio/dac/ltc2664.c > @@ -0,0 +1,785 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * LTC2664 4 channel, 16 bit Voltage Output SoftSpan DAC driver > + * LTC2672 5 channel, 16 bit Current Output Softspan DAC driver > + * > + * Copyright 2024 Analog Devices Inc. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/cleanup.h> > +#include <linux/device.h> > +#include <linux/gpio/consumer.h> > +#include <linux/iio/iio.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/mutex.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/spi/spi.h> > + > +#define LTC2664_CMD_WRITE_N(n) (0x00 + (n)) > +#define LTC2664_CMD_UPDATE_N(n) (0x10 + (n)) > +#define LTC2664_CMD_WRITE_N_UPDATE_ALL 0x20 > +#define LTC2664_CMD_WRITE_N_UPDATE_N(n) (0x30 + (n)) > +#define LTC2664_CMD_POWER_DOWN_N(n) (0x40 + (n)) > +#define LTC2664_CMD_POWER_DOWN_ALL 0x50 > +#define LTC2664_CMD_SPAN_N(n) (0x60 + (n)) > +#define LTC2664_CMD_CONFIG 0x70 > +#define LTC2664_CMD_MUX 0xB0 > +#define LTC2664_CMD_TOGGLE_SEL 0xC0 > +#define LTC2664_CMD_GLOBAL_TOGGLE 0xD0 > +#define LTC2664_CMD_NO_OPERATION 0xF0 > +#define LTC2664_REF_DISABLE 0x0001 > +#define LTC2664_MSPAN_SOFTSPAN 7 > + > +#define LTC2672_MAX_CHANNEL 5 > +#define LTC2672_MAX_SPAN 7 > +#define LTC2672_OFFSET_CODE 384 Comment for OFFSET_CODE would be good. I'm not sure what it is or where the value comes from. > +#define LTC2672_SCALE_MULTIPLIER(n) (50 * BIT(n)) > + > +enum ltc2664_ids { > + LTC2664, > + LTC2672, I've mentioned it inline, but this is often a bad sign as it means that some control logic is used in place of data in a chip_info structure. Better to avoid tying what happens directly to IDs, but instead have the chip_info structure cover the particular set of things that a device does. That ends up a lot easier to extend when a driver gains additional part support. > +}; > + > +static const u16 ltc2664_mspan_lut[8][2] = { > + {LTC2664_SPAN_RANGE_M10V_10V, 32768}, /* MPS2=0, MPS1=0, MSP0=0 (0)*/ > + {LTC2664_SPAN_RANGE_M5V_5V, 32768}, /* MPS2=0, MPS1=0, MSP0=1 (1)*/ > + {LTC2664_SPAN_RANGE_M2V5_2V5, 32768}, /* MPS2=0, MPS1=1, MSP0=0 (2)*/ > + {LTC2664_SPAN_RANGE_0V_10V, 0}, /* MPS2=0, MPS1=1, MSP0=1 (3)*/ > + {LTC2664_SPAN_RANGE_0V_10V, 32768}, /* MPS2=1, MPS1=0, MSP0=0 (4)*/ > + {LTC2664_SPAN_RANGE_0V_5V, 0}, /* MPS2=1, MPS1=0, MSP0=1 (5)*/ > + {LTC2664_SPAN_RANGE_0V_5V, 32768}, /* MPS2=1, MPS1=1, MSP0=0 (6)*/ > + {LTC2664_SPAN_RANGE_0V_5V, 0} /* MPS2=1, MPS1=1, MSP0=1 (7)*/ As below, spaces after { and before } > +}; > + > +struct ltc2664_chip_info { > + enum ltc2664_ids id; > + const char *name; > + unsigned int num_channels; > + const struct iio_chan_spec *iio_chan; > + const int (*span_helper)[2]; > + unsigned int num_span; > +}; > + > +static const int ltc2664_span_helper[][2] = { > + {0, 5000}, {0, 10000}, {-5000, 5000}, {-10000, 10000}, {-2500, 2500}, > +}; > + > +static const int ltc2672_span_helper[][2] = { > + {0, 3125}, {0, 6250}, {0, 12500}, {0, 25000}, {0, 50000}, {0, 100000}, > + {0, 200000}, {0, 300000}, Spaces preferred and better on separate lines. { 0, 3125 }, { 0, 6250 }, etc as more readable. > +}; > + > +static int ltc2664_scale_get(const struct ltc2664_state *st, int c, int *val) > +{ > + const struct ltc2664_chan *chan = &st->channels[c]; > + const int (*span_helper)[2] = st->chip_info->span_helper; > + int span, fs; > + > + span = chan->span; > + if (span < 0) > + return span; > + > + switch (st->chip_info->id) { Make this 'data' rather than using an ID. There shouldn't be an ID in chip_info because it almost always means that a coding pattern has been used that isn't as extensible as either an appropriate callback or appropriate data. > + case LTC2664: > + fs = span_helper[span][1] - span_helper[span][0]; > + > + if (st->vref) How do we get here with st->vref == 0 ? > + *val = (fs / 2500) * st->vref; > + else > + *val = fs; > + > + return 0; > + case LTC2672: > + if (span == LTC2672_MAX_SPAN) > + *val = 4800 * (1000 * st->vref / st->rfsadj); > + else > + *val = LTC2672_SCALE_MULTIPLIER(span) * > + (1000 * st->vref / st->rfsadj); Seem to divide by same thing so add a local variable for that and use it in both paths. > + > + return 0; > + default: > + return -EINVAL; > + } > +} > + > +static int ltc2664_offset_get(const struct ltc2664_state *st, int c, int *val) > +{ > + const struct ltc2664_chan *chan = &st->channels[c]; > + int span; > + > + span = chan->span; > + if (span < 0) > + return span; > + > + if (st->chip_info->span_helper[span][0] < 0) > + *val = -32768; > + else if (chan->raw[0] >= LTC2672_OFFSET_CODE || > + chan->raw[1] >= LTC2672_OFFSET_CODE) This would benefit from a comment. I'm not sure what it is doing. > + *val = st->chip_info->span_helper[1][span] / 250; > + else > + *val = 0; > + > + return 0; > +} ... > +/* > + * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is > + * not provided in dts. > + */ > +static const struct iio_chan_spec_ext_info ltc2664_toggle_sym_ext_info[] = { > + LTC2664_CHAN_EXT_INFO("raw0", LTC2664_INPUT_A, IIO_SEPARATE, > + ltc2664_dac_input_read, ltc2664_dac_input_write), > + LTC2664_CHAN_EXT_INFO("raw1", LTC2664_INPUT_B, IIO_SEPARATE, > + ltc2664_dac_input_read, ltc2664_dac_input_write), > + LTC2664_CHAN_EXT_INFO("powerdown", LTC2664_POWERDOWN, IIO_SEPARATE, > + ltc2664_reg_bool_get, ltc2664_reg_bool_set), > + LTC2664_CHAN_EXT_INFO("symbol", LTC2664_GLOBAL_TOGGLE, IIO_SEPARATE, > + ltc2664_reg_bool_get, ltc2664_reg_bool_set), > + LTC2664_CHAN_EXT_INFO("toggle_en", LTC2664_TOGGLE_EN, > + IIO_SEPARATE, ltc2664_reg_bool_get, > + ltc2664_reg_bool_set), > + {} > +}; > + > +static const struct iio_chan_spec_ext_info ltc2664_ext_info[] = { > + LTC2664_CHAN_EXT_INFO("powerdown", LTC2664_POWERDOWN, IIO_SEPARATE, > + ltc2664_reg_bool_get, ltc2664_reg_bool_set), as mentioned in the ABI review, powerdown usually comes with a matching powerdown_mode to say what state the DAC output enters on power down. Here there is only one option, so should have a read only Ext info attribute to say it is always 42kohm_to_gnd You should also add that as an option to the main docs. Perhaps this is the point where we define that as a catch all rather than listing every value though. > + {} > +}; ... > +static int ltc2664_channel_config(struct ltc2664_state *st) > +{ > + const struct ltc2664_chip_info *chip_info = st->chip_info; > + struct device *dev = &st->spi->dev; > + u32 reg, tmp[2], mspan; > + int ret, span; > + > + mspan = LTC2664_MSPAN_SOFTSPAN; > + ret = device_property_read_u32(dev, "adi,manual-span-operation-config", > + &mspan); > + if (!ret) { > + if (chip_info->id != LTC2664) Check for a flag that says this config is supported or not, rather than ID. To support new devices we can just add the flag to their chip_info rather than having to change the ID checks throughout the code. > + return dev_err_probe(dev, -EINVAL, > + "adi,manual-span-operation-config not supported\n"); > + > + if (mspan > ARRAY_SIZE(ltc2664_mspan_lut)) > + return dev_err_probe(dev, -EINVAL, > + "adi,manual-span-operation-config not in range\n"); > + } > + > + st->rfsadj = 20000; > + ret = device_property_read_u32(dev, "adi,rfsadj-ohms", &st->rfsadj); > + if (!ret) { > + if (chip_info->id != LTC2672) As before. Use a flag, not an ID match. > + return dev_err_probe(dev, -EINVAL, > + "adi,rfsadj-ohms not supported\n"); > + > + if ((st->rfsadj < 19000 || st->rfsadj > 41000) || > + chip_info->id != LTC2672) You already checked the ID. > + return dev_err_probe(dev, -EINVAL, > + "adi,rfsadj-ohms not in range\n"); > + } > + > + device_for_each_child_node_scoped(dev, child) { > + struct ltc2664_chan *chan; > + > + ret = fwnode_property_read_u32(child, "reg", ®); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to get reg property\n"); > + > + if (reg >= chip_info->num_channels) > + return dev_err_probe(dev, -EINVAL, > + "reg bigger than: %d\n", > + chip_info->num_channels); > + > + chan = &st->channels[reg]; > + > + if (fwnode_property_read_bool(child, "adi,toggle-mode")) { > + chan->toggle_chan = true; > + /* assume sw toggle ABI */ > + st->iio_channels[reg].ext_info = ltc2664_toggle_sym_ext_info; > + /* > + * Clear IIO_CHAN_INFO_RAW bit as toggle channels expose > + * out_voltage/current_raw{0|1} files. > + */ > + __clear_bit(IIO_CHAN_INFO_RAW, > + &st->iio_channels[reg].info_mask_separate); > + } > + > + chan->raw[0] = ltc2664_mspan_lut[mspan][1]; > + chan->raw[1] = ltc2664_mspan_lut[mspan][1]; > + > + chan->span = ltc2664_mspan_lut[mspan][0]; > + > + ret = fwnode_property_read_u32_array(child, "adi,output-range-microvolt", > + tmp, ARRAY_SIZE(tmp)); > + if (!ret && mspan == LTC2664_MSPAN_SOFTSPAN) { I don't like this pattern of doing things only on error. Probably easiet is to factor each of these blocks out as a function to call and then you can just return from that if !ret. Will end up more redable than this is. > + /* voltage type measurement */ > + st->iio_channels[reg].type = IIO_VOLTAGE; > + > + span = ltc2664_span_lookup(st, (int)tmp[0] / 1000, > + tmp[1] / 1000); > + if (span < 0) > + return dev_err_probe(dev, -EINVAL, > + "output range not valid"); > + > + ret = regmap_write(st->regmap, > + LTC2664_CMD_SPAN_N(reg), > + span); > + if (ret) > + return dev_err_probe(dev, -EINVAL, > + "failed to set chan settings\n"); > + > + chan->span = span; > + } > + > + ret = fwnode_property_read_u32(child, > + "adi,output-range-microamp", > + &tmp[0]); > + if (!ret) { > + /* current type measurement */ > + st->iio_channels[reg].type = IIO_CURRENT; > + > + span = ltc2664_span_lookup(st, 0, > + (int)tmp[0] / 1000); > + if (span < 0) > + return dev_err_probe(dev, -EINVAL, > + "output range not valid"); > + > + ret = regmap_write(st->regmap, > + LTC2664_CMD_SPAN_N(reg), > + span + 1); > + if (ret) > + return dev_err_probe(dev, -EINVAL, > + "failed to set chan settings\n"); > + > + chan->span = span; > + } > + } > + > + return 0; > +} > + > +static int ltc2664_setup(struct ltc2664_state *st, struct regulator *vref) > +{ > + const struct ltc2664_chip_info *chip_info = st->chip_info; > + struct gpio_desc *gpio; > + int ret; > + > + /* > + * If we have a clr/reset pin, use that to reset the chip. Single line comment style is /* If we have.. chip. */ > + */ > + gpio = devm_gpiod_get_optional(&st->spi->dev, "clr", GPIOD_OUT_HIGH); > + if (IS_ERR(gpio)) > + return dev_err_probe(&st->spi->dev, PTR_ERR(gpio), > + "Failed to get reset gpio"); > + if (gpio) { > + usleep_range(1000, 1200); > + /* bring device out of reset */ I'd argue that comment is obvious enough we don't need it. > + gpiod_set_value_cansleep(gpio, 0); > + } > + > + /* > + * Duplicate the default channel configuration as it can change during > + * @ltc2664_channel_config() > + */ > + st->iio_channels = devm_kmemdup(&st->spi->dev, chip_info->iio_chan, > + chip_info->num_channels * > + sizeof(*chip_info->iio_chan), > + GFP_KERNEL); Is that big enough? I'd expect 1 more entry as a null terminator. > + > + ret = ltc2664_channel_config(st); > + if (ret) > + return ret; > + > + if (!vref) > + return 0; > + > + return regmap_set_bits(st->regmap, LTC2664_CMD_CONFIG, LTC2664_REF_DISABLE); > +} ... > +static int ltc2664_probe(struct spi_device *spi) > +{ > + static const char * const regulators[] = { "vcc", "iovcc" }; > + const struct ltc2664_chip_info *chip_info; > + struct device *dev = &spi->dev; > + struct regulator *vref_reg; > + struct iio_dev *indio_dev; > + struct ltc2664_state *st; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > + if (!indio_dev) > + return -ENOMEM; > + > + st = iio_priv(indio_dev); > + st->spi = spi; > + > + chip_info = spi_get_device_match_data(spi); > + if (!chip_info) > + return -ENOMEM; > + > + st->chip_info = chip_info; > + > + mutex_init(&st->lock); > + > + st->regmap = devm_regmap_init_spi(spi, <c2664_regmap_config); > + if (IS_ERR(st->regmap)) > + return dev_err_probe(dev, PTR_ERR(st->regmap), > + "Failed to init regmap"); > + > + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators), > + regulators); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to enable regulators\n"); > + > + vref_reg = devm_regulator_get_optional(dev, "vref"); > + if (IS_ERR(vref_reg)) { > + if (PTR_ERR(vref_reg) != -ENODEV) > + return dev_err_probe(dev, PTR_ERR(vref_reg), > + "Failed to get vref regulator"); > + > + vref_reg = NULL; > + > + /* internal reference */ > + if (chip_info->id == LTC2664) Push all chip specific data into the chip_info structure. Never match on ID value as it is extensible as you add more devices to a driver over time. st->vref = chip_info->internal_vref; or something like that. > + st->vref = 2500; > + else > + st->vref = 1250; > + } else { > + ret = regulator_enable(vref_reg); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to enable vref regulators\n"); > + > + ret = devm_add_action_or_reset(dev, ltc2664_disable_regulator, > + vref_reg); > + if (ret) > + return ret; > + > + ret = regulator_get_voltage(vref_reg); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Failed to get vref\n"); > + > + st->vref = ret / 1000; > + } > + > + ret = ltc2664_setup(st, vref_reg); > + if (ret) > + return ret; > + > + indio_dev->name = chip_info->name; > + indio_dev->info = <c2664_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = st->iio_channels; > + indio_dev->num_channels = chip_info->num_channels; > + > + return devm_iio_device_register(dev, indio_dev); > +} > + > +static const struct spi_device_id ltc2664_id[] = { > + { "ltc2664", (kernel_ulong_t)<c2664_chip }, > + { "ltc2672", (kernel_ulong_t)<c2672_chip }, > + { }, No trailing commas needed or desirable on terminating entries like this. We will never add anything after them. Jonathan ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2024-04-20 10:24 UTC | newest] Thread overview: 24+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-12 3:20 [PATCH 0/4] Add driver for LTC2664 and LTC2672 Kim Seer Paller 2024-04-12 3:20 ` [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml Kim Seer Paller 2024-04-12 5:50 ` Krzysztof Kozlowski 2024-04-13 14:54 ` Jonathan Cameron 2024-04-16 14:16 ` Paller, Kim Seer 2024-04-12 21:23 ` David Lechner 2024-04-13 15:06 ` Jonathan Cameron 2024-04-13 15:11 ` Jonathan Cameron 2024-04-13 16:21 ` David Lechner 2024-04-13 17:10 ` Jonathan Cameron 2024-04-16 14:40 ` Paller, Kim Seer 2024-04-20 10:13 ` Jonathan Cameron 2024-04-12 3:21 ` [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC Kim Seer Paller 2024-04-12 21:26 ` David Lechner 2024-04-13 15:25 ` Jonathan Cameron 2024-04-13 20:38 ` David Lechner 2024-04-15 12:45 ` Nuno Sá 2024-04-20 10:22 ` Jonathan Cameron 2024-04-12 3:21 ` [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC Kim Seer Paller 2024-04-13 15:26 ` Jonathan Cameron 2024-04-16 14:18 ` Paller, Kim Seer 2024-04-20 10:23 ` Jonathan Cameron 2024-04-12 3:21 ` [PATCH 4/4] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672 Kim Seer Paller 2024-04-13 15:55 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).