lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<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

Powered by Openwall GNU/*/Linux Powered by OpenVZ