Message-ID: <6daaee01-36a0-4dc5-86c7-106aabbfff4e@linaro.org>
Date: Thu, 31 Oct 2024 11:58:24 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Jishnu Prakash <quic_jprakash@...cinc.com>, 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

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.

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

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

> +          items:
> +            - description: SDAM0 base address in the SPMI PMIC register map
> +            - description: SDAM1 base address
> +        interrupts:
> +          minItems: 1


Why this is flexible?


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