| 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
| ||
|
Date: Sun, 5 Mar 2023 16:54:55 +0100
From: Jakob Hauser <jahau@...ketmail.com>
To: Rob Herring <robh@...nel.org>
Cc: Sebastian Reichel <sre@...nel.org>, Lee Jones <lee@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Beomho Seo <beomho.seo@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
Stephan Gerhold <stephan@...hold.net>,
Raymond Hackley <raymondhackley@...tonmail.com>,
linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH 10/10] dt-bindings: Add documentation for rt5033 mfd,
regulator and charger
Hi Rob,
thanks for the remarks and sorry for not running 'make
dt_binding_check'. I'm not familiar with devicetree bindings and
obviously missed to read the file
Documentation/devicetree/bindings/submitting-patches.rst.
On 01.03.23 03:35, Rob Herring wrote:
> On Tue, Feb 28, 2023 at 11:32:27PM +0100, Jakob Hauser wrote:
>> Add device tree binding documentation for rt5033 multifunction device, voltage
>> regulator and battery charger.
>>
>> Cc: Beomho Seo <beomho.seo@...sung.com>
>> Cc: Chanwoo Choi <cw00.choi@...sung.com>
>> Signed-off-by: Jakob Hauser <jahau@...ketmail.com>
>> ---
>> .../bindings/mfd/richtek,rt5033.yaml | 102 ++++++++++++++++++
>> .../power/supply/richtek,rt5033-charger.yaml | 76 +++++++++++++
>> .../regulator/richtek,rt5033-regulator.yaml | 45 ++++++++
>> 3 files changed, 223 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
>> create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
>> create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
>> new file mode 100644
>> index 000000000000..f1a58694c81e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
>> @@ -0,0 +1,102 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/richtek,rt5033.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Richtek RT5033 Power Management Integrated Circuit
>> +
>> +maintainers:
>> + - Jakob Hauser <jahau@...ketmail.com>
>> +
>> +description: |
>
> Don't need '|' unless you care about line endings.
OK
>> + RT5033 is a multifunction device which includes battery charger, fuel gauge,
>> + flash LED current source, LDO and synchronous Buck converter for portable
>> + applications. It is interfaced to host controller using I2C interface. The
>> + battery fuel gauge uses a separate I2C bus.
>> +
>> +properties:
>> + compatible:
>> + const: richtek,rt5033
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + regulators:
>> + type: object
>> + $ref: /schemas/regulator/richtek,rt5033-regulator.yaml#
>> +
>> + charger:
>> + type: object
>> + $ref: /schemas/power/supply/richtek,rt5033-charger.yaml#
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> + i2c@0 {
>
> i2c {
OK
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + pmic@34 {
>> + compatible = "richtek,rt5033";
>> + reg = <0x34>;
>> +
>> + interrupt-parent = <&msmgpio>;
>> + interrupts = <62 IRQ_TYPE_EDGE_FALLING>;
>> +
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pmic_int_default>;
>> +
>> + regulators {
>> + safe_ldo_reg: SAFE_LDO {
>> + regulator-name = "SAFE_LDO";
>> + regulator-min-microvolt = <4900000>;
>> + regulator-max-microvolt = <4900000>;
>> + regulator-always-on;
>> + };
>> + ldo_reg: LDO {
>> + regulator-name = "LDO";
>> + regulator-min-microvolt = <2800000>;
>> + regulator-max-microvolt = <2800000>;
>> + };
>> + buck_reg: BUCK {
>> + regulator-name = "BUCK";
>> + regulator-min-microvolt = <1200000>;
>> + regulator-max-microvolt = <1200000>;
>> + };
>> + };
>> +
>> + charger {
>> + compatible = "richtek,rt5033-charger";
>> + richtek,pre-uamp = <450000>;
>> + richtek,fast-uamp = <1000000>;
>> + richtek,eoc-uamp = <150000>;
>> + richtek,pre-threshold-uvolt = <3500000>;
>> + richtek,const-uvolt = <4350000>;
>> + extcon = <&muic>;
>> + };
>> + };
>> + };
>> +
>> + i2c@1 {
>
> This should be a separate example entry.
I'll skip it then.
The battery fuel gauge is not handled as a part of the MFD, it has a
separate I2C line. Accordingly, it has a separate documentation
including examples [1].
I had implemented into the MFD example to make clear this is separated.
But as it is not part of the MFD, I guess it shouldn't show up in the
MFD documentation.
In the description of the MFD there is the statement "The battery fuel
gauge uses a separate I2C bus." I hope this is clear enough, I'm not
sure if I should modify/extent that statement or add some kind of
reference to the battery fuel gauge after removing it from the example.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/power/supply/richtek,rt5033-battery.yaml?h=v6.2
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + battery@35 {
>> + compatible = "richtek,rt5033-battery";
>> + reg = <0x35>;
>> + interrupt-parent = <&msmgpio>;
>> + interrupts = <121 IRQ_TYPE_EDGE_FALLING>;
>> + };
>> + };
>> diff --git a/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
>> new file mode 100644
>> index 000000000000..996c2932927d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
>> @@ -0,0 +1,76 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/supply/richtek,rt5033-charger.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Richtek RT5033 PIMC Battery Charger
>> +
>> +maintainers:
>> + - Jakob Hauser <jahau@...ketmail.com>
>> +
>> +description: |
>> + The battery charger of the multifunction device RT5033 has to be instantiated
>> + under sub-node named "charger" using the following format.
>> +
>> +properties:
>> + compatible:
>> + const: richtek,rt5033-charger
>> +
>> + richtek,pre-uamp:
>
> Use defined standard unit type suffixes.
That makes sense. I took the current names from the 2015 patchset and
wasn't aware of the standard suffixes.
Just for the record: Chaning the names will also impact patch 06 "power:
supply: rt5033_charger: Add RT5033 charger device driver", as the names
are parsed there.
>> + description: |
>> + Current of pre-charge mode. The pre-charge current levels are 350 mA to
>> + 650 mA programmed by I2C per 100 mA.
>> + maxItems: 1
>> +
>> + richtek,fast-uamp:
>> + description: |
>> + Current of fast-charge mode. The fast-charge current levels are 700 mA
>> + to 2000 mA programmed by I2C per 100 mA.
>> + maxItems: 1
>> +
>> + richtek,eoc-uamp:
>> + description: |
>> + This property is end of charge current. Its level ranges from 150 mA to
>> + 600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and 600 mA
>> + in 100 mA steps.
>> + maxItems: 1
>> +
>> + richtek,pre-threshold-uvolt:
>> + description: |
>> + Voltage of pre-charge mode. If the battery voltage is below the pre-charge
>> + threshold voltage, the charger is in pre-charge mode with pre-charge current.
>> + Its levels are 2.3 V to 3.8 V programmed by I2C per 0.1 V.
>> + maxItems: 1
>> +
>> + richtek,const-uvolt:
>> + description: |
>> + Battery regulation voltage of constant voltage mode. This voltage levels from
>> + 3.65 V to 4.4 V by I2C per 0.025 V.
>> + maxItems: 1
>> +
>> + extcon:
>
> This is deprecated. There's standard connector bindings now.
How does this work? I couldn't find an example.
I found Documentation/devicetree/bindings/connector/usb-connector.yaml
[2] but I don't see how this would be applied here.
The extcon device entry in the samsung-serranove devicetree [3] looks like:
i2c-muic {
compatible = "i2c-gpio";
sda-gpios = <&msmgpio 105 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
scl-gpios = <&msmgpio 106 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
pinctrl-names = "default";
pinctrl-0 = <&muic_i2c_default>;
#address-cells = <1>;
#size-cells = <0>;
muic: extcon@14 {
compatible = "siliconmitus,sm5504-muic";
reg = <0x14>;
interrupt-parent = <&msmgpio>;
interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
pinctrl-names = "default";
pinctrl-0 = <&muic_irq_default>;
};
};
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/connector/usb-connector.yaml?h=v6.2
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts?h=v6.2#n123
>> + description: |
>> + Phandle to the extcon device.
>> + maxItems: 1
>> +
>> +required:
>> + - richtek,pre-uamp
>> + - richtek,fast-uamp
>> + - richtek,eoc-uamp
>> + - richtek,pre-threshold-uvolt
>> + - richtek,const-uvolt
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + charger {
>> + compatible = "richtek,rt5033-charger";
>> + richtek,pre-uamp = <450000>;
>> + richtek,fast-uamp = <1000000>;
>> + richtek,eoc-uamp = <150000>;
>> + richtek,pre-threshold-uvolt = <3500000>;
>> + richtek,const-uvolt = <4350000>;
>> + extcon = <&muic>;
>> + };
>> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
>> new file mode 100644
>> index 000000000000..61b074488db4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
>> @@ -0,0 +1,45 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/richtek,rt5033-regulator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Richtek RT5033 PIMC Voltage Regulator
>> +
>> +maintainers:
>> + - Jakob Hauser <jahau@...ketmail.com>
>> +
>> +description: |
>> + The regulators of RT5033 have to be instantiated under a sub-node named
>> + "regulators". For SAFE_LDO voltage there is only one value of 4.9 V. LDO
>> + voltage ranges from 1.2 V to 3.0 V in 0.1 V steps. BUCK voltage ranges from
>> + 1.0 V to 3.0 V in 0.1 V steps.
>> +
>> +patternProperties:
>> + "^(SAFE_LDO|LDO|BUCK)$":
>
> Lowercase preferred for node names.
OK, I can change to lowercase.
Though I have to change the already existing driver rt5033-regulator as
well then [4]. I'm not sure if this has an impact on already existing
implementations. Although within the upstream kernel I think there is no
usage of the rt5033-regulator driver yet.
[4]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/rt5033-regulator.c?h=v6.2#n42
>> + type: object
>> + $ref: /schemas/regulator/regulator.yaml#
>> + unevaluatedProperties: false
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + regulators {
>
> Just 1 complete example in the MFD binding please.
OK, I'll skip the examples part here then.
>> + safe_ldo_reg: SAFE_LDO {
>> + regulator-name = "SAFE_LDO";
>> + regulator-min-microvolt = <4900000>;
>> + regulator-max-microvolt = <4900000>;
>> + regulator-always-on;
>> + };
>> + ldo_reg: LDO {
>> + regulator-name = "LDO";
>> + regulator-min-microvolt = <2800000>;
>> + regulator-max-microvolt = <2800000>;
>> + };
>> + buck_reg: BUCK {
>> + regulator-name = "BUCK";
>> + regulator-min-microvolt = <1200000>;
>> + regulator-max-microvolt = <1200000>;
>> + };
>> + };
>> --
>> 2.39.1
>>
I'll wait with implementing the changes as there will be likely further
comments on the other patches.
Kind regards,
Jakob
Powered by blists - more mailing lists