| Message ID | 5bd8b90713a61129acf292a941eb7fb5ccaa3db4.1682636929.git.jahau@rocketmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/8] mfd: rt5033: Drop rt5033-battery sub-device | expand |
On Fri, Apr 28, 2023 at 1:36 AM Jakob Hauser <jahau@rocketmail.com> 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> (...) > Changes in v3: I'm happy with the changes requested by me in v3 so: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> I see other reviewers have more comments. Yours, Linus Walleij
On 01/05/2023 23:16, Jakob Hauser wrote: > Hi Krzysztof, > > On 01.05.23 19:35, Jakob Hauser wrote: >> On 01.05.23 09:21, Krzysztof Kozlowski wrote: >>> On 28/04/2023 01:30, 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> >>> >>> >>> (...) >>> >>>> + >>>> +required: >>>> + - monitored-battery >>>> + >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + charger { >>>> + compatible = "richtek,rt5033-charger"; >>>> + monitored-battery = <&battery>; >>>> + extcon = <&muic>; >>> >>> >>> Everything up to here looked ok, but extcon is not a hardware property. >>> Please do not mix adding missing bindings for existing device with >>> adding new properties. You should use connector for the USB port. >>> > > ... > >> And how to set up the rt5033-charger to retrieve the information of the >> extcon/muic driver in that case? >> > > ... > > To add more context: > According to my understanding, the extcon subsystem provides three ways > to get an extcon device [3]: > - by name > - by devicetree node > - by phandle > > For rt5033-charger, the extcon device name can be different depending on > the consumer device. For the node I wouldn't know how to get from the > charger/mfd node to the extcon node, I don't see a direct relation in > case of rt5033-charger (it's no parent node or something like that). > Therefore I chose the third option: phandle. > > In the rt5033-charger driver, the location of the > extcon_get_edev_by_phandle() call is shown in link [4], it gets added in > patch 6. > Hi Jakob, I am currently busy, so I won't be able to help you and dig in your reply. I will get to you a bit later (or maybe Rob will help here). However please check if ports graph does not solve your case: https://lore.kernel.org/all/20230501121111.1058190-6-bryan.odonoghue@linaro.org/ It is already used for orientation and usb-role-switch (which should solve the need for extcon, AFAIR). If it does not help, ping me again, and I'll try to get to you a bit later. Apologies for this, just very busy times. :) Best regards, Krzysztof
Hi Krzysztof, Hi all, On 02.05.23 12:59, Krzysztof Kozlowski wrote: ... > Apologies for this, just very busy times. :) > Thanks for letting me know. Take the time you need. Writing towards the list: I think there is a misunderstanding here. The connector node provides information about the installed USB hardware. E.g. property "usb-role-switch" means "Indicates that the device is capable of assigning the USB data role (USB host or USB device) for a given USB connector [...]" [5]. To my understanding, in relation with a port node this actually says that this port has this capability. This is not relevant to the rt5033-charger driver. The rt5033-charger driver needs to pair with the extcon chip because it needs the information about *external* connectors being attached [6]. Extcon devices like SM5502 or SM5504 are real hardware. I'm not adding new properties. The way of getting an excton device from the devicetree by phandle is part of the extcon subsystem: - function to get the excton device by phandle: [7] - line that's looking for the property "extcon": [8] The connector node is the wrong approach, as far as I can tell on my current state of knowledge. It doesn't provide the information needed by the rt5033-charger driver. [5] https://github.com/torvalds/linux/blob/v6.3/Documentation/devicetree/bindings/usb/usb-drd.yaml#L51-L55 [6] https://github.com/torvalds/linux/blob/v6.3/include/linux/extcon.h#L32-L60 [7] https://github.com/torvalds/linux/blob/v6.3/drivers/extcon/extcon.c#L1361-L1392 [8] https://github.com/torvalds/linux/blob/v6.3/drivers/extcon/extcon.c#L1381 Kind regards, Jakob
On Wed, May 03, 2023 at 09:33:49PM +0200, Jakob Hauser wrote: > Hi Krzysztof, Hi all, > > On 02.05.23 12:59, Krzysztof Kozlowski wrote: > ... > > Apologies for this, just very busy times. :) > > > > Thanks for letting me know. Take the time you need. > > Writing towards the list: > > I think there is a misunderstanding here. > > The connector node provides information about the installed USB hardware. > E.g. property "usb-role-switch" means "Indicates that the device is capable > of assigning the USB data role (USB host or USB device) for a given USB > connector [...]" [5]. To my understanding, in relation with a port node this > actually says that this port has this capability. This is not relevant to > the rt5033-charger driver. > > The rt5033-charger driver needs to pair with the extcon chip because it > needs the information about *external* connectors being attached [6]. > > Extcon devices like SM5502 or SM5504 are real hardware. I'm not adding new > properties. The way of getting an excton device from the devicetree by > phandle is part of the extcon subsystem: > - function to get the excton device by phandle: [7] > - line that's looking for the property "extcon": [8] extcon as a binding is inadequate for handling the increasing complexities of connectors. Whether we need another property to link things to connector nodes, perhaps. > The connector node is the wrong approach, as far as I can tell on my current > state of knowledge. It doesn't provide the information needed by the > rt5033-charger driver. What information is that? You need information from the DT or run-time information from the 'extcon chip driver'? In the latter case, I'd expect the charger driver to get its connector node (either TBD phandle or search the DT if there's only 1 possible connector), and from that get the driver controlling the connector. Rob
Hi Rob, On 05.05.23 22:13, Rob Herring wrote: > On Wed, May 03, 2023 at 09:33:49PM +0200, Jakob Hauser wrote: >> Hi Krzysztof, Hi all, >> >> On 02.05.23 12:59, Krzysztof Kozlowski wrote: >> ... >>> Apologies for this, just very busy times. :) >>> >> >> Thanks for letting me know. Take the time you need. >> >> Writing towards the list: >> >> I think there is a misunderstanding here. >> >> The connector node provides information about the installed USB hardware. >> E.g. property "usb-role-switch" means "Indicates that the device is capable >> of assigning the USB data role (USB host or USB device) for a given USB >> connector [...]" [5]. To my understanding, in relation with a port node this >> actually says that this port has this capability. This is not relevant to >> the rt5033-charger driver. >> >> The rt5033-charger driver needs to pair with the extcon chip because it >> needs the information about *external* connectors being attached [6]. >> >> Extcon devices like SM5502 or SM5504 are real hardware. I'm not adding new >> properties. The way of getting an excton device from the devicetree by >> phandle is part of the extcon subsystem: >> - function to get the excton device by phandle: [7] >> - line that's looking for the property "extcon": [8] > > extcon as a binding is inadequate for handling the increasing > complexities of connectors. Whether we need another property to link > things to connector nodes, perhaps. Thanks for clarifying. >> The connector node is the wrong approach, as far as I can tell on my current >> state of knowledge. It doesn't provide the information needed by the >> rt5033-charger driver. > > What information is that? > > You need information from the DT or run-time information from the > 'extcon chip driver'? In the latter case, I'd expect the charger driver > to get its connector node (either TBD phandle or search the DT if > there's only 1 possible connector), and from that get the driver > controlling the connector. Yes, the latter case: run-time information from the 'extcon chip driver'. Hm, so I need to add a connector node below the extcon node, search for that connector and go one level up to get the extcon. In the specific case that's an unnecessary detour, I'm not too happy about it :/ Though I understand that in a broader perspective the connector thing can be more stringent. I'll prepare something like this for v4 then... Kind regard, Jakob
diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml new file mode 100644 index 000000000000..ee704914201f --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml @@ -0,0 +1,105 @@ +# 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: + 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: + 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. + type: object + patternProperties: + "^(SAFE_LDO|LDO|BUCK)$": + type: object + $ref: /schemas/regulator/regulator.yaml# + unevaluatedProperties: false + additionalProperties: false + + 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> + + battery: battery { + compatible = "simple-battery"; + precharge-current-microamp = <450000>; + constant-charge-current-max-microamp = <1000000>; + charge-term-current-microamp = <150000>; + precharge-upper-limit-microvolt = <3500000>; + constant-charge-voltage-max-microvolt = <4350000>; + }; + + 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"; + monitored-battery = <&battery>; + extcon = <&muic>; + }; + }; + }; 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..bf08e8db365e --- /dev/null +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml @@ -0,0 +1,63 @@ +# 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 + + monitored-battery: + description: | + Phandle to the monitored battery according to battery.yaml. The battery + node needs to contain five parameters. + + precharge-current-microamp: + Current of pre-charge mode. The pre-charge current levels are 350 mA + to 650 mA programmed by I2C per 100 mA. + + constant-charge-current-max-microamp: + Current of fast-charge mode. The fast-charge current levels are 700 mA + to 2000 mA programmed by I2C per 100 mA. + + charge-term-current-microamp: + 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. + + precharge-upper-limit-microvolt: + 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. + + constant-charge-voltage-max-microvolt: + Battery regulation voltage of constant voltage mode. This voltage levels + from 3.65 V to 4.4 V by I2C per 0.025 V. + + extcon: + description: + Phandle to the extcon device. + maxItems: 1 + +required: + - monitored-battery + +additionalProperties: false + +examples: + - | + charger { + compatible = "richtek,rt5033-charger"; + monitored-battery = <&battery>; + extcon = <&muic>; + };
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> --- The patch is based on next-20230413. The drivers for rt5033 (mfd) and rt5033-regulator are existing. Whereas the the driver rt5033-charger is new in this patchset. The extcon phandle is still there, as I didn't know what Rob meant by "standard connector bindings". Changes in v3: - Removed redundant "documentation" in the commit title. - Squashed regulator into the mfd binding. - Restored the regulator node names to uppercase because it is an existing driver. - In the charger binding replaced the vendor properties by "battery" node style. Accodringly updated the example in the mfd. .../bindings/mfd/richtek,rt5033.yaml | 105 ++++++++++++++++++ .../power/supply/richtek,rt5033-charger.yaml | 63 +++++++++++ 2 files changed, 168 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml