LinuxLists.cc - [PATCH 0/4] Add driver for LTC2664 and LTC2672

[permalink] [raw]

Subject: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

Add documentation for ltc2664 and ltc2672.

Co-developed-by: Michael Hennerich <[email protected]>
Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Kim Seer Paller <[email protected]>
---
.../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 <[email protected]>
+ - Kim Seer Paller <[email protected]>
+
+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 <[email protected]>
+M: Kim Seer Paller <[email protected]>
+L: [email protected]
+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á <[email protected]>
L: [email protected]
--
2.34.1

2024-04-12 03:23:19

[permalink] [raw]

Subject: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC

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 <[email protected]>
Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Kim Seer Paller <[email protected]>
---
.../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: [email protected]
+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: [email protected]
+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: [email protected]
+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 <[email protected]>
L: [email protected]
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

2024-04-12 05:50:31

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

On 12/04/2024 05:20, Kim Seer Paller wrote:
> Add documentation for ltc2664 and ltc2672.
>
> Co-developed-by: Michael Hennerich <[email protected]>
> Signed-off-by: Michael Hennerich <[email protected]>
> Signed-off-by: Kim Seer Paller <[email protected]>
> ---
> .../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 <[email protected]>
> + - Kim Seer Paller <[email protected]>
> +
> +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

2024-04-12 21:23:26

[permalink] [raw]

Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
<[email protected]> wrote:
>
> Add documentation for ltc2664 and ltc2672.
>
> Co-developed-by: Michael Hennerich <[email protected]>
> Signed-off-by: Michael Hennerich <[email protected]>
> Signed-off-by: Kim Seer Paller <[email protected]>
> ---
> .../bindings/iio/dac/adi,ltc2664.yaml | 230 ++++++++++++++++++
> MAINTAINERS | 8 +
> 2 files changed, 238 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2664yaml
>
> 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 <[email protected]>
> + - Kim Seer Paller <[email protected]>
> +
> +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
> +

2024-04-12 21:27:00

[permalink] [raw]

Subject: Re: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC

On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
<[email protected]> 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 <[email protected]>
> Signed-off-by: Michael Hennerich <[email protected]>
> Signed-off-by: Kim Seer Paller <[email protected]>
> ---
> .../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: [email protected]
> +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: [email protected]
> +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: [email protected]
> +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 <[email protected]>
> L: [email protected]
> 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
>

2024-04-13 14:55:16

[permalink] [raw]

Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

On Fri, 12 Apr 2024 07:50:17 +0200
Krzysztof Kozlowski <[email protected]> wrote:

> On 12/04/2024 05:20, Kim Seer Paller wrote:
> > Add documentation for ltc2664 and ltc2672.
> >
> > Co-developed-by: Michael Hennerich <[email protected]>
> > Signed-off-by: Michael Hennerich <[email protected]>
> > Signed-off-by: Kim Seer Paller <[email protected]>
> > ---
> > .../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 <[email protected]>
> > + - Kim Seer Paller <[email protected]>
> > +
> > +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

2024-04-13 15:06:39

[permalink] [raw]

Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

On Fri, 12 Apr 2024 16:23:00 -0500
David Lechner <[email protected]> wrote:

> On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
> <[email protected]> wrote:
> >
> > Add documentation for ltc2664 and ltc2672.
> >
> > Co-developed-by: Michael Hennerich <[email protected]>
> > Signed-off-by: Michael Hennerich <[email protected]>
> > Signed-off-by: Kim Seer Paller <[email protected]>

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 <[email protected]>
> > + - Kim Seer Paller <[email protected]>
> > +
> > +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
> > +

2024-04-13 15:12:02

[permalink] [raw]

Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

On Sat, 13 Apr 2024 16:06:10 +0100
Jonathan Cameron <[email protected]> wrote:

> On Fri, 12 Apr 2024 16:23:00 -0500
> David Lechner <[email protected]> wrote:
>
> > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
> > <[email protected]> wrote:
> > >
> > > Add documentation for ltc2664 and ltc2672.
> > >
> > > Co-developed-by: Michael Hennerich <[email protected]>
> > > Signed-off-by: Michael Hennerich <[email protected]>
> > > Signed-off-by: Kim Seer Paller <[email protected]>
>
> 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 <[email protected]>
> > > + - Kim Seer Paller <[email protected]>
> > > +
> > > +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
> > > +
>
>

2024-04-13 15:25:41

[permalink] [raw]

Subject: Re: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC

On Fri, 12 Apr 2024 16:26:17 -0500
David Lechner <[email protected]> wrote:

> On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
> <[email protected]> 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 <[email protected]>
> > Signed-off-by: Michael Hennerich <[email protected]>
> > Signed-off-by: Kim Seer Paller <[email protected]>
> > ---
> > .../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: [email protected]
> > +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: [email protected]
> > +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: [email protected]
> > +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 <[email protected]>
> > L: [email protected]
> > 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
> >

