| 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
| ||
|
Message-ID: <3f812ffa-ec33-448e-b72a-ce698618a8c1@linaro.org>
Date: Thu, 4 Jan 2024 09:18:14 +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,
andersson@...nel.org, konrad.dybcio@...aro.org, lee@...nel.org,
andriy.shevchenko@...ux.intel.com, daniel.lezcano@...aro.org,
dmitry.baryshkov@...aro.org
Cc: lars@...afoo.de, luca@...tu.xyz, marijn.suijten@...ainline.org,
agross@...nel.org, sboyd@...nel.org, rafael@...nel.org, rui.zhang@...el.com,
lukasz.luba@....com, linus.walleij@...aro.org, quic_subbaram@...cinc.com,
quic_collinsd@...cinc.com, quic_amelende@...cinc.com,
quic_kamalw@...cinc.com, kernel@...cinc.com, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-arm-msm-owner@...r.kernel.org,
linux-iio@...r.kernel.org, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, cros-qcom-dts-watchers@...omium.org
Subject: Re: [PATCH v3 2/3] dt-bindings: iio: adc: Add support for QCOM PMIC5
Gen3 ADC
On 31/12/2023 18:12, 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.
>
> Changes since v2:
> - Moved ADC5 Gen3 documentation into a separate new file.
Changelog goes under ---.
Why did you do this? What is the rationale? Sorry, this patchset goes
nowhere.
>
> 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-adc5-gen3.yaml | 212 ++++++++++++++++++
> .../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 +++++
> .../iio/adc/qcom,spmi-adc7-pmr735b.h | 2 +-
> .../iio/adc/qcom,spmi-adc7-smb139x.h | 2 +-
> include/dt-bindings/iio/adc/qcom,spmi-vadc.h | 81 +++++++
> 8 files changed, 512 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml
> 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-adc5-gen3.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml
> new file mode 100644
> index 000000000000..ed5bb53e7628
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5-gen3.yaml
> @@ -0,0 +1,212 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/qcom,spmi-adc5-gen3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm's SPMI PMIC ADC5 Gen3
> +
> +maintainers:
> + - Jishnu Prakash <quic_jprakash@...cinc.com>
> +
> +description: |
Do not need '|' unless you need to preserve formatting.
> + SPMI PMIC5 Gen3 voltage ADC (ADC) provides interface to
> + clients to read voltage. It is a 16-bit sigma-delta ADC.
> + It also performs the same thermal monitoring function as
> + the existing ADC_TM devices.
> +
> +properties:
> + compatible:
> + const: qcom,spmi-adc5-gen3
> +
> + reg:
> + description: |
> + - Each reg corresponds to an SDAM peripheral base address that is being used for ADC.
Missing constraints, weird formatting and not wrapped according to
coding style (read Linux coding style).
> +
> + '#address-cells':
> + const: 1
> +
> + '#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. This property is required for
> + ADC devices with channels used for TM (thermal monitoring) functionality.
> +
> + '#io-channel-cells':
> + const: 1
> +
> + interrupts:
> + description: |
> + End of conversion interrupt. Interrupts are defined for each SDAM being used.
Same problemns.
You already got comments on this in v2.
> +
> + interrupt-names:
> + minItems: 1
> + maxItems: 10
> + items:
> + pattern: "^adc-sdam[0-9]+$"
> + description: |
> + Names should be defined as "adc-sdam<N>" where <N> represents the SDAM index.
Don't repeat constraints in free form text.
> +
> +required:
required: goes after all properties.
> + - compatible
> + - reg
> + - '#address-cells'
> + - '#size-cells'
> + - '#io-channel-cells'
> + - interrupts
> + - interrupt-names
> +
> +patternProperties:
> + "^channel@[0-9a-f]+$":
> + type: object
> + additionalProperties: false
> + description: |
Do not need '|' unless you need to preserve formatting.
> + Represents the external channels which are connected to the ADC.
> +
> + properties:
> + reg:
> + maxItems: 1
> + description: |
> + ADC channel number.
> + See include/dt-bindings/iio/adc/qcom,spmi-vadc.h
> + For PMIC5 Gen3 ADC, the channel numbers are specified separately
> + per PMIC in the PMIC-specific files in include/dt-bindings/iio/adc.
> +
> + label:
> + $ref: /schemas/types.yaml#/definitions/string
Why do you need it in the first place? Don't you miss some $ref?
> + description: |
Do not need '|' unless you need to preserve formatting. Applies everywhere.
> + ADC input of the platform as seen in the schematics.
> + For thermistor inputs connected to generic AMUX or GPIO inputs
> + these can vary across platform for the same pins. Hence select
> + the platform schematics name for this channel.
> +
> + qcom,decimation:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: |
> + This parameter is used to decrease ADC sampling rate.
> + Quicker measurements can be made by reducing decimation ratio.
> + enum: [ 85, 340, 1360 ]
> + default: 1360
> +
> + qcom,pre-scaling:
> + description: |
> + Used for scaling the channel input signal before the signal is
> + fed to VADC. The configuration for this node is to know the
> + pre-determined ratio and use it for post scaling. It is a pair of
> + integers, denoting the numerator and denominator of the fraction by which
> + input signal is multiplied. For example, <1 3> indicates the signal is scaled
> + down to 1/3 of its value before ADC measurement.
> + If property is not found default value depending on chip will be used.
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + items:
> + - const: 1
> + - enum: [ 1, 3, 6, 16 ]
> +
> + qcom,ratiometric:
> + description: |
> + Channel calibration type.
> + - 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:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: |
> + Time between AMUX getting configured and the ADC starting
> + conversion. The 'hw_settle_time' is an index used from valid values
> + and programmed in hardware to achieve the hardware settling delay.
> + enum: [ 15, 100, 200, 300, 400, 500, 600, 700, 1000, 2000, 4000,
> + 8000, 16000, 32000, 64000, 128000 ]
> + default: 15
> +
> + qcom,avg-samples:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: |
> + Number of samples to be used for measurement.
> + Averaging provides the option to obtain a single measurement
> + from the ADC that is an average of multiple samples. The value
> + selected is 2^(value).
> + enum: [ 1, 2, 4, 8, 16 ]
> + default: 1
> +
> + qcom,adc-tm:
> + description: |
> + Indicates if ADC_TM monitoring is done on this channel.
> + Defined for compatible property "qcom,spmi-adc5-gen3".
> + This is the same functionality as in the existing QCOM ADC_TM
> + device, documented at devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml.
> + type: boolean
> +
Why do you duplicate entire vadc file? Why it cannot be part of that
file? Oh wait, it was in v2.
You now duplicated a lot of property definitions without clear reason.
If this is intention, then you need to put them in common schema.
> + required:
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h>
> + #include <dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h>
> + #include <dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h>
> + #include <dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + pmic {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + /* VADC node */
> + pmk8550_vadc: vadc@...0 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Really, duplicating entire file and adding mistakes? Just open the
original file which you changed in v2 and look how it is done there.
> + compatible = "qcom,spmi-adc5-gen3";
> + reg = <0x9000>, <0x9100>;
> + interrupts = <0x0 0x90 0x1 IRQ_TYPE_EDGE_RISING>,
> + <0x0 0x91 0x1 IRQ_TYPE_EDGE_RISING>;
wrong alignment.
> + interrupt-names = "adc-sdam0", "adc-sdam1";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + #io-channel-cells = <1>;
> + #thermal-sensor-cells = <1>;
> +
> + /* PMK8550 Channel nodes */
> + channel@3 {
> + reg = <PMK8550_ADC5_GEN3_DIE_TEMP>;
> + label = "pmk8550_die_temp";
> + qcom,pre-scaling = <1 1>;
> + };
> +
> + channel@44 {
> + reg = <PMK8550_ADC5_GEN3_AMUX_THM1_XO_THERM_100K_PU>;
> + label = "pmk8550_xo_therm";
> + qcom,pre-scaling = <1 1>;
> + qcom,ratiometric;
> + qcom,hw-settle-time = <200>;
> + qcom,adc-tm;
> + };
> +
> + /* PM8550 Channel nodes */
> + channel@103 {
> + reg = <PM8550_ADC5_GEN3_DIE_TEMP>;
> + label = "pm8550_die_temp";
> + qcom,pre-scaling = <1 1>;
> + };
> +
> + /* PM8550B Channel nodes */
> + channel@78f {
> + reg = <PM8550B_ADC5_GEN3_VBAT_SNS_QBG>;
> + label = "pm8550b_vbat_sns_qbg";
> + qcom,pre-scaling = <1 3>;
> + };
> +
> + /* PM8550VS_C Channel nodes */
> + channel@203 {
> + reg = <PM8550VS_ADC5_GEN3_DIE_TEMP(2)>;
> + label = "pm8550vs_c_die_temp";
> + qcom,pre-scaling = <1 1>;
> + };
> + };
> + };
> diff --git a/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
> new file mode 100644
> index 000000000000..0f25ef87ed5c
> --- /dev/null
> +++ b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _DT_BINDINGS_QCOM_SPMI_VADC_PM8550_H
> +#define _DT_BINDINGS_QCOM_SPMI_VADC_PM8550_H
> +
> +#ifndef PM8550_SID
> +#define PM8550_SID 1
> +#endif
Drop
> +
> +#include <dt-bindings/iio/adc/qcom,spmi-vadc.h>
> +
> +/* ADC channels for PM8550_ADC for PMIC5 Gen3 */
> +#define PM8550_ADC5_GEN3_REF_GND (PM8550_SID << 8 | ADC5_GEN3_REF_GND)
> +#define PM8550_ADC5_GEN3_1P25VREF (PM8550_SID << 8 | ADC5_GEN3_1P25VREF)
> +#define PM8550_ADC5_GEN3_VREF_VADC (PM8550_SID << 8 | ADC5_GEN3_VREF_VADC)
> +#define PM8550_ADC5_GEN3_DIE_TEMP (PM8550_SID << 8 | ADC5_GEN3_DIE_TEMP)
> +
> +#define PM8550_ADC5_GEN3_AMUX_THM1 (PM8550_SID << 8 | ADC5_GEN3_AMUX1_THM)
> +#define PM8550_ADC5_GEN3_AMUX_THM2 (PM8550_SID << 8 | ADC5_GEN3_AMUX2_THM)
> +#define PM8550_ADC5_GEN3_AMUX_THM3 (PM8550_SID << 8 | ADC5_GEN3_AMUX3_THM)
> +#define PM8550_ADC5_GEN3_AMUX_THM4 (PM8550_SID << 8 | ADC5_GEN3_AMUX4_THM)
> +#define PM8550_ADC5_GEN3_AMUX_THM5 (PM8550_SID << 8 | ADC5_GEN3_AMUX5_THM)
> +#define PM8550_ADC5_GEN3_AMUX_THM6_GPIO2 (PM8550_SID << 8 | ADC5_GEN3_AMUX6_THM)
> +#define PM8550_ADC5_GEN3_AMUX1_GPIO3 (PM8550_SID << 8 | ADC5_GEN3_AMUX1_GPIO)
> +#define PM8550_ADC5_GEN3_AMUX2_GPIO4 (PM8550_SID << 8 | ADC5_GEN3_AMUX2_GPIO)
> +#define PM8550_ADC5_GEN3_AMUX3_GPIO7 (PM8550_SID << 8 | ADC5_GEN3_AMUX3_GPIO)
> +#define PM8550_ADC5_GEN3_AMUX4_GPIO12 (PM8550_SID << 8 | ADC5_GEN3_AMUX4_GPIO)
> +
> +/* 100k pull-up */
> +#define PM8550_ADC5_GEN3_AMUX_THM1_100K_PU (PM8550_SID << 8 | ADC5_GEN3_AMUX1_THM_100K_PU)
> +#define PM8550_ADC5_GEN3_AMUX_THM2_100K_PU (PM8550_SID << 8 | ADC5_GEN3_AMUX2_THM_100K_PU)
> +#define PM8550_ADC5_GEN3_AMUX_THM3_100K_PU (PM8550_SID << 8 | ADC5_GEN3_AMUX3_THM_100K_PU)
> +#define PM8550_ADC5_GEN3_AMUX_THM4_100K_PU (PM8550_SID << 8 | ADC5_GEN3_AMUX4_THM_100K_PU)
> +#define PM8550_ADC5_GEN3_AMUX_THM5_100K_PU (PM8550_SID << 8 | ADC5_GEN3_AMUX5_THM_100K_PU)
> +#define PM8550_ADC5_GEN3_AMUX_THM6_GPIO2_100K_PU (PM8550_SID << 8 | ADC5_GEN3_AMUX6_THM_100K_PU)
> +#define PM8550_ADC5_GEN3_AMUX1_GPIO3_100K_PU (PM8550_SID << 8 | ADC5_GEN3_AMUX1_GPIO_100K_PU)
> +#define PM8550_ADC5_GEN3_AMUX2_GPIO4_100K_PU (PM8550_SID << 8 | ADC5_GEN3_AMUX2_GPIO_100K_PU)
> +#define PM8550_ADC5_GEN3_AMUX3_GPIO7_100K_PU (PM8550_SID << 8 | ADC5_GEN3_AMUX3_GPIO_100K_PU)
> +#define PM8550_ADC5_GEN3_AMUX4_GPIO12_100K_PU (PM8550_SID << 8 | ADC5_GEN3_AMUX4_GPIO_100K_PU)
> +
> +/* 1/3 Divider */
> +#define PM8550_ADC5_GEN3_AMUX3_GPIO7_DIV3 (PM8550_SID << 8 | ADC5_GEN3_AMUX3_GPIO_DIV3)
> +#define PM8550_ADC5_GEN3_AMUX4_GPIO12_DIV3 (PM8550_SID << 8 | ADC5_GEN3_AMUX4_GPIO_DIV3)
> +
> +#define PM8550_ADC5_GEN3_VPH_PWR (PM8550_SID << 8 | ADC5_GEN3_VPH_PWR)
> +
> +#endif /* _DT_BINDINGS_QCOM_SPMI_VADC_PM8550_H */
> diff --git a/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
> new file mode 100644
> index 000000000000..47116bbe45de
> --- /dev/null
> +++ b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
> @@ -0,0 +1,89 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _DT_BINDINGS_QCOM_SPMI_VADC_PM8550B_H
> +#define _DT_BINDINGS_QCOM_SPMI_VADC_PM8550B_H
> +
> +#ifndef PM8550B_SID
> +#define PM8550B_SID 7
Drop
> +#endif
> +
...
> +#endif /* _DT_BINDINGS_QCOM_SPMI_VADC_PM8550VX_H */
> diff --git a/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
> new file mode 100644
> index 000000000000..3fc829ebdf6d
> --- /dev/null
> +++ b/include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _DT_BINDINGS_QCOM_SPMI_VADC_PMK8550_H
> +#define _DT_BINDINGS_QCOM_SPMI_VADC_PMK8550_H
> +
> +#ifndef PMK8550_SID
> +#define PMK8550_SID 0
> +#endif
Drop
>
> /* ADC channels for PMR735B_ADC for PMIC7 */
> #define PMR735B_ADC7_REF_GND (PMR735B_SID << 8 | ADC7_REF_GND)
> diff --git a/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h b/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h
> index c0680d1285cf..750a526af2c1 100644
> --- a/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h
> +++ b/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h
> @@ -6,7 +6,7 @@
> #ifndef _DT_BINDINGS_QCOM_SPMI_VADC_SMB139X_H
> #define _DT_BINDINGS_QCOM_SPMI_VADC_SMB139X_H
>
> -#include <dt-bindings/iio/qcom,spmi-vadc.h>
> +#include <dt-bindings/iio/adc/qcom,spmi-vadc.h>
? How is it related?
>
> #define SMB139x_1_ADC7_SMB_TEMP (SMB139x_1_SID << 8 | ADC7_SMB_TEMP)
> #define SMB139x_1_ADC7_ICHG_SMB (SMB139x_1_SID << 8 | ADC7_ICHG_SMB)
> diff --git a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
> index ef07ecd4d585..cfe653d945a4 100644
> --- a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
> +++ b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
> @@ -1,6 +1,8 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> /*
> * Copyright (c) 2012-2014,2018,2020 The Linux Foundation. All rights reserved.
> + *
Drop stray blank line
Best regards,
Krzysztof
Powered by blists - more mailing lists