2022-09-27 14:53:52

by Tilki, Ibrahim

[permalink] [raw]
Subject: [PATCH v6 0/3] iio: adc: add max11410 adc driver

Hello devicetree maintainers,

We had a discussion regarding the reg propert of adc channel nodes:
The adc.yaml binding requires the @X number to match with reg. Reg specifies
the channel index but channels are specified with "diff-channels" property
when the channel is differential.

Our driver supports mixing single ended and differential channels and the reg
property is not used in differential mode.
We want to constrain reg between 0 and 9 since ADC only has 10 channels.
That means we also need to constrain @address of channel but that wouldn't allow
us to define more than 10 channels which users might need to do. How should we
specify the @address and reg property in this case for differential channels?

Please refer to the discussions below:
https://marc.info/?l=linux-iio&m=166402759331673&w=2
https://marc.info/?l=linux-iio&m=166402779031712&w=2

Hi Jonathan,

I removed the check for "avdd-supply" at probe time and treated it as a regular vref.
No other change is made to the driver.

Best regards,
Ibrahim Tilki

Note: No sign-off tag for David as he was unreachable when the initial patch was sent.

Changelog:
since v6:
- don't require avdd supply if not needed

since v5:
- allow user to specify both interrupt pins
- keep irq info in max11410_state struct and use irq by name
- get irqs by fwnode_get_irq_byname
- don't allocate trigger when no irq supplied
- fix deadlock condition in write_raw
- minor style fixes
- fix devicetree binding errors reported by dt_binding_check
- convert module license to GPL as suggested by checkpatch

since v4:
- add in_voltage_filter2_notch_{center,en} attrs for sinc4 filter
- add ABI documentation for filter sysfs
- check interrupt-names property for configuring gpio of max11410
- remove hardwaregain property
- add scale_available property for channes using PGA
- separate vref regulator error -ENODEV from other errors
- don't register trigger if no irq specified
- style fixes

since v3:
- prefix defines with MAX11410_
- group vref regulators
- use builtin iio_validate_scan_mask_onehot
- validate iio trigger
- move scan data into state struct
- require vrefn regulator in DT if used by any channel
- don't require irq for triggered buffer
- remove filter sysfs attr and ABI documentation
- add in_voltage_filter[0-1]_notch_{center,en} attrs

since v2:
- remove bit position shifting, use field_prep instead
- reduce the amount of reg writes in max11410_configure_channel
- add error checking in max11410_parse_channels
- remove some unneeded blank lines and minor style fixes
- remove scan data assignment in max11410_trigger_handler


Ibrahim Tilki (3):
iio: adc: add max11410 adc driver
dt-bindings: iio: adc: add adi,max11410.yaml
Documentation: ABI: testing: add max11410 doc

.../ABI/testing/sysfs-bus-iio-adc-max11410 | 13 +
.../bindings/iio/adc/adi,max11410.yaml | 176 +++
.../devicetree/bindings/rtc/adi,max313xx.yaml | 195 +++
drivers/iio/adc/Kconfig | 13 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max11410.c | 1049 +++++++++++++++++
6 files changed, 1447 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max11410
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
create mode 100644 drivers/iio/adc/max11410.c

--
2.25.1


2022-09-27 14:58:55

by Tilki, Ibrahim

[permalink] [raw]
Subject: [PATCH v6 2/3] dt-bindings: iio: adc: add adi,max11410.yaml

Adding devicetree binding documentation for max11410 adc.

