Re: [PATCH V2 1/3] dt-bindings: thermal: qcom: add PMIC5 Gen2 ADC_TM bindings

[Jishnu Prakash]

Hi Jonathan,

On 10/27/2021 11:21 PM, Jonathan Cameron wrote:
> On Tue, 26 Oct 2021 21:34:33 +0530
> Jishnu Prakash <quic_jprakash@xxxxxxxxxxx> wrote:
>
> > Add documentation for PMIC5 Gen2 ADC_TM peripheral.
> > It is used for monitoring ADC channel thresholds for PMIC7-type
> > PMICs. It is present on PMK8350, like PMIC7 ADC and can be used
> > to monitor up to 8 ADC channels, from any of the PMIC7 PMICs
> > on a target, through PBS(Programmable Boot Sequence).
> >
> > Signed-off-by: Jishnu Prakash <quic_jprakash@xxxxxxxxxxx>
> Hi Jishnu,
>
> A few comments inline.
>
> Thanks,
>
> Jonathan
>
> > ---
> >   .../bindings/thermal/qcom-spmi-adc-tm5.yaml        | 83 +++++++++++++++++++++-
> >   1 file changed, 81 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml b/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml
> > index 3ea8c0c..71a05a3 100644
> > --- a/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml
> > +++ b/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml
> > @@ -10,7 +10,9 @@ maintainers:
> >   
> >   properties:
> >     compatible:
> > -    const: qcom,spmi-adc-tm5
> > +    enum:
> > +      - qcom,spmi-adc-tm5
> > +      - qcom,spmi-adc-tm5-gen2
> >   
> >     reg:
> >       maxItems: 1
> > @@ -33,6 +35,7 @@ properties:
> >     qcom,avg-samples:
> >       $ref: /schemas/types.yaml#/definitions/uint32
> >       description: Number of samples to be used for measurement.
> > +            Not applicable for Gen2 ADC_TM peripheral.
> Why not use an matching statement to set
> qcom,avg_samples: false
> for that compatible rather than relying on the fuzzy nature of a coment.


I'll add this condition in the next post.


> >       enum:
> >         - 1
> >         - 2
> > @@ -45,6 +48,7 @@ properties:
> >       $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.
> > +            Not applicable for Gen2 ADC_TM peripheral.
> >       enum:
> >         - 250
> >         - 420
> > @@ -93,6 +97,29 @@ patternProperties:
> >             - const: 1
> >             - enum: [ 1, 3, 4, 6, 20, 8, 10 ]
> >   
> > +      qcom,avg-samples:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: Number of samples to be used for measurement.
> > +          This property in child node is applicable only for Gen2 ADC_TM peripheral.
> > +        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.
> > +          This property in child node is applicable only for Gen2 ADC_TM peripheral.
> > +        enum:
> > +          - 85
> > +          - 340
> > +          - 1360
> > +        default: 1360
> > +
> >       required:
> >         - reg
> >         - io-channels
> > @@ -124,7 +151,7 @@ examples:
> >               #size-cells = <0>;
> >               #io-channel-cells = <1>;
> >   
> > -            /* Other propreties are omitted */
> > +            /* Other properties are omitted */
> Should really be a separate patch, but up to Rob.
>
>
> >               conn-therm@4f {
> >                   reg = <ADC5_AMUX_THM3_100K_PU>;
> >                   qcom,ratiometric;
> > @@ -148,4 +175,56 @@ examples:
> >               };
> >           };
> >       };
> > +
> > +  - |
> > +    #include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h>
> > +    #include <dt-bindings/iio/qcom,spmi-adc7-pm8350.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    spmi_bus {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        pmk8350_vadc: adc@3100 {
> > +            reg = <0x3100>;
> > +            compatible = "qcom,spmi-adc7";
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            #io-channel-cells = <1>;
> > +
> > +            /* Other properties are omitted */
> > +            xo-therm@44 {
> > +                reg = <PMK8350_ADC7_AMUX_THM1_100K_PU>;
> > +                qcom,ratiometric;
> > +                qcom,hw-settle-time = <200>;
> > +            };
> > +
> > +            conn-therm@47 {
> > +                reg = <PM8350_ADC7_AMUX_THM4_100K_PU>;
> > +                qcom,ratiometric;
> > +                qcom,hw-settle-time = <200>;
> > +            };
> > +        };
> > +
> > +        pmk8350_adc_tm: adc-tm@3400 {
> > +            compatible = "qcom,spmi-adc-tm5-gen2";
> > +            reg = <0x3400>;
> > +            interrupts = <0x0 0x34 0x0 IRQ_TYPE_EDGE_RISING>;
> > +            #thermal-sensor-cells = <1>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            pmk8350-xo-therm@0 {
> > +                reg = <0>;
> > +                io-channels = <&pmk8350_vadc PMK8350_ADC7_AMUX_THM1_100K_PU>;
> > +                qcom,ratiometric;
> > +                qcom,hw-settle-time-us = <200>;
> Perhaps include the new properties you are defining in the example?


I'll add them in the next post.


> > +            };
> > +
> > +            conn-therm@1 {
> > +                reg = <1>;
> > +                io-channels = <&pmk8350_vadc PM8350_ADC7_AMUX_THM4_100K_PU>;
> > +                qcom,ratiometric;
> > +                qcom,hw-settle-time-us = <200>;
> > +            };
> > +        };
> > +    };
> >   ...

Thanks,

Jishnu