Hi Rob,

Thank you for reviewing the patch.

My responses in line.

-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: Tuesday 13 July 2021 11:31 PM
To: Anand Ashok Dumbre <redacted>
Cc: jic23@kernel.org; lars@metafoo.de; linux-iio@vger.kernel.org; git-dev
[off-list ref]; Michal Simek [off-list ref];
pmeerw@pmeerw.net; devicetree@vger.kernel.org; linux-
kernel@vger.kernel.org
Subject: Re: [PATCH v6 3/4] dt-bindings: iio: adc: Add Xilinx AMS binding
documentation

On Thu, Jun 24, 2021 at 07:29:38PM +0100, Anand Ashok Dumbre wrote:

quoted

Xilinx AMS have several ADC channels that can be used for measurement
of different voltages and temperatures. Document the same in the

bindings.

quoted

Signed-off-by: Anand Ashok Dumbre <redacted>
---
 .../bindings/iio/adc/xlnx,zynqmp-ams.yaml     | 228 ++++++++++++++++++
 1 file changed, 228 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/iio/adc/xlnx,zynqmp-ams.yaml

diff --git
a/Documentation/devicetree/bindings/iio/adc/xlnx,zynqmp-ams.yaml
b/Documentation/devicetree/bindings/iio/adc/xlnx,zynqmp-ams.yaml
new file mode 100644
index 000000000000..a065ddd55d38

--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/xlnx,zynqmp-ams.yaml

@@ -0,0 +1,228 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/xlnx,zynqmp-ams.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx Zynq Ultrascale AMS controller
+
+maintainers:
+  - Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com>
+
+description: |
+  The AMS (Analog Monitoring System) includes an ADC as well as
+on-chip sensors
+  that can be used to sample external voltages and monitor on-die
+operating
+  conditions, such as temperature and supply voltage levels.
+  The AMS has two SYSMON blocks which are PL (Programmable Logic)
+SYSMON and
+  PS (Processing System) SYSMON.
+  All designs should have AMS registers, but PS and PL are optional.
+The
+  AMS controller can work with only PS, only PL and both PS and PL
+  configurations. Please specify registers according to your design.
+Devicetree
+  should always have AMS module property. Providing PS & PL module is

optional.

quoted