2024-04-13 16:22:23

[permalink] [raw]

Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

On Sat, Apr 13, 2024 at 10:06 AM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 12 Apr 2024 16:23:00 -0500
> David Lechner <[email protected]> wrote:
>
> > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
> > <[email protected]> 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.

2024-04-13 17:10:51

[permalink] [raw]

Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

On Sat, 13 Apr 2024 11:21:55 -0500
David Lechner <[email protected]> wrote:

> On Sat, Apr 13, 2024 at 10:06 AM Jonathan Cameron <[email protected]> wrote:
> >
> > On Fri, 12 Apr 2024 16:23:00 -0500
> > David Lechner <[email protected]> wrote:
> >
> > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
> > > <[email protected]> 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

2024-04-13 20:38:43

[permalink] [raw]

Subject: Re: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC

On 4/13/24 10:25 AM, Jonathan Cameron wrote:
> On Fri, 12 Apr 2024 16:26:17 -0500
> David Lechner <[email protected]> wrote:
>
>> On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
>> <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Michael Hennerich <[email protected]>
>>> Signed-off-by: Kim Seer Paller <[email protected]>
>>> ---
>>> .../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: [email protected]
>>> +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: [email protected]
>>> +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: [email protected]
>>> +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 <[email protected]>
>>> L: [email protected]
>>> 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.

2024-04-15 12:42:38

by Nuno Sá

[permalink] [raw]

Subject: Re: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC

On Sat, 2024-04-13 at 16:25 +0100, Jonathan Cameron wrote:
> On Fri, 12 Apr 2024 16:26:17 -0500
> David Lechner <[email protected]> wrote:
>
> > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
> > <[email protected]> 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á
>

2024-04-16 14:19:31

[permalink] [raw]

Subject: RE: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Saturday, April 13, 2024 10:55 PM
> To: Krzysztof Kozlowski <[email protected]>
> Cc: Paller, Kim Seer <[email protected]>; [email protected];
> [email protected]; [email protected]; Lars-Peter Clausen
> <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>;
> Liam Girdwood <[email protected]>; Mark Brown <[email protected]>;
> David Lechner <[email protected]>; Hennerich, Michael
> <[email protected]>
> 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 <[email protected]> wrote:
>
> > On 12/04/2024 05:20, Kim Seer Paller wrote:
> > > Add documentation for ltc2664 and ltc2672.
> > >
> > > Co-developed-by: Michael Hennerich <[email protected]>
> > > Signed-off-by: Michael Hennerich <[email protected]>
> > > Signed-off-by: Kim Seer Paller <[email protected]>
> > > ---
> > > .../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 <[email protected]>
> > > + - Kim Seer Paller <[email protected]>
> > > +
> > > +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

2024-04-16 14:41:28

[permalink] [raw]

Subject: RE: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Sunday, April 14, 2024 1:10 AM
> To: David Lechner <[email protected]>
> Cc: Paller, Kim Seer <[email protected]>; [email protected];
> [email protected]; [email protected]; Lars-Peter Clausen
> <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>;
> Liam Girdwood <[email protected]>; Mark Brown <[email protected]>;
> Hennerich, Michael <[email protected]>
> 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 <[email protected]> wrote:
>
> > On Sat, Apr 13, 2024 at 10:06 AM Jonathan Cameron <[email protected]>
> wrote:
> > >
> > > On Fri, 12 Apr 2024 16:23:00 -0500
> > > David Lechner <[email protected]> wrote:
> > >
> > > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
> > > > <[email protected]> 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
>

2024-04-20 10:13:49

[permalink] [raw]

Subject: Re: [PATCH 1/4] dt-bindings: iio: dac: Add adi,ltc2664.yaml

On Tue, 16 Apr 2024 14:40:56 +0000
"Paller, Kim Seer" <[email protected]> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <[email protected]>
> > Sent: Sunday, April 14, 2024 1:10 AM
> > To: David Lechner <[email protected]>
> > Cc: Paller, Kim Seer <[email protected]>; [email protected];
> > [email protected]; [email protected]; Lars-Peter Clausen
> > <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski
> > <[email protected]>; Conor Dooley <[email protected]>;
> > Liam Girdwood <[email protected]>; Mark Brown <[email protected]>;
> > Hennerich, Michael <[email protected]>
> > 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 <[email protected]> wrote:
> >
> > > On Sat, Apr 13, 2024 at 10:06 AM Jonathan Cameron <[email protected]>
> > wrote:
> > > >
> > > > On Fri, 12 Apr 2024 16:23:00 -0500
> > > > David Lechner <[email protected]> wrote:
> > > >
> > > > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller
> > > > > <[email protected]> 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
> >
>

2024-04-20 10:22:54