Re: [PATCH v5 1/9] dt-bindings: thermal: qcom: add adc-thermal monitor bindings

On Wed, 23 Sep 2020 08:00:29 -0600
Rob Herring [off-list ref] wrote:

On Wed, Sep 23, 2020 at 3:07 AM Dmitry Baryshkov
[off-list ref] wrote:

quoted

On 23/09/2020 02:40, Rob Herring wrote:  

quoted

On Mon, Sep 14, 2020 at 06:48:01PM +0300, Dmitry Baryshkov wrote:  

quoted

Add bindings for thermal monitor, part of Qualcomm PMIC5 chips. It is a
close counterpart of VADC part of those PMICs.

Signed-off-by: Dmitry Baryshkov <redacted>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
  .../bindings/thermal/qcom-spmi-adc-tm5.yaml   | 151 ++++++++++++++++++
  1 file changed, 151 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml

diff --git a/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml b/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml
new file mode 100644
index 000000000000..432a65839b89
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml

@@ -0,0 +1,151 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/qcom-spmi-adc-tm5.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm's SPMI PMIC ADC Thermal Monitoring
+maintainers:
+  - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+
+properties:
+  compatible:
+    const: qcom,spmi-adc-tm5
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#thermal-sensor-cells":
+    const: 1
+    description:
+      Number of cells required to uniquely identify the thermal sensors. Since
+      we have multiple sensors this is set to 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  qcom,avg-samples:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Number of samples to be used for measurement.
+    enum:
+      - 1
+      - 2
+      - 4
+      - 8
+      - 16
+    default: 1
+
+  qcom,decimation:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: This parameter is used to decrease ADC sampling rate.
+            Quicker measurements can be made by reducing decimation ratio.
+    enum:
+      - 250
+      - 420
+      - 840
+    default: 840
+
+patternProperties:
+  "^([-a-z0-9]*)@[0-9]+$":  

Less than 10 as unit-addresses are hex?  

8 channels at max currently. I'll fix to use hex though.  

Then it should be @[0-7]$

quoted

quoted

quoted

+    type: object
+    description:
+      Represent one thermal sensor.
+
+    properties:
+      reg:
+        description: Specify the sensor channel.
+        maxItems: 1  

You need a range of values here.  

ok.
 

quoted

quoted

+
+      io-channels:
+        description:
+          From common IIO binding. Used to pipe PMIC ADC channel to thermal monitor
+
+      qcom,adc-channel:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: Corresponding ADC channel ID.  

Why is this not a cell in io-channels?  

Do you mean parsing a cell from io-channels rather than specifying it
again? Sounds like a good idea.  

Yes.

quoted

quoted

quoted

+      qcom,ratiometric:
+        $ref: /schemas/types.yaml#/definitions/flag
+        description:
+          Channel calibration type.
+          If this property is specified VADC will use the VDD reference
+          (1.875V) and GND for channel calibration. If property is not found,
+          channel will be calibrated with 0V and 1.25V reference channels,
+          also known as absolute calibration.
+
+      qcom,hw-settle-time:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: Time between AMUX getting configured and the ADC starting conversion.  

Time values should have a unit suffix. Seems like a commmon ADC
property...  

Could you please be more specific here? Would you like for me to just
specify the unit in the description?  

More a question for Jonathan I guess as to whether this should be
common or not. Maybe we have something already. Settle or acquisition
time is a common thing for ADCs, right?

It's not common in my experience, but not unheard of.
Only cases currently supporting controlling it explicitly in the
kernel that I can spot, are currently all qcom parts
(I'd guess they are all using the same IP underneath)

I think it is usually it's a case of building your analog
circuitry to match the spec of your ADC / MUX rather than
relaxing that spec by adding a delay.

It's a property with obvious enough meaning though that I
wouldn't have a problem with it being a generic property even
it is one that doesn't actually get used much.

Properties with units need a suffix as defined in
.../bindings/property-units.txt.

We have a bunch of legacy bindings that don't have units but
all new ones certainly should!

Thanks,

Jonathan

Rob