[PATCH V4 2/4] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Jishnu Prakash posted 4 patches 3 weeks, 4 days ago
[PATCH V4 2/4] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC
Posted by Jishnu Prakash 3 weeks, 4 days ago
For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.

It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
going through PBS(Programmable Boot Sequence) firmware through a single
register interface. This interface is implemented on an SDAM (Shared
Direct Access Memory) peripheral on the master PMIC PMK8550 rather
than a dedicated ADC peripheral.

Add documentation for PMIC5 Gen3 ADC and macro definitions for ADC
channels and virtual channels (combination of ADC channel number and
PMIC SID number) per PMIC, to be used by clients of this device.

Co-developed-by: Anjelique Melendez <quic_amelende@quicinc.com>
Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
---
Changes since v3:
- Added ADC5 Gen3 documentation changes in existing qcom,spmi-vadc.yaml file
  instead of adding separate file and updated top-level constraints in documentation
  file based on discussion with reviewers.
- Dropped default SID definitions.
- Addressed other reviewer comments.

Changes since v2:
- Moved ADC5 Gen3 documentation into a separate new file.

Changes since v1:
- Updated properties separately for all compatibles to clarify usage
  of new properties and updates in usage of old properties for ADC5 Gen3.
- Avoided updating 'adc7' name to 'adc5 gen2' and just left a comment
  mentioning this convention.
- Used predefined channel IDs in individual PMIC channel definitions
  instead of numeric IDs.
- Addressed other comments from reviewers.

 .../bindings/iio/adc/qcom,spmi-vadc.yaml      | 220 ++++++++++++++++--
 .../iio/adc/qcom,spmi-adc5-gen3-pm8550.h      |  46 ++++
 .../iio/adc/qcom,spmi-adc5-gen3-pm8550b.h     |  85 +++++++
 .../iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h    |  22 ++
 .../iio/adc/qcom,spmi-adc5-gen3-pmk8550.h     |  52 +++++
 include/dt-bindings/iio/adc/qcom,spmi-vadc.h  |  81 +++++++
 6 files changed, 486 insertions(+), 20 deletions(-)
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h

diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
index a4f72c0c1ec6..e6e1795af886 100644
--- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
@@ -13,8 +13,12 @@ maintainers:
 description: |
   SPMI PMIC voltage ADC (VADC) provides interface to clients to read
   voltage. The VADC is a 15-bit sigma-delta ADC.
-  SPMI PMIC5/PMIC7 voltage ADC (ADC) provides interface to clients to read
-  voltage. The VADC is a 16-bit sigma-delta ADC.
+  SPMI PMIC5/PMIC7/PMIC5 Gen3 voltage ADC voltage ADC (ADC) provides
+  interface to clients to read voltage. The VADC is a 16-bit sigma-delta ADC.
+  Note that PMIC7 ADC is the generation between PMIC5 and PMIC5 Gen3 ADC,
+  it can be considered like PMIC5 Gen2.
+  In addition, PMIC5 Gen3 ADC also performs the same thermal monitoring function
+  as the existing ADC_TM devices.
 
 properties:
   compatible:
@@ -23,14 +27,20 @@ properties:
           - const: qcom,pms405-adc
           - const: qcom,spmi-adc-rev2
       - enum:
-          - qcom,spmi-vadc
-          - qcom,spmi-adc5
           - qcom,spmi-adc-rev2
+          - qcom,spmi-adc5
+          - qcom,spmi-adc5-gen3
           - qcom,spmi-adc7
+          - qcom,spmi-vadc
 
   reg:
-    description: VADC base address in the SPMI PMIC register map
-    maxItems: 1
+    description:
+      For compatible properties "qcom,spmi-vadc", "qcom,spmi-adc5", "qcom,spmi-adc-rev2"
+      and "qcom,spmi-adc7", reg is the VADC base address in the SPMI PMIC register map.
+      For compatible property "qcom,spmi-adc5-gen3", each reg corresponds to an SDAM
+      peripheral base address that is being used for ADC functionality.
+    minItems: 1
+    maxItems: 2
 
   '#address-cells':
     const: 1
@@ -38,20 +48,28 @@ properties:
   '#size-cells':
     const: 0
 
+  "#thermal-sensor-cells":
+    const: 1
+    description:
+      Number of cells required to uniquely identify the thermal sensors.
+      For compatible property "qcom,spmi-adc5-gen3", this property is
+      required for if any channels under it are used for ADC_TM.
+      Since we have multiple sensors this is set to 1.
+
   '#io-channel-cells':
     const: 1
 
   interrupts:
-    maxItems: 1
     description:
       End of conversion interrupt.
+      For compatible property "qcom,spmi-adc5-gen3", interrupts are defined
+      for each SDAM being used.
+    minItems: 1
+    maxItems: 2
 
-required:
-  - compatible
-  - reg
-  - '#address-cells'
-  - '#size-cells'
-  - '#io-channel-cells'
+  interrupt-names:
+    minItems: 1
+    maxItems: 2
 
 patternProperties:
   "^channel@[0-9a-f]+$":
@@ -71,8 +89,8 @@ patternProperties:
         description: |
           ADC channel number.
           See include/dt-bindings/iio/adc/qcom,spmi-vadc.h
-          For PMIC7 ADC, the channel numbers are specified separately per PMIC
-          in the PMIC-specific files in include/dt-bindings/iio/adc.
+          For PMIC7 and PMIC5 Gen3 ADC, the channel numbers are specified separately
+          per PMIC in the PMIC-specific files in include/dt-bindings/iio/adc.
 
       label:
         description: |
@@ -113,11 +131,11 @@ patternProperties:
               channel calibration. If property is not found, channel will be
               calibrated with 0.625V and 1.25V reference channels, also
               known as absolute calibration.
-            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc7" and
-              "qcom,spmi-adc-rev2", if this property is specified VADC will use
-              the VDD reference (1.875V) and GND for channel calibration. If
-              property is not found, channel will be calibrated with 0V and 1.25V
-              reference channels, also known as absolute calibration.
+            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc7",
+              "qcom,spmi-adc-rev2" and "qcom,spmi-adc5-gen3", if this property is
+              specified VADC will use the VDD reference (1.875V) and GND for channel
+              calibration. If property is not found, channel will be calibrated with
+              0V and 1.25V reference channels, also known as absolute calibration.
         type: boolean
 
       qcom,hw-settle-time:
@@ -135,9 +153,24 @@ patternProperties:
             from the ADC that is an average of multiple samples. The value
             selected is 2^(value).
 
+      qcom,adc-tm:
+        description:
+          Indicates if ADC_TM monitoring is done on this channel.
+          Defined for compatible property "qcom,spmi-adc5-gen3".
+          This is the same functionality as in the existing QCOM ADC_TM
+          device, documented at devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml.
+        type: boolean
+
     required:
       - reg
 
+required:
+  - compatible
+  - reg
+  - '#address-cells'
+  - '#size-cells'
+  - '#io-channel-cells'
+
 allOf:
   - if:
       properties:
@@ -146,6 +179,15 @@ allOf:
             const: qcom,spmi-vadc
 
     then:
+      properties:
+        reg:
+          minItems: 1
+          maxItems: 1
+        interrupts:
+          minItems: 1
+          maxItems: 1
+        "#thermal-sensor-cells": false
+        interrupt-names: false
       patternProperties:
         "^channel@[0-9a-f]+$":
           properties:
@@ -162,6 +204,8 @@ allOf:
               enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512 ]
               default: 1
 
+            qcom,adc-tm: false
+
   - if:
       properties:
         compatible:
@@ -169,6 +213,15 @@ allOf:
             const: qcom,spmi-adc-rev2
 
     then:
+      properties:
+        reg:
+          minItems: 1
+          maxItems: 1
+        interrupts:
+          minItems: 1
+          maxItems: 1
+        "#thermal-sensor-cells": false
+        interrupt-names: false
       patternProperties:
         "^channel@[0-9a-f]+$":
           properties:
@@ -185,6 +238,8 @@ allOf:
               enum: [ 1, 2, 4, 8, 16 ]
               default: 1
 
+            qcom,adc-tm: false
+
   - if:
       properties:
         compatible:
@@ -192,6 +247,15 @@ allOf:
             const: qcom,spmi-adc5
 
     then:
+      properties:
+        reg:
+          minItems: 1
+          maxItems: 1
+        interrupts:
+          minItems: 1
+          maxItems: 1
+        "#thermal-sensor-cells": false
+        interrupt-names: false
       patternProperties:
         "^channel@[0-9a-f]+$":
           properties:
@@ -208,6 +272,8 @@ allOf:
               enum: [ 1, 2, 4, 8, 16 ]
               default: 1
 
+            qcom,adc-tm: false
+
   - if:
       properties:
         compatible:
@@ -215,6 +281,59 @@ allOf:
             const: qcom,spmi-adc7
 
     then:
+      properties:
+        reg:
+          minItems: 1
+          maxItems: 1
+        interrupts:
+          minItems: 1
+          maxItems: 1
+        "#thermal-sensor-cells": false
+        interrupt-names: false
+      patternProperties:
+        "^channel@[0-9a-f]+$":
+          properties:
+            qcom,decimation:
+              enum: [ 85, 340, 1360 ]
+              default: 1360
+
+            qcom,hw-settle-time:
+              enum: [ 15, 100, 200, 300, 400, 500, 600, 700, 1000, 2000, 4000,
+                      8000, 16000, 32000, 64000, 128000 ]
+              default: 15
+
+            qcom,avg-samples:
+              enum: [ 1, 2, 4, 8, 16 ]
+              default: 1
+
+            qcom,adc-tm: false
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: qcom,spmi-adc5-gen3
+
+    then:
+      properties:
+        reg:
+          minItems: 1
+          items:
+            - description: SDAM0 base address in the SPMI PMIC register map
+            - description: SDAM1 base address
+        interrupts:
+          minItems: 1
+          items:
+            - description: SDAM0 end of conversion (EOC) interrupt
+            - description: SDAM1 EOC interrupt
+        interrupt-names:
+          minItems: 1
+          items:
+            - const: adc-sdam0
+            - const: adc-sdam1
+      required:
+        - interrupts
+        - interrupt-names
       patternProperties:
         "^channel@[0-9a-f]+$":
           properties:
