[v7] Add support for QCOM SPMI PMIC5 Gen3 ADC

[PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Jishnu Prakash 5 months, 2 weeks 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 SDAM (Shared
Direct Access Memory) peripherals 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. Also
update SPMI PMIC bindings to allow ADC5 Gen3 as adc@ subnode.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
---
Changes since v6:
- Updated SPMI PMIC bindings to allow ADC5 Gen3 as adc@ subnode and
  copyright license in newly added files.

Changes since v5:
- Added more details in binding description explaining how number
  of SDAM peripherals used for ADC is allocated per SoC.
- Renamed per-PMIC binding files listing ADC channel macro names 
  and addressed other reviewer comments.

Changes since v4:
- Added ADC5 Gen3 documentation in a separate new file to avoid complicating
  existing VADC documentation file further to accomodate this device, as
  suggested by reviewer.

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-adc5-gen3.yaml | 155 ++++++++++++++++++
 .../iio/adc/qcom,spmi-vadc-common.yaml        |   4 +-
 .../bindings/iio/adc/qcom,spmi-vadc.yaml      |   2 +
 .../bindings/mfd/qcom,spmi-pmic.yaml          |   1 +
 .../iio/adc/qcom,pm8550-adc5-gen3.h           |  46 ++++++
 .../iio/adc/qcom,pm8550b-adc5-gen3.h          |  85 ++++++++++
 .../iio/adc/qcom,pm8550vx-adc5-gen3.h         |  22 +++
 .../iio/adc/qcom,pmk8550-adc5-gen3.h          |  52 ++++++
 include/dt-bindings/iio/adc/qcom,spmi-vadc.h  |  79 +++++++++
 9 files changed, 444 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml
 create mode 100644 include/dt-bindings/iio/adc/qcom,pm8550-adc5-gen3.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,pm8550b-adc5-gen3.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,pm8550vx-adc5-gen3.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,pmk8550-adc5-gen3.h

diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml
new file mode 100644
index 000000000000..40eb20b9d9de
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml
@@ -0,0 +1,155 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/qcom,spmi-adc5-gen3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm's SPMI PMIC ADC5 Gen3
+
+maintainers:
+  - Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
+
+description: |
+  SPMI PMIC5 Gen3 voltage ADC (ADC) provides interface to clients to read
+  voltage. It is a 16-bit sigma-delta ADC. It also performs the same thermal
+  monitoring function as the existing ADC_TM devices.
+
+  The interface is implemented on SDAM (Shared Direct Access Memory) peripherals
+  on the master PMIC rather than a dedicated ADC peripheral. The number of PMIC
+  SDAM peripherals allocated for ADC is not correlated with the PMIC used, it is
+  programmed in FW (PBS) and is fixed per SOC, based on the SOC requirements.
+  All boards using a particular (SOC + master PMIC) combination will have the
+  same number of ADC SDAMs supported on that PMIC.
+
+properties:
+  compatible:
+    const: qcom,spmi-adc5-gen3
+
+  reg:
+    items:
+      - description: SDAM0 base address in the SPMI PMIC register map
+      - description: SDAM1 base address
+    minItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  '#io-channel-cells':
+    const: 1
+
+  "#thermal-sensor-cells":
+    const: 1
+
+  interrupts:
+    items:
+      - description: SDAM0 end of conversion (EOC) interrupt
+      - description: SDAM1 EOC interrupt
+    minItems: 1
+
+patternProperties:
+  "^channel@[0-9a-f]+$":
+    type: object
+    unevaluatedProperties: false
+    $ref: /schemas/iio/adc/qcom,spmi-vadc-common.yaml
+    description:
+      Represents the external channels which are connected to the ADC.
+
+    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:
+        description:
+          ADC_TM is a threshold monitoring feature in HW which can be enabled
+          on any ADC channel, to trigger an IRQ for threshold violation. In
+          earlier ADC generations, it was implemented in a separate device
+          (documented in Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml.)
+          In Gen3, this feature can be enabled in the same ADC device for any
+          channel and threshold monitoring and IRQ triggering are handled in FW
+          (PBS) instead of another dedicated HW block.
+          This property indicates ADC_TM monitoring is done on this channel.
+        type: boolean
+
+required:
+  - compatible
+  - reg
+  - '#address-cells'
+  - '#size-cells'
+  - '#io-channel-cells'
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/iio/adc/qcom,pmk8550-adc5-gen3.h>
+    #include <dt-bindings/iio/adc/qcom,pm8550-adc5-gen3.h>
+    #include <dt-bindings/iio/adc/qcom,pm8550b-adc5-gen3.h>
+    #include <dt-bindings/iio/adc/qcom,pm8550vx-adc5-gen3.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>;
+        #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/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc-common.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc-common.yaml
index cd087911ee88..1531153e6ea8 100644
--- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc-common.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc-common.yaml
@@ -17,8 +17,8 @@ properties:
     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 ADC and PMIC5 Gen3 ADC, the channel numbers are specified
+      separately per PMIC in the PMIC-specific files in include/dt-bindings/iio/adc.
     maxItems: 1
 
   label:
diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
index cf6f6ed2a378..a539d3e13673 100644
--- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
@@ -15,6 +15,8 @@ description: |
   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.
+  Note that PMIC7 ADC is the generation between PMIC5 and PMIC5 Gen3 ADC,
+  it can be considered like PMIC5 Gen2.
 
 properties:
   compatible:
diff --git a/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml b/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml
index 11da55644262..b97f0e7b269e 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml
@@ -127,6 +127,7 @@ patternProperties:
   "^adc@[0-9a-f]+$":
     type: object
     oneOf:
+      - $ref: /schemas/iio/adc/qcom,spmi-adc5-gen3.yaml#
       - $ref: /schemas/iio/adc/qcom,spmi-iadc.yaml#
       - $ref: /schemas/iio/adc/qcom,spmi-rradc.yaml#
       - $ref: /schemas/iio/adc/qcom,spmi-vadc.yaml#
diff --git a/include/dt-bindings/iio/adc/qcom,pm8550-adc5-gen3.h b/include/dt-bindings/iio/adc/qcom,pm8550-adc5-gen3.h
new file mode 100644
index 000000000000..8b9ad328861e
--- /dev/null
+++ b/include/dt-bindings/iio/adc/qcom,pm8550-adc5-gen3.h
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+#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,pm8550b-adc5-gen3.h b/include/dt-bindings/iio/adc/qcom,pm8550b-adc5-gen3.h
new file mode 100644
index 000000000000..bd84975210ea
--- /dev/null
+++ b/include/dt-bindings/iio/adc/qcom,pm8550b-adc5-gen3.h
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+#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,pm8550vx-adc5-gen3.h b/include/dt-bindings/iio/adc/qcom,pm8550vx-adc5-gen3.h
new file mode 100644
index 000000000000..51a25959da98
--- /dev/null
+++ b/include/dt-bindings/iio/adc/qcom,pm8550vx-adc5-gen3.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+#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,pmk8550-adc5-gen3.h b/include/dt-bindings/iio/adc/qcom,pmk8550-adc5-gen3.h
new file mode 100644
index 000000000000..b49e28fe573f
--- /dev/null
+++ b/include/dt-bindings/iio/adc/qcom,pmk8550-adc5-gen3.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+#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..b1b89e874316 100644
--- a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
+++ b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
@@ -300,4 +300,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 V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Krzysztof Kozlowski 5 months, 2 weeks ago

On Tue, Aug 26, 2025 at 02:06:55PM +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 SDAM (Shared
> Direct Access Memory) peripherals 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. Also
> update SPMI PMIC bindings to allow ADC5 Gen3 as adc@ subnode.
> 
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
> ---
> Changes since v6:
> - Updated SPMI PMIC bindings to allow ADC5 Gen3 as adc@ subnode and
>   copyright license in newly added files.

So you did not implement my requests/comments from v5? I did not request
above, I had many, many other comments.



> 
> Changes since v5:
> - Added more details in binding description explaining how number
>   of SDAM peripherals used for ADC is allocated per SoC.
> - Renamed per-PMIC binding files listing ADC channel macro names 
>   and addressed other reviewer comments.
> 
> Changes since v4:
> - Added ADC5 Gen3 documentation in a separate new file to avoid complicating
>   existing VADC documentation file further to accomodate this device, as
>   suggested by reviewer.
> 
> 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-adc5-gen3.yaml | 155 ++++++++++++++++++
>  .../iio/adc/qcom,spmi-vadc-common.yaml        |   4 +-
>  .../bindings/iio/adc/qcom,spmi-vadc.yaml      |   2 +
>  .../bindings/mfd/qcom,spmi-pmic.yaml          |   1 +
>  .../iio/adc/qcom,pm8550-adc5-gen3.h           |  46 ++++++
>  .../iio/adc/qcom,pm8550b-adc5-gen3.h          |  85 ++++++++++
>  .../iio/adc/qcom,pm8550vx-adc5-gen3.h         |  22 +++
>  .../iio/adc/qcom,pmk8550-adc5-gen3.h          |  52 ++++++
>  include/dt-bindings/iio/adc/qcom,spmi-vadc.h  |  79 +++++++++
>  9 files changed, 444 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml
>  create mode 100644 include/dt-bindings/iio/adc/qcom,pm8550-adc5-gen3.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,pm8550b-adc5-gen3.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,pm8550vx-adc5-gen3.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,pmk8550-adc5-gen3.h
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml
> new file mode 100644
> index 000000000000..40eb20b9d9de
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml
> @@ -0,0 +1,155 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/qcom,spmi-adc5-gen3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm's SPMI PMIC ADC5 Gen3
> +
> +maintainers:
> +  - Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
> +
> +description: |
> +  SPMI PMIC5 Gen3 voltage ADC (ADC) provides interface to clients to read
> +  voltage. It is a 16-bit sigma-delta ADC. It also performs the same thermal
> +  monitoring function as the existing ADC_TM devices.
> +
> +  The interface is implemented on SDAM (Shared Direct Access Memory) peripherals
> +  on the master PMIC rather than a dedicated ADC peripheral. The number of PMIC
> +  SDAM peripherals allocated for ADC is not correlated with the PMIC used, it is
> +  programmed in FW (PBS) and is fixed per SOC, based on the SOC requirements.
> +  All boards using a particular (SOC + master PMIC) combination will have the
> +  same number of ADC SDAMs supported on that PMIC.
> +
> +properties:
> +  compatible:
> +    const: qcom,spmi-adc5-gen3
> +
> +  reg:
> +    items:
> +      - description: SDAM0 base address in the SPMI PMIC register map
> +      - description: SDAM1 base address
> +    minItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  '#io-channel-cells':
> +    const: 1
> +
> +  "#thermal-sensor-cells":

Nothing improved here, still mess with quotes.

I am not going to check the rest of comments, because:
1. Your changelog is vague and claims you did not implement them,
2. b4 diff does not work, base-commit is unknown.
3. Main changelog is even more vague.

You make it difficult for us to review your patches, fine. You will get:

NAK (plus one more comment below)

> diff --git a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
> index ef07ecd4d585..b1b89e874316 100644
> --- a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
> +++ b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
> @@ -300,4 +300,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

You cannot have empty spaces in ID constants. These are abstract
numbers.

Otherwise please point me to driver using this constant.

Best regards,
Krzysztof

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Jishnu Prakash 4 months, 3 weeks ago

Hi Krzysztof,

On 8/29/2025 12:49 PM, Krzysztof Kozlowski wrote:
> On Tue, Aug 26, 2025 at 02:06:55PM +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 SDAM (Shared
>> Direct Access Memory) peripherals 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. Also
>> update SPMI PMIC bindings to allow ADC5 Gen3 as adc@ subnode.
>>
>> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
>> ---
>> Changes since v6:
>> - Updated SPMI PMIC bindings to allow ADC5 Gen3 as adc@ subnode and
>>   copyright license in newly added files.
> 
> So you did not implement my requests/comments from v5? I did not request
> above, I had many, many other comments.
> 
> 

In my v6 patch, I had implemented all the changes you requested in
my v5 patch, that is what I had meant by the last part ('addressed other reviewer comments').
I will update the below section when pushing my next patch series
to list out all the changes explicitly.

> 
>>
>> Changes since v5:
>> - Added more details in binding description explaining how number
>>   of SDAM peripherals used for ADC is allocated per SoC.
>> - Renamed per-PMIC binding files listing ADC channel macro names 
>>   and addressed other reviewer comments.
>>
>> Changes since v4:
>> - Added ADC5 Gen3 documentation in a separate new file to avoid complicating
>>   existing VADC documentation file further to accomodate this device, as
>>   suggested by reviewer.
>>
>> 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.
>>

...

>> +
>> +  The interface is implemented on SDAM (Shared Direct Access Memory) peripherals
>> +  on the master PMIC rather than a dedicated ADC peripheral. The number of PMIC
>> +  SDAM peripherals allocated for ADC is not correlated with the PMIC used, it is
>> +  programmed in FW (PBS) and is fixed per SOC, based on the SOC requirements.
>> +  All boards using a particular (SOC + master PMIC) combination will have the
>> +  same number of ADC SDAMs supported on that PMIC.
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,spmi-adc5-gen3
>> +
>> +  reg:
>> +    items:
>> +      - description: SDAM0 base address in the SPMI PMIC register map
>> +      - description: SDAM1 base address
>> +    minItems: 1
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +  '#io-channel-cells':
>> +    const: 1
>> +
>> +  "#thermal-sensor-cells":
> 
> Nothing improved here, still mess with quotes.

I will fix this and check for any other things to fix
before pushing the next patch.

> 
> I am not going to check the rest of comments, because:
> 1. Your changelog is vague and claims you did not implement them,
> 2. b4 diff does not work, base-commit is unknown.
> 3. Main changelog is even more vague.
> 
> You make it difficult for us to review your patches, fine. You will get:
> 
> NAK (plus one more comment below)
> 
>> diff --git a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
>> index ef07ecd4d585..b1b89e874316 100644
>> --- a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
>> +++ b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
>> @@ -300,4 +300,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
> 
> You cannot have empty spaces in ID constants. These are abstract
> numbers.
> 
> Otherwise please point me to driver using this constant.

These constants are for ADC channel numbers, which are fixed in HW.

They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
which is added in patch 4 of this series.

They can be found in the array named adc5_gen3_chans_pmic[].

Thanks,
Jishnu

> 
> Best regards,
> Krzysztof
>

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Krzysztof Kozlowski 4 months, 3 weeks ago

On 16/09/2025 16:28, Jishnu Prakash wrote:
>> You cannot have empty spaces in ID constants. These are abstract
>> numbers.
>>
>> Otherwise please point me to driver using this constant.
> 
> These constants are for ADC channel numbers, which are fixed in HW.
> 
> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
> which is added in patch 4 of this series.
> 
> They can be found in the array named adc5_gen3_chans_pmic[].

Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.

You responded three weeks after review, this patch goes nowhere and way
you handle upstream work makes it difficult for us and probably for you.

NAK

> 
> Thanks,
> Jishnu
> 
>>
>> Best regards,
>> Krzysztof
>>
> 


Best regards,
Krzysztof

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Jishnu Prakash 4 months, 3 weeks ago

Hi Krzysztof,

On 9/17/2025 5:59 AM, Krzysztof Kozlowski wrote:
> On 16/09/2025 16:28, Jishnu Prakash wrote:
>>> You cannot have empty spaces in ID constants. These are abstract
>>> numbers.
>>>
>>> Otherwise please point me to driver using this constant.
>>
>> These constants are for ADC channel numbers, which are fixed in HW.
>>
>> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
>> which is added in patch 4 of this series.
>>
>> They can be found in the array named adc5_gen3_chans_pmic[].
> 
> Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.
> 

We may not be using all of these channels right now - we can add them
later based on requirements coming up. For now, I'll remove the channels
not used in adc5_gen3_chans_pmic[].

Thanks,
Jishnu

> You responded three weeks after review, this patch goes nowhere and way
> you handle upstream work makes it difficult for us and probably for you.
> 
> NAK
> 
>>
>> Thanks,
>> Jishnu
>>
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Krzysztof Kozlowski 4 months, 3 weeks ago

On 18/09/2025 04:47, Jishnu Prakash wrote:
> Hi Krzysztof,
> 
> On 9/17/2025 5:59 AM, Krzysztof Kozlowski wrote:
>> On 16/09/2025 16:28, Jishnu Prakash wrote:
>>>> You cannot have empty spaces in ID constants. These are abstract
>>>> numbers.
>>>>
>>>> Otherwise please point me to driver using this constant.
>>>
>>> These constants are for ADC channel numbers, which are fixed in HW.
>>>
>>> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
>>> which is added in patch 4 of this series.
>>>
>>> They can be found in the array named adc5_gen3_chans_pmic[].
>>
>> Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.
>>
> 
> We may not be using all of these channels right now - we can add them
> later based on requirements coming up. For now, I'll remove the channels
> not used in adc5_gen3_chans_pmic[].

You are not implementing the feedback then. Please read it carefully.

Best regards,
Krzysztof

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Jishnu Prakash 4 months, 3 weeks ago

Hi Krzysztof,

On 9/18/2025 5:45 AM, Krzysztof Kozlowski wrote:
> On 18/09/2025 04:47, Jishnu Prakash wrote:
>> Hi Krzysztof,
>>
>> On 9/17/2025 5:59 AM, Krzysztof Kozlowski wrote:
>>> On 16/09/2025 16:28, Jishnu Prakash wrote:
>>>>> You cannot have empty spaces in ID constants. These are abstract
>>>>> numbers.
>>>>>
>>>>> Otherwise please point me to driver using this constant.
>>>>
>>>> These constants are for ADC channel numbers, which are fixed in HW.
>>>>
>>>> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
>>>> which is added in patch 4 of this series.
>>>>
>>>> They can be found in the array named adc5_gen3_chans_pmic[].
>>>
>>> Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.
>>>
>>
>> We may not be using all of these channels right now - we can add them
>> later based on requirements coming up. For now, I'll remove the channels
>> not used in adc5_gen3_chans_pmic[].
> 
> You are not implementing the feedback then. Please read it carefully.
> 

Sorry, I misunderstood - so you actually meant I should remove the
empty spaces in the definitions, like this?

-#define ADC5_GEN3_VREF_BAT_THERM               0x15
+#define ADC5_GEN3_VREF_BAT_THERM 0x15

I thought this at first, but I somehow doubted this later, as I saw some
other recently added files with empty spaces in #define lines, like:

include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h
include/dt-bindings/regulator/st,stm32mp15-regulator.h

I can make this change, if you prefer this. Please let me know
if I'm still missing something.

Also please let me know if you want me to remove the unused
channels - I would prefer to keep them if there's no issue,
as we might need them later.

Thanks,
Jishnu

> Best regards,
> Krzysztof

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Jonathan Cameron 4 months, 2 weeks ago

On Fri, 19 Sep 2025 20:17:43 +0530
Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:

> Hi Krzysztof,
> 
> On 9/18/2025 5:45 AM, Krzysztof Kozlowski wrote:
> > On 18/09/2025 04:47, Jishnu Prakash wrote:  
> >> Hi Krzysztof,
> >>
> >> On 9/17/2025 5:59 AM, Krzysztof Kozlowski wrote:  
> >>> On 16/09/2025 16:28, Jishnu Prakash wrote:  
> >>>>> You cannot have empty spaces in ID constants. These are abstract
> >>>>> numbers.
> >>>>>
> >>>>> Otherwise please point me to driver using this constant.  
> >>>>
> >>>> These constants are for ADC channel numbers, which are fixed in HW.
> >>>>
> >>>> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
> >>>> which is added in patch 4 of this series.
> >>>>
> >>>> They can be found in the array named adc5_gen3_chans_pmic[].  
> >>>
> >>> Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.
> >>>  
> >>
> >> We may not be using all of these channels right now - we can add them
> >> later based on requirements coming up. For now, I'll remove the channels
> >> not used in adc5_gen3_chans_pmic[].  
> > 
> > You are not implementing the feedback then. Please read it carefully.
> >   
> 
> Sorry, I misunderstood - so you actually meant I should remove the
> empty spaces in the definitions, like this?
> 
> -#define ADC5_GEN3_VREF_BAT_THERM               0x15
> +#define ADC5_GEN3_VREF_BAT_THERM 0x15
> 
> I thought this at first, but I somehow doubted this later, as I saw some
> other recently added files with empty spaces in #define lines, like:
> 
> include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h
> include/dt-bindings/regulator/st,stm32mp15-regulator.h
> 
> I can make this change, if you prefer this. Please let me know
> if I'm still missing something.
> 
> Also please let me know if you want me to remove the unused
> channels - I would prefer to keep them if there's no issue,
> as we might need them later.
> 
He is referring to 0x14 and below not being defined values.  So what
do they mean if they turn up in the DT?

Hence the request for context on how this define is being used so that
you can get some feedback on how it should be done.

J
> Thanks,
> Jishnu
> 
> > Best regards,
> > Krzysztof  
>

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Jishnu Prakash 4 months, 1 week ago

Hi Jonathan,

On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
> On Fri, 19 Sep 2025 20:17:43 +0530
> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
> 
>> Hi Krzysztof,
>>
>> On 9/18/2025 5:45 AM, Krzysztof Kozlowski wrote:
>>> On 18/09/2025 04:47, Jishnu Prakash wrote:  
>>>> Hi Krzysztof,
>>>>
>>>> On 9/17/2025 5:59 AM, Krzysztof Kozlowski wrote:  
>>>>> On 16/09/2025 16:28, Jishnu Prakash wrote:  
>>>>>>> You cannot have empty spaces in ID constants. These are abstract
>>>>>>> numbers.
>>>>>>>
>>>>>>> Otherwise please point me to driver using this constant.  
>>>>>>
>>>>>> These constants are for ADC channel numbers, which are fixed in HW.
>>>>>>
>>>>>> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
>>>>>> which is added in patch 4 of this series.
>>>>>>
>>>>>> They can be found in the array named adc5_gen3_chans_pmic[].  
>>>>>
>>>>> Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.
>>>>>  
>>>>
>>>> We may not be using all of these channels right now - we can add them
>>>> later based on requirements coming up. For now, I'll remove the channels
>>>> not used in adc5_gen3_chans_pmic[].  
>>>
>>> You are not implementing the feedback then. Please read it carefully.
>>>   
>>
>> Sorry, I misunderstood - so you actually meant I should remove the
>> empty spaces in the definitions, like this?
>>
>> -#define ADC5_GEN3_VREF_BAT_THERM               0x15
>> +#define ADC5_GEN3_VREF_BAT_THERM 0x15
>>
>> I thought this at first, but I somehow doubted this later, as I saw some
>> other recently added files with empty spaces in #define lines, like:
>>
>> include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h
>> include/dt-bindings/regulator/st,stm32mp15-regulator.h
>>
>> I can make this change, if you prefer this. Please let me know
>> if I'm still missing something.
>>
>> Also please let me know if you want me to remove the unused
>> channels - I would prefer to keep them if there's no issue,
>> as we might need them later.
>>
> He is referring to 0x14 and below not being defined values.  So what
> do they mean if they turn up in the DT?
> 

Thanks for your clarification. To address your first point above, the macros
added here only represent the ADC channel numbers which are supported for
ADC5 Gen3 devices. If there are numbers missing in between (like 0x14),
that is because there exist no valid ADC channels in HW matching those
channel numbers.

For your question above, if any of the undefined channels are used in the DT,
they should ideally be treated as invalid when parsed in the driver probe and
lead to an error. When I checked the code again, I saw we do not have such an
explicit check right now, so I will add that in the next patch series.

And to be clear on which channel numbers are supported, I think it may be
best if, for now, we only add support for the channel numbers referenced in
the array adc5_gen3_chans_pmic[] in drivers/iio/adc/qcom-spmi-adc5-gen3.c.

There are only 18 channel numbers used in this array and I would remove
all channels except for these from the binding files. During parsing, we
would use this array to confirm if an ADC channel added in DT is supported.

In case we need to add support for any more channels later, we could add
their macros in the binding file and update the array correspondingly at
that time.

Does all this sound fine? Please let me know if you have any more concerns
or queries.

Thanks,
Jishnu

> Hence the request for context on how this define is being used so that
> you can get some feedback on how it should be done.
> 
> J
>> Thanks,
>> Jishnu
>>
>>> Best regards,
>>> Krzysztof  
>>
>

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Krzysztof Kozlowski 4 months, 1 week ago

On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
<jishnu.prakash@oss.qualcomm.com> wrote:
>
> Hi Jonathan,
>
> On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
> > On Fri, 19 Sep 2025 20:17:43 +0530
> > Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
> >
> >> Hi Krzysztof,
> >>
> >> On 9/18/2025 5:45 AM, Krzysztof Kozlowski wrote:
> >>> On 18/09/2025 04:47, Jishnu Prakash wrote:
> >>>> Hi Krzysztof,
> >>>>
> >>>> On 9/17/2025 5:59 AM, Krzysztof Kozlowski wrote:
> >>>>> On 16/09/2025 16:28, Jishnu Prakash wrote:
> >>>>>>> You cannot have empty spaces in ID constants. These are abstract
> >>>>>>> numbers.
> >>>>>>>
> >>>>>>> Otherwise please point me to driver using this constant.
> >>>>>>
> >>>>>> These constants are for ADC channel numbers, which are fixed in HW.
> >>>>>>
> >>>>>> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
> >>>>>> which is added in patch 4 of this series.
> >>>>>>
> >>>>>> They can be found in the array named adc5_gen3_chans_pmic[].
> >>>>>
> >>>>> Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.
> >>>>>
> >>>>
> >>>> We may not be using all of these channels right now - we can add them
> >>>> later based on requirements coming up. For now, I'll remove the channels
> >>>> not used in adc5_gen3_chans_pmic[].
> >>>
> >>> You are not implementing the feedback then. Please read it carefully.
> >>>
> >>
> >> Sorry, I misunderstood - so you actually meant I should remove the
> >> empty spaces in the definitions, like this?
> >>
> >> -#define ADC5_GEN3_VREF_BAT_THERM               0x15
> >> +#define ADC5_GEN3_VREF_BAT_THERM 0x15
> >>
> >> I thought this at first, but I somehow doubted this later, as I saw some
> >> other recently added files with empty spaces in #define lines, like:
> >>
> >> include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h
> >> include/dt-bindings/regulator/st,stm32mp15-regulator.h
> >>
> >> I can make this change, if you prefer this. Please let me know
> >> if I'm still missing something.
> >>
> >> Also please let me know if you want me to remove the unused
> >> channels - I would prefer to keep them if there's no issue,
> >> as we might need them later.
> >>
> > He is referring to 0x14 and below not being defined values.  So what
> > do they mean if they turn up in the DT?
> >
>
> Thanks for your clarification. To address your first point above, the macros
> added here only represent the ADC channel numbers which are supported for
> ADC5 Gen3 devices. If there are numbers missing in between (like 0x14),
> that is because there exist no valid ADC channels in HW matching those
> channel numbers.
>
> For your question above, if any of the undefined channels are used in the DT,
> they should ideally be treated as invalid when parsed in the driver probe and
> lead to an error. When I checked the code again, I saw we do not have such an
> explicit check right now, so I will add that in the next patch series.
>
> And to be clear on which channel numbers are supported, I think it may be
> best if, for now, we only add support for the channel numbers referenced in
> the array adc5_gen3_chans_pmic[] in drivers/iio/adc/qcom-spmi-adc5-gen3.c.
>
> There are only 18 channel numbers used in this array and I would remove
> all channels except for these from the binding files. During parsing, we
> would use this array to confirm if an ADC channel added in DT is supported.
>
> In case we need to add support for any more channels later, we could add
> their macros in the binding file and update the array correspondingly at
> that time.
>
> Does all this sound fine? Please let me know if you have any more concerns
> or queries.

No, it doesn't.  You keep ignoring my arguments and responding to
something else. I prefer not to store hardware values as bindings,
because these are not bindings (and you failed to prove which SW
interface they bind) and it's really not necessary.

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Jishnu Prakash 4 months ago

Hi Krzysztof,

On 10/4/2025 12:22 PM, Krzysztof Kozlowski wrote:
> On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
> <jishnu.prakash@oss.qualcomm.com> wrote:
>>
>> Hi Jonathan,
>>
>> On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
>>> On Fri, 19 Sep 2025 20:17:43 +0530
>>> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
>>>
>>>> Hi Krzysztof,
>>>>
>>>> On 9/18/2025 5:45 AM, Krzysztof Kozlowski wrote:
>>>>> On 18/09/2025 04:47, Jishnu Prakash wrote:
>>>>>> Hi Krzysztof,
>>>>>>
>>>>>> On 9/17/2025 5:59 AM, Krzysztof Kozlowski wrote:
>>>>>>> On 16/09/2025 16:28, Jishnu Prakash wrote:
>>>>>>>>> You cannot have empty spaces in ID constants. These are abstract
>>>>>>>>> numbers.
>>>>>>>>>
>>>>>>>>> Otherwise please point me to driver using this constant.
>>>>>>>>
>>>>>>>> These constants are for ADC channel numbers, which are fixed in HW.
>>>>>>>>
>>>>>>>> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
>>>>>>>> which is added in patch 4 of this series.
>>>>>>>>
>>>>>>>> They can be found in the array named adc5_gen3_chans_pmic[].
>>>>>>>
>>>>>>> Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.
>>>>>>>
>>>>>>
>>>>>> We may not be using all of these channels right now - we can add them
>>>>>> later based on requirements coming up. For now, I'll remove the channels
>>>>>> not used in adc5_gen3_chans_pmic[].
>>>>>
>>>>> You are not implementing the feedback then. Please read it carefully.
>>>>>
>>>>
>>>> Sorry, I misunderstood - so you actually meant I should remove the
>>>> empty spaces in the definitions, like this?
>>>>
>>>> -#define ADC5_GEN3_VREF_BAT_THERM               0x15
>>>> +#define ADC5_GEN3_VREF_BAT_THERM 0x15
>>>>
>>>> I thought this at first, but I somehow doubted this later, as I saw some
>>>> other recently added files with empty spaces in #define lines, like:
>>>>
>>>> include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h
>>>> include/dt-bindings/regulator/st,stm32mp15-regulator.h
>>>>
>>>> I can make this change, if you prefer this. Please let me know
>>>> if I'm still missing something.
>>>>
>>>> Also please let me know if you want me to remove the unused
>>>> channels - I would prefer to keep them if there's no issue,
>>>> as we might need them later.
>>>>
>>> He is referring to 0x14 and below not being defined values.  So what
>>> do they mean if they turn up in the DT?
>>>
>>
>> Thanks for your clarification. To address your first point above, the macros
>> added here only represent the ADC channel numbers which are supported for
>> ADC5 Gen3 devices. If there are numbers missing in between (like 0x14),
>> that is because there exist no valid ADC channels in HW matching those
>> channel numbers.
>>
>> For your question above, if any of the undefined channels are used in the DT,
>> they should ideally be treated as invalid when parsed in the driver probe and
>> lead to an error. When I checked the code again, I saw we do not have such an
>> explicit check right now, so I will add that in the next patch series.
>>
>> And to be clear on which channel numbers are supported, I think it may be
>> best if, for now, we only add support for the channel numbers referenced in
>> the array adc5_gen3_chans_pmic[] in drivers/iio/adc/qcom-spmi-adc5-gen3.c.
>>
>> There are only 18 channel numbers used in this array and I would remove
>> all channels except for these from the binding files. During parsing, we
>> would use this array to confirm if an ADC channel added in DT is supported.
>>
>> In case we need to add support for any more channels later, we could add
>> their macros in the binding file and update the array correspondingly at
>> that time.
>>
>> Does all this sound fine? Please let me know if you have any more concerns
>> or queries.
> 
> No, it doesn't.  You keep ignoring my arguments and responding to
> something else. I prefer not to store hardware values as bindings,
> because these are not bindings (and you failed to prove which SW
> interface they bind) and it's really not necessary.

In my previous replies in this thread, I missed mentioning that the macros
defined in include/dt-bindings/iio/adc/qcom,spmi-vadc.h are also used in
other places than the driver file - they are also used in the PMIC-specific
binding files added in this patch, for channel definitions. Considering
one channel for example:
 
We have this in include/dt-bindings/iio/adc/qcom,spmi-vadc.h:
+#define ADC5_GEN3_DIE_TEMP			0x03
 
The above is used in include/dt-bindings/iio/adc/qcom,pm8550vx-adc5-gen3.h:
+#define PM8550VS_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | ADC5_GEN3_DIE_TEMP)
 
And the above definition may be used in device tree, like in the example added
in Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml:
 
+        channel@203 {
+          reg = <PM8550VS_ADC5_GEN3_DIE_TEMP(2)>;
+          label = "pm8550vs_c_die_temp";
+          qcom,pre-scaling = <1 1>;
+        };

Referencing the same macros in driver and device tree should also help with
readability and lower the chances of accidental wrong configurations.
Based on this, can we consider ADC5_GEN3_DIE_TEMP is a valid binding and keep
it in place?
 
If not, and if you want the ADC5_GEN3_DIE_TEMP definition removed from bindings,
I can see two ways to do this:
 
1. Keep the PMIC-specific binding definitions, making updates to them like this:
 
-#define PM8550VS_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | ADC5_GEN3_DIE_TEMP)
+#define PM8550VS_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | 0x03)
 
and use the same macros in device tree, like above.
 
2. Drop the PMIC-specific binding definitions completely and update reg property like this:

-          reg = <PM8550VS_ADC5_GEN3_DIE_TEMP(2)>;
+          reg = <0x203>;
 
Which way would you prefer here?

Thanks,
Jishnu

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Krzysztof Kozlowski 4 months ago

On 08/10/2025 23:20, Jishnu Prakash wrote:
> Hi Krzysztof,
> 
> On 10/4/2025 12:22 PM, Krzysztof Kozlowski wrote:
>> On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
>> <jishnu.prakash@oss.qualcomm.com> wrote:
>>>
>>> Hi Jonathan,
>>>
>>> On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
>>>> On Fri, 19 Sep 2025 20:17:43 +0530
>>>> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
>>>>
>>>>> Hi Krzysztof,
>>>>>
>>>>> On 9/18/2025 5:45 AM, Krzysztof Kozlowski wrote:
>>>>>> On 18/09/2025 04:47, Jishnu Prakash wrote:
>>>>>>> Hi Krzysztof,
>>>>>>>
>>>>>>> On 9/17/2025 5:59 AM, Krzysztof Kozlowski wrote:
>>>>>>>> On 16/09/2025 16:28, Jishnu Prakash wrote:
>>>>>>>>>> You cannot have empty spaces in ID constants. These are abstract
>>>>>>>>>> numbers.
>>>>>>>>>>
>>>>>>>>>> Otherwise please point me to driver using this constant.
>>>>>>>>>
>>>>>>>>> These constants are for ADC channel numbers, which are fixed in HW.
>>>>>>>>>
>>>>>>>>> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
>>>>>>>>> which is added in patch 4 of this series.
>>>>>>>>>
>>>>>>>>> They can be found in the array named adc5_gen3_chans_pmic[].
>>>>>>>>
>>>>>>>> Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.
>>>>>>>>
>>>>>>>
>>>>>>> We may not be using all of these channels right now - we can add them
>>>>>>> later based on requirements coming up. For now, I'll remove the channels
>>>>>>> not used in adc5_gen3_chans_pmic[].
>>>>>>
>>>>>> You are not implementing the feedback then. Please read it carefully.
>>>>>>
>>>>>
>>>>> Sorry, I misunderstood - so you actually meant I should remove the
>>>>> empty spaces in the definitions, like this?
>>>>>
>>>>> -#define ADC5_GEN3_VREF_BAT_THERM               0x15
>>>>> +#define ADC5_GEN3_VREF_BAT_THERM 0x15
>>>>>
>>>>> I thought this at first, but I somehow doubted this later, as I saw some
>>>>> other recently added files with empty spaces in #define lines, like:
>>>>>
>>>>> include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h
>>>>> include/dt-bindings/regulator/st,stm32mp15-regulator.h
>>>>>
>>>>> I can make this change, if you prefer this. Please let me know
>>>>> if I'm still missing something.
>>>>>
>>>>> Also please let me know if you want me to remove the unused
>>>>> channels - I would prefer to keep them if there's no issue,
>>>>> as we might need them later.
>>>>>
>>>> He is referring to 0x14 and below not being defined values.  So what
>>>> do they mean if they turn up in the DT?
>>>>
>>>
>>> Thanks for your clarification. To address your first point above, the macros
>>> added here only represent the ADC channel numbers which are supported for
>>> ADC5 Gen3 devices. If there are numbers missing in between (like 0x14),
>>> that is because there exist no valid ADC channels in HW matching those
>>> channel numbers.
>>>
>>> For your question above, if any of the undefined channels are used in the DT,
>>> they should ideally be treated as invalid when parsed in the driver probe and
>>> lead to an error. When I checked the code again, I saw we do not have such an
>>> explicit check right now, so I will add that in the next patch series.
>>>
>>> And to be clear on which channel numbers are supported, I think it may be
>>> best if, for now, we only add support for the channel numbers referenced in
>>> the array adc5_gen3_chans_pmic[] in drivers/iio/adc/qcom-spmi-adc5-gen3.c.
>>>
>>> There are only 18 channel numbers used in this array and I would remove
>>> all channels except for these from the binding files. During parsing, we
>>> would use this array to confirm if an ADC channel added in DT is supported.
>>>
>>> In case we need to add support for any more channels later, we could add
>>> their macros in the binding file and update the array correspondingly at
>>> that time.
>>>
>>> Does all this sound fine? Please let me know if you have any more concerns
>>> or queries.
>>
>> No, it doesn't.  You keep ignoring my arguments and responding to
>> something else. I prefer not to store hardware values as bindings,
>> because these are not bindings (and you failed to prove which SW
>> interface they bind) and it's really not necessary.
> 
> In my previous replies in this thread, I missed mentioning that the macros
> defined in include/dt-bindings/iio/adc/qcom,spmi-vadc.h are also used in
> other places than the driver file - they are also used in the PMIC-specific
> binding files added in this patch, for channel definitions. Considering
> one channel for example:
>  
> We have this in include/dt-bindings/iio/adc/qcom,spmi-vadc.h:
> +#define ADC5_GEN3_DIE_TEMP			0x03
>  
> The above is used in include/dt-bindings/iio/adc/qcom,pm8550vx-adc5-gen3.h:
> +#define PM8550VS_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | ADC5_GEN3_DIE_TEMP)
>  
> And the above definition may be used in device tree, like in the example added
> in Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml:
>  
> +        channel@203 {
> +          reg = <PM8550VS_ADC5_GEN3_DIE_TEMP(2)>;
> +          label = "pm8550vs_c_die_temp";
> +          qcom,pre-scaling = <1 1>;
> +        };

This is not a driver. I do not understand your argumentation at all.

Best regards,
Krzysztof

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Jishnu Prakash 3 months, 3 weeks ago

Hi Krzysztof,

On 10/9/2025 5:22 AM, Krzysztof Kozlowski wrote:
> On 08/10/2025 23:20, Jishnu Prakash wrote:
>> Hi Krzysztof,
>>
>> On 10/4/2025 12:22 PM, Krzysztof Kozlowski wrote:
>>> On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
>>> <jishnu.prakash@oss.qualcomm.com> wrote:
>>>>
>>>> Hi Jonathan,
>>>>
>>>> On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
>>>>> On Fri, 19 Sep 2025 20:17:43 +0530
>>>>> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
>>>>>
>>>>>> Hi Krzysztof,
>>>>>>
>>>>>> On 9/18/2025 5:45 AM, Krzysztof Kozlowski wrote:
>>>>>>> On 18/09/2025 04:47, Jishnu Prakash wrote:
>>>>>>>> Hi Krzysztof,
>>>>>>>>
>>>>>>>> On 9/17/2025 5:59 AM, Krzysztof Kozlowski wrote:
>>>>>>>>> On 16/09/2025 16:28, Jishnu Prakash wrote:
>>>>>>>>>>> You cannot have empty spaces in ID constants. These are abstract
>>>>>>>>>>> numbers.
>>>>>>>>>>>
>>>>>>>>>>> Otherwise please point me to driver using this constant.
>>>>>>>>>>
>>>>>>>>>> These constants are for ADC channel numbers, which are fixed in HW.
>>>>>>>>>>
>>>>>>>>>> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
>>>>>>>>>> which is added in patch 4 of this series.
>>>>>>>>>>
>>>>>>>>>> They can be found in the array named adc5_gen3_chans_pmic[].
>>>>>>>>>
>>>>>>>>> Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.
>>>>>>>>>
>>>>>>>>
>>>>>>>> We may not be using all of these channels right now - we can add them
>>>>>>>> later based on requirements coming up. For now, I'll remove the channels
>>>>>>>> not used in adc5_gen3_chans_pmic[].
>>>>>>>
>>>>>>> You are not implementing the feedback then. Please read it carefully.
>>>>>>>
>>>>>>
>>>>>> Sorry, I misunderstood - so you actually meant I should remove the
>>>>>> empty spaces in the definitions, like this?
>>>>>>
>>>>>> -#define ADC5_GEN3_VREF_BAT_THERM               0x15
>>>>>> +#define ADC5_GEN3_VREF_BAT_THERM 0x15
>>>>>>
>>>>>> I thought this at first, but I somehow doubted this later, as I saw some
>>>>>> other recently added files with empty spaces in #define lines, like:
>>>>>>
>>>>>> include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h
>>>>>> include/dt-bindings/regulator/st,stm32mp15-regulator.h
>>>>>>
>>>>>> I can make this change, if you prefer this. Please let me know
>>>>>> if I'm still missing something.
>>>>>>
>>>>>> Also please let me know if you want me to remove the unused
>>>>>> channels - I would prefer to keep them if there's no issue,
>>>>>> as we might need them later.
>>>>>>
>>>>> He is referring to 0x14 and below not being defined values.  So what
>>>>> do they mean if they turn up in the DT?
>>>>>
>>>>
>>>> Thanks for your clarification. To address your first point above, the macros
>>>> added here only represent the ADC channel numbers which are supported for
>>>> ADC5 Gen3 devices. If there are numbers missing in between (like 0x14),
>>>> that is because there exist no valid ADC channels in HW matching those
>>>> channel numbers.
>>>>
>>>> For your question above, if any of the undefined channels are used in the DT,
>>>> they should ideally be treated as invalid when parsed in the driver probe and
>>>> lead to an error. When I checked the code again, I saw we do not have such an
>>>> explicit check right now, so I will add that in the next patch series.
>>>>
>>>> And to be clear on which channel numbers are supported, I think it may be
>>>> best if, for now, we only add support for the channel numbers referenced in
>>>> the array adc5_gen3_chans_pmic[] in drivers/iio/adc/qcom-spmi-adc5-gen3.c.
>>>>
>>>> There are only 18 channel numbers used in this array and I would remove
>>>> all channels except for these from the binding files. During parsing, we
>>>> would use this array to confirm if an ADC channel added in DT is supported.
>>>>
>>>> In case we need to add support for any more channels later, we could add
>>>> their macros in the binding file and update the array correspondingly at
>>>> that time.
>>>>
>>>> Does all this sound fine? Please let me know if you have any more concerns
>>>> or queries.
>>>
>>> No, it doesn't.  You keep ignoring my arguments and responding to
>>> something else. I prefer not to store hardware values as bindings,
>>> because these are not bindings (and you failed to prove which SW
>>> interface they bind) and it's really not necessary.

Sorry about the delay in replying. Let me go step by step
over the use of the macros and how they are used by clients
SW.

1. In ADC Gen3, this is the superset of channels supported on all
PMICs (with ADC):

Ref: include/dt-bindings/iio/adc/qcom,spmi-vadc.h

/* 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
....


2. Since some PMICs may not have all of these channels supported in
HW, we have the PMIC-specific channel definitions (starting with PMIC
name like PM8550_..) made referencing the above definitions.

Ref: include/dt-bindings/iio/adc/qcom,pm8550-adc5-gen3.h:
...
    #define PM8550_ADC5_GEN3_DIE_TEMP(sid)	((sid) << 8 | ADC5_GEN3_DIE_TEMP)
...

side note: This is also used for the "reg" property in the ADC channel
definition DT nodes.

Here `sid` is needed as there can be different instances of same PMIC
using different `sid`s on a single SoC, and also on different SoCs, the
same PMIC may have different `sid`s.


3. This PMIC-specific definition will be used by clients like below
(in io-channels) to get the ADC channel they need to read.

    pmic@1 {
        temp-alarm@a00 {
            compatible = "qcom,spmi-temp-alarm";
	    ...
            io-channels = <&pmk8550_adc PM8550_ADC5_GEN3_DIE_TEMP(1)>;
            io-channel-names = "thermal";
        };
    };


Can you please provide your suggestions on changes we can make
in the above points ?


>>
>> In my previous replies in this thread, I missed mentioning that the macros
>> defined in include/dt-bindings/iio/adc/qcom,spmi-vadc.h are also used in
>> other places than the driver file - they are also used in the PMIC-specific
>> binding files added in this patch, for channel definitions. Considering
>> one channel for example:
>>  
>> We have this in include/dt-bindings/iio/adc/qcom,spmi-vadc.h:
>> +#define ADC5_GEN3_DIE_TEMP			0x03
>>  
>> The above is used in include/dt-bindings/iio/adc/qcom,pm8550vx-adc5-gen3.h:
>> +#define PM8550VS_ADC5_GEN3_DIE_TEMP(sid)			((sid) << 8 | ADC5_GEN3_DIE_TEMP)
>>  
>> And the above definition may be used in device tree, like in the example added
>> in Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml:
>>  
>> +        channel@203 {
>> +          reg = <PM8550VS_ADC5_GEN3_DIE_TEMP(2)>;
>> +          label = "pm8550vs_c_die_temp";
>> +          qcom,pre-scaling = <1 1>;
>> +        };
> 
> This is not a driver. I do not understand your argumentation at all.
> 

I hope with the comments above I was able to explain the purpose of
the different macros added in dt-bindings. Please let me know if
you want me to elaborate anything more.

Thanks,
Jishnu

> Best regards,
> Krzysztof

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Krzysztof Kozlowski 3 months, 3 weeks ago

On 17/10/2025 13:18, Jishnu Prakash wrote:
> Hi Krzysztof,
> 
> On 10/9/2025 5:22 AM, Krzysztof Kozlowski wrote:
>> On 08/10/2025 23:20, Jishnu Prakash wrote:
>>> Hi Krzysztof,
>>>
>>> On 10/4/2025 12:22 PM, Krzysztof Kozlowski wrote:
>>>> On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
>>>> <jishnu.prakash@oss.qualcomm.com> wrote:
>>>>>
>>>>> Hi Jonathan,
>>>>>
>>>>> On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
>>>>>> On Fri, 19 Sep 2025 20:17:43 +0530
>>>>>> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
>>>>>>
>>>>>>> Hi Krzysztof,
>>>>>>>
>>>>>>> On 9/18/2025 5:45 AM, Krzysztof Kozlowski wrote:
>>>>>>>> On 18/09/2025 04:47, Jishnu Prakash wrote:
>>>>>>>>> Hi Krzysztof,
>>>>>>>>>
>>>>>>>>> On 9/17/2025 5:59 AM, Krzysztof Kozlowski wrote:
>>>>>>>>>> On 16/09/2025 16:28, Jishnu Prakash wrote:
>>>>>>>>>>>> You cannot have empty spaces in ID constants. These are abstract
>>>>>>>>>>>> numbers.
>>>>>>>>>>>>
>>>>>>>>>>>> Otherwise please point me to driver using this constant.
>>>>>>>>>>>
>>>>>>>>>>> These constants are for ADC channel numbers, which are fixed in HW.
>>>>>>>>>>>
>>>>>>>>>>> They are used in this driver: drivers/iio/adc/qcom-spmi-adc5-gen3.c,
>>>>>>>>>>> which is added in patch 4 of this series.
>>>>>>>>>>>
>>>>>>>>>>> They can be found in the array named adc5_gen3_chans_pmic[].
>>>>>>>>>>
>>>>>>>>>> Really? So point me to the line there using ADC5_GEN3_VREF_BAT_THERM.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> We may not be using all of these channels right now - we can add them
>>>>>>>>> later based on requirements coming up. For now, I'll remove the channels
>>>>>>>>> not used in adc5_gen3_chans_pmic[].
>>>>>>>>
>>>>>>>> You are not implementing the feedback then. Please read it carefully.
>>>>>>>>
>>>>>>>
>>>>>>> Sorry, I misunderstood - so you actually meant I should remove the
>>>>>>> empty spaces in the definitions, like this?
>>>>>>>
>>>>>>> -#define ADC5_GEN3_VREF_BAT_THERM               0x15
>>>>>>> +#define ADC5_GEN3_VREF_BAT_THERM 0x15
>>>>>>>
>>>>>>> I thought this at first, but I somehow doubted this later, as I saw some
>>>>>>> other recently added files with empty spaces in #define lines, like:
>>>>>>>
>>>>>>> include/dt-bindings/iio/adc/mediatek,mt6373-auxadc.h
>>>>>>> include/dt-bindings/regulator/st,stm32mp15-regulator.h
>>>>>>>
>>>>>>> I can make this change, if you prefer this. Please let me know
>>>>>>> if I'm still missing something.
>>>>>>>
>>>>>>> Also please let me know if you want me to remove the unused
>>>>>>> channels - I would prefer to keep them if there's no issue,
>>>>>>> as we might need them later.
>>>>>>>
>>>>>> He is referring to 0x14 and below not being defined values.  So what
>>>>>> do they mean if they turn up in the DT?
>>>>>>
>>>>>
>>>>> Thanks for your clarification. To address your first point above, the macros
>>>>> added here only represent the ADC channel numbers which are supported for
>>>>> ADC5 Gen3 devices. If there are numbers missing in between (like 0x14),
>>>>> that is because there exist no valid ADC channels in HW matching those
>>>>> channel numbers.
>>>>>
>>>>> For your question above, if any of the undefined channels are used in the DT,
>>>>> they should ideally be treated as invalid when parsed in the driver probe and
>>>>> lead to an error. When I checked the code again, I saw we do not have such an
>>>>> explicit check right now, so I will add that in the next patch series.
>>>>>
>>>>> And to be clear on which channel numbers are supported, I think it may be
>>>>> best if, for now, we only add support for the channel numbers referenced in
>>>>> the array adc5_gen3_chans_pmic[] in drivers/iio/adc/qcom-spmi-adc5-gen3.c.
>>>>>
>>>>> There are only 18 channel numbers used in this array and I would remove
>>>>> all channels except for these from the binding files. During parsing, we
>>>>> would use this array to confirm if an ADC channel added in DT is supported.
>>>>>
>>>>> In case we need to add support for any more channels later, we could add
>>>>> their macros in the binding file and update the array correspondingly at
>>>>> that time.
>>>>>
>>>>> Does all this sound fine? Please let me know if you have any more concerns
>>>>> or queries.
>>>>
>>>> No, it doesn't.  You keep ignoring my arguments and responding to
>>>> something else. I prefer not to store hardware values as bindings,
>>>> because these are not bindings (and you failed to prove which SW
>>>> interface they bind) and it's really not necessary.
> 
> Sorry about the delay in replying. Let me go step by step
> over the use of the macros and how they are used by clients
> SW.
> 
> 1. In ADC Gen3, this is the superset of channels supported on all
> PMICs (with ADC):
> 
> Ref: include/dt-bindings/iio/adc/qcom,spmi-vadc.h

That's not a driver. Not SW.

> 
> /* 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
> ....
> 
> 
> 2. Since some PMICs may not have all of these channels supported in
> HW, we have the PMIC-specific channel definitions (starting with PMIC
> name like PM8550_..) made referencing the above definitions.
> 
> Ref: include/dt-bindings/iio/adc/qcom,pm8550-adc5-gen3.h:

That's not a driver. Not SW.


> ...
>     #define PM8550_ADC5_GEN3_DIE_TEMP(sid)	((sid) << 8 | ADC5_GEN3_DIE_TEMP)
> ...
> 
> side note: This is also used for the "reg" property in the ADC channel
> definition DT nodes.
> 
> Here `sid` is needed as there can be different instances of same PMIC
> using different `sid`s on a single SoC, and also on different SoCs, the
> same PMIC may have different `sid`s.
> 
> 
> 3. This PMIC-specific definition will be used by clients like below
> (in io-channels) to get the ADC channel they need to read.
> 
>     pmic@1 {
>         temp-alarm@a00 {
>             compatible = "qcom,spmi-temp-alarm";
> 	    ...
>             io-channels = <&pmk8550_adc PM8550_ADC5_GEN3_DIE_TEMP(1)>;
>             io-channel-names = "thermal";
>         };
>     };

That's not a driver. Not SW.

> 
> 
> Can you please provide your suggestions on changes we can make
> in the above points ?

You just pasted DT. I asked about SW, software. Please read carefully
previous comments.


Best regards,
Krzysztof

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Konrad Dybcio 3 months, 3 weeks ago

On 10/17/25 3:40 PM, Krzysztof Kozlowski wrote:
> On 17/10/2025 13:18, Jishnu Prakash wrote:
>> Hi Krzysztof,
>>
>> On 10/9/2025 5:22 AM, Krzysztof Kozlowski wrote:
>>> On 08/10/2025 23:20, Jishnu Prakash wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 10/4/2025 12:22 PM, Krzysztof Kozlowski wrote:
>>>>> On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
>>>>> <jishnu.prakash@oss.qualcomm.com> wrote:
>>>>>>
>>>>>> Hi Jonathan,
>>>>>>
>>>>>> On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
>>>>>>> On Fri, 19 Sep 2025 20:17:43 +0530
>>>>>>> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:

[...]

>> Can you please provide your suggestions on changes we can make
>> in the above points ?
> 
> You just pasted DT. I asked about SW, software. Please read carefully
> previous comments.

Is the problem that Jishnu included some indices in dt-bindings without
also adding them in the driver's adc5_gen3_chans_pmic[] array?

As in, would the resolution to this thread be simply handling all of
them in the driver correctly?

Konrad

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Krzysztof Kozlowski 3 months, 3 weeks ago

On 20/10/2025 14:51, Konrad Dybcio wrote:
> On 10/17/25 3:40 PM, Krzysztof Kozlowski wrote:
>> On 17/10/2025 13:18, Jishnu Prakash wrote:
>>> Hi Krzysztof,
>>>
>>> On 10/9/2025 5:22 AM, Krzysztof Kozlowski wrote:
>>>> On 08/10/2025 23:20, Jishnu Prakash wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On 10/4/2025 12:22 PM, Krzysztof Kozlowski wrote:
>>>>>> On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
>>>>>> <jishnu.prakash@oss.qualcomm.com> wrote:
>>>>>>>
>>>>>>> Hi Jonathan,
>>>>>>>
>>>>>>> On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
>>>>>>>> On Fri, 19 Sep 2025 20:17:43 +0530
>>>>>>>> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
> 
> [...]
> 
>>> Can you please provide your suggestions on changes we can make
>>> in the above points ?
>>
>> You just pasted DT. I asked about SW, software. Please read carefully
>> previous comments.
> 
> Is the problem that Jishnu included some indices in dt-bindings without
> also adding them in the driver's adc5_gen3_chans_pmic[] array?
> 
> As in, would the resolution to this thread be simply handling all of
> them in the driver correctly?

The solution is to remove them from the bindings, just like we do with
many other hardware constants. Of course if these are not hardware
constants, but part of ABI, then solution would be different but no one
provided proof or argument that this is any binding. All proofs were
"but I want to use it in my DTS", which proofs nothing. Not a binding.

While this issue is not that important, we keep discussing it because
author does not try to understand the problem or even keep up the
discussion. Instead repeats the same without really reading my
messages... and then disappears for month or more.

Best regards,
Krzysztof

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Konrad Dybcio 3 months, 2 weeks ago

On 20-Oct-25 17:55, Krzysztof Kozlowski wrote:
> On 20/10/2025 14:51, Konrad Dybcio wrote:
>> On 10/17/25 3:40 PM, Krzysztof Kozlowski wrote:
>>> On 17/10/2025 13:18, Jishnu Prakash wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 10/9/2025 5:22 AM, Krzysztof Kozlowski wrote:
>>>>> On 08/10/2025 23:20, Jishnu Prakash wrote:
>>>>>> Hi Krzysztof,
>>>>>>
>>>>>> On 10/4/2025 12:22 PM, Krzysztof Kozlowski wrote:
>>>>>>> On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
>>>>>>> <jishnu.prakash@oss.qualcomm.com> wrote:
>>>>>>>>
>>>>>>>> Hi Jonathan,
>>>>>>>>
>>>>>>>> On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
>>>>>>>>> On Fri, 19 Sep 2025 20:17:43 +0530
>>>>>>>>> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
>>
>> [...]
>>
>>>> Can you please provide your suggestions on changes we can make
>>>> in the above points ?
>>>
>>> You just pasted DT. I asked about SW, software. Please read carefully
>>> previous comments.
>>
>> Is the problem that Jishnu included some indices in dt-bindings without
>> also adding them in the driver's adc5_gen3_chans_pmic[] array?
>>
>> As in, would the resolution to this thread be simply handling all of
>> them in the driver correctly?
> 
> The solution is to remove them from the bindings, just like we do with
> many other hardware constants. Of course if these are not hardware
> constants, but part of ABI, then solution would be different but no one
> provided proof or argument that this is any binding. All proofs were
> "but I want to use it in my DTS", which proofs nothing. Not a binding.
> 
> While this issue is not that important, we keep discussing it because
> author does not try to understand the problem or even keep up the
> discussion. Instead repeats the same without really reading my
> messages... and then disappears for month or more.

In Bulgaria, people shake their heads left to right to say "yes", and
up&down to say "no" (or so I've heard).. I feel like we're having a
similar situation here..

I'll try to make a case for keeping these defines in some form.
Here's hopefully all the related aspects, condensed down:

1. In a multi-PMIC setup, only the main PMIC's ADC is accessible by the OS.
   It then mediates accesses to secondary PMICs' ADCs through internal
   mechanisms, which requires the SID of the target to be retrieved and written
   to a register, along with the physical index of the desired channel to be
   measured (see patch 3/5 commit msg).

2. The PMIC SIDs are fixed per board and are the values of PMIC top-level
   nodes' reg property (since forever)

3. The channel indices are fixed in HW, but this patchset proposed to reuse
   them for logical mappings consumed through io-channels = <> as well (because
   of 1.), with the drivers taking the lower 8 bits that of reg/io-channels[1]
   value as the ADC channel id and the higher 8 bits as the SID (this is the
   define macros with an argument)

4. Fixing 3. in a "simply define all possible options and bind them to
   consecutive integers" fashion would require a huge table matching 0..n to
   [0-max_sid][0-max_chan] which is unreasonable

The alternative to the SID packing would be to reference the target PMIC
somehow, be it by referencing the PMIC itself:

io-channels = <&pm8550_adc &pmr735a CHANNEL_XYZ>

or by creating a faux node for the actual inaccessible ADC onboard each of
the PMICs:

io-channels = <&pm8550_adc &pmr735a_adc CHANNEL_XYZ>

and have the OS retrieve the SID from the DT node & encode that value instead
of hardcoding it in the DT, leaving just the actual channel IDs in dt-bindings.

The define macros without an argument do specify physical channel indices, but
we do need some sort of an identifier to put into io-channels (which is why this
lives in dt-bindings in the first place), and a 1:1 mapping to the physical id
sounds like a good option.

I don't think anyone objects to any of these resolutions, so long as they
are acceptable from your side

Konrad

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Krzysztof Kozlowski 3 months, 2 weeks ago

On 22/10/2025 13:02, Konrad Dybcio wrote:
>>>>>>>
>>>>>>> On 10/4/2025 12:22 PM, Krzysztof Kozlowski wrote:
>>>>>>>> On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
>>>>>>>> <jishnu.prakash@oss.qualcomm.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi Jonathan,
>>>>>>>>>
>>>>>>>>> On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
>>>>>>>>>> On Fri, 19 Sep 2025 20:17:43 +0530
>>>>>>>>>> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
>>>
>>> [...]
>>>
>>>>> Can you please provide your suggestions on changes we can make
>>>>> in the above points ?
>>>>
>>>> You just pasted DT. I asked about SW, software. Please read carefully
>>>> previous comments.
>>>
>>> Is the problem that Jishnu included some indices in dt-bindings without
>>> also adding them in the driver's adc5_gen3_chans_pmic[] array?
>>>
>>> As in, would the resolution to this thread be simply handling all of
>>> them in the driver correctly?
>>
>> The solution is to remove them from the bindings, just like we do with
>> many other hardware constants. Of course if these are not hardware
>> constants, but part of ABI, then solution would be different but no one
>> provided proof or argument that this is any binding. All proofs were
>> "but I want to use it in my DTS", which proofs nothing. Not a binding.
>>
>> While this issue is not that important, we keep discussing it because
>> author does not try to understand the problem or even keep up the
>> discussion. Instead repeats the same without really reading my
>> messages... and then disappears for month or more.
> 
> In Bulgaria, people shake their heads left to right to say "yes", and
> up&down to say "no" (or so I've heard).. I feel like we're having a
> similar situation here..
> 
> I'll try to make a case for keeping these defines in some form.
> Here's hopefully all the related aspects, condensed down:
> 
> 1. In a multi-PMIC setup, only the main PMIC's ADC is accessible by the OS.
>    It then mediates accesses to secondary PMICs' ADCs through internal
>    mechanisms, which requires the SID of the target to be retrieved and written
>    to a register, along with the physical index of the desired channel to be
>    measured (see patch 3/5 commit msg).


SID is still a hardware value, right? Combination of two hardware values
is still a hardware value, not a software ABI construct. Even if you
claim only the driver can decode it. These are still the hardware values.

If you had two IIO cells - one for SID and one for ADC channel - would
you claim they are both needed for software? Probably not.

io-channels = <&pm8550_adc SID_whatever CHANNEL_XYZ>

It's basically the same as some pin muxes, like NXP:
git grep MX8MM_IOMUXC_GPIO1_IO04_GPIO1_IO4

Complex value, which driver parses. Is it SW construct? No. These are
register values.


> 
> 2. The PMIC SIDs are fixed per board and are the values of PMIC top-level
>    nodes' reg property (since forever)
> 
> 3. The channel indices are fixed in HW, but this patchset proposed to reuse
>    them for logical mappings consumed through io-channels = <> as well (because
>    of 1.), with the drivers taking the lower 8 bits that of reg/io-channels[1]
>    value as the ADC channel id and the higher 8 bits as the SID (this is the
>    define macros with an argument)
> 
> 4. Fixing 3. in a "simply define all possible options and bind them to
>    consecutive integers" fashion would require a huge table matching 0..n to
>    [0-max_sid][0-max_chan] which is unreasonable

I do not insist on fixing anything or changing the interface. I only
question their necessity to be a binding.

> 
> The alternative to the SID packing would be to reference the target PMIC
> somehow, be it by referencing the PMIC itself:
> 
> io-channels = <&pm8550_adc &pmr735a CHANNEL_XYZ>
> 
> or by creating a faux node for the actual inaccessible ADC onboard each of
> the PMICs:
> 
> io-channels = <&pm8550_adc &pmr735a_adc CHANNEL_XYZ>
> 
> and have the OS retrieve the SID from the DT node & encode that value instead
> of hardcoding it in the DT, leaving just the actual channel IDs in dt-bindings.
> 
> 
> The define macros without an argument do specify physical channel indices, but
> we do need some sort of an identifier to put into io-channels (which is why this
> lives in dt-bindings in the first place), and a 1:1 mapping to the physical id
> sounds like a good option.
> 
> 
> I don't think anyone objects to any of these resolutions, so long as they
> are acceptable from your side

You can go these ways, but I never proposed to change the interface.
Just don't store these as bindings, so they don't have to be treated as
ABI, because they are not. They do not constitute the contract between
software (driver) and DTS. Treat them the same as we treat all hardware
constants - directly encoded or moved to DTS headers (which we did for
many of such cases already - see commit
9d9292576810d0b36897718c24dfbc1a2835314b 3 years ago and other)

Best regards,
Krzysztof

Re: [PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Posted by Jishnu Prakash 3 months, 1 week ago

Hi Krzysztof,

On 10/27/2025 10:00 PM, Krzysztof Kozlowski wrote:
> On 22/10/2025 13:02, Konrad Dybcio wrote:
>>>>>>>>
>>>>>>>> On 10/4/2025 12:22 PM, Krzysztof Kozlowski wrote:
>>>>>>>>> On Sat, 4 Oct 2025 at 11:42, Jishnu Prakash
>>>>>>>>> <jishnu.prakash@oss.qualcomm.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Jonathan,
>>>>>>>>>>
>>>>>>>>>> On 9/27/2025 7:17 PM, Jonathan Cameron wrote:
>>>>>>>>>>> On Fri, 19 Sep 2025 20:17:43 +0530
>>>>>>>>>>> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
>>>>
>>>> [...]
>>>>
>>>>>> Can you please provide your suggestions on changes we can make
>>>>>> in the above points ?
>>>>>
>>>>> You just pasted DT. I asked about SW, software. Please read carefully
>>>>> previous comments.
>>>>
>>>> Is the problem that Jishnu included some indices in dt-bindings without
>>>> also adding them in the driver's adc5_gen3_chans_pmic[] array?
>>>>
>>>> As in, would the resolution to this thread be simply handling all of
>>>> them in the driver correctly?
>>>
>>> The solution is to remove them from the bindings, just like we do with
>>> many other hardware constants. Of course if these are not hardware
>>> constants, but part of ABI, then solution would be different but no one
>>> provided proof or argument that this is any binding. All proofs were
>>> "but I want to use it in my DTS", which proofs nothing. Not a binding.
>>>
>>> While this issue is not that important, we keep discussing it because
>>> author does not try to understand the problem or even keep up the
>>> discussion. Instead repeats the same without really reading my
>>> messages... and then disappears for month or more.
>>
>> In Bulgaria, people shake their heads left to right to say "yes", and
>> up&down to say "no" (or so I've heard).. I feel like we're having a
>> similar situation here..
>>
>> I'll try to make a case for keeping these defines in some form.
>> Here's hopefully all the related aspects, condensed down:
>>
>> 1. In a multi-PMIC setup, only the main PMIC's ADC is accessible by the OS.
>>    It then mediates accesses to secondary PMICs' ADCs through internal
>>    mechanisms, which requires the SID of the target to be retrieved and written
>>    to a register, along with the physical index of the desired channel to be
>>    measured (see patch 3/5 commit msg).
> 
> 
> SID is still a hardware value, right? Combination of two hardware values
> is still a hardware value, not a software ABI construct. Even if you
> claim only the driver can decode it. These are still the hardware values.
> 
> If you had two IIO cells - one for SID and one for ADC channel - would
> you claim they are both needed for software? Probably not.
> 
> io-channels = <&pm8550_adc SID_whatever CHANNEL_XYZ>
> 
> It's basically the same as some pin muxes, like NXP:
> git grep MX8MM_IOMUXC_GPIO1_IO04_GPIO1_IO4
> 
> Complex value, which driver parses. Is it SW construct? No. These are
> register values.
> 
> 
>>
>> 2. The PMIC SIDs are fixed per board and are the values of PMIC top-level
>>    nodes' reg property (since forever)
>>
>> 3. The channel indices are fixed in HW, but this patchset proposed to reuse
>>    them for logical mappings consumed through io-channels = <> as well (because
>>    of 1.), with the drivers taking the lower 8 bits that of reg/io-channels[1]
>>    value as the ADC channel id and the higher 8 bits as the SID (this is the
>>    define macros with an argument)
>>
>> 4. Fixing 3. in a "simply define all possible options and bind them to
>>    consecutive integers" fashion would require a huge table matching 0..n to
>>    [0-max_sid][0-max_chan] which is unreasonable
> 
> I do not insist on fixing anything or changing the interface. I only
> question their necessity to be a binding.
> 
>>
>> The alternative to the SID packing would be to reference the target PMIC
>> somehow, be it by referencing the PMIC itself:
>>
>> io-channels = <&pm8550_adc &pmr735a CHANNEL_XYZ>
>>
>> or by creating a faux node for the actual inaccessible ADC onboard each of
>> the PMICs:
>>
>> io-channels = <&pm8550_adc &pmr735a_adc CHANNEL_XYZ>
>>
>> and have the OS retrieve the SID from the DT node & encode that value instead
>> of hardcoding it in the DT, leaving just the actual channel IDs in dt-bindings.
>>
>>
>> The define macros without an argument do specify physical channel indices, but
>> we do need some sort of an identifier to put into io-channels (which is why this
>> lives in dt-bindings in the first place), and a 1:1 mapping to the physical id
>> sounds like a good option.
>>
>>
>> I don't think anyone objects to any of these resolutions, so long as they
>> are acceptable from your side
> 
> You can go these ways, but I never proposed to change the interface.
> Just don't store these as bindings, so they don't have to be treated as
> ABI, because they are not. They do not constitute the contract between
> software (driver) and DTS. Treat them the same as we treat all hardware
> constants - directly encoded or moved to DTS headers (which we did for
> many of such cases already - see commit
> 9d9292576810d0b36897718c24dfbc1a2835314b 3 years ago and other)
> 

Thanks for your explanation and the above suggestion and example,
it's clear now. I'll move the macros we need into a DTS header file.


Also to Lee/Rob/Jonathan, as you had given your Acked-by tags on
patch 1 - based on the discussion in this thread, I think it's
also best to update patch 1 similarly now, to move the ADC channel macros
from the include/dt-bindings/iio folder to the arch/arm(64)/boot/dts/qcom
folders. I'll make this update in the next version, please have a look
at it too.

Thanks,
Jishnu

> Best regards,
> Krzysztof

[PATCH V7 1/5] dt-bindings: iio/adc: Move QCOM ADC bindings to iio/adc folder
[PATCH V7 2/5] dt-bindings: iio: adc: Split out QCOM VADC channel properties
[PATCH V7 3/5] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC
[PATCH V7 4/5] iio: adc: Add support for QCOM PMIC5 Gen3 ADC
[PATCH V7 5/5] thermal: qcom: add support for PMIC5 Gen3 ADC thermal monitoring