Signed-off-by: Ibrahim Tilki <[email protected]>
---
.../bindings/iio/adc/adi,max11410.yaml | 176 ++++++++++++++++
.../devicetree/bindings/rtc/adi,max313xx.yaml | 195 ++++++++++++++++++
2 files changed, 371 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
new file mode 100644
index 0000000000..46a37303da
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
@@ -0,0 +1,176 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2022 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,max11410.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX11410 ADC device driver
+
+maintainers:
+ - Ibrahim Tilki <[email protected]>
+
+description: |
+ Bindings for the Analog Devices MAX11410 ADC device. Datasheet can be
+ found here:
+ https://datasheets.maximintegrated.com/en/ds/MAX11410.pdf
+
+properties:
+ compatible:
+ enum:
+ - adi,max11410
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ minItems: 1
+ maxItems: 2
+
+ interrupt-names:
+ description: Name of the gpio pin of max11410 used for IRQ
+ items:
+ - enum:
+ - gpio0
+ - gpio1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ avdd-supply:
+ description: Optional avdd supply. Used as reference when no explicit reference supplied.
+
+ vref0p-supply:
+ description: vref0p supply can be used as reference for conversion.
+
+ vref1p-supply:
+ description: vref1p supply can be used as reference for conversion.
+
+ vref2p-supply:
+ description: vref2p supply can be used as reference for conversion.
+
+ vref0n-supply:
+ description: vref0n supply can be used as reference for conversion.
+
+ vref1n-supply:
+ description: vref1n supply can be used as reference for conversion.
+
+ vref2n-supply:
+ description: vref2n supply can be used as reference for conversion.
+
+ spi-max-frequency:
+ maximum: 8000000
+
+required:
+ - compatible
+ - reg
+
+patternProperties:
+ "^channel(@[0-9])?$":
+ $ref: "adc.yaml"
+ type: object
+ description: Represents the external channels which are connected to the ADC.
+
+ properties:
+ reg:
+ description: The channel number in single-ended mode.
+ minimum: 0
+ maximum: 9
+
+ adi,reference:
+ description: |
+ Select the reference source to use when converting on
+ the specific channel. Valid values are:
+ 0: VREF0P/VREF0N
+ 1: VREF1P/VREF1N
+ 2: VREF2P/VREF2N
+ 3: AVDD/AGND
+ 4: VREF0P/AGND
+ 5: VREF1P/AGND
+ 6: VREF2P/AGND
+ If this field is left empty, AVDD/AGND is selected.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1, 2, 3, 4, 5, 6]
+ default: 3
+
+ adi,input-mode:
+ description: |
+ Select signal path of input channels. Valid values are:
+ 0: Buffered, low-power, unity-gain path (default)
+ 1: Bypass path
+ 2: PGA path
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1, 2]
+ default: 0
+
+ diff-channels: true
+
+ bipolar: true
+
+ settling-time-us: true
+
+ adi,buffered-vrefp:
+ description: Enable buffered mode for positive reference.
+ type: boolean
+
+ adi,buffered-vrefn:
+ description: Enable buffered mode for negative reference.
+ type: boolean
+
+ required:
+ - reg
+
+ additionalProperties: false
+
+allOf:
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adc@0 {
+ reg = <0>;
+ compatible = "adi,max11410";
+ spi-max-frequency = <8000000>;
+
+ interrupt-parent = <&gpio>;
+ interrupts = <25 2>;
+ interrupt-names = "gpio1";
+
+ avdd-supply = <&adc_avdd>;
+
+ vref1p-supply = <&adc_vref1p>;
+ vref1n-supply = <&adc_vref1n>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ channel@0 {
+ reg = <0>;
+ };
+
+ channel@1 {
+ reg = <1>;
+ diff-channels = <2 3>;
+ adi,reference = <1>;
+ bipolar;
+ settling-time-us = <100000>;
+ };
+
+ channel@2 {
+ reg = <2>;
+ diff-channels = <7 9>;
+ adi,reference = <5>;
+ adi,input-mode = <2>;
+ settling-time-us = <50000>;
+ };
+ };
+ };
diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
new file mode 100644
index 0000000000..43576ec066
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
@@ -0,0 +1,195 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2022 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX313XX series I2C RTC driver
+
+maintainers:
+ - Ibrahim Tilki <[email protected]>
+ - Zeynep Arslanbenzer <[email protected]>
+
+description: Bindings for the Analog Devices MAX313XX series RTCs.
+
+properties:
+ compatible:
+ oneOf:
+ - enum:
+ - adi,max31328
+ - adi,max31329
+ - adi,max31331
+ - adi,max31334
+ - adi,max31341
+ - adi,max31342
+ - adi,max31343
+
+ reg:
+ description: I2C address of the RTC
+ items:
+ enum: [0x68, 0x69]
+
+ interrupts:
+ minItems: 1
+ maxItems: 2
+
+ interrupt-names:
+ description: |
+ Name of the interrupt pin of the RTC used for IRQ. Not required for
+ RTCs that only have single interrupt pin available. Some of the RTCs
+ share interrupt pins with clock input/output pins.
+ minItems: 1
+ items:
+ - enum:
+ - INTA
+ - INTB
+ - enum:
+ - INTA
+ - INTB
+
+ "#clock-cells":
+ description: |
+ RTC can be used as a clock source through its clock output pin when
+ supplied.
+ const: 0
+
+ clocks:
+ description: |
+ RTC uses this clock for clock input when supplied. Clock has to provide
+ one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
+ maxItems: 1
+
+ trickle-diode-disable: true
+
+ trickle-resistor-ohms:
+ description: Enables trickle charger with specified resistor value.
+ items:
+ enum: [3000, 6000, 11000]
+
+ wakeup-source: true
+
+additionalProperties: false
+
+allOf:
+ - $ref: "rtc.yaml#"
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - adi,max31328
+ - adi,max31342
+
+ then:
+ properties:
+ trickle-diode-disable: false
+ trickle-resistor-ohms: false
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - adi,max31328
+ - adi,max31331
+ - adi,max31334
+ - adi,max31343
+
+ then:
+ properties:
+ clocks: false
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - adi,max31341
+ - adi,max31342
+
+ then:
+ properties:
+ reg:
+ items:
+ - const: 0x69
+
+ else:
+ properties:
+ reg:
+ items:
+ - const: 0x68
+
+required:
+ - compatible
+ - reg
+
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rtc@68 {
+ reg = <0x68>;
+ compatible = "adi,max31328";
+ };
+ };
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rtc@68 {
+ reg = <0x68>;
+ compatible = "adi,max31329";
+ clocks = <&clkin>;
+ interrupt-parent = <&gpio>;
+ interrupts = <26 2>;
+ interrupt-names = "INTB";
+ };
+ };
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rtc@68 {
+ reg = <0x68>;
+ compatible = "adi,max31331";
+ #clock-cells = <0>;
+ interrupt-parent = <&gpio>;
+ interrupts = <25 2>, <26 2>;
+ interrupt-names = "INTA", "INTB";
+ };
+ };
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rtc@69 {
+ reg = <0x69>;
+ compatible = "adi,max31341";
+ #clock-cells = <0>;
+ clocks = <&clkin>;
+ };
+ };
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ max31341: rtc@69 {
+ reg = <0x69>;
+ compatible = "adi,max31341";
+ #clock-cells = <0>;
+ };
+
+ rtc@68 {
+ reg = <0x68>;
+ compatible = "adi,max31329";
+ clocks = <&max31341>;
+ };
+ };
--
2.25.1