@@ -307,3 +426,64 @@ examples:
             };
         };
     };
+
+  - |
+    #include <dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h>
+    #include <dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h>
+    #include <dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h>
+    #include <dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    pmic {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      adc@9000 {
+        compatible = "qcom,spmi-adc5-gen3";
+        reg = <0x9000>, <0x9100>;
+        interrupts = <0x0 0x90 0x1 IRQ_TYPE_EDGE_RISING>,
+                      <0x0 0x91 0x1 IRQ_TYPE_EDGE_RISING>;
+        interrupt-names = "adc-sdam0", "adc-sdam1";
+        #address-cells = <1>;
+        #size-cells = <0>;
+        #io-channel-cells = <1>;
+        #thermal-sensor-cells = <1>;
+
+        /* PMK8550 Channel nodes */
+        channel@3 {
+          reg = <PMK8550_ADC5_GEN3_DIE_TEMP(0)>;
+          label = "pmk8550_die_temp";
+          qcom,pre-scaling = <1 1>;
+        };
+
+        channel@44 {
+          reg = <PMK8550_ADC5_GEN3_AMUX_THM1_XO_THERM_100K_PU(0)>;
+          label = "pmk8550_xo_therm";
+          qcom,pre-scaling = <1 1>;
+          qcom,ratiometric;
+          qcom,hw-settle-time = <200>;
+          qcom,adc-tm;
+        };
+
+        /* PM8550 Channel nodes */
+        channel@103 {
+          reg = <PM8550_ADC5_GEN3_DIE_TEMP(1)>;
+          label = "pm8550_die_temp";
+          qcom,pre-scaling = <1 1>;
+        };
+
+        /* PM8550B Channel nodes */
+        channel@78f {
+          reg = <PM8550B_ADC5_GEN3_VBAT_SNS_QBG(7)>;
+          label = "pm8550b_vbat_sns_qbg";
+          qcom,pre-scaling = <1 3>;
+        };
+
+        /* PM8550VS_C Channel nodes */
+        channel@203 {
+          reg = <PM8550VS_ADC5_GEN3_DIE_TEMP(2)>;
+          label = "pm8550vs_c_die_temp";
+          qcom,pre-scaling = <1 1>;
+        };
+      };
+    };
diff --git a/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
new file mode 100644
index 000000000000..21cc607ce40e
--- /dev/null
+++ b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_QCOM_SPMI_VADC_PM8550_H
+#define _DT_BINDINGS_QCOM_SPMI_VADC_PM8550_H
+
+#include <dt-bindings/iio/adc/qcom,spmi-vadc.h>
+
+/* ADC channels for PM8550_ADC for PMIC5 Gen3 */
+#define PM8550_ADC5_GEN3_REF_GND(sid)			((sid) << 8 | ADC5_GEN3_REF_GND)
+#define PM8550_ADC5_GEN3_1P25VREF(sid)			((sid) << 8 | ADC5_GEN3_1P25VREF)
+#define PM8550_ADC5_GEN3_VREF_VADC(sid)			((sid) << 8 | ADC5_GEN3_VREF_VADC)
+#define PM8550_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | ADC5_GEN3_DIE_TEMP)
+
+#define PM8550_ADC5_GEN3_AMUX_THM1(sid)			((sid) << 8 | ADC5_GEN3_AMUX1_THM)
+#define PM8550_ADC5_GEN3_AMUX_THM2(sid)			((sid) << 8 | ADC5_GEN3_AMUX2_THM)
+#define PM8550_ADC5_GEN3_AMUX_THM3(sid)			((sid) << 8 | ADC5_GEN3_AMUX3_THM)
+#define PM8550_ADC5_GEN3_AMUX_THM4(sid)			((sid) << 8 | ADC5_GEN3_AMUX4_THM)
+#define PM8550_ADC5_GEN3_AMUX_THM5(sid)			((sid) << 8 | ADC5_GEN3_AMUX5_THM)
+#define PM8550_ADC5_GEN3_AMUX_THM6_GPIO2(sid)		((sid) << 8 | ADC5_GEN3_AMUX6_THM)
+#define PM8550_ADC5_GEN3_AMUX1_GPIO3(sid)		((sid) << 8 | ADC5_GEN3_AMUX1_GPIO)
+#define PM8550_ADC5_GEN3_AMUX2_GPIO4(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_GPIO)
+#define PM8550_ADC5_GEN3_AMUX3_GPIO7(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_GPIO)
+#define PM8550_ADC5_GEN3_AMUX4_GPIO12(sid)		((sid) << 8 | ADC5_GEN3_AMUX4_GPIO)
+
+/* 100k pull-up */
+#define PM8550_ADC5_GEN3_AMUX_THM1_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX1_THM_100K_PU)
+#define PM8550_ADC5_GEN3_AMUX_THM2_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_THM_100K_PU)
+#define PM8550_ADC5_GEN3_AMUX_THM3_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_THM_100K_PU)
+#define PM8550_ADC5_GEN3_AMUX_THM4_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX4_THM_100K_PU)
+#define PM8550_ADC5_GEN3_AMUX_THM5_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX5_THM_100K_PU)
+#define PM8550_ADC5_GEN3_AMUX_THM6_GPIO2_100K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX6_THM_100K_PU)
+#define PM8550_ADC5_GEN3_AMUX1_GPIO3_100K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX1_GPIO_100K_PU)
+#define PM8550_ADC5_GEN3_AMUX2_GPIO4_100K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX2_GPIO_100K_PU)
+#define PM8550_ADC5_GEN3_AMUX3_GPIO7_100K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX3_GPIO_100K_PU)
+#define PM8550_ADC5_GEN3_AMUX4_GPIO12_100K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX4_GPIO_100K_PU)
+
+/* 1/3 Divider */
+#define PM8550_ADC5_GEN3_AMUX3_GPIO7_DIV3(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_GPIO_DIV3)
+#define PM8550_ADC5_GEN3_AMUX4_GPIO12_DIV3(sid)		((sid) << 8 | ADC5_GEN3_AMUX4_GPIO_DIV3)
+
+#define PM8550_ADC5_GEN3_VPH_PWR(sid)			((sid) << 8 | ADC5_GEN3_VPH_PWR)
+
+#endif /* _DT_BINDINGS_QCOM_SPMI_VADC_PM8550_H */
diff --git a/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
new file mode 100644
index 000000000000..ae5677a26434
--- /dev/null
+++ b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_QCOM_SPMI_VADC_PM8550B_H
+#define _DT_BINDINGS_QCOM_SPMI_VADC_PM8550B_H
+
+#include <dt-bindings/iio/adc/qcom,spmi-vadc.h>
+
+/* ADC channels for PM8550B_ADC for PMIC5 Gen3 */
+#define PM8550B_ADC5_GEN3_REF_GND(sid)			((sid) << 8 | ADC5_GEN3_REF_GND)
+#define PM8550B_ADC5_GEN3_1P25VREF(sid)			((sid) << 8 | ADC5_GEN3_1P25VREF)
+#define PM8550B_ADC5_GEN3_VREF_VADC(sid)		((sid) << 8 | ADC5_GEN3_VREF_VADC)
+#define PM8550B_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | ADC5_GEN3_DIE_TEMP)
+
+#define PM8550B_ADC5_GEN3_AMUX_THM1_BATT_THERM(sid)	((sid) << 8 | ADC5_GEN3_AMUX1_THM)
+#define PM8550B_ADC5_GEN3_AMUX_THM2_BATT_ID(sid)	((sid) << 8 | ADC5_GEN3_AMUX2_THM)
+#define PM8550B_ADC5_GEN3_AMUX_THM3_SMB_TEMP_V(sid)	((sid) << 8 | ADC5_GEN3_AMUX3_THM)
+#define PM8550B_ADC5_GEN3_AMUX_THM4_USB_THERM(sid)	((sid) << 8 | ADC5_GEN3_AMUX4_THM)
+#define PM8550B_ADC5_GEN3_AMUX_THM5_OPTION(sid)		((sid) << 8 | ADC5_GEN3_AMUX5_THM)
+#define PM8550B_ADC5_GEN3_AMUX_THM6_GPIO10(sid)		((sid) << 8 | ADC5_GEN3_AMUX6_THM)
+#define PM8550B_ADC5_GEN3_AMUX1_GPIO1(sid)		((sid) << 8 | ADC5_GEN3_AMUX1_GPIO)
+#define PM8550B_ADC5_GEN3_AMUX2_GPIO5(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_GPIO)
+#define PM8550B_ADC5_GEN3_AMUX3_GPIO6(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_GPIO)
+#define PM8550B_ADC5_GEN3_AMUX4_GPIO12(sid)		((sid) << 8 | ADC5_GEN3_AMUX4_GPIO)
+
+#define PM8550B_ADC5_GEN3_CHG_TEMP(sid)			((sid) << 8 | ADC5_GEN3_CHG_TEMP)
+#define PM8550B_ADC5_GEN3_USB_SNS_V_16(sid)		((sid) << 8 | ADC5_GEN3_USB_SNS_V_16)
+#define PM8550B_ADC5_GEN3_VIN_DIV16_MUX(sid)		((sid) << 8 | ADC5_GEN3_VIN_DIV16_MUX)
+#define PM8550B_ADC5_GEN3_VREF_BAT_THERM(sid)		((sid) << 8 | ADC5_GEN3_VREF_BAT_THERM)
+#define PM8550B_ADC5_GEN3_IIN_FB(sid)			((sid) << 8 | ADC5_GEN3_IIN_FB)
+#define PM8550B_ADC5_GEN3_TEMP_ALARM_LITE(sid)		((sid) << 8 | ADC5_GEN3_TEMP_ALARM_LITE)
+#define PM8550B_ADC5_GEN3_SMB_IIN(sid)			((sid) << 8 | ADC5_GEN3_IIN_SMB)
+#define PM8550B_ADC5_GEN3_SMB_ICHG(sid)			((sid) << 8 | ADC5_GEN3_ICHG_SMB)
+#define PM8550B_ADC5_GEN3_ICHG_FB(sid)			((sid) << 8 | ADC5_GEN3_ICHG_FB)
+
+/* 30k pull-up */
+#define PM8550B_ADC5_GEN3_AMUX_THM1_BATT_THERM_30K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX1_THM_30K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM2_BATT_ID_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_THM_30K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM3_SMB_TEMP_V_30K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX3_THM_30K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM4_USB_THERM_30K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX4_THM_30K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM5_OPTION_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX5_THM_30K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM6_GPIO10_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX6_THM_30K_PU)
+#define PM8550B_ADC5_GEN3_AMUX1_GPIO1_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX1_GPIO_30K_PU)
+#define PM8550B_ADC5_GEN3_AMUX2_GPIO5_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_GPIO_30K_PU)
+#define PM8550B_ADC5_GEN3_AMUX3_GPIO6_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_GPIO_30K_PU)
+#define PM8550B_ADC5_GEN3_AMUX4_GPIO12_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX4_GPIO_30K_PU)
+
+/* 100k pull-up */
+#define PM8550B_ADC5_GEN3_AMUX_THM1_BATT_THERM_100K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX1_THM_100K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM2_BATT_ID_100K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX2_THM_100K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM3_SMB_TEMP_V_100K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX3_THM_100K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM4_USB_THERM_100K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX4_THM_100K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM5_OPTION_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX5_THM_100K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM6_GPIO10_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX6_THM_100K_PU)
+#define PM8550B_ADC5_GEN3_AMUX1_GPIO1_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX1_GPIO_100K_PU)
+#define PM8550B_ADC5_GEN3_AMUX2_GPIO5_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_GPIO_100K_PU)
+#define PM8550B_ADC5_GEN3_AMUX3_GPIO6_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_GPIO_100K_PU)
+#define PM8550B_ADC5_GEN3_AMUX4_GPIO12_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX4_GPIO_100K_PU)
+
+/* 400k pull-up */
+#define PM8550B_ADC5_GEN3_AMUX_THM1_BATT_THERM_400K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX1_THM_400K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM2_BATT_ID_400K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX2_THM_400K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM3_SMB_TEMP_V_400K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX3_THM_400K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM4_USB_THERM_400K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX4_THM_400K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM5_OPTION_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX5_THM_400K_PU)
+#define PM8550B_ADC5_GEN3_AMUX_THM6_GPIO10_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX6_THM_400K_PU)
+#define PM8550B_ADC5_GEN3_AMUX1_GPIO1_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX1_GPIO_400K_PU)
+#define PM8550B_ADC5_GEN3_AMUX2_GPIO5_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_GPIO_400K_PU)
+#define PM8550B_ADC5_GEN3_AMUX3_GPIO6_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_GPIO_400K_PU)
+#define PM8550B_ADC5_GEN3_AMUX4_GPIO12_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX4_GPIO_400K_PU)
+
+/* 1/3 Divider */
+#define PM8550B_ADC5_GEN3_AMUX1_GPIO1_DIV3(sid)		((sid) << 8 | ADC5_GEN3_AMUX1_GPIO_DIV3)
+#define PM8550B_ADC5_GEN3_AMUX2_GPIO5_DIV3(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_GPIO_DIV3)
+#define PM8550B_ADC5_GEN3_AMUX3_GPIO6_DIV3(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_GPIO_DIV3)
+
+#define PM8550B_ADC5_GEN3_VPH_PWR(sid)			((sid) << 8 | ADC5_GEN3_VPH_PWR)
+#define PM8550B_ADC5_GEN3_VBAT_SNS_QBG(sid)		((sid) << 8 | ADC5_GEN3_VBAT_SNS_QBG)
+#define PM8550B_ADC5_GEN3_VBAT_SNS_CHGR(sid)		((sid) << 8 | ADC5_GEN3_VBAT_SNS_CHGR)
+#define PM8550B_ADC5_GEN3_VBAT_2S_MID_QBG(sid)		((sid) << 8 | ADC5_GEN3_VBAT_2S_MID_QBG)
+#define PM8550B_ADC5_GEN3_VBAT_2S_MID_CHGR(sid)		((sid) << 8 | ADC5_GEN3_VBAT_2S_MID_CHGR)
+
+#endif /* _DT_BINDINGS_QCOM_SPMI_VADC_PM8550B_H */
diff --git a/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h
new file mode 100644
index 000000000000..c6d2857d5c0c
--- /dev/null
+++ b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_QCOM_SPMI_VADC_PM8550VX_H
+#define _DT_BINDINGS_QCOM_SPMI_VADC_PM8550VX_H
+
+#include <dt-bindings/iio/adc/qcom,spmi-vadc.h>
+
+/* ADC channels for PM8550VX_ADC for PMIC5 Gen3 */
+#define PM8550VS_ADC5_GEN3_REF_GND(sid)			((sid) << 8 | ADC5_GEN3_REF_GND)
+#define PM8550VS_ADC5_GEN3_1P25VREF(sid)			((sid) << 8 | ADC5_GEN3_1P25VREF)
+#define PM8550VS_ADC5_GEN3_VREF_VADC(sid)			((sid) << 8 | ADC5_GEN3_VREF_VADC)
+#define PM8550VS_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | ADC5_GEN3_DIE_TEMP)
+
+#define PM8550VE_ADC5_GEN3_OFFSET_REF(sid)			((sid) << 8 | ADC5_GEN3_REF_GND)
+#define PM8550VE_ADC5_GEN3_1P25VREF(sid)			((sid) << 8 | ADC5_GEN3_1P25VREF)
+#define PM8550VE_ADC5_GEN3_VREF_VADC(sid)			((sid) << 8 | ADC5_GEN3_VREF_VADC)
+#define PM8550VE_ADC5_GEN3_DIE_TEMP(sid)		((sid) << 8 | ADC5_GEN3_DIE_TEMP)
+
+#endif /* _DT_BINDINGS_QCOM_SPMI_VADC_PM8550VX_H */
diff --git a/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
new file mode 100644
index 000000000000..2e43f7e3acb8
--- /dev/null
+++ b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_QCOM_SPMI_VADC_PMK8550_H
+#define _DT_BINDINGS_QCOM_SPMI_VADC_PMK8550_H
+
+#include <dt-bindings/iio/adc/qcom,spmi-vadc.h>
+
+/* ADC channels for PMK8550_ADC for PMIC5 Gen3 */
+#define PMK8550_ADC5_GEN3_REF_GND(sid)			((sid) << 8 | ADC5_GEN3_REF_GND)
+#define PMK8550_ADC5_GEN3_1P25VREF(sid)			((sid) << 8 | ADC5_GEN3_1P25VREF)
+#define PMK8550_ADC5_GEN3_VREF_VADC(sid)		((sid) << 8 | ADC5_GEN3_VREF_VADC)
+#define PMK8550_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | ADC5_GEN3_DIE_TEMP)
+
+#define PMK8550_ADC5_GEN3_AMUX_THM1_XO_THERM(sid)	((sid) << 8 | ADC5_GEN3_AMUX1_THM)
+#define PMK8550_ADC5_GEN3_AMUX_THM2_GPIO1(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_THM)
+#define PMK8550_ADC5_GEN3_AMUX_THM3_GPIO2(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_THM)
+#define PMK8550_ADC5_GEN3_AMUX_THM4_GPIO3(sid)		((sid) << 8 | ADC5_GEN3_AMUX4_THM)
+#define PMK8550_ADC5_GEN3_AMUX_THM5_GPIO4(sid)		((sid) << 8 | ADC5_GEN3_AMUX5_THM)
+#define PMK8550_ADC5_GEN3_AMUX_THM6_GPIO5(sid)		((sid) << 8 | ADC5_GEN3_AMUX6_THM)
+#define PMK8550_ADC5_GEN3_AMUX1_GPIO6(sid)		((sid) << 8 | ADC5_GEN3_AMUX1_GPIO)
+
+/* 30k pull-up */
+#define PMK8550_ADC5_GEN3_AMUX_THM1_XO_THERM_30K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX1_THM_30K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM2_GPIO1_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_THM_30K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM3_GPIO2_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_THM_30K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM4_GPIO3_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX4_THM_30K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM5_GPIO4_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX5_THM_30K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM6_GPIO5_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX6_THM_30K_PU)
+#define PMK8550_ADC5_GEN3_AMUX1_GPIO6_30K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX1_GPIO_30K_PU)
+
+/* 100k pull-up */
+#define PMK8550_ADC5_GEN3_AMUX_THM1_XO_THERM_100K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX1_THM_100K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM2_GPIO1_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_THM_100K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM3_GPIO2_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_THM_100K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM4_GPIO3_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX4_THM_100K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM5_GPIO4_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX5_THM_100K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM6_GPIO5_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX6_THM_100K_PU)
+#define PMK8550_ADC5_GEN3_AMUX1_GPIO6_100K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX1_GPIO_100K_PU)
+
+/* 400k pull-up */
+#define PMK8550_ADC5_GEN3_AMUX_THM1_XO_THERM_400K_PU(sid)	((sid) << 8 | ADC5_GEN3_AMUX1_THM_400K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM2_GPIO1_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX2_THM_400K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM3_GPIO2_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX3_THM_400K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM4_GPIO3_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX4_THM_400K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM5_GPIO4_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX5_THM_400K_PU)
+#define PMK8550_ADC5_GEN3_AMUX_THM6_GPIO5_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX6_THM_400K_PU)
+#define PMK8550_ADC5_GEN3_AMUX1_GPIO6_400K_PU(sid)		((sid) << 8 | ADC5_GEN3_AMUX1_GPIO_400K_PU)
+
+#endif /* _DT_BINDINGS_QCOM_SPMI_VADC_PMK8550_H */
diff --git a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
index ef07ecd4d585..ed786f6c3425 100644
--- a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
+++ b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
@@ -1,6 +1,8 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /*
  * Copyright (c) 2012-2014,2018,2020 The Linux Foundation. All rights reserved.
+ *
+ * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #ifndef _DT_BINDINGS_QCOM_SPMI_VADC_H
@@ -300,4 +302,83 @@
 #define ADC7_SBUx				0x94
 #define ADC7_VBAT_2S_MID			0x96
 
+/* ADC channels for PMIC5 Gen3 */
+
+#define ADC5_GEN3_REF_GND			0x00
+#define ADC5_GEN3_1P25VREF			0x01
+#define ADC5_GEN3_VREF_VADC			0x02
+#define ADC5_GEN3_DIE_TEMP			0x03
+
+#define ADC5_GEN3_AMUX1_THM			0x04
+#define ADC5_GEN3_AMUX2_THM			0x05
+#define ADC5_GEN3_AMUX3_THM			0x06
+#define ADC5_GEN3_AMUX4_THM			0x07
+#define ADC5_GEN3_AMUX5_THM			0x08
+#define ADC5_GEN3_AMUX6_THM			0x09
+#define ADC5_GEN3_AMUX1_GPIO			0x0a
+#define ADC5_GEN3_AMUX2_GPIO			0x0b
+#define ADC5_GEN3_AMUX3_GPIO			0x0c
+#define ADC5_GEN3_AMUX4_GPIO			0x0d
+
+#define ADC5_GEN3_CHG_TEMP			0x10
+#define ADC5_GEN3_USB_SNS_V_16			0x11
+#define ADC5_GEN3_VIN_DIV16_MUX			0x12
+#define ADC5_GEN3_VREF_BAT_THERM		0x15
+#define ADC5_GEN3_IIN_FB			0x17
+#define ADC5_GEN3_TEMP_ALARM_LITE		0x18
+#define ADC5_GEN3_IIN_SMB			0x19
+#define ADC5_GEN3_ICHG_SMB			0x1b
+#define ADC5_GEN3_ICHG_FB			0xa1
+
+/* 30k pull-up1 */
+#define ADC5_GEN3_AMUX1_THM_30K_PU		0x24
+#define ADC5_GEN3_AMUX2_THM_30K_PU		0x25
+#define ADC5_GEN3_AMUX3_THM_30K_PU		0x26
+#define ADC5_GEN3_AMUX4_THM_30K_PU		0x27
+#define ADC5_GEN3_AMUX5_THM_30K_PU		0x28
+#define ADC5_GEN3_AMUX6_THM_30K_PU		0x29
+#define ADC5_GEN3_AMUX1_GPIO_30K_PU		0x2a
+#define ADC5_GEN3_AMUX2_GPIO_30K_PU		0x2b
+#define ADC5_GEN3_AMUX3_GPIO_30K_PU		0x2c
+#define ADC5_GEN3_AMUX4_GPIO_30K_PU		0x2d
+
+/* 100k pull-up2 */
+#define ADC5_GEN3_AMUX1_THM_100K_PU		0x44
+#define ADC5_GEN3_AMUX2_THM_100K_PU		0x45
+#define ADC5_GEN3_AMUX3_THM_100K_PU		0x46
+#define ADC5_GEN3_AMUX4_THM_100K_PU		0x47
+#define ADC5_GEN3_AMUX5_THM_100K_PU		0x48
+#define ADC5_GEN3_AMUX6_THM_100K_PU		0x49
+#define ADC5_GEN3_AMUX1_GPIO_100K_PU		0x4a
+#define ADC5_GEN3_AMUX2_GPIO_100K_PU		0x4b
+#define ADC5_GEN3_AMUX3_GPIO_100K_PU		0x4c
+#define ADC5_GEN3_AMUX4_GPIO_100K_PU		0x4d
+
+/* 400k pull-up3 */
+#define ADC5_GEN3_AMUX1_THM_400K_PU		0x64
+#define ADC5_GEN3_AMUX2_THM_400K_PU		0x65
+#define ADC5_GEN3_AMUX3_THM_400K_PU		0x66
+#define ADC5_GEN3_AMUX4_THM_400K_PU		0x67
+#define ADC5_GEN3_AMUX5_THM_400K_PU		0x68
+#define ADC5_GEN3_AMUX6_THM_400K_PU		0x69
+#define ADC5_GEN3_AMUX1_GPIO_400K_PU		0x6a
+#define ADC5_GEN3_AMUX2_GPIO_400K_PU		0x6b
+#define ADC5_GEN3_AMUX3_GPIO_400K_PU		0x6c
+#define ADC5_GEN3_AMUX4_GPIO_400K_PU		0x6d
+
+/* 1/3 Divider */
+#define ADC5_GEN3_AMUX1_GPIO_DIV3		0x8a
+#define ADC5_GEN3_AMUX2_GPIO_DIV3		0x8b
+#define ADC5_GEN3_AMUX3_GPIO_DIV3		0x8c
+#define ADC5_GEN3_AMUX4_GPIO_DIV3		0x8d
+
+#define ADC5_GEN3_VPH_PWR			0x8e
+#define ADC5_GEN3_VBAT_SNS_QBG			0x8f
+
+#define ADC5_GEN3_VBAT_SNS_CHGR			0x94
+#define ADC5_GEN3_VBAT_2S_MID_QBG		0x96
+#define ADC5_GEN3_VBAT_2S_MID_CHGR		0x9d
+
+#define ADC5_GEN3_OFFSET_EXT2			0xf8
+
 #endif /* _DT_BINDINGS_QCOM_SPMI_VADC_H */
