power_supply: Introduce generic psy charging driver]
A message that is not in the archive (sent off-list or not yet received): 20140429062526.GA12879@jenny-desktop
Hi!
(Only part of original cc-list preserved.)
Quoted 1 line
RFC: Fixed comments for patch v8, removed sorting and string comparisons
Ok, its better now.
Quoted 38 lines
The Power Supply charging driver connects multiple subsystems
to do charging in a generic way. The subsystems involvespower_supply, thermal and battery communication subsystems (1wire).With this the charging is handled in a generic way.The driver makes use of different new features - Battery Identification interfaces, pluggable charging algorithms, charger cable arbitrations etc. The patch also introduces generic interface for charger cable notifications. Charger cable events and capabilities can be notified using the generic
power_supply_notifierchain.Overall this driver removes the charging logic out of the charger chip driver and the charger chip driver can just listen to the request from the power supply charging driver to set the charger properties. This can be implemented by exposing
get_propertyand set property callbacks.Signed-off-by Jenny TC <[email protected]>--- Documentation/power/power_supply_charger.txt | 350 +++++++++ drivers/power/Kconfig | 8 + drivers/power/Makefile | 1 + drivers/power/power_supply_charger.c | 1066 ++++++++++++++++++++++++++ drivers/power/power_supply_charger.h | 226 ++++++ drivers/power/power_supply_core.c | 3 + include/linux/power/power_supply_charger.h | 304 ++++++++ include/linux/power_supply.h | 161 ++++ 8 files changed, 2119 insertions(+) create mode 100644 Documentation/power/power_supply_charger.txt create mode 100644 drivers/power/power_supply_charger.c create mode 100644 drivers/power/power_supply_charger.h create mode 100644 include/linux/power/power_supply_charger.hDocumentation/power/power_supply_charger.txt new file +0
-0
Documentation/power/power_supply_charger.txt line 6
Quoted 6 lines
Documentation/power/power_supply_charger.txt new file +6
-0@@ -0,0 +1,350 @@ +1. Introduction +=============== + +The Power Supply charging driver connects multiple subsystems +to do charging in a generic way. The subsystems involves power_supply, +thermal and battery communication subsystems (1wire).With this the charging is
Space after '.'.
Quoted 13 lines
Documentation/power/power_supply_charger.txt new file +13
-0+static struct charger_cable cable_list[] = { + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_SDP, + }, + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_CDP, + }, + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_DCP, + }, + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_ACA, + },
Documentation/power/power_supply_charger.txt line 29
Quoted 10 lines
Documentation/power/power_supply_charger.txt new file +10
-0+ { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_ACA_DOCK, + }, + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_SE1, + }, + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_AC, + }, +};
Can we get rid of this?
Documentation/power/power_supply_charger.txt line 35
Quoted 6 lines
Documentation/power/power_supply_charger.txt new file +6
-0+struct usb_phy *otg_xceiver; +static int handle_event_notification(struct notifier_block *nb, + unsigned long event, void *data); +struct notifier_block nb = { + .notifier_call = handle_event_notification, + };
You need to add way more statics.
Documentation/power/power_supply_charger.txt line 38
Quoted 3 lines
Documentation/power/power_supply_charger.txt new file +3
-0+static void update_supplied_to_psy(struct power_supply *psy) +{ + WARN_ON(charger_context == NULL);
Useless.
Documentation/power/power_supply_charger.txt line 44
Quoted 6 lines
Documentation/power/power_supply_charger.txt new file +6
-0+ for (i = 0; i < psy->num_supplicants; i++) { + charger_context->supplied_to_psy[cnt++] = + power_supply_get_by_name(psy->supplied_to[i]); + charger_context->supplied_to_psy[cnt] = NULL; + } +}
Still some name lookups to be killed.
Documentation/power/power_supply_charger.txt line 53
Quoted 9 lines
Documentation/power/power_supply_charger.txt new file +9
-0+ for (i = 0; i < pst->num_supplicants; i++) { + psb = power_supply_get_by_name(pst->supplied_to[i]); + if (psb == psy) { + batt_context->supplied_by_psy[cnt++] = pst; + batt_context->supplied_by_psy[cnt] = NULL; + break; + } + } + }
And here.
Documentation/power/power_supply_charger.txt line 54
Quoted 1 line
Documentation/power/power_supply_charger.txt new file +1
-0+ WARN_ON(psy->data == NULL);
Useless.
Quoted 5 lines
Documentation/power/power_supply_charger.txt new file +4
-0+ charger_context = (struct psy_charger_context *)psy->data; +static inline bool is_supplied_to_has_ext_pwr_changed(struct power_supply *psy) +{ + int i;
Documentation/power/power_supply_charger.txt
Quoted 10 lines
Documentation/power/power_supply_charger.txt new file +10
-0+ struct power_supply *psb; + bool is_pwr_changed_defined = true; + + for (i = 0; i < psy->num_supplicants; i++) { + psb = + power_supply_get_by_name(psy-> + supplied_to[i]); + if (psb && !psb->external_power_changed) + is_pwr_changed_defined &= false; + }
You did not really get rid of the name lookups..
&= false. Bad idea.
Quoted 9 lines
Documentation/power/power_supply_charger.txt new file +9
-0+static int trigger_algo(struct power_supply *psy) +{ + unsigned long cc = 0, cv = 0, cc_min; + struct psy_batt_context *batt_context; + struct psy_charging_algo *algo; + struct psy_batt_chrg_prof chrg_profile; + struct power_supply *psc; + int cnt = 0; +
Documentation/power/power_supply_charger.txt
Quoted 10 lines
Documentation/power/power_supply_charger.txt new file +10
-0+ if (psy->type != POWER_SUPPLY_TYPE_BATTERY) + return 0; + + if (psy_get_battery_prop(&chrg_profile)) { + pr_err("Error in getting charge profile:%s:%d\n", __FILE__, + __LINE__); + return -EINVAL; + } + + batt_context = (struct psy_batt_context *)psy->data;
Create inline function doing the cast. You have too many opencoded casts.
Documentation/power/power_supply_charger.txt
Quoted 4 lines
Documentation/power/power_supply_charger.txt new file +4
-0+ batt_context->algo_stat = algo->get_next_cc_cv + (&batt_context->batt_props, chrg_profile, &cc, &cv); +static inline void enable_supplied_by_charging + (struct power_supply *psy, bool is_enable)
This is not how you indent that, is it.
Documentation/power/power_supply_charger.txt
Quoted 13 lines
Documentation/power/power_supply_charger.txt new file +13
-0+static void __power_supply_trigger_charging_handler(struct power_supply *psy) +{ + int i; + struct power_supply *psb = NULL; + + if (!is_trigger_charging_algo(psy)) + return; + + if (psy_is_battery(psy)) { + if (trigger_algo(psy)) + enable_supplied_by_charging(psy, false); + else + enable_supplied_by_charging(psy, true);
enable...(psy, trigger_algo())
Documentation/power/power_supply_charger.txt
Quoted 2 lines
Documentation/power/power_supply_charger.txt new file +2
-0+static inline int psy_throttle_cc_value + (struct power_supply *psy, unsigned int state)
Indent.
Quoted 7 lines
Documentation/power/power_supply_charger.txt new file +7
-0+#define PSY_MAX_CV(psy) \ + psy_get_ps_int_property(psy,\ + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX) +#define PSY_VOLTAGE_NOW(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW) +#define PSY_VOLTAGE_OCV(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_OCV)
Documentation/power/power_supply_charger.txt
Quoted 10 lines
Documentation/power/power_supply_charger.txt new file +10
-0+#define PSY_CURRENT_NOW(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_CURRENT_NOW) +#define PSY_STATUS(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_STATUS) +#define PSY_TEMPERATURE(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TEMP) +#define PSY_BATTERY_TYPE(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TECHNOLOGY) +#define PSY_ONLINE(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_ONLINE)
This looks like bad idea. Just opencode it.
s/return /return /g
Thanks,
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 17 lines
Quoted 17 lines
(quoted diff) +17
-0+static struct charger_cable cable_list[] = { + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_SDP, + }, + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_CDP, + }, + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_DCP, + }, + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_USB_ACA, + }, + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_ACA_DOCK, + }, + {
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ .psy_cable_type = PSY_CHARGER_CABLE_TYPE_SE1, + }, + { + .psy_cable_type = PSY_CHARGER_CABLE_TYPE_AC, + }, +};Can we get rid of this?
Agreed, can initialize it runtime.
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ for (i = 0; i < psy->num_supplicants; i++) { + charger_context->supplied_to_psy[cnt++] = + power_supply_get_by_name(psy->supplied_to[i]); + charger_context->supplied_to_psy[cnt] = NULL; + } +}Still some name lookups to be killed.
The look up is only once, when the charger/battery driver is registered. The supplied_to is defined as character pointer in struct power_supply.
char **supplied_to;size_t num_supplicants;
This allows the drivers to define the supplied_to argument even if the supplied_to driver is loaded at later stage. So the name lookup cannot be avoided. But it is optimized to do lookup only once
Quoted 12 lines
Review
Quoted 10 lines
(quoted diff) +9
-0+ for (i = 0; i < pst->num_supplicants; i++) { + psb = power_supply_get_by_name(pst->supplied_to[i]); + if (psb == psy) { + batt_context->supplied_by_psy[cnt++] = pst; + batt_context->supplied_by_psy[cnt] = NULL; + break; + } + } + }And here.
Same here
Quoted 17 lines
Quoted 5 lines
(quoted diff) +4
-0+ charger_context = (struct psy_charger_context *)psy->data; +static inline bool is_supplied_to_has_ext_pwr_changed(struct power_supply *psy) +{ + int i;Review
Quoted 10 lines
(quoted diff) +10
-0+ struct power_supply *psb; + bool is_pwr_changed_defined = true; + + for (i = 0; i < psy->num_supplicants; i++) { + psb = + power_supply_get_by_name(psy-> + supplied_to[i]); + if (psb && !psb->external_power_changed) + is_pwr_changed_defined &= false; + }You did not really get rid of the name lookups..
This lookup can be removed
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+#define PSY_MAX_CV(psy) \ + psy_get_ps_int_property(psy,\ + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX) +#define PSY_VOLTAGE_NOW(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW) +#define PSY_VOLTAGE_OCV(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_OCV) +#define PSY_CURRENT_NOW(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_CURRENT_NOW) +#define PSY_STATUS(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_STATUS)
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+#define PSY_TEMPERATURE(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TEMP) +#define PSY_BATTERY_TYPE(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TECHNOLOGY) +#define PSY_ONLINE(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_ONLINE)This looks like bad idea. Just opencode it.
This was to make it more readable, and avoids open codes in multiple places. Initially Anton had positive thoughts about this. Isn't it more readable with the macros?
Hi!
Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+#define PSY_MAX_CV(psy) \ + psy_get_ps_int_property(psy,\ + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX) +#define PSY_VOLTAGE_NOW(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW) +#define PSY_VOLTAGE_OCV(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_VOLTAGE_OCV) +#define PSY_CURRENT_NOW(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_CURRENT_NOW) +#define PSY_STATUS(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_STATUS)Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+#define PSY_TEMPERATURE(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TEMP) +#define PSY_BATTERY_TYPE(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_TECHNOLOGY) +#define PSY_ONLINE(psy) \ + psy_get_ps_int_property(psy, POWER_SUPPLY_PROP_ONLINE)This looks like bad idea. Just opencode it.
Quoted 3 lines
This was to make it more readable, and avoids open codes in multiple places. Initially Anton had positive thoughts about this. Isn't it more readable with the macros?
Well... I'd expect PSY_ONLINE() macro to do something like (psy & 0x01), not call function. Yes, it is shorter, but IMO it is not clearer.
But if Anton likes it...
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html