| Message ID | cover.1682636929.git.jahau@rocketmail.com |
|---|---|
| Headers | show |
| Series | Add RT5033 charger device driver | expand |
Hi Henrik, On 28.04.23 16:39, Henrik Grimler wrote: > On Fri, Apr 28, 2023 at 01:30:11AM +0200, Jakob Hauser wrote: ... >> + 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 > > Is only 4.9 V valid for SAFE_LDO? If I am reading driver found in > vendor kernel for SM-A500F it seems to to allow values between 3.3 and > 4.95 V [1]. Same range is also written in the devicetree for the > device [2]. > > [1] https://github.com/msm8916-mainline/linux-downstream/blob/SM-A500F/drivers/regulator/rt5033_regulator.c#L109-L114 > [2] https://github.com/msm8916-mainline/linux-downstream/blob/SM-A500F/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-a5u-eur-r01.dtsi#L148-L149 > You're right, the RT5033 regulator SAFE_LDO is capable of more than one voltage. In the mainline rt5033-regulator driver, however, for SAFE_LDO only the one voltage at 4.9 V is implemented so far. https://github.com/torvalds/linux/blob/v6.3/include/linux/mfd/rt5033-private.h#L211-L212 https://github.com/torvalds/linux/blob/v6.3/drivers/regulator/rt5033-regulator.c#L83 For the charger driver this seems sufficient. Kind regards, Jakob
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
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. [3] https://github.com/torvalds/linux/blob/v6.3/include/linux/extcon.h#L223-L229 [4] https://github.com/Jakko3/linux/blob/rt5033-charger_v3/drivers/power/supply/rt5033_charger.c#L677 Kind regards, Jakob
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
This patchset adds the charger driver "rt5033-charger". It is part of the multifunction device rt5033. The patchset is based on an older version by Beomho Seo of March 2015. For more information on the history and setup of the patchset see the cover sheet of version v1, there is a link further down below the changelog. The patchset is still based on next-20230413. Changes in v3: - Drobbed v2 patch 5 "regulator: rt5033: Change regulator names to lowercase". It affects an existing driver, therefore the uppercase node names are tolerable. - Patch 5: In function rt5033_get_charger_state() after "if (!regmap)" changed "return state" to "return POWER_SUPPLY_STATUS_UNKNOWN". If the regmap isn't available, the status is unknown. - Patch 5: Changed function rt5033_charger_dt_init() to the devicetree "battery" node readout style. - Patch 5: Added dev_err() messages if the values are out of range in the init functions rt5033_init_const_charge(), rt5033_init_fast_charge() and rt5033_init_pre_charge(). - Patch 5: In function rt5033_charger_probe() moved rt5033_charger_dt_init() and rt5033_charger_reg_init() behind devm_power_supply_register() because the power supply needs to be initiated to read out the "battery" node. - Patch 7: In function rt5033_battery_get_status() after "if (!charger)" changed "return -ENODEV" to "return POWER_SUPPLY_STATUS_UNKNOWN". If the charger isn't available, the status is unknown. - Changes on patch 8 are described in that patch below the commit message. v1: https://lore.kernel.org/linux-pm/cover.1677620677.git.jahau@rocketmail.com/T/#t v2: https://lore.kernel.org/linux-pm/cover.1681646904.git.jahau@rocketmail.com/T/#t The result of the patchset v3 can be seen at: https://github.com/Jakko3/linux/blob/rt5033-charger_v3/drivers/power/supply/rt5033_charger.c Jakob Hauser (7): mfd: rt5033: Fix chip revision readout mfd: rt5033: Fix STAT_MASK, HZ_MASK and AICR defines mfd: rt5033: Apply preparatory changes before adding rt5033-charger driver power: supply: rt5033_charger: Add RT5033 charger device driver power: supply: rt5033_charger: Add cable detection and USB OTG supply power: supply: rt5033_battery: Adopt status property from charger dt-bindings: Add rt5033 mfd, regulator and charger Stephan Gerhold (1): mfd: rt5033: Drop rt5033-battery sub-device .../bindings/mfd/richtek,rt5033.yaml | 105 +++ .../power/supply/richtek,rt5033-charger.yaml | 63 ++ drivers/mfd/rt5033.c | 8 +- drivers/power/supply/Kconfig | 8 + drivers/power/supply/Makefile | 1 + drivers/power/supply/rt5033_battery.c | 24 + drivers/power/supply/rt5033_charger.c | 725 ++++++++++++++++++ include/linux/mfd/rt5033-private.h | 64 +- include/linux/mfd/rt5033.h | 10 +- 9 files changed, 981 insertions(+), 27 deletions(-) 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 drivers/power/supply/rt5033_charger.c