-- 
2.25.1
Re: [PATCH V4 2/4] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC
Posted by Dmitry Baryshkov 3 weeks, 3 days ago
On Thu, Oct 31, 2024 at 12:28:52AM +0530, Jishnu Prakash wrote:
> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
> 
> It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
> going through PBS(Programmable Boot Sequence) firmware through a single
> register interface. This interface is implemented on an SDAM (Shared
> Direct Access Memory) peripheral on the master PMIC PMK8550 rather
> than a dedicated ADC peripheral.
> 
> Add documentation for PMIC5 Gen3 ADC and macro definitions for ADC
> channels and virtual channels (combination of ADC channel number and
> PMIC SID number) per PMIC, to be used by clients of this device.
> 
> Co-developed-by: Anjelique Melendez <quic_amelende@quicinc.com>
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
> ---
> Changes since v3:
> - Added ADC5 Gen3 documentation changes in existing qcom,spmi-vadc.yaml file
>   instead of adding separate file and updated top-level constraints in documentation
>   file based on discussion with reviewers.

I think it has been better, when it was a separate file. Krzysztof asked
for rationale, not for merging it back. Two different things.

> - Dropped default SID definitions.
> - Addressed other reviewer comments.
> 
> Changes since v2:
> - Moved ADC5 Gen3 documentation into a separate new file.
> 
> Changes since v1:
> - Updated properties separately for all compatibles to clarify usage
>   of new properties and updates in usage of old properties for ADC5 Gen3.
> - Avoided updating 'adc7' name to 'adc5 gen2' and just left a comment
>   mentioning this convention.
> - Used predefined channel IDs in individual PMIC channel definitions
>   instead of numeric IDs.
> - Addressed other comments from reviewers.
> 

