Messages in this thread Patch in this message | | | Date | Sun, 5 Mar 2023 16:54:55 +0100 | Subject | Re: [PATCH 10/10] dt-bindings: Add documentation for rt5033 mfd, regulator and charger | From | Jakob Hauser <> |
| |
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@samsung.com> >> Cc: Chanwoo Choi <cw00.choi@samsung.com> >> Signed-off-by: Jakob Hauser <jahau@rocketmail.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@rocketmail.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@rocketmail.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@rocketmail.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
| |