Message-ID: <bb877daa-8cdb-4c52-a70a-2206e67d014e@quicinc.com>
Date: Wed, 13 Nov 2024 19:35:40 +0530
From: Jishnu Prakash <quic_jprakash@...cinc.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, <jic23@...nel.org>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <conor+dt@...nel.org>, <agross@...nel.org>, <andersson@...nel.org>,
        <dmitry.baryshkov@...aro.org>, <konrad.dybcio@...aro.org>,
        <daniel.lezcano@...aro.org>, <sboyd@...nel.org>,
        <quic_subbaram@...cinc.com>, <quic_collinsd@...cinc.com>,
        <quic_amelende@...cinc.com>, <quic_kamalw@...cinc.com>,
        <amitk@...nel.org>
CC: <lee@...nel.org>, <rafael@...nel.org>, <rui.zhang@...el.com>,
        <lukasz.luba@....com>, <lars@...afoo.de>, <quic_skakitap@...cinc.com>,
        <neil.armstrong@...aro.org>, <devicetree@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-iio@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-pm@...r.kernel.org>,
        <cros-qcom-dts-watchers@...omium.org>
Subject: Re: [PATCH V4 2/4] dt-bindings: iio: adc: Add support for QCOM PMIC5
 Gen3 ADC

Hi Krzysztof,

On 10/31/2024 4:28 PM, Krzysztof Kozlowski wrote:
> On 30/10/2024 19:58, 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 an SDAM (Shared
>> Direct Access Memory) peripheral 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.
>>
>> Co-developed-by: Anjelique Melendez <quic_amelende@...cinc.com>
>> Signed-off-by: Anjelique Melendez <quic_amelende@...cinc.com>
>> Signed-off-by: Jishnu Prakash <quic_jprakash@...cinc.com>
>> ---
> 
> This has still test failures, so limited review follows.
> 
>>  properties:
>>    compatible:
>> @@ -23,14 +27,20 @@ properties:
>>            - const: qcom,pms405-adc
>>            - const: qcom,spmi-adc-rev2
>>        - enum:
>> -          - qcom,spmi-vadc
>> -          - qcom,spmi-adc5
>>            - qcom,spmi-adc-rev2
>> +          - qcom,spmi-adc5
>> +          - qcom,spmi-adc5-gen3
>>            - qcom,spmi-adc7
>> +          - qcom,spmi-vadc
>>  
>>    reg:
>> -    description: VADC base address in the SPMI PMIC register map
>> -    maxItems: 1
>> +    description:
>> +      For compatible properties "qcom,spmi-vadc", "qcom,spmi-adc5", "qcom,spmi-adc-rev2"
>> +      and "qcom,spmi-adc7", reg is the VADC base address in the SPMI PMIC register map.
>> +      For compatible property "qcom,spmi-adc5-gen3", each reg corresponds to an SDAM
>> +      peripheral base address that is being used for ADC functionality.
> 
> This description is not really needed. You need to provide constraints
> in schema.
> 
>> +    minItems: 1
>> +    maxItems: 2
>>  
>>    '#address-cells':
>>      const: 1
>> @@ -38,20 +48,28 @@ properties:
>>    '#size-cells':
>>      const: 0
>>  
>> +  "#thermal-sensor-cells":
>> +    const: 1
>> +    description:
>> +      Number of cells required to uniquely identify the thermal sensors.
> 
> Drop, redundant.
> 
>> +      For compatible property "qcom,spmi-adc5-gen3", this property is
>> +      required for if any channels under it are used for ADC_TM.
>> +      Since we have multiple sensors this is set to 1.
> 
> Drop sentence, redundant.
> 
>> +
>>    '#io-channel-cells':
>>      const: 1
>>  
>>    interrupts:
>> -    maxItems: 1
>>      description:
>>        End of conversion interrupt.
>> +      For compatible property "qcom,spmi-adc5-gen3", interrupts are defined
>> +      for each SDAM being used.
> 
> Drop descriptions and instead rather list and describe items. You keep
> repeating schema in free form text. That's not the point.
> 
>> +    minItems: 1
>> +    maxItems: 2
>>  
>> -required:
>> -  - compatible
>> -  - reg
>> -  - '#address-cells'
>> -  - '#size-cells'
>> -  - '#io-channel-cells'
>> +  interrupt-names:
>> +    minItems: 1
>> +    maxItems: 2
>>  
>>  patternProperties:
>>    "^channel@[0-9a-f]+$":
>> @@ -71,8 +89,8 @@ patternProperties:
>>          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 and PMIC5 Gen3 ADC, the channel numbers are specified separately
>> +          per PMIC in the PMIC-specific files in include/dt-bindings/iio/adc.
>>  
>>        label:
>>          description: |
>> @@ -113,11 +131,11 @@ patternProperties:
>>                channel calibration. If property is not found, channel will be
>>                calibrated with 0.625V and 1.25V reference channels, also
>>                known as absolute calibration.
>> -            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc7" and
>> -              "qcom,spmi-adc-rev2", 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.
>> +            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc7",
>> +              "qcom,spmi-adc-rev2" and "qcom,spmi-adc5-gen3", 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.
>>          type: boolean
>>  
>>        qcom,hw-settle-time:
>> @@ -135,9 +153,24 @@ patternProperties:
>>              from the ADC that is an average of multiple samples. The value
>>              selected is 2^(value).
>>  
>> +      qcom,adc-tm:
>> +        description:
>> +          Indicates if ADC_TM monitoring is done on this channel.
> 
> What is "ADC_TM"? Why this would be property of a board? This does not
> look like suitable for DT, at least based on such very vague explanation.
> 
>> +          Defined for compatible property "qcom,spmi-adc5-gen3".
> 
> Drop redundant.
> 
>> +          This is the same functionality as in the existing QCOM ADC_TM
>> +          device, documented at devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml.
> 
> What does it mean? How property can represent functionality of entire
> binding?
> 
> BTW, use full paths when refering to files.
> 