-- 
With best wishes
Dmitry
Re: [PATCH V4 2/4] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC
Posted by Jishnu Prakash 1 week, 4 days ago
Hi Dmitry,

On 10/31/2024 11:27 PM, Dmitry Baryshkov wrote:
> On Thu, Oct 31, 2024 at 12:28:52AM +0530, Jishnu Prakash wrote:
>> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
>> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
>>
>> It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
>> going through PBS(Programmable Boot Sequence) firmware through a single
>> register interface. This interface is implemented on an SDAM (Shared
>> Direct Access Memory) peripheral on the master PMIC PMK8550 rather
>> than a dedicated ADC peripheral.
>>
>> Add documentation for PMIC5 Gen3 ADC and macro definitions for ADC
>> channels and virtual channels (combination of ADC channel number and
>> PMIC SID number) per PMIC, to be used by clients of this device.
>>
>> Co-developed-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
>> ---
>> Changes since v3:
>> - Added ADC5 Gen3 documentation changes in existing qcom,spmi-vadc.yaml file
>>   instead of adding separate file and updated top-level constraints in documentation
>>   file based on discussion with reviewers.
> 
> I think it has been better, when it was a separate file. Krzysztof asked
> for rationale, not for merging it back. Two different things.

Actually I made that change in a separate file due to a misunderstanding at that time - 
I thought a separate file was the only way to accommodate a change in the top-level 'reg' and 'interrupts'
constraints, but I realized later that they could be updated.

