| 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: <NEH5LR.URZKYH8VLESF1@crapouillou.net>
Date: Thu, 10 Nov 2022 21:08:47 +0000
From: Paul Cercueil <paul@...pouillou.net>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
list@...ndingux.net
Subject: Re: [PATCH v2] dt-bindings: Convert active-semi PMIC docs to YAML
schemas
Hi Krzysztof,
Le dim. 6 nov. 2022 à 10:55:11 +0100, Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> a écrit :
> On 05/11/2022 23:58, Paul Cercueil wrote:
>> Create YAML bindings for the Active-semi PMICs and remove the old
>> text
>> files.
>>
>> The bindings aren't perfect, for instance I couldn't find good
>> descriptions for the vendor properties in the "charger" node of the
>> ACT8945A because I am not familiar with the hardware and these
>> properties were not documented anywhere.
>>
>> The YAML schemas are a bit different than what is described in the
>> old
>> text files, because these were sometimes wrong or had missing
>> information. This is the case for the ACT8600 documentation, which
>> specified the valid node names for the regulators, while the driver
>> was
>> expecting different names. This led to the current situation where
>> we
>> have two different boards using different names for the regulators:
>> - arch/mips/boot/dts/ingenic/ci20.dts uses the names documented in
>> the
>> text file,
>> - arch/mips/boot/dts/ingenic/gcw0.dts uses the names that the driver
>> expects.
>> In theory, the driver should be fixed to follow the documentation,
>> and
>> accept both naming schemes. In practice though, when the PMIC node
>> was
>> added to the ci20.dts board file, the names were already wrong in
>> regards to what the driver expected, so it never really worked
>> correctly and wasn't tested properly. Furthermore, in that board the
>> consumers of the regulators aren't working for various other reasons
>> (invalid GPIOs, etc.).
>>
>> For that reason, for the ACT8600 bindings I decided to only use the
>> node
>> names that the driver expects (and that gcw0.dts uses), instead of
>> accepting both old and new names. A follow-up patch will update the
>> CI20
>> board to use the new regulator names.
>>
>> Signed-off-by: Paul Cercueil <paul@...pouillou.net>
>> ---
>>
>> Notes:
>> v2:
>> - Avoid | character in descriptions that can be single-line
>> - Remove unevaluatedProperties when additionalProperties is
>> also present
>> - Remove useless inner parentheses in regular expressions
>> - Rename I2C nodes to just... i2c
>> - Remove node handles
>>
>> Note:
>> I set Liam Girdwood and Mark Brown as the maintainers by
>> default, since
>> it doesn't appear that anybody is managing the Active-semi
>> drivers, but
>> if anybody steps up I can update it.
>
> It should not be Liam and Mark, but someone having/knowing this
> particular hardware.
Well, who would that be?
I do have a board with the ACT8600, but that's about it.
-Paul
>>
>> .../bindings/regulator/act8865-regulator.txt | 117 --------
>> .../bindings/regulator/act8945a-regulator.txt | 113 --------
>> .../regulator/active-semi,act8600.yaml | 141 ++++++++++
>> .../regulator/active-semi,act8846.yaml | 207 ++++++++++++++
>> .../regulator/active-semi,act8865.yaml | 162 +++++++++++
>> .../regulator/active-semi,act8945a.yaml | 261
>> ++++++++++++++++++
>> 6 files changed, 771 insertions(+), 230 deletions(-)
>> delete mode 100644
>> Documentation/devicetree/bindings/regulator/act8865-regulator.txt
>> delete mode 100644
>> Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
>> create mode 100644
>> Documentation/devicetree/bindings/regulator/active-semi,act8600.yaml
>> create mode 100644
>> Documentation/devicetree/bindings/regulator/active-semi,act8846.yaml
>> create mode 100644
>> Documentation/devicetree/bindings/regulator/active-semi,act8865.yaml
>> create mode 100644
>> Documentation/devicetree/bindings/regulator/active-semi,act8945a.yaml
>>
>> diff --git
>> a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
>> b/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
>> deleted file mode 100644
>> index b9f58e480349..000000000000
>> ---
>> a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
>> +++ /dev/null
>> @@ -1,117 +0,0 @@
>> -ACT88xx regulators
>> --------------------
>> -
>> -Required properties:
>> -- compatible: "active-semi,act8846" or "active-semi,act8865" or
>> "active-semi,act8600"
>> -- reg: I2C slave address
>> -
>> -Optional properties:
>> -- system-power-controller: Telling whether or not this pmic is
>> controlling
>> - the system power. See
>> Documentation/devicetree/bindings/power/power-controller.txt .
>> -- active-semi,vsel-high: Indicates the VSEL pin is high.
>> - If this property is missing, assume the VSEL pin is low(0).
>> -
>> -Optional input supply properties:
>> -- for act8600:
>> - - vp1-supply: The input supply for DCDC_REG1
>> - - vp2-supply: The input supply for DCDC_REG2
>> - - vp3-supply: The input supply for DCDC_REG3
>> - - inl-supply: The input supply for LDO_REG5, LDO_REG6, LDO_REG7
>> and LDO_REG8
>> - SUDCDC_REG4, LDO_REG9 and LDO_REG10 do not have separate
>> supplies.
>> -- for act8846:
>> - - vp1-supply: The input supply for REG1
>> - - vp2-supply: The input supply for REG2
>> - - vp3-supply: The input supply for REG3
>> - - vp4-supply: The input supply for REG4
>> - - inl1-supply: The input supply for REG5, REG6 and REG7
>> - - inl2-supply: The input supply for REG8 and LDO_REG9
>> - - inl3-supply: The input supply for REG10, REG11 and REG12
>> -- for act8865:
>> - - vp1-supply: The input supply for DCDC_REG1
>> - - vp2-supply: The input supply for DCDC_REG2
>> - - vp3-supply: The input supply for DCDC_REG3
>> - - inl45-supply: The input supply for LDO_REG1 and LDO_REG2
>> - - inl67-supply: The input supply for LDO_REG3 and LDO_REG4
>> -
>> -Any standard regulator properties can be used to configure the
>> single regulator.
>> -regulator-initial-mode, regulator-allowed-modes and regulator-mode
>> could be specified
>> -for act8865 using mode values from
>> dt-bindings/regulator/active-semi,8865-regulator.h
>> -file.
>> -
>> -The valid names for regulators are:
>> - - for act8846:
>> - REG1, REG2, REG3, REG4, REG5, REG6, REG7, REG8, REG9, REG10,
>> REG11, REG12
>> - - for act8865:
>> - DCDC_REG1, DCDC_REG2, DCDC_REG3, LDO_REG1, LDO_REG2, LDO_REG3,
>> LDO_REG4.
>> - - for act8600:
>> - DCDC_REG1, DCDC_REG2, DCDC_REG3, SUDCDC_REG4, LDO_REG5, LDO_REG6,
>> LDO_REG7,
>> - LDO_REG8, LDO_REG9, LDO_REG10,
>> -
>> -Example:
>> ---------
>> -
>> -#include <dt-bindings/regulator/active-semi,8865-regulator.h>
>> -
>> - i2c1: i2c@...18000 {
>> - pmic: act8865@5b {
>> - compatible = "active-semi,act8865";
>> - reg = <0x5b>;
>> - active-semi,vsel-high;
>> -
>> - regulators {
>> - vcc_1v8_reg: DCDC_REG1 {
>> - regulator-name = "VCC_1V8";
>> - regulator-min-microvolt = <1800000>;
>> - regulator-max-microvolt = <1800000>;
>> - regulator-always-on;
>> - };
>> -
>> - vcc_1v2_reg: DCDC_REG2 {
>> - regulator-name = "VCC_1V2";
>> - regulator-min-microvolt = <1100000>;
>> - regulator-max-microvolt = <1300000>;
>> - regulator-always-on;
>> -
>> - regulator-allowed-modes = <ACT8865_REGULATOR_MODE_FIXED>,
>> - <ACT8865_REGULATOR_MODE_LOWPOWER>;
>> - regulator-initial-mode = <ACT8865_REGULATOR_MODE_FIXED>;
>> -
>> - regulator-state-mem {
>> - regulator-on-in-suspend;
>> - regulator-suspend-min-microvolt = <1150000>;
>> - regulator-suspend-max-microvolt = <1150000>;
>> - regulator-changeable-in-suspend;
>> - regulator-mode = <ACT8865_REGULATOR_MODE_LOWPOWER>;
>> - };
>> - };
>> -
>> - vcc_3v3_reg: DCDC_REG3 {
>> - regulator-name = "VCC_3V3";
>> - regulator-min-microvolt = <3300000>;
>> - regulator-max-microvolt = <3300000>;
>> - regulator-always-on;
>> - };
>> -
>> - vddana_reg: LDO_REG1 {
>> - regulator-name = "VDDANA";
>> - regulator-min-microvolt = <3300000>;
>> - regulator-max-microvolt = <3300000>;
>> - regulator-always-on;
>> -
>> - regulator-allowed-modes = <ACT8865_REGULATOR_MODE_NORMAL>,
>> - <ACT8865_REGULATOR_MODE_LOWPOWER>;
>> - regulator-initial-mode = <ACT8865_REGULATOR_MODE_NORMAL>;
>> -
>> - regulator-state-mem {
>> - regulator-off-in-suspend;
>> - };
>> - };
>> -
>> - vddfuse_reg: LDO_REG2 {
>> - regulator-name = "FUSE_2V5";
>> - regulator-min-microvolt = <2500000>;
>> - regulator-max-microvolt = <2500000>;
>> - };
>> - };
>> - };
>> - };
>> diff --git
>> a/Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
>> b/Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
>> deleted file mode 100644
>> index 4017527619ab..000000000000
>> ---
>> a/Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
>> +++ /dev/null
>> @@ -1,113 +0,0 @@
>> -Device-Tree bindings for regulators of Active-semi ACT8945A
>> Multi-Function Device
>> -
>> -Required properties:
>> - - compatible: "active-semi,act8945a", please refer to
>> ../mfd/act8945a.txt.
>> -
>> -Optional properties:
>> -- active-semi,vsel-high: Indicates if the VSEL pin is set to
>> logic-high.
>> - If this property is missing, assume the VSEL pin is set to
>> logic-low.
>> -
>> -Optional input supply properties:
>> - - vp1-supply: The input supply for REG_DCDC1
>> - - vp2-supply: The input supply for REG_DCDC2
>> - - vp3-supply: The input supply for REG_DCDC3
>> - - inl45-supply: The input supply for REG_LDO1 and REG_LDO2
>> - - inl67-supply: The input supply for REG_LDO3 and REG_LDO4
>> -
>> -Any standard regulator properties can be used to configure the
>> single regulator.
>> -regulator-initial-mode, regulator-allowed-modes and regulator-mode
>> could be
>> -specified using mode values from
>> dt-bindings/regulator/active-semi,8945a-regulator.h
>> -file.
>> -
>> -The valid names for regulators are:
>> - REG_DCDC1, REG_DCDC2, REG_DCDC3, REG_LDO1, REG_LDO2, REG_LDO3,
>> REG_LDO4.
>> -
>> -Example:
>> -
>> -#include <dt-bindings/regulator/active-semi,8945a-regulator.h>
>> -
>> - pmic@5b {
>> - compatible = "active-semi,act8945a";
>> - reg = <0x5b>;
>> -
>> - active-semi,vsel-high;
>> -
>> - regulators {
>> - vdd_1v35_reg: REG_DCDC1 {
>> - regulator-name = "VDD_1V35";
>> - regulator-min-microvolt = <1350000>;
>> - regulator-max-microvolt = <1350000>;
>> - regulator-always-on;
>> -
>> - regulator-allowed-modes = <ACT8945A_REGULATOR_MODE_FIXED>,
>> - <ACT8945A_REGULATOR_MODE_LOWPOWER>;
>> - regulator-initial-mode = <ACT8945A_REGULATOR_MODE_FIXED>;
>> -
>> - regulator-state-mem {
>> - regulator-on-in-suspend;
>> - regulator-suspend-min-microvolt=<1400000>;
>> - regulator-suspend-max-microvolt=<1400000>;
>> - regulator-changeable-in-suspend;
>> - regulator-mode=<ACT8945A_REGULATOR_MODE_LOWPOWER>;
>> - };
>> - };
>> -
>> - vdd_1v2_reg: REG_DCDC2 {
>> - regulator-name = "VDD_1V2";
>> - regulator-min-microvolt = <1100000>;
>> - regulator-max-microvolt = <1300000>;
>> - regulator-always-on;
>> -
>> - regulator-allowed-modes = <ACT8945A_REGULATOR_MODE_FIXED>,
>> - <ACT8945A_REGULATOR_MODE_LOWPOWER>;
>> - regulator-initial-mode = <ACT8945A_REGULATOR_MODE_FIXED>;
>> -
>> - regulator-state-mem {
>> - regulator-off-in-suspend;
>> - };
>> - };
>> -
>> - vdd_3v3_reg: REG_DCDC3 {
>> - regulator-name = "VDD_3V3";
>> - regulator-min-microvolt = <3300000>;
>> - regulator-max-microvolt = <3300000>;
>> - regulator-always-on;
>> - };
>> -
>> - vdd_fuse_reg: REG_LDO1 {
>> - regulator-name = "VDD_FUSE";
>> - regulator-min-microvolt = <2500000>;
>> - regulator-max-microvolt = <2500000>;
>> - regulator-always-on;
>> -
>> - regulator-allowed-modes = <ACT8945A_REGULATOR_MODE_NORMAL>,
>> - <ACT8945A_REGULATOR_MODE_LOWPOWER>;
>> - regulator-initial-mode = <ACT8945A_REGULATOR_MODE_NORMAL>;
>> -
>> - regulator-state-mem {
>> - regulator-off-in-suspend;
>> - };
>> - };
>> -
>> - vdd_3v3_lp_reg: REG_LDO2 {
>> - regulator-name = "VDD_3V3_LP";
>> - regulator-min-microvolt = <3300000>;
>> - regulator-max-microvolt = <3300000>;
>> - regulator-always-on;
>> - };
>> -
>> - vdd_led_reg: REG_LDO3 {
>> - regulator-name = "VDD_LED";
>> - regulator-min-microvolt = <3300000>;
>> - regulator-max-microvolt = <3300000>;
>> - regulator-always-on;
>> - };
>> -
>> - vdd_sdhc_1v8_reg: REG_LDO4 {
>> - regulator-name = "VDD_SDHC_1V8";
>> - regulator-min-microvolt = <1800000>;
>> - regulator-max-microvolt = <1800000>;
>> - regulator-always-on;
>> - };
>> - };
>> - };
>> diff --git
>> a/Documentation/devicetree/bindings/regulator/active-semi,act8600.yaml
>> b/Documentation/devicetree/bindings/regulator/active-semi,act8600.yaml
>> new file mode 100644
>> index 000000000000..ca0ac316dff2
>> --- /dev/null
>> +++
>> b/Documentation/devicetree/bindings/regulator/active-semi,act8600.yaml
>> @@ -0,0 +1,141 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id:
>> http://devicetree.org/schemas/regulator/active-semi,act8600.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Active-semi ACT8600 regulator
>> +
>> +maintainers:
>> + - Liam Girdwood <lgirdwood@...il.com>
>> + - Mark Brown <broonie@...nel.org>
>> +
>> +properties:
>> + compatible:
>> + const: active-semi,act8600
>> +
>> + reg:
>> + description: I2C address
>
> Drop description, it's obvious. The same in other files.
>
>> + maxItems: 1
>> +
>> + system-power-controller:
>> + description:
>> + Indicates that the ACT8600 is responsible for powering OFF
>> + the system.
>> + type: boolean
>> +
>
> (...)
>
>> +examples:
>> + - |
>> + #include <dt-bindings/regulator/active-semi,8865-regulator.h>
>> +
>> + i2c1 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + pmic: act8865@5b {
>> + compatible = "active-semi,act8865";
>> + reg = <0x5b>;
>> + active-semi,vsel-high;
>> +
>> + regulators {
>> + vcc_1v8_reg: DCDC_REG1 {
>> + regulator-name = "VCC_1V8";
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-always-on;
>> + };
>> +
>> + vcc_1v2_reg: DCDC_REG2 {
>> + regulator-name = "VCC_1V2";
>> + regulator-min-microvolt = <1100000>;
>> + regulator-max-microvolt = <1300000>;
>> + regulator-always-on;
>> +
>> + regulator-allowed-modes =
>> <ACT8865_REGULATOR_MODE_FIXED>,
>> +
>> <ACT8865_REGULATOR_MODE_LOWPOWER>;
>> + regulator-initial-mode =
>> <ACT8865_REGULATOR_MODE_FIXED>;
>> +
>> + regulator-state-mem {
>> + regulator-on-in-suspend;
>> + regulator-suspend-min-microvolt = <1150000>;
>> + regulator-suspend-max-microvolt = <1150000>;
>> + regulator-changeable-in-suspend;
>> + regulator-mode = <ACT8865_REGULATOR_MODE_LOWPOWER>;
>> + };
>> + };
>> +
>> + vcc_3v3_reg: DCDC_REG3 {
>> + regulator-name = "VCC_3V3";
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-always-on;
>> + };
>> +
>> + vddana_reg: LDO_REG1 {
>> + regulator-name = "VDDANA";
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-always-on;
>> +
>> + regulator-allowed-modes =
>> <ACT8865_REGULATOR_MODE_NORMAL>,
>> + <ACT8865_REGULATOR_MODE_LOWPOWER>;
>
> Align it with previous <.
>
>> + regulator-initial-mode =
>> <ACT8865_REGULATOR_MODE_NORMAL>;
>
>
> (...)
>
>> +
>> + charger {
>> + compatible = "active-semi,act8945a-charger";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_charger_chglev
>> &pinctrl_charger_lbo &pinctrl_charger_irq>;
>> + interrupt-parent = <&pioA>;
>> + interrupts = <45 IRQ_TYPE_EDGE_RISING>;
>> +
>> + active-semi,chglev-gpios = <&pioA 12 GPIO_ACTIVE_HIGH>;
>> + active-semi,lbo-gpios = <&pioA 72 GPIO_ACTIVE_LOW>;
>> + active-semi,input-voltage-threshold-microvolt = <6600>;
>> + active-semi,precondition-timeout = <40>;
>> + active-semi,total-timeout = <3>;
>> + status = "okay";
>
> Drop status
>
>> + };
>> + };
>> + };
>
> Best regards,
> Krzysztof
>
Powered by blists - more mailing lists