Re: [PATCH V2 1/3] dt-bindings: iio: adc: Add QCOM PMIC5 Gen3 ADC bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 16/11/2023 04:25, 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.
> 

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.

> 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.
> 
> Changes since v1:
> - Updated properties separately for all compatibles to clarify usage
>   of new properties and updates in usage of old properties for ADC5 Gen3.
> - Avoided updating 'adc7' name to 'adc5 gen2' and just left a comment
>   mentioning this convention.
> - Used predefined channel IDs in individual PMIC channel definitions
>   instead of numeric IDs.
> - Addressed other comments from reviewers.
> 
> Co-developed-by: Anjelique Melendez <quic_amelende@quicinc.com>
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
> ---
>  .../bindings/iio/adc/qcom,spmi-vadc.yaml      | 181 ++++++++++++++++--
>  .../iio/adc/qcom,spmi-adc5-gen3-pm8550.h      |  50 +++++
>  .../iio/adc/qcom,spmi-adc5-gen3-pm8550b.h     |  89 +++++++++
>  .../iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h    |  22 +++
>  .../iio/adc/qcom,spmi-adc5-gen3-pmk8550.h     |  56 ++++++
>  include/dt-bindings/iio/qcom,spmi-vadc.h      |  81 ++++++++
>  6 files changed, 464 insertions(+), 15 deletions(-)
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
> index ad7d6fc49de5..c988bd74a247 100644
> --- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
> @@ -13,8 +13,10 @@ maintainers:
>  description: |
>    SPMI PMIC voltage ADC (VADC) provides interface to clients to read
>    voltage. The VADC is a 15-bit sigma-delta ADC.
> -  SPMI PMIC5/PMIC7 voltage ADC (ADC) provides interface to clients to read
> -  voltage. The VADC is a 16-bit sigma-delta ADC.
> +  SPMI PMIC5/PMIC7/PMIC5 Gen3 voltage ADC (ADC) provides interface to
> +  clients to read voltage. The VADC is a 16-bit sigma-delta ADC.
> +  Note that PMIC7 ADC is the generation between PMIC5 and PMIC5 Gen3 ADC,
> +  it can be considered like PMIC5 Gen2.
>  
>  properties:
>    compatible:
> @@ -23,14 +25,18 @@ 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

NAK.

I wrote it multiple times. You canno remove the widest constraints from
top-level property.

> +    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.
>  
>    '#address-cells':
>      const: 1
> @@ -38,13 +44,26 @@ properties:
>    '#size-cells':
>      const: 0
>  
> +  "#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. For compatible property
> +      "qcom,spmi-adc5-gen3", this property is required for ADC_TM device.
> +
>    '#io-channel-cells':
>      const: 1
>  
>    interrupts:
> -    maxItems: 1

No, srsly. We went through it.


> -    description:
> +    description: |
>        End of conversion interrupt.
> +      - For compatible property "qcom,spmi-adc5-gen3", interrupts are defined
> +        for each SDAM being used.
> +
> +  interrupt-names:
> +    description: |

You must describe the names which also provides constraints.

I am not going to review the rest of the file.

Best regards,
Krzysztof