From our side, we would prefer to add ADC5 Gen3 documentation in the same file, as it is
mostly the same functionality which reuses all the existing properties present in this file.

Thanks,
Jishnu

> 
>> - Dropped default SID definitions.
>> - Addressed other reviewer comments.
>>
>> Changes since v2:
>> - Moved ADC5 Gen3 documentation into a separate new file.
>>
>> Changes since v1:
>> - Updated properties separately for all compatibles to clarify usage
>>   of new properties and updates in usage of old properties for ADC5 Gen3.
>> - Avoided updating 'adc7' name to 'adc5 gen2' and just left a comment
>>   mentioning this convention.
>> - Used predefined channel IDs in individual PMIC channel definitions
>>   instead of numeric IDs.
>> - Addressed other comments from reviewers.
>>
>
Re: [PATCH V4 2/4] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC
Posted by Dmitry Baryshkov 1 week, 2 days ago
On Wed, Nov 13, 2024 at 07:36:13PM +0530, Jishnu Prakash wrote:
> Hi Dmitry,
> 
> On 10/31/2024 11:27 PM, Dmitry Baryshkov wrote:
> > On Thu, Oct 31, 2024 at 12:28:52AM +0530, Jishnu Prakash wrote:
> >> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
> >> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
> >>
> >> It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
> >> going through PBS(Programmable Boot Sequence) firmware through a single
> >> register interface. This interface is implemented on an SDAM (Shared
> >> Direct Access Memory) peripheral on the master PMIC PMK8550 rather
> >> than a dedicated ADC peripheral.
> >>
> >> Add documentation for PMIC5 Gen3 ADC and macro definitions for ADC
> >> channels and virtual channels (combination of ADC channel number and
> >> PMIC SID number) per PMIC, to be used by clients of this device.
> >>
> >> Co-developed-by: Anjelique Melendez <quic_amelende@quicinc.com>
> >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> >> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
> >> ---
> >> Changes since v3:
> >> - Added ADC5 Gen3 documentation changes in existing qcom,spmi-vadc.yaml file
> >>   instead of adding separate file and updated top-level constraints in documentation
> >>   file based on discussion with reviewers.
> > 
> > I think it has been better, when it was a separate file. Krzysztof asked
> > for rationale, not for merging it back. Two different things.
> 
> Actually I made that change in a separate file due to a misunderstanding at that time - 
> I thought a separate file was the only way to accommodate a change in the top-level 'reg' and 'interrupts'
> constraints, but I realized later that they could be updated.
> 
> From our side, we would prefer to add ADC5 Gen3 documentation in the same file, as it is
> mostly the same functionality which reuses all the existing properties present in this file.

Export the existing properties and reuse them in the new file. Gen3 (in
my opinion) changed the hardware too much. Having all the differences
via conditionals bloats the schema and makes it significantly unreadable
in my opinion.

But please refer to DT maintainers (Rob/Krzysztof/Conor) for the final
opinion.

-- 
With best wishes
Dmitry
Re: [PATCH V4 2/4] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC
Posted by Krzysztof Kozlowski 3 weeks, 3 days ago
On 30/10/2024 19:58, Jishnu Prakash wrote:
> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
> 
> It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
> going through PBS(Programmable Boot Sequence) firmware through a single
> register interface. This interface is implemented on an SDAM (Shared
> Direct Access Memory) peripheral on the master PMIC PMK8550 rather
> than a dedicated ADC peripheral.
> 
> Add documentation for PMIC5 Gen3 ADC and macro definitions for ADC
> channels and virtual channels (combination of ADC channel number and
> PMIC SID number) per PMIC, to be used by clients of this device.
> 
> Co-developed-by: Anjelique Melendez <quic_amelende@quicinc.com>
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
> ---

This has still test failures, so limited review follows.

>  properties:
>    compatible:
> @@ -23,14 +27,20 @@ properties:
>            - const: qcom,pms405-adc
>            - const: qcom,spmi-adc-rev2
>        - enum:
> -          - qcom,spmi-vadc
> -          - qcom,spmi-adc5
>            - qcom,spmi-adc-rev2
> +          - qcom,spmi-adc5
> +          - qcom,spmi-adc5-gen3
>            - qcom,spmi-adc7
> +          - qcom,spmi-vadc
>  
>    reg:
> -    description: VADC base address in the SPMI PMIC register map
> -    maxItems: 1
> +    description:
> +      For compatible properties "qcom,spmi-vadc", "qcom,spmi-adc5", "qcom,spmi-adc-rev2"
> +      and "qcom,spmi-adc7", reg is the VADC base address in the SPMI PMIC register map.
> +      For compatible property "qcom,spmi-adc5-gen3", each reg corresponds to an SDAM
> +      peripheral base address that is being used for ADC functionality.

This description is not really needed. You need to provide constraints
in schema.

> +    minItems: 1
> +    maxItems: 2
>  
>    '#address-cells':
>      const: 1
> @@ -38,20 +48,28 @@ properties:
>    '#size-cells':
>      const: 0
>  
> +  "#thermal-sensor-cells":
> +    const: 1
> +    description:
> +      Number of cells required to uniquely identify the thermal sensors.

Drop, redundant.

> +      For compatible property "qcom,spmi-adc5-gen3", this property is
> +      required for if any channels under it are used for ADC_TM.
> +      Since we have multiple sensors this is set to 1.

Drop sentence, redundant.

> +
>    '#io-channel-cells':
>      const: 1
>  
>    interrupts:
> -    maxItems: 1
>      description:
>        End of conversion interrupt.
> +      For compatible property "qcom,spmi-adc5-gen3", interrupts are defined
> +      for each SDAM being used.

Drop descriptions and instead rather list and describe items. You keep
repeating schema in free form text. That's not the point.

> +    minItems: 1
> +    maxItems: 2
>  
> -required:
> -  - compatible
> -  - reg
> -  - '#address-cells'
> -  - '#size-cells'
> -  - '#io-channel-cells'
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 2
>  
>  patternProperties:
>    "^channel@[0-9a-f]+$":
> @@ -71,8 +89,8 @@ patternProperties:
>          description: |
>            ADC channel number.
>            See include/dt-bindings/iio/adc/qcom,spmi-vadc.h
> -          For PMIC7 ADC, the channel numbers are specified separately per PMIC
> -          in the PMIC-specific files in include/dt-bindings/iio/adc.
> +          For PMIC7 and PMIC5 Gen3 ADC, the channel numbers are specified separately
> +          per PMIC in the PMIC-specific files in include/dt-bindings/iio/adc.
>  
>        label:
>          description: |
> @@ -113,11 +131,11 @@ patternProperties:
>                channel calibration. If property is not found, channel will be
>                calibrated with 0.625V and 1.25V reference channels, also
>                known as absolute calibration.
> -            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc7" and
> -              "qcom,spmi-adc-rev2", if this property is specified VADC will use
> -              the VDD reference (1.875V) and GND for channel calibration. If
> -              property is not found, channel will be calibrated with 0V and 1.25V
> -              reference channels, also known as absolute calibration.
> +            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc7",
> +              "qcom,spmi-adc-rev2" and "qcom,spmi-adc5-gen3", if this property is
> +              specified VADC will use the VDD reference (1.875V) and GND for channel
> +              calibration. If property is not found, channel will be calibrated with
> +              0V and 1.25V reference channels, also known as absolute calibration.
>          type: boolean
>  
>        qcom,hw-settle-time:
> @@ -135,9 +153,24 @@ patternProperties:
>              from the ADC that is an average of multiple samples. The value
>              selected is 2^(value).
>  
> +      qcom,adc-tm:
> +        description:
> +          Indicates if ADC_TM monitoring is done on this channel.

What is "ADC_TM"? Why this would be property of a board? This does not
look like suitable for DT, at least based on such very vague explanation.

> +          Defined for compatible property "qcom,spmi-adc5-gen3".

Drop redundant.

> +          This is the same functionality as in the existing QCOM ADC_TM
> +          device, documented at devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml.

What does it mean? How property can represent functionality of entire
binding?

BTW, use full paths when refering to files.

> +        type: boolean
> +
>      required:
>        - reg
>  
> +required:
> +  - compatible
> +  - reg
> +  - '#address-cells'
> +  - '#size-cells'
> +  - '#io-channel-cells'
> +
>  allOf:
>    - if:
>        properties:
> @@ -146,6 +179,15 @@ allOf:
>              const: qcom,spmi-vadc
>  
>      then:
> +      properties:
> +        reg:
> +          minItems: 1

min is redundant.

> +          maxItems: 1
> +        interrupts:
> +          minItems: 1
> +          maxItems: 1

So here you list and describe items instead.

> +        "#thermal-sensor-cells": false
> +        interrupt-names: false

Keep things properly ordered. xxx-names is always next to xxx.

