Re: [PATCH 10/10] dt-bindings: Add documentation for rt5033 mfd, regulator and charger

From: Jakob Hauser
Date: Sun Mar 05 2023 - 10:55:59 EST

Next message: Uros Bizjak: "[PATCH v4 0/3] Improve trace/ring_buffer.c"
Previous message: Uros Bizjak: "[PATCH v4 1/3] ring_buffer: Change some static functions to void"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxx>
Cc: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
Signed-off-by: Jakob Hauser <jahau@xxxxxxxxxxxxxx>
---
.../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@xxxxxxxxxxxxxx>
+
+description: |

Don't need '|' unless you care about line endings.

+ 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 {

+ #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@xxxxxxxxxxxxxx>
+
+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@xxxxxxxxxxxxxx>
+
+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

Next message: Uros Bizjak: "[PATCH v4 0/3] Improve trace/ring_buffer.c"
Previous message: Uros Bizjak: "[PATCH v4 1/3] ring_buffer: Change some static functions to void"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]