| Message ID | 20160927081344.GC4394@kozik-lap (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Krzysztof Kozlowski writes: > On Tue, Sep 27, 2016 at 01:31:10AM +0200, Wolfgang Wiedmeyer wrote: >> This patch adds a listener for extcon cable events and enables >> charging if an USB cable is connected. It recognizes SDP and DCP cable >> types and treats them the same (same input current and fast charge >> current). The maximum input current is set before the charger is >> enabled and before the charger gets disabled, the maximum input >> current is set to zero. The listener is inspired by the listener >> implementation that was used for the AXP288 Charger driver. >> >> The patch also adds support for the CURRENT_NOW property. It reads the >> fast charge current that gets set before the charger is enabled or >> disabled. >> >> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de> > > No. This power supply driver should not manage regulators. It is not a > regulator consumer. For that specific need, there is a charger-manager driver. When I was in the middle of implementing this, I noticed that the charger manager does everything that is needed. But it took me quite some time to configure the DTS correctly until I realized that the charger manager used a deprecated function (extcon_register_interest()) and thus couldn't work. And as I didn't see the charger-manager in any other device's DTS, I thought that this might not be right way. But Chanwoo Choi has a fix: https://patchwork.kernel.org/patch/8898541/ So I will try to get it working with this patch. > I agree that you might configure here the charger. You might even expose > some writeable properties through power supply class. However the > purpose of this driver is to expose the battery charger to user-space, > not to replace the user-space with its work. > > So... NACK. Ok, then I will try to reduce the patch to the CURRENT_NOW property support. > If you would like to play with charger-manager, here is my old DTS for > Trats2 (might need updates): Is there a reason that this patch is not in the kernel? It would have been very helpful for me :) Thanks, Wolfgang > index 595ad4ba6977..b4361b4a9de7 100644 > --- a/arch/arm/boot/dts/exynos4412-trats2.dts > +++ b/arch/arm/boot/dts/exynos4412-trats2.dts > @@ -856,6 +856,44 @@ > }; > }; > > + charger-manager@0 { > + compatible = "charger-manager"; > + status = "okay"; > + chg-reg-supply = <&charger_reg>; > + > + cm-name = "battery"; > + /* Polling only for external power source */ > + cm-poll-mode = <2>; > + cm-poll-interval = <30000>; > + > + cm-fullbatt-vchkdrop-ms = <30000>; > + cm-fullbatt-vchkdrop-volt = <150000>; > + cm-fullbatt-soc = <100>; > + > + cm-battery-stat = <0>; > + cm-fuel-gauge = "max170xx_battery"; > + > + /* Allow charging for 5hr */ > + cm-charging-max = <18000000>; > + /* Allow discharging for 2hr */ > + cm-discharging-max = <7200000>; > + > + cm-num-chargers = <1>; > + cm-chargers = "max77693-charger"; > + > + charger@0 { > + cm-regulator-name = "chg-reg"; > + cable@0 { > + cm-cable-name = "USB"; > + cm-cable-extcon = "max77693-muic"; > + }; > + cable@1 { > + cm-cable-name = "TA"; > + cm-cable-extcon = "max77693-muic"; > + }; > + }; > + }; > + > exynos-usbphy@125B0000 { > status = "okay"; > }; > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > Best regards, > Krzysztof
On 09/27/2016 03:34 PM, Wolfgang Wiedmeyer wrote: > > Krzysztof Kozlowski writes: > >> On Tue, Sep 27, 2016 at 01:31:10AM +0200, Wolfgang Wiedmeyer wrote: >>> This patch adds a listener for extcon cable events and enables >>> charging if an USB cable is connected. It recognizes SDP and DCP cable >>> types and treats them the same (same input current and fast charge >>> current). The maximum input current is set before the charger is >>> enabled and before the charger gets disabled, the maximum input >>> current is set to zero. The listener is inspired by the listener >>> implementation that was used for the AXP288 Charger driver. >>> >>> The patch also adds support for the CURRENT_NOW property. It reads the >>> fast charge current that gets set before the charger is enabled or >>> disabled. >>> >>> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de> >> >> No. This power supply driver should not manage regulators. It is not a >> regulator consumer. For that specific need, there is a charger-manager driver. > > When I was in the middle of implementing this, I noticed that the > charger manager does everything that is needed. But it took me quite > some time to configure the DTS correctly until I realized that the > charger manager used a deprecated function > (extcon_register_interest()) and thus couldn't work. And as I didn't see > the charger-manager in any other device's DTS, I thought that this might > not be right way. > But Chanwoo Choi has a fix: https://patchwork.kernel.org/patch/8898541/ > So I will try to get it working with this patch. > >> I agree that you might configure here the charger. You might even expose >> some writeable properties through power supply class. However the >> purpose of this driver is to expose the battery charger to user-space, >> not to replace the user-space with its work. >> >> So... NACK. > > Ok, then I will try to reduce the patch to the CURRENT_NOW property > support. > >> If you would like to play with charger-manager, here is my old DTS for >> Trats2 (might need updates): > > Is there a reason that this patch is not in the kernel? It would have > been very helpful for me :) In general, DeviceTree should describe the hardware. Charger manager is an abstract device, not a real one. This DT node does not describe real hardware but it is a driver-specific glue needed to get things done. I am not convinced that we should add to DTS such abstract devices. What about people who want to control charging from user-space? They would have to disable charger-manager from config. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/arch/arm/boot/dts/exynos4412-trats2.dts +++ b/arch/arm/boot/dts/exynos4412-trats2.dts @@ -856,6 +856,44 @@ }; }; + charger-manager@0 { + compatible = "charger-manager"; + status = "okay"; + chg-reg-supply = <&charger_reg>; + + cm-name = "battery"; + /* Polling only for external power source */ + cm-poll-mode = <2>; + cm-poll-interval = <30000>; + + cm-fullbatt-vchkdrop-ms = <30000>; + cm-fullbatt-vchkdrop-volt = <150000>; + cm-fullbatt-soc = <100>; + + cm-battery-stat = <0>; + cm-fuel-gauge = "max170xx_battery"; + + /* Allow charging for 5hr */ + cm-charging-max = <18000000>; + /* Allow discharging for 2hr */ + cm-discharging-max = <7200000>; + + cm-num-chargers = <1>; + cm-chargers = "max77693-charger"; + + charger@0 { + cm-regulator-name = "chg-reg"; + cable@0 { + cm-cable-name = "USB"; + cm-cable-extcon = "max77693-muic"; + }; + cable@1 { + cm-cable-name = "TA"; + cm-cable-extcon = "max77693-muic"; + }; + }; + }; + exynos-usbphy@125B0000 { status = "okay"; };