mbox series

[v5,0/9] qcom: pm8150: add support for thermal monitoring

Message ID 20200914154809.192174-1-dmitry.baryshkov@linaro.org
Headers show
Series qcom: pm8150: add support for thermal monitoring | expand

Message

Dmitry Baryshkov Sept. 14, 2020, 3:48 p.m. UTC
This patch serie adds support for thermal monitoring block on Qualcomm's
PMIC5 chips. PM8150{,b,l} and sm8250-mtp board device trees are extended
to support thermal zones provided by this thermal monitoring block.
Unlike the rest of PMIC thermal senses, these thermal zones describe
particular thermistors, which differ between from board to board.

Changes since v4:
 - Added kernel-doc comments to ADC-TM structures
 - Used several sizeof(buf) instead of hand-conding register size

Changes since v3:
 - Fix DT description to spell "thermal monitoring" instead of just TM
 - Fix warnings in DT example
 - Add EXPORT_SYMBOL_GPL(of_iio_channel_get_by_name)
 - Fixed whitespace chanes in qcom-vadc-common.c
 - Removed error message if IIO chanel get returns -EPROBE_DEFER

Changes since v2:
 - IIO: export of_iio_channel_get_by_name() function
 - dt-bindings: move individual io-channels to each thermal monitoring
   channel rather than listing them all in device node
 - added fallback defaults to of_device_get_match_data calls in
   qcom-spmi-adc5 and qcom-spmi-adc-tm5 drivers
 - minor typo fixes

Changes since v1:
 - Introduce fixp_linear_interpolate() by Craig Tatlor
 - Lots of syntax/whitespace changes
 - Cleaned up register definitions per Jonathan's suggestion
 - Implemented most of the suggestions from Bjorn's and Jonathan's
   review

Comments

Rob Herring (Arm) Sept. 22, 2020, 11:40 p.m. UTC | #1
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?

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

You need a range of values here.

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

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

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

> +        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
>
Dmitry Baryshkov Sept. 23, 2020, 9:07 a.m. UTC | #2
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
>>
Rob Herring (Arm) Sept. 23, 2020, 2 p.m. UTC | #3
On Wed, Sep 23, 2020 at 3:07 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> 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.

Then it should be @[0-7]$

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

Yes.

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

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

Rob
Jonathan Cameron Sept. 23, 2020, 3:22 p.m. UTC | #4
On Wed, 23 Sep 2020 08:00:29 -0600
Rob Herring <robh@kernel.org> wrote:

> On Wed, Sep 23, 2020 at 3:07 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > 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.  
> 
> Then it should be @[0-7]$
> 
> > >> +    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.  
> 
> Yes.
> 
> > >> +      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
Jishnu Prakash Sept. 25, 2020, 11:42 a.m. UTC | #5
Hi Dmitry,

Recently I was testing the ADC_TM driver added with these changes on 
SC7180 and I could see that this patch needs a few changes, which I'll 
mention below.

On 9/14/2020 9:18 PM, Dmitry Baryshkov wrote:
> Add support for Thermal Monitoring part of PMIC5. This part is closely
> coupled with ADC, using it's channels directly. ADC-TM support
> generating interrupts on ADC value crossing low or high voltage bounds,
> which is used to support thermal trip points.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>   drivers/iio/adc/qcom-vadc-common.c       |  62 +++
>   drivers/iio/adc/qcom-vadc-common.h       |   3 +
>   drivers/thermal/qcom/Kconfig             |  11 +
>   drivers/thermal/qcom/Makefile            |   1 +
>   drivers/thermal/qcom/qcom-spmi-adc-tm5.c | 583 +++++++++++++++++++++++
>   5 files changed, 660 insertions(+)
>   create mode 100644 drivers/thermal/qcom/qcom-spmi-adc-tm5.c
>
> diff --git a/drivers/iio/adc/qcom-vadc-common.c b/drivers/iio/adc/qcom-vadc-common.c
> index 40d77b3af1bb..e58e393b8713 100644
> --- a/drivers/iio/adc/qcom-vadc-common.c
> +++ b/drivers/iio/adc/qcom-vadc-common.c
> @@ -377,6 +377,42 @@ static int qcom_vadc_map_voltage_temp(const struct vadc_map_pt *pts,
>   	return 0;
>   }
>   