To address all your above questions for ADC_TM:

The file "Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml" describes
the Qualcomm ADC thermal monitoring device, which existed as a separate device on older
PMIC generations. ADC_TM refers to this functionality.

In ADC5 Gen3, ADC_TM functionality is combined with the existing ADC read functionality
described in this file, under a single device.

In the earlier ADC_TM DT nodes, each child node would describe one of the IIO ADC channels being
monitored by ADC_TM HW. In this ADC5 Gen3 device, setting the property 'qcom,adc-tm' for a channel
node means that it will also be monitored in HW exactly like an ADC_TM channel.

It can be considered a hardware property as the monitoring is done by a sequence under
PBS (Programmable Boot Sequence, can be considered firmware), which periodically gets the
channel reading and checks it against upper/lower thresholds set by clients of this driver, 
for threshold violations.


>> +        type: boolean
>> +
>>      required:
>>        - reg
>>  
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#address-cells'
>> +  - '#size-cells'
>> +  - '#io-channel-cells'
>> +
>>  allOf:
>>    - if:
>>        properties:
>> @@ -146,6 +179,15 @@ allOf:
>>              const: qcom,spmi-vadc
>>  
>>      then:
>> +      properties:
>> +        reg:
>> +          minItems: 1
> 
> min is redundant.
> 
>> +          maxItems: 1
>> +        interrupts:
>> +          minItems: 1
>> +          maxItems: 1
> 
> So here you list and describe items instead.

Do you mean interrupts should be updated to something like this?

        interrupts:
          maxItems: 1
	  description: 
            End of conversion interrupt.

Does this look right?

> 
>> +        "#thermal-sensor-cells": false
>> +        interrupt-names: false
> 
> Keep things properly ordered. xxx-names is always next to xxx.
> 
>>        patternProperties:
>>          "^channel@[0-9a-f]+$":
>>            properties:
>> @@ -162,6 +204,8 @@ allOf:
>>                enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512 ]
>>                default: 1
>>  
>> +            qcom,adc-tm: false
>> +
>>    - if:
>>        properties:
>>          compatible:
>> @@ -169,6 +213,15 @@ allOf:
>>              const: qcom,spmi-adc-rev2
>>  
>>      then:
>> +      properties:
>> +        reg:
>> +          minItems: 1
>> +          maxItems: 1
>> +        interrupts:
>> +          minItems: 1
>> +          maxItems: 1
>> +        "#thermal-sensor-cells": false
>> +        interrupt-names: false
>>        patternProperties:
>>          "^channel@[0-9a-f]+$":
>>            properties:
>> @@ -185,6 +238,8 @@ allOf:
>>                enum: [ 1, 2, 4, 8, 16 ]
>>                default: 1
>>  
>> +            qcom,adc-tm: false
>> +
>>    - if:
>>        properties:
>>          compatible:
>> @@ -192,6 +247,15 @@ allOf:
>>              const: qcom,spmi-adc5
>>  
>>      then:
>> +      properties:
>> +        reg:
>> +          minItems: 1
>> +          maxItems: 1
>> +        interrupts:
>> +          minItems: 1
>> +          maxItems: 1
>> +        "#thermal-sensor-cells": false
>> +        interrupt-names: false
>>        patternProperties:
>>          "^channel@[0-9a-f]+$":
>>            properties:
>> @@ -208,6 +272,8 @@ allOf:
>>                enum: [ 1, 2, 4, 8, 16 ]
>>                default: 1
>>  
>> +            qcom,adc-tm: false
>> +
>>    - if:
>>        properties:
>>          compatible:
>> @@ -215,6 +281,59 @@ allOf:
>>              const: qcom,spmi-adc7
>>  
>>      then:
>> +      properties:
>> +        reg:
>> +          minItems: 1
>> +          maxItems: 1
>> +        interrupts:
>> +          minItems: 1
>> +          maxItems: 1
>> +        "#thermal-sensor-cells": false
>> +        interrupt-names: false
>> +      patternProperties:
>> +        "^channel@[0-9a-f]+$":
>> +          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: false
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: qcom,spmi-adc5-gen3
>> +
>> +    then:
>> +      properties:
>> +        reg:
>> +          minItems: 1
> 
> Why this is flexible?

I'm assuming you are asking why it can be either 1 or 2 instead of exactly 2.
Both configurations can be supported in HW and it varies between boards. Some of them
have exactly one SDAM peripheral assigned for ADC usage and some may have two.


> 
>> +          items:
>> +            - description: SDAM0 base address in the SPMI PMIC register map
>> +            - description: SDAM1 base address
>> +        interrupts:
>> +          minItems: 1
> 
> 
> Why this is flexible?

reg, interrupts and interrupt-names are all added per SDAM, so they can all be
either 1 or 2.

Will address all your other comments in the next patch version.

Thanks,
Jishnu

> 
> 
>> +          items:
>> +            - description: SDAM0 end of conversion (EOC) interrupt
>> +            - description: SDAM1 EOC interrupt
>> +        interrupt-names:
>> +          minItems: 1
>> +          items:
>> +            - const: adc-sdam0
> 
> sdam0
> 
>> +            - const: adc-sdam1
> 
> sdam1
> 
>> +      required:
>> +        - interrupts
>> +        - interrupt-names
>>        patternProperties:
>>          "^channel@[0-9a-f]+$":
>>            properties:
>> @@ -307,3 +426,64 @@ examples:
> 
> 
> 
> Best regards,
> Krzysztof
> 

Powered by blists - more mailing lists