>        patternProperties:
>          "^channel@[0-9a-f]+$":
>            properties:
> @@ -162,6 +204,8 @@ allOf:
>                enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512 ]
>                default: 1
>  
> +            qcom,adc-tm: false
> +
>    - if:
>        properties:
>          compatible:
> @@ -169,6 +213,15 @@ allOf:
>              const: qcom,spmi-adc-rev2
>  
>      then:
> +      properties:
> +        reg:
> +          minItems: 1
> +          maxItems: 1
> +        interrupts:
> +          minItems: 1
> +          maxItems: 1
> +        "#thermal-sensor-cells": false
> +        interrupt-names: false
>        patternProperties:
>          "^channel@[0-9a-f]+$":
>            properties:
> @@ -185,6 +238,8 @@ allOf:
>                enum: [ 1, 2, 4, 8, 16 ]
>                default: 1
>  
> +            qcom,adc-tm: false
> +
>    - if:
>        properties:
>          compatible:
> @@ -192,6 +247,15 @@ allOf:
>              const: qcom,spmi-adc5
>  
>      then:
> +      properties:
> +        reg:
> +          minItems: 1
> +          maxItems: 1
> +        interrupts:
> +          minItems: 1
> +          maxItems: 1
> +        "#thermal-sensor-cells": false
> +        interrupt-names: false
>        patternProperties:
>          "^channel@[0-9a-f]+$":
>            properties:
> @@ -208,6 +272,8 @@ allOf:
>                enum: [ 1, 2, 4, 8, 16 ]
>                default: 1
>  
> +            qcom,adc-tm: false
> +
>    - if:
>        properties:
>          compatible:
> @@ -215,6 +281,59 @@ allOf:
>              const: qcom,spmi-adc7
>  
>      then:
> +      properties:
> +        reg:
> +          minItems: 1
> +          maxItems: 1
> +        interrupts:
> +          minItems: 1
> +          maxItems: 1
> +        "#thermal-sensor-cells": false
> +        interrupt-names: false
> +      patternProperties:
> +        "^channel@[0-9a-f]+$":
> +          properties:
> +            qcom,decimation:
> +              enum: [ 85, 340, 1360 ]
> +              default: 1360
> +
> +            qcom,hw-settle-time:
> +              enum: [ 15, 100, 200, 300, 400, 500, 600, 700, 1000, 2000, 4000,
> +                      8000, 16000, 32000, 64000, 128000 ]
> +              default: 15
> +
> +            qcom,avg-samples:
> +              enum: [ 1, 2, 4, 8, 16 ]
> +              default: 1
> +
> +            qcom,adc-tm: false
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,spmi-adc5-gen3
> +
> +    then:
> +      properties:
> +        reg:
> +          minItems: 1

Why this is flexible?

> +          items:
> +            - description: SDAM0 base address in the SPMI PMIC register map
> +            - description: SDAM1 base address
> +        interrupts:
> +          minItems: 1


Why this is flexible?


> +          items:
> +            - description: SDAM0 end of conversion (EOC) interrupt
> +            - description: SDAM1 EOC interrupt
> +        interrupt-names:
> +          minItems: 1
> +          items:
> +            - const: adc-sdam0

sdam0

> +            - const: adc-sdam1

sdam1

> +      required:
> +        - interrupts
> +        - interrupt-names
>        patternProperties:
>          "^channel@[0-9a-f]+$":
>            properties:
> @@ -307,3 +426,64 @@ examples:



Best regards,
Krzysztof
Re: [PATCH V4 2/4] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC
Posted by Jishnu Prakash 1 week, 4 days ago
Hi Krzysztof,

On 10/31/2024 4:28 PM, Krzysztof Kozlowski wrote:
> On 30/10/2024 19:58, Jishnu Prakash wrote:
>> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
>> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
>>
>> It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
>> going through PBS(Programmable Boot Sequence) firmware through a single
>> register interface. This interface is implemented on an SDAM (Shared
>> Direct Access Memory) peripheral on the master PMIC PMK8550 rather
>> than a dedicated ADC peripheral.
>>
>> Add documentation for PMIC5 Gen3 ADC and macro definitions for ADC
>> channels and virtual channels (combination of ADC channel number and
>> PMIC SID number) per PMIC, to be used by clients of this device.
>>
>> Co-developed-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
>> ---
> 
> This has still test failures, so limited review follows.
> 
>>  properties:
>>    compatible:
>> @@ -23,14 +27,20 @@ properties:
>>            - const: qcom,pms405-adc
>>            - const: qcom,spmi-adc-rev2
>>        - enum:
>> -          - qcom,spmi-vadc
>> -          - qcom,spmi-adc5
>>            - qcom,spmi-adc-rev2
>> +          - qcom,spmi-adc5
>> +          - qcom,spmi-adc5-gen3
>>            - qcom,spmi-adc7
>> +          - qcom,spmi-vadc
>>  
>>    reg:
>> -    description: VADC base address in the SPMI PMIC register map
>> -    maxItems: 1
>> +    description:
>> +      For compatible properties "qcom,spmi-vadc", "qcom,spmi-adc5", "qcom,spmi-adc-rev2"
>> +      and "qcom,spmi-adc7", reg is the VADC base address in the SPMI PMIC register map.
>> +      For compatible property "qcom,spmi-adc5-gen3", each reg corresponds to an SDAM
>> +      peripheral base address that is being used for ADC functionality.
> 
> This description is not really needed. You need to provide constraints
> in schema.
> 
>> +    minItems: 1
>> +    maxItems: 2
>>  
>>    '#address-cells':
>>      const: 1
>> @@ -38,20 +48,28 @@ properties:
>>    '#size-cells':
>>      const: 0
>>  
>> +  "#thermal-sensor-cells":
>> +    const: 1
>> +    description:
>> +      Number of cells required to uniquely identify the thermal sensors.
> 
> Drop, redundant.
> 
>> +      For compatible property "qcom,spmi-adc5-gen3", this property is
>> +      required for if any channels under it are used for ADC_TM.
>> +      Since we have multiple sensors this is set to 1.
> 
> Drop sentence, redundant.
> 
>> +
>>    '#io-channel-cells':
>>      const: 1
>>  
>>    interrupts:
>> -    maxItems: 1
>>      description:
>>        End of conversion interrupt.
>> +      For compatible property "qcom,spmi-adc5-gen3", interrupts are defined
>> +      for each SDAM being used.
> 
> Drop descriptions and instead rather list and describe items. You keep
> repeating schema in free form text. That's not the point.
> 
>> +    minItems: 1
>> +    maxItems: 2
>>  
>> -required:
>> -  - compatible
>> -  - reg
>> -  - '#address-cells'
>> -  - '#size-cells'
>> -  - '#io-channel-cells'
>> +  interrupt-names:
>> +    minItems: 1
>> +    maxItems: 2
>>  
>>  patternProperties:
>>    "^channel@[0-9a-f]+$":
>> @@ -71,8 +89,8 @@ patternProperties:
>>          description: |
>>            ADC channel number.
>>            See include/dt-bindings/iio/adc/qcom,spmi-vadc.h
>> -          For PMIC7 ADC, the channel numbers are specified separately per PMIC
>> -          in the PMIC-specific files in include/dt-bindings/iio/adc.
>> +          For PMIC7 and PMIC5 Gen3 ADC, the channel numbers are specified separately
>> +          per PMIC in the PMIC-specific files in include/dt-bindings/iio/adc.
>>  
>>        label:
>>          description: |
>> @@ -113,11 +131,11 @@ patternProperties:
>>                channel calibration. If property is not found, channel will be
>>                calibrated with 0.625V and 1.25V reference channels, also
>>                known as absolute calibration.
>> -            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc7" and
>> -              "qcom,spmi-adc-rev2", if this property is specified VADC will use
>> -              the VDD reference (1.875V) and GND for channel calibration. If
>> -              property is not found, channel will be calibrated with 0V and 1.25V
>> -              reference channels, also known as absolute calibration.
>> +            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc7",
>> +              "qcom,spmi-adc-rev2" and "qcom,spmi-adc5-gen3", if this property is
>> +              specified VADC will use the VDD reference (1.875V) and GND for channel
>> +              calibration. If property is not found, channel will be calibrated with
>> +              0V and 1.25V reference channels, also known as absolute calibration.
>>          type: boolean
>>  
>>        qcom,hw-settle-time:
>> @@ -135,9 +153,24 @@ patternProperties:
>>              from the ADC that is an average of multiple samples. The value
>>              selected is 2^(value).
>>  
>> +      qcom,adc-tm:
>> +        description:
>> +          Indicates if ADC_TM monitoring is done on this channel.
> 
> What is "ADC_TM"? Why this would be property of a board? This does not
> look like suitable for DT, at least based on such very vague explanation.
> 
>> +          Defined for compatible property "qcom,spmi-adc5-gen3".
> 
> Drop redundant.
> 
>> +          This is the same functionality as in the existing QCOM ADC_TM
>> +          device, documented at devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml.
> 
> What does it mean? How property can represent functionality of entire
> binding?
> 
> BTW, use full paths when refering to files.
> 

To address all your above questions for ADC_TM:

The file "Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml" describes
the Qualcomm ADC thermal monitoring device, which existed as a separate device on older
PMIC generations. ADC_TM refers to this functionality.

In ADC5 Gen3, ADC_TM functionality is combined with the existing ADC read functionality
described in this file, under a single device.

In the earlier ADC_TM DT nodes, each child node would describe one of the IIO ADC channels being
monitored by ADC_TM HW. In this ADC5 Gen3 device, setting the property 'qcom,adc-tm' for a channel
node means that it will also be monitored in HW exactly like an ADC_TM channel.

It can be considered a hardware property as the monitoring is done by a sequence under
PBS (Programmable Boot Sequence, can be considered firmware), which periodically gets the
channel reading and checks it against upper/lower thresholds set by clients of this driver, 
for threshold violations.


