[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <832053f4-bd5d-4e58-81bb-1a8188e7f364@linaro.org>
Date: Thu, 16 Nov 2023 12:43:19 +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,
konrad.dybcio@...aro.org, daniel.lezcano@...aro.org,
dmitry.baryshkov@...aro.org, linus.walleij@...aro.org,
linux-arm-msm@...r.kernel.org, andriy.shevchenko@...ux.intel.com,
quic_subbaram@...cinc.com, quic_collinsd@...cinc.com,
quic_amelende@...cinc.com, quic_kamalw@...cinc.com,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
marijn.suijten@...ainline.org
Cc: lars@...afoo.de, luca@...tu.xyz, linux-iio@...r.kernel.org,
lee@...nel.org, rafael@...nel.org, rui.zhang@...el.com,
lukasz.luba@....com, cros-qcom-dts-watchers@...omium.org,
sboyd@...nel.org, linux-pm@...r.kernel.org,
linux-arm-msm-owner@...r.kernel.org, kernel@...cinc.com
Subject: Re: [PATCH V2 1/3] dt-bindings: iio: adc: Add QCOM PMIC5 Gen3 ADC
bindings
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@...cinc.com>
> Signed-off-by: Anjelique Melendez <quic_amelende@...cinc.com>
> Signed-off-by: Jishnu Prakash <quic_jprakash@...cinc.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
Powered by blists - more mailing lists