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

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Rob Herring <robh@kernel.org>
Cc: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Zhang Rui <rui.zhang@intel.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Amit Kucheria <amitk@kernel.org>,
	Jonathan Cameron <jic23@kernel.org>,
	Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
	devicetree@vger.kernel.org, linux-iio@vger.kernel.org,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH v5 1/9] dt-bindings: thermal: qcom: add adc-thermal monitor bindings
Date: Wed, 23 Sep 2020 12:07:22 +0300	[thread overview]
Message-ID: <55d3f181-b9e6-4963-9d0c-cefee875058c@linaro.org> (raw)
In-Reply-To: <20200922234025.GA3476652@bogus>

On 23/09/2020 02:40, Rob Herring wrote:
> On Mon, Sep 14, 2020 at 06:48:01PM +0300, Dmitry Baryshkov wrote:
>> 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 <dmitry.baryshkov@linaro.org>
>> 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.

> 
>> +    type: object
>> +    description:
>> +      Represent one thermal sensor.
>> +
>> +    properties:
>> +      reg:
>> +        description: Specify the sensor channel.
>> +        maxItems: 1
> 
> You need a range of values here.

ok.

> 
>> +
>> +      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.

> 
>> +
>> +      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?

> 
>> +
>> +      qcom,pre-scaling:
>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>> +        description: Used for scaling the channel input signal before the
>> +          signal is fed to VADC. See qcom,spi-vadc specification for the list
>> +          of possible values.
> 
> I'd rather not. Need the values here to validate a DT.

OK

> 
>> +        minItems: 2
>> +        maxItems: 2
>> +
>> +    required:
>> +      - reg
>> +      - qcom,adc-channel
>> +
>> +    additionalProperties:
>> +      false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +  - "#thermal-sensor-cells"
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/iio/qcom,spmi-vadc.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    spmi_bus {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        pm8150b_adc: adc@3100 {
>> +            reg = <0x3100>;
>> +            compatible = "qcom,spmi-adc5";
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +            #io-channel-cells = <1>;
>> +            io-channel-ranges;
>> +
>> +            /* Other propreties are omitted */
>> +            conn-therm@4f {
>> +                reg = <ADC5_AMUX_THM3_100K_PU>;
>> +                qcom,ratiometric;
>> +                qcom,hw-settle-time = <200>;
>> +            };
>> +        };
>> +
>> +        pm8150b_adc_tm: adc-tm@3500 {
>> +            compatible = "qcom,spmi-adc-tm5";
>> +            reg = <0x3500>;
>> +            interrupts = <0x2 0x35 0x0 IRQ_TYPE_EDGE_RISING>;
>> +            #thermal-sensor-cells = <1>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            conn-therm@0 {
>> +                reg = <0>;
>> +                io-channels = <&pm8150b_adc ADC5_AMUX_THM3_100K_PU>;
>> +                qcom,adc-channel = <ADC5_AMUX_THM3_100K_PU>;
>> +                qcom,ratiometric;
>> +                qcom,hw-settle-time = <200>;
>> +            };
>> +        };
>> +    };
>> +...
>> -- 
>> 2.28.0
>>

-- 
With best wishes
Dmitry