>> +        type: boolean
>> +
>>      required:
>>        - reg
>>  
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#address-cells'
>> +  - '#size-cells'
>> +  - '#io-channel-cells'
>> +
>>  allOf:
>>    - if:
>>        properties:
>> @@ -146,6 +179,15 @@ allOf:
>>              const: qcom,spmi-vadc
>>  
>>      then:
>> +      properties:
>> +        reg:
>> +          minItems: 1
> 
> min is redundant.
> 
>> +          maxItems: 1
>> +        interrupts:
>> +          minItems: 1
>> +          maxItems: 1
> 
> So here you list and describe items instead.

Do you mean interrupts should be updated to something like this?

        interrupts:
          maxItems: 1
	  description: 
            End of conversion interrupt.

Does this look right?

> 
>> +        "#thermal-sensor-cells": false
>> +        interrupt-names: false
> 
> Keep things properly ordered. xxx-names is always next to xxx.
> 
>>        patternProperties:
>>          "^channel@[0-9a-f]+$":
>>            properties:
>> @@ -162,6 +204,8 @@ allOf:
>>                enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512 ]
>>                default: 1
>>  
>> +            qcom,adc-tm: false
>> +
>>    - if:
>>        properties:
>>          compatible:
>> @@ -169,6 +213,15 @@ allOf:
>>              const: qcom,spmi-adc-rev2
>>  
>>      then:
>> +      properties:
>> +        reg:
>> +          minItems: 1
>> +          maxItems: 1
>> +        interrupts:
>> +          minItems: 1
>> +          maxItems: 1
>> +        "#thermal-sensor-cells": false
>> +        interrupt-names: false
>>        patternProperties:
>>          "^channel@[0-9a-f]+$":
>>            properties:
>> @@ -185,6 +238,8 @@ allOf:
>>                enum: [ 1, 2, 4, 8, 16 ]
>>                default: 1
>>  
>> +            qcom,adc-tm: false
>> +
>>    - if:
>>        properties:
>>          compatible:
>> @@ -192,6 +247,15 @@ allOf:
>>              const: qcom,spmi-adc5
>>  
>>      then:
>> +      properties:
>> +        reg:
>> +          minItems: 1
>> +          maxItems: 1
>> +        interrupts:
>> +          minItems: 1
>> +          maxItems: 1
>> +        "#thermal-sensor-cells": false
>> +        interrupt-names: false
>>        patternProperties:
>>          "^channel@[0-9a-f]+$":
>>            properties:
>> @@ -208,6 +272,8 @@ allOf:
>>                enum: [ 1, 2, 4, 8, 16 ]
>>                default: 1
>>  
>> +            qcom,adc-tm: false
>> +
>>    - if:
>>        properties:
>>          compatible:
>> @@ -215,6 +281,59 @@ allOf:
>>              const: qcom,spmi-adc7
>>  
>>      then:
>> +      properties:
>> +        reg:
>> +          minItems: 1
>> +          maxItems: 1
>> +        interrupts:
>> +          minItems: 1
>> +          maxItems: 1
>> +        "#thermal-sensor-cells": false
>> +        interrupt-names: false
>> +      patternProperties:
>> +        "^channel@[0-9a-f]+$":
>> +          properties:
>> +            qcom,decimation:
>> +              enum: [ 85, 340, 1360 ]
>> +              default: 1360
>> +
>> +            qcom,hw-settle-time:
>> +              enum: [ 15, 100, 200, 300, 400, 500, 600, 700, 1000, 2000, 4000,
>> +                      8000, 16000, 32000, 64000, 128000 ]
>> +              default: 15
>> +
>> +            qcom,avg-samples:
>> +              enum: [ 1, 2, 4, 8, 16 ]
>> +              default: 1
>> +
>> +            qcom,adc-tm: false
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: qcom,spmi-adc5-gen3
>> +
>> +    then:
>> +      properties:
>> +        reg:
>> +          minItems: 1
> 
> Why this is flexible?

I'm assuming you are asking why it can be either 1 or 2 instead of exactly 2.
Both configurations can be supported in HW and it varies between boards. Some of them
have exactly one SDAM peripheral assigned for ADC usage and some may have two.


> 
>> +          items:
>> +            - description: SDAM0 base address in the SPMI PMIC register map
>> +            - description: SDAM1 base address
>> +        interrupts:
>> +          minItems: 1
> 
> 
> Why this is flexible?

reg, interrupts and interrupt-names are all added per SDAM, so they can all be
either 1 or 2.

Will address all your other comments in the next patch version.

Thanks,
Jishnu

> 
> 
>> +          items:
>> +            - description: SDAM0 end of conversion (EOC) interrupt
>> +            - description: SDAM1 EOC interrupt
>> +        interrupt-names:
>> +          minItems: 1
>> +          items:
>> +            - const: adc-sdam0
> 
> sdam0
> 
>> +            - const: adc-sdam1
> 
> sdam1
> 
>> +      required:
>> +        - interrupts
>> +        - interrupt-names
>>        patternProperties:
>>          "^channel@[0-9a-f]+$":
>>            properties:
>> @@ -307,3 +426,64 @@ examples:
> 
> 
> 
> Best regards,
> Krzysztof
>
Re: [PATCH V4 2/4] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC
Posted by Krzysztof Kozlowski 5 days, 17 hours ago
On 13/11/2024 15:05, Jishnu Prakash wrote:
> Hi Krzysztof,
> 
> On 10/31/2024 4:28 PM, Krzysztof Kozlowski wrote:
>> On 30/10/2024 19:58, Jishnu Prakash wrote:
>>> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
>>> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
>>>
>>> It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
>>> going through PBS(Programmable Boot Sequence) firmware through a single
>>> register interface. This interface is implemented on an SDAM (Shared
>>> Direct Access Memory) peripheral on the master PMIC PMK8550 rather
>>> than a dedicated ADC peripheral.
>>>
>>> Add documentation for PMIC5 Gen3 ADC and macro definitions for ADC
>>> channels and virtual channels (combination of ADC channel number and
>>> PMIC SID number) per PMIC, to be used by clients of this device.
>>>
>>> Co-developed-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
>>> ---
>>
>> This has still test failures, so limited review follows.
>>
>>>  properties:
>>>    compatible:
>>> @@ -23,14 +27,20 @@ properties:
>>>            - const: qcom,pms405-adc
>>>            - const: qcom,spmi-adc-rev2
>>>        - enum:
>>> -          - qcom,spmi-vadc
>>> -          - qcom,spmi-adc5
>>>            - qcom,spmi-adc-rev2
>>> +          - qcom,spmi-adc5
>>> +          - qcom,spmi-adc5-gen3
>>>            - qcom,spmi-adc7
>>> +          - qcom,spmi-vadc
>>>  
>>>    reg:
>>> -    description: VADC base address in the SPMI PMIC register map
>>> -    maxItems: 1
>>> +    description:
>>> +      For compatible properties "qcom,spmi-vadc", "qcom,spmi-adc5", "qcom,spmi-adc-rev2"
>>> +      and "qcom,spmi-adc7", reg is the VADC base address in the SPMI PMIC register map.
>>> +      For compatible property "qcom,spmi-adc5-gen3", each reg corresponds to an SDAM
>>> +      peripheral base address that is being used for ADC functionality.
>>
>> This description is not really needed. You need to provide constraints
>> in schema.
>>
>>> +    minItems: 1
>>> +    maxItems: 2
>>>  
>>>    '#address-cells':
>>>      const: 1
>>> @@ -38,20 +48,28 @@ properties:
>>>    '#size-cells':
>>>      const: 0
>>>  
>>> +  "#thermal-sensor-cells":
>>> +    const: 1
>>> +    description:
>>> +      Number of cells required to uniquely identify the thermal sensors.
>>
>> Drop, redundant.
>>
>>> +      For compatible property "qcom,spmi-adc5-gen3", this property is
>>> +      required for if any channels under it are used for ADC_TM.
>>> +      Since we have multiple sensors this is set to 1.
>>
>> Drop sentence, redundant.
>>
>>> +
>>>    '#io-channel-cells':
>>>      const: 1
>>>  
>>>    interrupts:
>>> -    maxItems: 1
>>>      description:
>>>        End of conversion interrupt.
>>> +      For compatible property "qcom,spmi-adc5-gen3", interrupts are defined
>>> +      for each SDAM being used.
>>
>> Drop descriptions and instead rather list and describe items. You keep
>> repeating schema in free form text. That's not the point.
>>
>>> +    minItems: 1
>>> +    maxItems: 2
>>>  
>>> -required:
>>> -  - compatible
>>> -  - reg
>>> -  - '#address-cells'
>>> -  - '#size-cells'
>>> -  - '#io-channel-cells'
>>> +  interrupt-names:
>>> +    minItems: 1
>>> +    maxItems: 2
>>>  
>>>  patternProperties:
>>>    "^channel@[0-9a-f]+$":
>>> @@ -71,8 +89,8 @@ patternProperties:
>>>          description: |
>>>            ADC channel number.
>>>            See include/dt-bindings/iio/adc/qcom,spmi-vadc.h
>>> -          For PMIC7 ADC, the channel numbers are specified separately per PMIC
>>> -          in the PMIC-specific files in include/dt-bindings/iio/adc.
>>> +          For PMIC7 and PMIC5 Gen3 ADC, the channel numbers are specified separately
>>> +          per PMIC in the PMIC-specific files in include/dt-bindings/iio/adc.
>>>  
>>>        label:
>>>          description: |
>>> @@ -113,11 +131,11 @@ patternProperties:
>>>                channel calibration. If property is not found, channel will be
>>>                calibrated with 0.625V and 1.25V reference channels, also
>>>                known as absolute calibration.
>>> -            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc7" and
>>> -              "qcom,spmi-adc-rev2", if this property is specified VADC will use
>>> -              the VDD reference (1.875V) and GND for channel calibration. If
>>> -              property is not found, channel will be calibrated with 0V and 1.25V
>>> -              reference channels, also known as absolute calibration.
>>> +            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc7",
>>> +              "qcom,spmi-adc-rev2" and "qcom,spmi-adc5-gen3", if this property is
>>> +              specified VADC will use the VDD reference (1.875V) and GND for channel
>>> +              calibration. If property is not found, channel will be calibrated with
>>> +              0V and 1.25V reference channels, also known as absolute calibration.
>>>          type: boolean
>>>  
>>>        qcom,hw-settle-time:
>>> @@ -135,9 +153,24 @@ patternProperties:
>>>              from the ADC that is an average of multiple samples. The value
>>>              selected is 2^(value).
>>>  
>>> +      qcom,adc-tm:
>>> +        description:
>>> +          Indicates if ADC_TM monitoring is done on this channel.
>>
>> What is "ADC_TM"? Why this would be property of a board? This does not
>> look like suitable for DT, at least based on such very vague explanation.
>>
>>> +          Defined for compatible property "qcom,spmi-adc5-gen3".
>>
>> Drop redundant.
>>
>>> +          This is the same functionality as in the existing QCOM ADC_TM
>>> +          device, documented at devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml.
>>
>> What does it mean? How property can represent functionality of entire
>> binding?
>>
>> BTW, use full paths when refering to files.
>>
> 
> To address all your above questions for ADC_TM:
> 
> The file "Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml" describes
> the Qualcomm ADC thermal monitoring device, which existed as a separate device on older
> PMIC generations. ADC_TM refers to this functionality.
> 
> In ADC5 Gen3, ADC_TM functionality is combined with the existing ADC read functionality
> described in this file, under a single device.
> 
> In the earlier ADC_TM DT nodes, each child node would describe one of the IIO ADC channels being
> monitored by ADC_TM HW. In this ADC5 Gen3 device, setting the property 'qcom,adc-tm' for a channel
> node means that it will also be monitored in HW exactly like an ADC_TM channel.
> 
> It can be considered a hardware property as the monitoring is done by a sequence under
> PBS (Programmable Boot Sequence, can be considered firmware), which periodically gets the
> channel reading and checks it against upper/lower thresholds set by clients of this driver, 
> for threshold violations.