2022-09-27 15:14:03

by Tilki, Ibrahim

[permalink] [raw]
Subject: [PATCH v6 3/3] Documentation: ABI: testing: add max11410 doc

Adding documentation for Analog Devices max11410 adc userspace sysfs.

Signed-off-by: Ibrahim Tilki <[email protected]>
---
.../ABI/testing/sysfs-bus-iio-adc-max11410 | 13 +++++++++++++
1 file changed, 13 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max11410

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max11410 b/Documentation/ABI/testing/sysfs-bus-iio-adc-max11410
new file mode 100644
index 0000000000..2a53c6b373
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max11410
@@ -0,0 +1,13 @@
+What: /sys/bus/iio/devices/iio:deviceX/in_voltage_filterY_notch_en
+Date: September 2022
+KernelVersion: 6.0
+Contact: [email protected]
+Description:
+ Enable or disable a notch filter.
+
+What: /sys/bus/iio/devices/iio:deviceX/in_voltage_filterY_notch_center
+Date: September 2022
+KernelVersion: 6.0
+Contact: [email protected]
+Description:
+ Center frequency of the notch filter in Hz.
--
2.25.1

2022-09-29 10:02:59

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v6 2/3] dt-bindings: iio: adc: add adi,max11410.yaml

On 27/09/2022 16:18, Ibrahim Tilki wrote:
> Adding devicetree binding documentation for max11410 adc.
>
> Signed-off-by: Ibrahim Tilki <[email protected]>
> ---
> .../bindings/iio/adc/adi,max11410.yaml | 176 ++++++++++++++++
> .../devicetree/bindings/rtc/adi,max313xx.yaml | 195 ++++++++++++++++++
> 2 files changed, 371 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
>