> +static irqreturn_t adc_tm5_isr(int irq, void *data)
> +{
> +	struct adc_tm5_chip *chip = data;
> +	u8 status_low, status_high, ctl;
> +	int ret = 0, i = 0;
> +
> +	ret = adc_tm5_read(chip, ADC_TM5_STATUS_LOW, &status_low, 1);
> +	if (ret) {
> +		dev_err(chip->dev, "read status low failed with %d\n", ret);
> +		return IRQ_HANDLED;
> +	}
> +
> +	ret = adc_tm5_read(chip, ADC_TM5_STATUS_HIGH, &status_high, 1);
> +	if (ret) {
> +		dev_err(chip->dev, "read status high failed with %d\n", ret);
> +		return IRQ_HANDLED;
> +	}
> +
> +	for (i = 0; i < chip->nchannels; i++) {
> +		bool upper_set = false, lower_set = false;
> +		unsigned int ch = chip->channels[i].channel;
> +
> +		if (!chip->channels[i].tzd) {
> +			dev_err_once(chip->dev, "thermal device not found\n");
> +			continue;
> +		}
> +
> +		ret = adc_tm5_read(chip, ADC_TM5_M_EN(ch), &ctl, 1);
> +
> +		if (ret) {
> +			dev_err(chip->dev, "ctl read failed with %d\n", ret);
> +			continue;
> +		}
> +
> +		lower_set = (status_low & BIT(ch)) &&
> +			(ctl & ADC_TM5_M_MEAS_EN) &&
> +			(ctl & ADC_TM5_M_LOW_THR_INT_EN);
> +
> +		upper_set = (status_high & BIT(ch)) &&
> +			(ctl & ADC_TM5_M_MEAS_EN) &&
> +			(ctl & ADC_TM5_M_HIGH_THR_INT_EN);
> +
> +		if (upper_set || lower_set)
> +			thermal_zone_device_update(chip->channels[i].tzd,
> +						   THERMAL_EVENT_UNSPECIFIED);

When using thermal_zone_device_update here, it internally calls 
tz->ops->get_temp, which maps to adc_tm5_get_temp defined just below. 
This in turn calls iio_read_channel_processed, which internally calls a 
mutex and this results in a mutex being called from atomic context.

To avoid this, the interrupt should be requested as a threaded IRQ.

> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int adc_tm5_get_temp(void *data, int *temp)
> +{
> +	struct adc_tm5_channel *channel = data;
> +	int ret, milli_celsius;
> +
> +	if (!channel || !channel->iio)
> +		return -EINVAL;
> +
> +	ret = iio_read_channel_processed(channel->iio, &milli_celsius);
> +	if (ret < 0)
> +		return ret;
> +
> +	*temp = milli_celsius;
> +
> +	return 0;
> +}
> +
> +static int adc_tm5_disable_channel(struct adc_tm5_channel *channel)
> +{
> +	struct adc_tm5_chip *chip = channel->chip;
> +	unsigned int reg = ADC_TM5_M_EN(channel->channel);
> +
> +	return adc_tm5_reg_update(chip, reg,
> +			ADC_TM5_M_MEAS_EN | ADC_TM5_M_HIGH_THR_INT_EN | ADC_TM5_M_LOW_THR_INT_EN,
> +			0);
> +}
> +
> +static int adc_tm5_configure(struct adc_tm5_channel *channel, int low_temp, int high_temp)
> +{
> +	struct adc_tm5_chip *chip = channel->chip;
> +	u8 buf[8];
> +	u16 reg = ADC_TM5_M_ADC_CH_SEL_CTL(channel->channel);
> +	int ret = 0;
> +
> +	ret = adc_tm5_read(chip, reg, buf, sizeof(buf));
> +	if (ret) {
> +		dev_err(chip->dev, "block read failed with %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Update ADC channel select */
> +	buf[0] = channel->adc_channel;
> +
> +	/* Warm temperature corresponds to low voltage threshold */
> +	if (high_temp != INT_MAX) {
> +		u16 adc_code = qcom_adc_tm5_temp_volt_scale(channel->prescale,
> +				chip->data->full_scale_code_volt, high_temp);
> +
> +		buf[1] = adc_code & 0xff;
> +		buf[2] = adc_code >> 8;
> +		buf[7] |= ADC_TM5_M_LOW_THR_INT_EN;
> +	}
> +
> +	/* Cool temperature corresponds to high voltage threshold */
> +	if (low_temp != -INT_MAX) {
> +		u16 adc_code = qcom_adc_tm5_temp_volt_scale(channel->prescale,
> +				chip->data->full_scale_code_volt, low_temp);
> +
> +		buf[3] = adc_code & 0xff;
> +		buf[4] = adc_code >> 8;
> +		buf[7] |= ADC_TM5_M_HIGH_THR_INT_EN;
> +	}
> +
> +	/* Update timer select */
> +	buf[5] = ADC5_TIMER_SEL_2;
> +
> +	/* Set calibration select, hw_settle delay */
> +	buf[6] &= ~ADC_TM5_M_CTL_HW_SETTLE_DELAY_MASK;
> +	buf[6] |= FIELD_PREP(ADC_TM5_M_CTL_HW_SETTLE_DELAY_MASK, channel->hw_settle_time);
> +	buf[6] &= ~ADC_TM5_M_CTL_CAL_SEL_MASK;
> +	buf[6] |= FIELD_PREP(ADC_TM5_M_CTL_CAL_SEL_MASK, channel->cal_method);
> +
> +	buf[7] |= ADC_TM5_M_MEAS_EN;
> +
> +	ret = adc_tm5_write(chip, reg, buf, sizeof(buf));
> +	if (ret)
> +		dev_err(chip->dev, "buf write failed\n");
> +
> +	return ret;
> +}
> +
> +static int adc_tm5_set_trips(void *data, int low_temp, int high_temp)
> +{
> +	struct adc_tm5_channel *channel = data;
> +	struct adc_tm5_chip *chip;
> +	int ret;
> +
> +	if (!channel)
> +		return -EINVAL;
> +
> +	chip = channel->chip;
> +	dev_dbg(chip->dev, "%d:low_temp(mdegC):%d, high_temp(mdegC):%d\n",
> +		channel->channel, low_temp, high_temp);
> +
> +	if (high_temp == INT_MAX && low_temp <= -INT_MAX)
> +		ret = adc_tm5_disable_channel(channel);
> +	else
> +		ret = adc_tm5_configure(channel, low_temp, high_temp);
In addition to the configurations done in adc_tm5_configure, you also 
need to write to the registers at 0x3546 (write 0x80 to enable the 
ADC_TM peripheral overall) and 0x3547 (this is the conversion request 
strobe, you need to write to bit 7 here too, to initiate the recurring 
measurements).
> +
> +	return ret;
> +}
> +



> +static int adc_tm5_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct adc_tm5_chip *adc_tm;
> +	struct regmap *regmap;
> +	int ret, irq;
> +	u32 reg;
> +
> +	regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!regmap)
> +		return -ENODEV;
> +
> +	ret = of_property_read_u32(node, "reg", &reg);
> +	if (ret)
> +		return ret;
> +
> +	adc_tm = devm_kzalloc(&pdev->dev, sizeof(*adc_tm), GFP_KERNEL);
> +	if (!adc_tm)
> +		return -ENOMEM;
> +
> +	adc_tm->regmap = regmap;
> +	adc_tm->dev = dev;
> +	adc_tm->base = reg;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(dev, "get_irq failed: %d\n", irq);
> +		return irq;
> +	}
> +
> +	ret = adc_tm5_get_dt_data(adc_tm, node);
> +	if (ret) {
> +		dev_err(dev, "get dt data failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = adc_tm5_init(adc_tm);
> +	if (ret) {
> +		dev_err(dev, "adc-tm init failed\n");
> +		return ret;
> +	}
> +
> +	ret = adc_tm5_register_tzd(adc_tm);
> +	if (ret) {
> +		dev_err(dev, "tzd register failed\n");
> +		return ret;
> +	}
> +
> +	return devm_request_irq(dev, irq, adc_tm5_isr, 0, "pm-adc-tm5", adc_tm);
The interrupt should be requested with devm_request_threaded_irq, with 
the existing interrupt handler being given as the threaded function, for 
the reason mentioned above earlier.
> +}
> +
>