Anton, Does this address your concerns?
-Jenny > Subject: RE: [PATCH 1/7] power_supply: Add charger control properties > > > Subject: Re: [PATCH 1/7] power_supply: Add charger control properties > > > > On Sun, 27 Oct 2013 23:18:08 -0700 Anton Vorontsov <an...@enomsg.org> wrote: > > > > > On Mon, Oct 28, 2013 at 03:36:36AM +0000, Tc, Jenny wrote: > > > > > But do we really want to control the chargers through the > > > > > power_supply's user-visible interface? It makes the whole power > > > > > supply thing so complicated that I'm already losing track of it. > > > > > Right now I think I would prefer to move all the charger logic out of > > > > > the psy class. > > > > > > > > > > > > > I think exposing properties make the logic generic, otherwise it > > > > may end up in having callback functions. > > > > > > > > Also there are some scenarios where the charging algorithm has to > > > > be in the user space. > > > > > > Which scenarios? > > Charger may be of two different path. 1) Single path 2) Dual path > > With a single path charger, the charger will be having only one output path. > This path > supplies power to battery(IBAT) and to system (ISYS). With dual path > chargers, charger > will be having two separate output paths -to battery and to system . > > With single path chargers, consider the scenario when system is idle and the > maximum > Allowed charge current is 1A. So the charger output can be set to 1A so that > battery can > charge with 1A. But with a varying load, this may not be good. If the > platform consumes > 500mA, then the charging current would be reduced to 500mA. But increasing > the output > to 1.5A may result in high charge current if the load is reduced. > > Identifying different use case and their power profile helps to handle this > scenario. > This can be better handled by some algorithm in user space which can monitor > the running > applications and derive average platform load. The algorithm can proactively > decide the > charge current based on the power and thermal characteristics of different > use case. > > So in this scenario, there is a need to modify the charging parameters from > the user space. > > > > > > > > Plus, I am more questioning if the power supply framework is the > > > right thing to control the *chargers*. Chargers are not the power > > > supply to the system or any device (well, except for the batteries > > > themselves). > > > > I'm not sure this is (always) true. > > On my device (gta04.org) the battery, the USB OTG port, and a separate > > 5VDC input can each be the power source of the whole device. > > The USB and 5VDC cannot both be active concurrently, but either can be > > active together with the battery. > > > > The device can function without the battery, so the charger plugged > > into the USB-OTG must be supplying power to the system (not just the > > battery). > > > > The "charger" functionality sits between the battery and the external > > power supply monitoring the voltage on the battery and the current > > from the external supply. Based on these values (and some timers and > > a state machine) it enabled or disables the external supply and possibly > > imposes a > current-limit on it. > > > > The three power sources all have "power_supply" devices registered > > (though the battery only does because it contains a bq27000 charger > > counter). > > I've been wondering where to put sysfs attributes to control the charging. > > I currently place them in the power_supply device for each external power > > source. > > That makes some sense for the 'current limit' value, but not for the > > 'battery volts at which to re-start charging' value. > > There is also a setting which affects whether the external source is > > switched off if the voltage drops below 4.4V. In some circumstances I > > want to leave the charger enabled then, as it could just mean the cyclist > > is taking a break > and there should be current again soon. > > > > I think the sensible place for these tune-ables is with the battery. > > i.e. the power_supply that corresponds to the battery could register "min > > voltage" and > "min current". > > The charger driver needs to know about this battery and about any > > power sources that can charge it, and uses the state of the battery to > > decide how to tune > the state of the charger. > > > > I note that there is already something a lot like this between > > 88pm860x_battery.c > > and > > 88pm860x_charger.c > > > > They manage to locate each other and the charger call get_property and > > set_property on the battery. > > > > Maybe formalising this might be a useful way forward. > > > > I'm not sure that we really need a new driver-class for chargers. > > Maybe just a way to link external power supplies to battery power > > supplies, and maybe some agreement on what they are allowed to say to each > > other. > > > > Jenny: would something like that work for you?? > > The idea of charging framework is to connect different components to setup > and monitor > charging. This includes battery identification protocols, battery driver, > charger driver, > platform thermal profile for charging, different cables (USB/AC/Wireless/MHL > etc), > different charging algorithms (PSE compliant/ruler based/pulse charging etc). > This makes the charger driver simple. It can just act as a h/w abstraction > layer and doesn't > need to be aware of the cable types/properties/thermal/battery > characteristics. All logic > resides in the framework layer. > > The proposal separates the battery characteristics (different from runtime > battery > properties) from the charger/battery driver. The battery characteristics can > be read based on > the type of battery identification interface - Analog (BSI) or digital. In > case of Analog > interface, a BSI resistance is used to identify the battery. Based on the BSI > resistance > battery charging profile and characteristics can be loaded. > > In case of digital battery profile, a one wire protocol is used to read the > battery properties > from an EEPROM inside the Battery pack. In some platforms to reduce the > battery pack > cost, the EEPROM may be on the board and may be connected using I2C lines. > > Separating out this logic from battery driver makes it more modular. So the > proposal uses > a battery identification layer to read the battery profile. So the charger > and battery drivers > (fuel gauge drivers) can read the profile from the battery identification > layer and configure > the h/w register settings appropriately. > > So the tunable attributes resides in the battery identification layer and the > drivers reads > them from this layer. > > I uploaded a modified version of the Charging Framework ELCE presentation at > https://drive.google.com/file/d/0B3urTgiXiZF6eGt4MkVZaXNlNlE/edit?usp=sharing > Hope this would give more idea about the proposal. > > > > > > > > > > > > > > Using the patch https://lkml.org/lkml/2013/7/25/204, > > > > the power supply change notification can be broadcasted. We can > > > > add notifier events for power_supply_register and thermal throttling. > > > > This way power_supply_charger.c can be a separate driver and it > > > > can listen to psy > > notifications to take actions. > > > > > > If you ever need this particular notifier, I am OK with it (but I'll > > > consider applying it only together with some its users). > > > > > > Basically, I am more against these three patches: > > > > > > [PATCH 3/7] power_supply: add throttle state [PATCH 2/7] power_supply: > > > add charger cable properties [PATCH 1/7] power_supply: Add charger > > > control properties (enable_charger part) > > [PATCH 3/7] power_supply: add throttle state and [PATCH 2/7] power_supply: add > charger cable properties can be reworked I can move them out of > power_supply.h to a new > file include/linux/power/power_supply_charger.h > > But I think the control properties exposed in patch " [PATCH 1/7] > power_supply: Add > charger control properties" qualifies to be part of power_supply.h > > INPUT_CURRENT_LIMIT - Controls the output from external supply. > CHARGE_TERM_CUR - Decides when to stop supply to battery. When supply to > battery > is stopped, battery starts supplying power to platform. > > MIN_TEMP - minimum temperature for a power supply MAX_TEMP - maximum > temperature for a power supply ENABLE_CHARGING - enable/disable charging. > Enable/disable supply to battery. > ENABLE_CHARGER - enable/disable charger. Enable/disable supply to system and > battery. > CABLE_TYPE - type of a cable, helps to identify the cable type PRIORITY - > priority for > charging if multiple charger drivers are present. > > All attributes control the external power supply or the battery. The term > charger may get > confused as "charger IC". Do you have better suggestions? > > > > > > > These three add too much "charger" specifics to the power_supply > > > stuff. I think they deserve their own subsystem/class/whatever. > > > > > > Also, the battid framework is written without any notion of > > > device/driver separation, uses global variables, and I suspect it > > > should not exist at all (psy_get_batt_prop function makes me think > > > that you should just register the i2c/spi/w1 battery with the > > > power_supply and not use the ad-hoc stuff). > > These drivers are not power supply. They just reads the static battery > profile and exposes > to the consumers. Hope previous explanation on this would be helpful > > > > > > > Anton > > > -- > > > To unsubscribe from this list: send the line "unsubscribe > > > linux-kernel" in the body of a message to majord...@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/