So it is a v6 and it is the first version you send to proper maintainers
using get_maintainers.pl... sigh...

> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> new file mode 100644
> index 0000000000..46a37303da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> @@ -0,0 +1,176 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,max11410.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX11410 ADC device driver
> +
> +maintainers:
> + - Ibrahim Tilki <[email protected]>
> +
> +description: |
> + Bindings for the Analog Devices MAX11410 ADC device. Datasheet can be
> + found here:
> + https://datasheets.maximintegrated.com/en/ds/MAX11410.pdf
> +
> +properties:
> + compatible:
> + enum:
> + - adi,max11410
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + minItems: 1
> + maxItems: 2
> +
> + interrupt-names:
> + description: Name of the gpio pin of max11410 used for IRQ
> + items:
> + - enum:
> + - gpio0
> + - gpio1

This is wrong. You said in interrupts you can have two items, but here
you list only one. I don't know what do you want to achieve here.

> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> + avdd-supply:
> + description: Optional avdd supply. Used as reference when no explicit reference supplied.
> +
> + vref0p-supply:
> + description: vref0p supply can be used as reference for conversion.
> +
> + vref1p-supply:
> + description: vref1p supply can be used as reference for conversion.
> +
> + vref2p-supply:
> + description: vref2p supply can be used as reference for conversion.
> +
> + vref0n-supply:
> + description: vref0n supply can be used as reference for conversion.
> +
> + vref1n-supply:
> + description: vref1n supply can be used as reference for conversion.
> +
> + vref2n-supply:
> + description: vref2n supply can be used as reference for conversion.
> +
> + spi-max-frequency:
> + maximum: 8000000
> +
> +required:
> + - compatible
> + - reg
> +
> +patternProperties:

This goes before required block