So you want to configure channels in different way? Then specify it
precisely - what is the nature of this feature/configuration. I have no
clue what is ADC TM and you keep using it over and over.

I still wait for answer why this is a property of a board.

> 
> 
>>> +        type: boolean
>>> +
>>>      required:
>>>        - reg
>>>  
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - '#address-cells'
>>> +  - '#size-cells'
>>> +  - '#io-channel-cells'
>>> +
>>>  allOf:
>>>    - if:
>>>        properties:
>>> @@ -146,6 +179,15 @@ allOf:
>>>              const: qcom,spmi-vadc
>>>  
>>>      then:
>>> +      properties:
>>> +        reg:
>>> +          minItems: 1
>>
>> min is redundant.
>>
>>> +          maxItems: 1
>>> +        interrupts:
>>> +          minItems: 1
>>> +          maxItems: 1
>>
>> So here you list and describe items instead.
> 
> Do you mean interrupts should be updated to something like this?
> 
>         interrupts:
>           maxItems: 1
> 	  description: 
>             End of conversion interrupt.
> 
> Does this look right?


No, you need to list the items. Look at qcom clocks.

> 
>>
>>> +        "#thermal-sensor-cells": false
>>> +        interrupt-names: false
>>
>> Keep things properly ordered. xxx-names is always next to xxx.
>>
>>>        patternProperties:
>>>          "^channel@[0-9a-f]+$":
>>>            properties:
>>> @@ -162,6 +204,8 @@ allOf:
>>>                enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512 ]
>>>                default: 1
>>>  
>>> +            qcom,adc-tm: false
>>> +
>>>    - if:
>>>        properties:
>>>          compatible:
>>> @@ -169,6 +213,15 @@ allOf:
>>>              const: qcom,spmi-adc-rev2
>>>  
>>>      then:
>>> +      properties:
>>> +        reg:
>>> +          minItems: 1
>>> +          maxItems: 1
>>> +        interrupts:
>>> +          minItems: 1
>>> +          maxItems: 1
>>> +        "#thermal-sensor-cells": false
>>> +        interrupt-names: false
>>>        patternProperties:
>>>          "^channel@[0-9a-f]+$":
>>>            properties:
>>> @@ -185,6 +238,8 @@ allOf:
>>>                enum: [ 1, 2, 4, 8, 16 ]
>>>                default: 1
>>>  
>>> +            qcom,adc-tm: false
>>> +
>>>    - if:
>>>        properties:
>>>          compatible:
>>> @@ -192,6 +247,15 @@ allOf:
>>>              const: qcom,spmi-adc5
>>>  
>>>      then:
>>> +      properties:
>>> +        reg:
>>> +          minItems: 1
>>> +          maxItems: 1
>>> +        interrupts:
>>> +          minItems: 1
>>> +          maxItems: 1
>>> +        "#thermal-sensor-cells": false
>>> +        interrupt-names: false
>>>        patternProperties:
>>>          "^channel@[0-9a-f]+$":
>>>            properties:
>>> @@ -208,6 +272,8 @@ allOf:
>>>                enum: [ 1, 2, 4, 8, 16 ]
>>>                default: 1
>>>  
>>> +            qcom,adc-tm: false
>>> +
>>>    - if:
>>>        properties:
>>>          compatible:
>>> @@ -215,6 +281,59 @@ allOf:
>>>              const: qcom,spmi-adc7
>>>  
>>>      then:
>>> +      properties:
>>> +        reg:
>>> +          minItems: 1
>>> +          maxItems: 1
>>> +        interrupts:
>>> +          minItems: 1
>>> +          maxItems: 1
>>> +        "#thermal-sensor-cells": false
>>> +        interrupt-names: false
>>> +      patternProperties:
>>> +        "^channel@[0-9a-f]+$":
>>> +          properties:
>>> +            qcom,decimation:
>>> +              enum: [ 85, 340, 1360 ]
>>> +              default: 1360
>>> +
>>> +            qcom,hw-settle-time:
>>> +              enum: [ 15, 100, 200, 300, 400, 500, 600, 700, 1000, 2000, 4000,
>>> +                      8000, 16000, 32000, 64000, 128000 ]
>>> +              default: 15
>>> +
>>> +            qcom,avg-samples:
>>> +              enum: [ 1, 2, 4, 8, 16 ]
>>> +              default: 1
>>> +
>>> +            qcom,adc-tm: false
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: qcom,spmi-adc5-gen3
>>> +
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          minItems: 1
>>
>> Why this is flexible?
> 
> I'm assuming you are asking why it can be either 1 or 2 instead of exactly 2.
> Both configurations can be supported in HW and it varies between boards. Some of them
> have exactly one SDAM peripheral assigned for ADC usage and some may have two.

That's odd. How this can vary between boards with the same, exactly the
same PMIC? Do you program entirely different FW for different boards
with the same hardware (PMIC)?

This is programming model, so any differences here must be obvious.

Best regards,
Krzysztof
Re: [PATCH V4 2/4] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC
Posted by Rob Herring (Arm) 3 weeks, 4 days ago
On Thu, 31 Oct 2024 00:28:52 +0530, Jishnu Prakash wrote:
> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
> 
> It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
> going through PBS(Programmable Boot Sequence) firmware through a single
> register interface. This interface is implemented on an SDAM (Shared
> Direct Access Memory) peripheral on the master PMIC PMK8550 rather
> than a dedicated ADC peripheral.
> 
> Add documentation for PMIC5 Gen3 ADC and macro definitions for ADC
> channels and virtual channels (combination of ADC channel number and
> PMIC SID number) per PMIC, to be used by clients of this device.
> 
> Co-developed-by: Anjelique Melendez <quic_amelende@quicinc.com>
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
> ---
> Changes since v3:
> - Added ADC5 Gen3 documentation changes in existing qcom,spmi-vadc.yaml file
>   instead of adding separate file and updated top-level constraints in documentation
>   file based on discussion with reviewers.
> - Dropped default SID definitions.
> - Addressed other reviewer comments.
> 
> Changes since v2:
> - Moved ADC5 Gen3 documentation into a separate new file.
> 
> Changes since v1:
> - Updated properties separately for all compatibles to clarify usage
>   of new properties and updates in usage of old properties for ADC5 Gen3.
> - Avoided updating 'adc7' name to 'adc5 gen2' and just left a comment
>   mentioning this convention.
> - Used predefined channel IDs in individual PMIC channel definitions
>   instead of numeric IDs.
> - Addressed other comments from reviewers.
> 
>  .../bindings/iio/adc/qcom,spmi-vadc.yaml      | 220 ++++++++++++++++--
>  .../iio/adc/qcom,spmi-adc5-gen3-pm8550.h      |  46 ++++
>  .../iio/adc/qcom,spmi-adc5-gen3-pm8550b.h     |  85 +++++++
>  .../iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h    |  22 ++
>  .../iio/adc/qcom,spmi-adc5-gen3-pmk8550.h     |  52 +++++
>  include/dt-bindings/iio/adc/qcom,spmi-vadc.h  |  81 +++++++
>  6 files changed, 486 insertions(+), 20 deletions(-)
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.example.dts:88:18: fatal error: dt-bindings/iio/adc/qcom,spmi-vadc.h: No such file or directory
   88 |         #include <dt-bindings/iio/adc/qcom,spmi-vadc.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [scripts/Makefile.dtbs:129: Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.example.dtb] Error 1

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241030185854.4015348-3-quic_jprakash@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.