Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bb225c12-f017-fac3-45f1-c828a10553e2@linaro.org>
Date:   Sun, 9 Jul 2023 19:23:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH 06/11] iio: adc: Add QCOM PMIC5 Gen3 ADC bindings
Content-Language: en-US
To:     Jishnu Prakash <quic_jprakash@quicinc.com>, agross@kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linus.walleij@linaro.org, Jonathan.Cameron@huawei.com,
        sboyd@kernel.org, dmitry.baryshkov@linaro.org,
        quic_subbaram@quicinc.com, quic_collinsd@quicinc.com,
        quic_kamalw@quicinc.com, quic_jestar@quicinc.com,
        marijn.suijten@somainline.org, andriy.shevchenko@linux.intel.com,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Luca Weiss <luca@z3ntu.xyz>, linux-iio@vger.kernel.org,
        linux-arm-msm@vger.kernel.org
Cc:     linux-arm-msm-owner@vger.kernel.org
References: <20230708072835.3035398-1-quic_jprakash@quicinc.com>
 <20230708072835.3035398-7-quic_jprakash@quicinc.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230708072835.3035398-7-quic_jprakash@quicinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 08/07/2023 09:28, 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 firmware through a single register interface. This
> interface is implemented on an SDAM peripheral on the master PMIC PMK8550
> rather than a dedicated ADC peripheral.
> 
> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
> ---
>  .../bindings/iio/adc/qcom,spmi-vadc.yaml      | 117 ++++++++++++++++--
>  .../iio/qcom,spmi-adc5-gen3-pm8550.h          |  48 +++++++
>  .../iio/qcom,spmi-adc5-gen3-pm8550b.h         |  97 +++++++++++++++
>  .../iio/qcom,spmi-adc5-gen3-pm8550vx.h        |  20 +++
>  .../iio/qcom,spmi-adc5-gen3-pmk8550.h         |  54 ++++++++
>  include/dt-bindings/iio/qcom,spmi-vadc.h      |  78 ++++++++++++
>  6 files changed, 402 insertions(+), 12 deletions(-)
>  create mode 100644 include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h
>  create mode 100644 include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550b.h
>  create mode 100644 include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550vx.h
>  create mode 100644 include/dt-bindings/iio/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 eb7d16e385ad..090113e62d52 100644
> --- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
> @@ -13,8 +13,8 @@ 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/PMIC5 Gen2 voltage ADC (ADC) provides interface to clients to read
> -  voltage. The VADC is a 16-bit sigma-delta ADC.
> +  SPMI PMIC5/PMIC5 Gen2/PMIC5 Gen3 voltage ADC (ADC) provides interface to
> +  clients to read voltage. The VADC is a 16-bit sigma-delta ADC.
>  
>  properties:
>    compatible:
> @@ -27,10 +27,11 @@ properties:
>            - qcom,spmi-adc5
>            - qcom,spmi-adc-rev2
>            - qcom,spmi-adc5-gen2
> +          - qcom,spmi-adc5-gen3


This could be ordered...
>  
>    reg:
>      description: VADC base address in the SPMI PMIC register map
> -    maxItems: 1
> +    minItems: 1

Why? This does not make any sense. With previous patches it looks like
random set of changes.


>  
>    '#address-cells':
>      const: 1
> @@ -38,6 +39,12 @@ 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.
> +
>    '#io-channel-cells':
>      const: 1
>  
> @@ -71,8 +78,8 @@ patternProperties:
>          description: |
>            ADC channel number.
>            See include/dt-bindings/iio/qcom,spmi-vadc.h
> -          For PMIC5 Gen2 ADC, the channel numbers are specified separately per PMIC
> -          in the PMIC-specific files in include/dt-bindings/iio/.
> +          For PMIC5 Gen2 and PMIC5 Gen3 ADC, the channel numbers are specified separately per
> +          PMIC in the PMIC-specific files in include/dt-bindings/iio/.
>  
>        label:
>          $ref: /schemas/types.yaml#/definitions/string
> @@ -114,11 +121,12 @@ 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-adc5-gen2" 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-adc5-gen2",
> +              "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:
> @@ -136,6 +144,12 @@ patternProperties:
>              from the ADC that is an average of multiple samples. The value
>              selected is 2^(value).
>  
> +      qcom,adc-tm-type:
> +        description: |
> +            Indicates if ADC_TM monitoring is done on this channel.

Description does not match property name.

> +            Defined for compatible property "qcom,spmi-adc5-gen3".

Then you need if:then:.

> +        type: boolean
> +
>      required:
>        - reg
>  
> @@ -213,7 +227,9 @@ allOf:
>        properties:
>          compatible:
>            contains:
> -            const: qcom,spmi-adc5-gen2
> +            enum:
> +              - qcom,spmi-adc5-gen2
> +              - qcom,spmi-adc5-gen3
>  
>      then:
>        patternProperties:
> @@ -299,7 +315,7 @@ examples:
>                  label = "xo_therm";
>              };
>  
> -            channel@47 {
> +            channel@147 {

Why?

>                  reg = <PM8350_ADC5_GEN2_AMUX_THM4_100K_PU(1)>;
>                  qcom,ratiometric;
>                  qcom,hw-settle-time = <200>;
> @@ -307,3 +323,80 @@ examples:
>              };
>          };
>      };
> +
> +  - |
> +    #include <dt-bindings/iio/qcom,spmi-adc5-gen3-pmk8550.h>
> +    #include <dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h>
> +    #include <dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550b.h>
> +    #include <dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550vx.h>
> +
> +    pmic {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      /* VADC node */
> +      pmk8550_vadc: vadc@9000 {
> +        compatible = "qcom,spmi-adc5-gen3";

Don't add new examples which differ only in compatible.


> diff --git a/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h b/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h
> new file mode 100644
> index 000000000000..74e6e2f6f9ed
> --- /dev/null
> +++ b/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */

Dual license.


Best regards,
Krzysztof