> + "^channel(@[0-9])?$":
> + $ref: "adc.yaml"
> + type: object
> + description: Represents the external channels which are connected to the ADC.
> +
> + properties:
> + reg:
> + description: The channel number in single-ended mode.
> + minimum: 0
> + maximum: 9
> +
> + adi,reference:
> + description: |
> + Select the reference source to use when converting on
> + the specific channel. Valid values are:
> + 0: VREF0P/VREF0N
> + 1: VREF1P/VREF1N
> + 2: VREF2P/VREF2N
> + 3: AVDD/AGND
> + 4: VREF0P/AGND
> + 5: VREF1P/AGND
> + 6: VREF2P/AGND
> + If this field is left empty, AVDD/AGND is selected.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1, 2, 3, 4, 5, 6]
> + default: 3
> +
> + adi,input-mode:
> + description: |
> + Select signal path of input channels. Valid values are:
> + 0: Buffered, low-power, unity-gain path (default)
> + 1: Bypass path
> + 2: PGA path
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1, 2]
> + default: 0
> +
> + diff-channels: true
> +
> + bipolar: true
> +
> + settling-time-us: true
> +
> + adi,buffered-vrefp:
> + description: Enable buffered mode for positive reference.
> + type: boolean
> +
> + adi,buffered-vrefn:
> + description: Enable buffered mode for negative reference.
> + type: boolean
> +
> + required:
> + - reg
> +
> + additionalProperties: false
> +
> +allOf:
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + adc@0 {
> + reg = <0>;
> + compatible = "adi,max11410";
> + spi-max-frequency = <8000000>;
> +
> + interrupt-parent = <&gpio>;
> + interrupts = <25 2>;
> + interrupt-names = "gpio1";
> +
> + avdd-supply = <&adc_avdd>;
> +
> + vref1p-supply = <&adc_vref1p>;
> + vref1n-supply = <&adc_vref1n>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + channel@0 {
> + reg = <0>;
> + };
> +
> + channel@1 {
> + reg = <1>;
> + diff-channels = <2 3>;
> + adi,reference = <1>;
> + bipolar;
> + settling-time-us = <100000>;
> + };
> +
> + channel@2 {
> + reg = <2>;
> + diff-channels = <7 9>;
> + adi,reference = <5>;
> + adi,input-mode = <2>;
> + settling-time-us = <50000>;
> + };
> + };
> + };
> diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> new file mode 100644
> index 0000000000..43576ec066
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> @@ -0,0 +1,195 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX313XX series I2C RTC driver
> +
> +maintainers:
> + - Ibrahim Tilki <[email protected]>
> + - Zeynep Arslanbenzer <[email protected]>
> +
> +description: Bindings for the Analog Devices MAX313XX series RTCs.
> +
> +properties:
> + compatible:
> + oneOf:

No need for oneOf

> + - enum:
> + - adi,max31328
> + - adi,max31329
> + - adi,max31331
> + - adi,max31334
> + - adi,max31341
> + - adi,max31342
> + - adi,max31343
> +
> + reg:
> + description: I2C address of the RTC
> + items:
> + enum: [0x68, 0x69]

One item, so:
items:
- enum: ......

> +
> + interrupts:
> + minItems: 1
> + maxItems: 2
> +
> + interrupt-names:
> + description: |
> + Name of the interrupt pin of the RTC used for IRQ. Not required for
> + RTCs that only have single interrupt pin available. Some of the RTCs
> + share interrupt pins with clock input/output pins.
> + minItems: 1
> + items:
> + - enum:
> + - INTA
> + - INTB
> + - enum:
> + - INTA
> + - INTB

Why this is so flexible? Any device allows any interrupt to be present
or not?

> +
> + "#clock-cells":
> + description: |
> + RTC can be used as a clock source through its clock output pin when
> + supplied.
> + const: 0
> +
> + clocks:
> + description: |
> + RTC uses this clock for clock input when supplied. Clock has to provide
> + one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
> + maxItems: 1
> +
> + trickle-diode-disable: true
> +
> + trickle-resistor-ohms:
> + description: Enables trickle charger with specified resistor value.
> + items:
> + enum: [3000, 6000, 11000]

This is not a list. Just enums, no items.

> +
> + wakeup-source: true
> +
> +additionalProperties: false
> +
> +allOf:
> + - $ref: "rtc.yaml#"

Skip quotes


> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - adi,max31328
> + - adi,max31342
> +
> + then:
> + properties:
> + trickle-diode-disable: false
> + trickle-resistor-ohms: false
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - adi,max31328
> + - adi,max31331
> + - adi,max31334
> + - adi,max31343
> +
> + then:
> + properties:
> + clocks: false
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - adi,max31341
> + - adi,max31342
> +
> + then:
> + properties:
> + reg:
> + items:
> + - const: 0x69
> +
> + else:
> + properties:
> + reg:
> + items:
> + - const: 0x68
> +
> +required:
> + - compatible
> + - reg
> +
> +

One blank line

> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rtc@68 {
> + reg = <0x68>;
> + compatible = "adi,max31328";

Skip the example, it's part of all others.

> + };
> + };
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rtc@68 {
> + reg = <0x68>;
> + compatible = "adi,max31329";
> + clocks = <&clkin>;
> + interrupt-parent = <&gpio>;
> + interrupts = <26 2>;

Aren't you using interrupt flags? If so, use defines.

> + interrupt-names = "INTB";
> + };
> + };
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rtc@68 {
> + reg = <0x68>;
> + compatible = "adi,max31331";
> + #clock-cells = <0>;
> + interrupt-parent = <&gpio>;
> + interrupts = <25 2>, <26 2>;
> + interrupt-names = "INTA", "INTB";
> + };
> + };
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rtc@69 {
> + reg = <0x69>;
> + compatible = "adi,max31341";
> + #clock-cells = <0>;
> + clocks = <&clkin>;
> + };
> + };
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;

Skip this example, it's the same.

> +
> + max31341: rtc@69 {
> + reg = <0x69>;
> + compatible = "adi,max31341";
> + #clock-cells = <0>;
> + };
> +
> + rtc@68 {
> + reg = <0x68>;
> + compatible = "adi,max31329";
> + clocks = <&max31341>;
> + };
> + };

Best regards,
Krzysztof

2022-10-02 13:33:17

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v6 2/3] dt-bindings: iio: adc: add adi,max11410.yaml

On Thu, 29 Sep 2022 11:35:10 +0200
Krzysztof Kozlowski <[email protected]> wrote:

> On 27/09/2022 16:18, Ibrahim Tilki wrote:
> > Adding devicetree binding documentation for max11410 adc.
> >
> > Signed-off-by: Ibrahim Tilki <[email protected]>
> > ---
> > .../bindings/iio/adc/adi,max11410.yaml | 176 ++++++++++++++++
> > .../devicetree/bindings/rtc/adi,max313xx.yaml | 195 ++++++++++++++++++
> > 2 files changed, 371 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> > create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> >
>
> So it is a v6 and it is the first version you send to proper maintainers
> using get_maintainers.pl... sigh...

I only noticed the missing cc on v5 - wasn't paying attention to the binding
as lots of stuff in the driver...

However, how did this pick up an rtc binding for a totally different part?


>
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> > new file mode 100644
> > index 0000000000..46a37303da
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> > @@ -0,0 +1,176 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2022 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/adi,max11410.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices MAX11410 ADC device driver
> > +
> > +maintainers:
> > + - Ibrahim Tilki <[email protected]>
> > +
> > +description: |
> > + Bindings for the Analog Devices MAX11410 ADC device. Datasheet can be
> > + found here:
> > + https://datasheets.maximintegrated.com/en/ds/MAX11410.pdf
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - adi,max11410
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + minItems: 1
> > + maxItems: 2
> > +
> > + interrupt-names:
> > + description: Name of the gpio pin of max11410 used for IRQ
> > + items:
> > + - enum:
> > + - gpio0
> > + - gpio1
>
> This is wrong. You said in interrupts you can have two items, but here
> you list only one. I don't know what do you want to achieve here.

Aim is 0, 1 or 2 interrupts + knowing which ones they are.
Device has two pins that have very similar functionality and board
designers are likely to pick one or the two more or less at random depending
on which trace is easier to route.

So my guess is this needs minItems, maxItems.