+
+  AMS Channel Details
+  ```````````````````
+  Sysmon Block  |Channel|                       Details

|Measurement

quoted

+                |Number |                                                               |Type
+  -----------------------------------------------------------------------------------------

----------------

quoted

+  AMS CTRL      |0      |System PLLs voltage measurement, VCC_PSPLL.

|Voltage

quoted

+                |1      |Battery voltage measurement, VCC_PSBATT.

|Voltage

quoted

+                |2      |PL Internal voltage measurement, VCCINT.

|Voltage

quoted

+                |3      |Block RAM voltage measurement, VCCBRAM.

|Voltage

quoted

+                |4      |PL Aux voltage measurement, VCCAUX.

|Voltage

quoted

+                |5      |Voltage measurement for six DDR I/O PLLs,

VCC_PSDDR_PLL.       |Voltage

quoted

+                |6      |VCC_PSINTFP_DDR voltage measurement.

|Voltage

quoted

+  -----------------------------------------------------------------------------------------

----------------

quoted

+  PS Sysmon     |7      |LPD temperature measurement.

|Temperature

quoted

+                |8      |FPD temperature measurement (REMOTE).

|Temperature

quoted

+                |9      |VCC PS LPD voltage measurement (supply1).

|Voltage

quoted

+                |10     |VCC PS FPD voltage measurement (supply2).

|Voltage

quoted

+                |11     |PS Aux voltage reference (supply3).                            |Voltage
+                |12     |DDR I/O VCC voltage measurement.

|Voltage

quoted

+                |13     |PS IO Bank 503 voltage measurement (supply5).

|Voltage

quoted

+                |14     |PS IO Bank 500 voltage measurement (supply6).

|Voltage

quoted

+                |15     |VCCO_PSIO1 voltage measurement.

|Voltage

quoted

+                |16     |VCCO_PSIO2 voltage measurement.

|Voltage

quoted

+                |17     |VCC_PS_GTR voltage measurement (VPS_MGTRAVCC).

|Voltage

quoted

+                |18     |VTT_PS_GTR voltage measurement (VPS_MGTRAVTT).

|Voltage

quoted

+                |19     |VCC_PSADC voltage measurement.

|Voltage

quoted

+  -----------------------------------------------------------------------------------------

----------------

quoted

+  PL Sysmon     |20     |PL temperature measurement.

|Temperature

quoted

+                |21     |PL Internal voltage measurement, VCCINT.

|Voltage

quoted

+                |22     |PL Auxiliary voltage measurement, VCCAUX.

|Voltage

quoted

+                |23     |ADC Reference P+ voltage measurement.

|Voltage

quoted

+                |24     |ADC Reference N- voltage measurement.

|Voltage

quoted

+                |25     |PL Block RAM voltage measurement, VCCBRAM.

|Voltage

quoted

+                |26     |LPD Internal voltage measurement, VCC_PSINTLP

(supply4).       |Voltage

quoted

+                |27     |FPD Internal voltage measurement, VCC_PSINTFP

(supply5).       |Voltage

quoted

+                |28     |PS Auxiliary voltage measurement (supply6).

|Voltage

quoted

+                |29     |PL VCCADC voltage measurement (vccams).

|Voltage

quoted

+                |30     |Differential analog input signal voltage measurment.

|Voltage

quoted

+                |31     |VUser0 voltage measurement (supply7).

|Voltage

quoted

+                |32     |VUser1 voltage measurement (supply8).

|Voltage

quoted

+                |33     |VUser2 voltage measurement (supply9).

|Voltage

quoted

+                |34     |VUser3 voltage measurement (supply10).

|Voltage

quoted

+                |35     |Auxiliary ch 0 voltage measurement (VAux0).

|Voltage

quoted

+                |36     |Auxiliary ch 1 voltage measurement (VAux1).

|Voltage

quoted

+                |37     |Auxiliary ch 2 voltage measurement (VAux2).

|Voltage

quoted

+                |38     |Auxiliary ch 3 voltage measurement (VAux3).

|Voltage

quoted

+                |39     |Auxiliary ch 4 voltage measurement (VAux4).

|Voltage

quoted

+                |40     |Auxiliary ch 5 voltage measurement (VAux5).

|Voltage

quoted

+                |41     |Auxiliary ch 6 voltage measurement (VAux6).

|Voltage

quoted

+                |42     |Auxiliary ch 7 voltage measurement (VAux7).

|Voltage

quoted

+                |43     |Auxiliary ch 8 voltage measurement (VAux8).

|Voltage

quoted

+                |44     |Auxiliary ch 9 voltage measurement (VAux9).

|Voltage

quoted

+                |45     |Auxiliary ch 10 voltage measurement (VAux10).

|Voltage

quoted

+                |46     |Auxiliary ch 11 voltage measurement (VAux11).

|Voltage

quoted

+                |47     |Auxiliary ch 12 voltage measurement (VAux12).

|Voltage

quoted

+                |48     |Auxiliary ch 13 voltage measurement (VAux13).

|Voltage

quoted

+                |49     |Auxiliary ch 14 voltage measurement (VAux14).

|Voltage

quoted

+                |50     |Auxiliary ch 15 voltage measurement (VAux15).

|Voltage

quoted

+
+ --------------------------------------------------------------------
+ ------------------------------------
+
+properties:
+  compatible:
+    enum:
+      - xlnx,zynqmp-ams
+
+  interrupts:
+    maxItems: 1
+
+  reg:
+    description: AMS Controller register space
+    maxItems: 1
+
+  ranges:
+    description:
+      Maps the child address space for PS and/or PL.
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 1
+
+  '#io-channel-cells':
+    const: 1
+
+patternProperties:
+  "^ams-ps@0,400$":

If your going to hardcode the unit-address, then it's a fixed string and should
be under 'properties' instead.

Can you point me to an example?
When I tried putting it under just properties, I was not able to do so and was getting errors when I ran check_dt_bindings.
The only way I was not getting the error was by putting it under pattern properties.

My node looked something like this,

   "ams-ps@0":
	type:object
	...
	...

The unit-address is also wrong. This is memory-mapped ranges, right? If so,
this should just be '0' and the next one '400'.

I will fix this.

quoted

+    type: object
+    description: |
+      PS (Processing System) SYSMON is memory mapped to PS. This block

has

quoted

+      built-in alarm generation logic that is used to interrupt the processor
+      based on condition set.
+
+    properties:
+      compatible:
+        enum:
+          - xlnx,zynqmp-ams-ps
+
+      reg:
+        description: Register Space for PS-SYSMON
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+
+    additionalProperties: false
+
+  "^ams-pl@400,400$":
+    type: object
+    description:
+      PL-SYSMON is capable of monitoring off chip voltage and temperature.
+      PL-SYSMON block has DRP, JTAG and I2C interface to enable

monitoring

quoted

+      from external master. Out of this interface currently only DRP is
+      supported. This block has alarm generation logic that is used to
+      interrupt the processor based on condition set.
+
+    properties:
+      compatible:
+        items:
+          - enum:
+              - xlnx,zynqmp-ams-pl
+
+      reg:
+        description: Register Space for PL-SYSMON.
+        maxItems: 1
+
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      "^channel@([2-4][0-9]|50)$":
+        type: object
+        description:
+          Describes the external channels connected.
+
+        properties:
+          reg:
+            description:
+              Pair of pins the channel is connected to. This value is
+              same as Channel Number for a particular channel.
+            minimum: 20
+            maximum: 50
+
+          xlnx,bipolar:

Don't we have a common property for this now?

quoted

+            $ref: /schemas/types.yaml#/definitions/flag
+            type: boolean
+            description:
+              If the set channel is used in bipolar mode.
+
+        required:
+          - reg
+
+        additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        xilinx_ams: ams@ffa50000 {
+            compatible = "xlnx,zynqmp-ams";
+            interrupt-parent = <&gic>;
+            interrupts = <0 56 4>;
+            reg = <0x0 0xffa50000 0x0 0x800>;
+            #address-cells = <1>;
+            #size-cells = <1>;
+            #io-channel-cells = <1>;
+            ranges = <0 0 0xffa50800 0x800>;
+
+            ams_ps: ams-ps@0,400 {
+                compatible = "xlnx,zynqmp-ams-ps";
+                reg = <0 0x400>;
+            };
+
+            ams_pl: ams-pl@400,400 {
+                compatible = "xlnx,zynqmp-ams-pl";
+                reg = <0x400 0x400>;
+                #address-cells = <1>;
+                #size-cells = <0>;
+                channel@30 {
+                    reg = <30>;
+                    xlnx,bipolar;
+                };
+                channel@31 {
+                    reg = <31>;
+                };
+                channel@38 {
+                    reg = <38>;
+                    xlnx,bipolar;
+                };
+            };
+        };
+    };
--
2.17.1