>
> > +
> > + '#address-cells':
> > + const: 1
> > +
> > + '#size-cells':
> > + const: 0
> > +
> > + avdd-supply:
> > + description: Optional avdd supply. Used as reference when no explicit reference supplied.
> > +
> > + vref0p-supply:
> > + description: vref0p supply can be used as reference for conversion.
> > +
> > + vref1p-supply:
> > + description: vref1p supply can be used as reference for conversion.
> > +
> > + vref2p-supply:
> > + description: vref2p supply can be used as reference for conversion.
> > +
> > + vref0n-supply:
> > + description: vref0n supply can be used as reference for conversion.
> > +
> > + vref1n-supply:
> > + description: vref1n supply can be used as reference for conversion.
> > +
> > + vref2n-supply:
> > + description: vref2n supply can be used as reference for conversion.
> > +
> > + spi-max-frequency:
> > + maximum: 8000000
> > +
> > +required:
> > + - compatible
> > + - reg
> > +
> > +patternProperties:
>
> This goes before required block
>
> > + "^channel(@[0-9])?$":
> > + $ref: "adc.yaml"
> > + type: object
> > + description: Represents the external channels which are connected to the ADC.
> > +
> > + properties:
> > + reg:
> > + description: The channel number in single-ended mode.
> > + minimum: 0
> > + maximum: 9
> > +
> > + adi,reference:
> > + description: |
> > + Select the reference source to use when converting on
> > + the specific channel. Valid values are:
> > + 0: VREF0P/VREF0N
> > + 1: VREF1P/VREF1N
> > + 2: VREF2P/VREF2N
> > + 3: AVDD/AGND
> > + 4: VREF0P/AGND
> > + 5: VREF1P/AGND
> > + 6: VREF2P/AGND
> > + If this field is left empty, AVDD/AGND is selected.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [0, 1, 2, 3, 4, 5, 6]
> > + default: 3
> > +
> > + adi,input-mode:
> > + description: |
> > + Select signal path of input channels. Valid values are:
> > + 0: Buffered, low-power, unity-gain path (default)
> > + 1: Bypass path
> > + 2: PGA path
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [0, 1, 2]
> > + default: 0
> > +
> > + diff-channels: true
> > +
> > + bipolar: true
> > +
> > + settling-time-us: true
> > +
> > + adi,buffered-vrefp:
> > + description: Enable buffered mode for positive reference.
> > + type: boolean
> > +
> > + adi,buffered-vrefn:
> > + description: Enable buffered mode for negative reference.
> > + type: boolean
> > +
> > + required:
> > + - reg
> > +
> > + additionalProperties: false
> > +
> > +allOf:
> > + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + spi {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + adc@0 {
> > + reg = <0>;
> > + compatible = "adi,max11410";
> > + spi-max-frequency = <8000000>;
> > +
> > + interrupt-parent = <&gpio>;
> > + interrupts = <25 2>;
> > + interrupt-names = "gpio1";
> > +
> > + avdd-supply = <&adc_avdd>;
> > +
> > + vref1p-supply = <&adc_vref1p>;
> > + vref1n-supply = <&adc_vref1n>;
> > +
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + channel@0 {
> > + reg = <0>;
> > + };
> > +
> > + channel@1 {
> > + reg = <1>;
> > + diff-channels = <2 3>;
> > + adi,reference = <1>;
> > + bipolar;
> > + settling-time-us = <100000>;
> > + };
> > +
> > + channel@2 {
> > + reg = <2>;
> > + diff-channels = <7 9>;
> > + adi,reference = <5>;
> > + adi,input-mode = <2>;
> > + settling-time-us = <50000>;
> > + };
> > + };
> > + };

2022-10-03 08:17:02

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v6 2/3] dt-bindings: iio: adc: add adi,max11410.yaml

On 02/10/2022 15:06, Jonathan Cameron wrote:
>>> + interrupts:
>>> + minItems: 1
>>> + maxItems: 2
>>> +
>>> + interrupt-names:
>>> + description: Name of the gpio pin of max11410 used for IRQ
>>> + items:
>>> + - enum:
>>> + - gpio0
>>> + - gpio1
>>
>> This is wrong. You said in interrupts you can have two items, but here
>> you list only one. I don't know what do you want to achieve here.
>
> Aim is 0, 1 or 2 interrupts + knowing which ones they are.
> Device has two pins that have very similar functionality and board
> designers are likely to pick one or the two more or less at random depending
> on which trace is easier to route.
>
> So my guess is this needs minItems, maxItems.

The current choice allows 0 or 1 interrupt. If you want 0-2 then it
should be:

minItems: 1
items:
- enum: [gpio0, gpio1]
- const: gpio1

This one would also work:

minItems: 1
maxItems: 2
items:
enum: [gpio0, gpio1]

but the order of interrupts should be rather defined, so I would prefer
first.


Best regards,
Krzysztof