Hi Jenny, On Tue, Jul 08, 2014 at 11:34:19AM +0530, Jenny TC wrote: > 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 > 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_notifier chain. > > 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_property and set property callbacks.
this seems to be a superset of charger-manager, which is already in
the kernel. I would prefer not to have two different charger
mangement frameworks in the kernel.
I suggest to add features supported by charger-manager to power
supply charging driver and convert users of charger-manager to
the improved driver.
I CC'd MyungJoo Ham, who wrote the charger-manager, so that he
can also give feedback.
> Signed-off-by: Jenny TC <jenny...@intel.com>
> ---
> Documentation/power/power_supply_charger.txt | 350 +++++++++
> drivers/power/Kconfig | 8 +
> drivers/power/Makefile | 1 +
> drivers/power/power_supply_charger.c | 1016
> ++++++++++++++++++++++++++
> drivers/power/power_supply_charger.h | 226 ++++++
> drivers/power/power_supply_core.c | 3 +
> include/linux/power/power_supply_charger.h | 307 ++++++++
> include/linux/power_supply.h | 161 ++++
> 8 files changed, 2072 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.h
>
> diff --git a/Documentation/power/power_supply_charger.txt
> b/Documentation/power/power_supply_charger.txt
> new file mode 100644
> index 0000000..e81754b
> --- /dev/null
> +++ b/Documentation/power/power_supply_charger.txt
> @@ -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
(e.g. 1wire)?
> +handled in a generic way by plugging the driver to power supply subsystem.
> +
> +The driver introduces different new features - Battery Identification
> +interfaces, pluggable charging algorithms, charger cable arbitrations etc.
> +
> +In existing driver implementations the charging is done based on the static
> +battery characteristics. This is done at the boot time by passing the battery
> +properties (max_voltage, capacity) etc. as a platform data to the
> +charger/battery driver. But new generation high volt batteries needs to be
> +identified dynamically to do charging in a safe manner. The batteries are
> +coming with different communication protocols. It become necessary to
> +communicate with battery and identify it's charging profiles before setup
> +charging.
> +
> +Also the charging algorithms can vary based on the battery characteristics
> +and the platform characteristics. To handle charging in a generic way it's
> +necessary to support pluggable charging algorithms. Power Supply Charging
> +driver selects algorithms based on the type of battery charging profile.
> +This is a simple binding and can be improved later. This may be improved to
> +select the algorithms based on the platform requirements. Also we can extend
> +this driver to plug algorithms from the user space.
> +
> +The driver also introduces the charger cable arbitration. A charger may
> +supports multiple cables, but it may not be able to charge with multiple
> +cables at a time (USB/AC/Wireless etc.). The arbitration logic inside the
> +driver selects the cable based on it's capabilities and the maximum
> +charge current the platform can support.
> +
> +Also the driver exposes features to control charging on different platform
> +states. One such feature is thermal. The driver handles the thermal
> +throttling requests for charging and control charging based on the thermal
> +subsystem requirements.
> +
> +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_property and set property callbacks.
It can be, or it is supposed to be?
> +2. Reading Battery charging profile
> +===================================
> +
> +Power Supply charging driver expose APIs to retrieve battery profile of a
> +battery. The battery profile can be read by battery identification driver
> which
> +may be 1wire/I2C/SFI driver. Battery identification driver can register the
> +battery profile with the power supply charging driver using the API
> +psy_battery_prop_changed(). The driver also exposes API
> +psy_get_battery_prop() to retrieve the battery profile which can be
> +used by power supply drivers to setup the charging. Also drivers
> +can register for battery removal/insertion notifications using
> +power_supply_reg_notifier()
I think _prop() should be either _profile() or _props().
> +3. Use Charging Driver to setup charging
> +===========================================
^^^
wrong length?
> [...]
> +3.1 Registering charger chip driver with power supply charging driver
> +================================================================
^^^^^^
wrong length?
> [...]
> +3.2 Properties exposed to power supply class driver
> +==================================================
^
wrong length?
> [...]
> +3.3 Properties exposed to power supply charging driver
> +=====================================================
^
wrong length?
> [...]
> +3.4 Throttling data configuration
> +=============================
^^^^
wrong length?
> [...]
> +5. Registering new charging algorithm
> +===================================
^^
wrong length?
> [...]
> +6. TODO
> +=======
> +* Replace static cable array with dynamic list
> +* Implement safety timer and watch dog timer features with more monitoring
> +* Move charge full detection logic to psy charging driver from algorithm
> driver
Just curious: What are your plans regarding the TODO list?
> [...]
> diff --git a/drivers/power/power_supply_charger.c
> b/drivers/power/power_supply_charger.c
> new file mode 100644
> index 0000000..1993c8c
> --- /dev/null
> +++ b/drivers/power/power_supply_charger.c
> @@ -0,0 +1,1016 @@
> +/*
> + * Copyright (C) 2012 Intel Corporation
should be updated probably :)
> [...]
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/power_supply.h>
> +#include <linux/power/power_supply_charger.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +#include <linux/types.h>
> +#include <linux/usb/otg.h>
> +#include "power_supply_charger.h"
> +
> +
> +#define MAX_CHARGER_COUNT 5
^^^^^^^^^^^^^^^^^
unused?
> +#define MAX_CABLE_COUNT 15
> +static LIST_HEAD(algo_list);
> +
> +struct psy_event_node {
> + struct list_head node;
> + unsigned long event;
> + struct psy_cable_props cap;
> + struct power_supply *psy;
> + struct psy_batt_chrg_prof batt_property;
> +};
> +
> +struct charger_cable {
> + struct psy_cable_props cable_props;
> + unsigned long psy_cable_type;
> +};
> +
> +struct psy_charger_drv_context {
> + bool is_usb_cable_evt_reg;
> + int psyc_cnt;
> + int batt_status;
> + /* cache battery and charger properties */
> + struct mutex event_lock;
> + struct list_head event_queue;
> + struct psy_batt_chrg_prof batt_property;
> + struct work_struct event_work;
> + struct charger_cable cable_list[MAX_CABLE_COUNT];
> + wait_queue_head_t wait_chrg_enable;
> + spinlock_t event_queue_lock;
> +};
> +
> +DEFINE_MUTEX(battid_lock);
> +
> +static struct psy_charger_drv_context psy_chrgr;
> +static struct usb_phy *otg_xceiver;
> +static int handle_event_notification(struct notifier_block *nb,
> + unsigned long event, void *data);
> +static struct notifier_block nb = {
> + .notifier_call = handle_event_notification,
> + };
> +static void configure_chrgr_source(struct charger_cable *cable_lst);
> +static struct psy_charging_algo *power_supply_get_charging_algo
> + (struct power_supply *, struct psy_batt_chrg_prof *);
> +static void __power_supply_trigger_charging_handler(struct power_supply
> *psy);
> +static void power_supply_trigger_charging_handler(struct power_supply *psy);
> +static void trigger_algo_psy_class(void);
> +static int psy_charger_throttle_charger(struct power_supply *psy,
> + unsigned long state);
> +
> +static inline bool psy_is_battery_prop_changed(struct psy_batt_props
> *bat_prop,
> + struct psy_batt_props *bat_cache)
> +{
> + if (!bat_cache)
> + return true;
> +
> + /* return true if temperature, health or throttling state changed */
> + if ((bat_cache->temperature != bat_prop->temperature) ||
> + (bat_cache->health != bat_prop->health) ||
> + (bat_cache->throttle_state != bat_prop->throttle_state))
> + return true;
> +
> + /* return true if voltage or current changed not within TTL limit */
> + if (time_after64(bat_prop->tstamp, bat_cache->tstamp + PROP_TTL) &&
> + (bat_cache->current_now != bat_prop->current_now ||
> + bat_cache->voltage_now != bat_prop->voltage_now))
> + return true;
> +
> + return false;
> +}
> +
> +static inline bool psy_is_charger_prop_changed(struct psy_charger_props
> *prop,
> + struct psy_charger_props *cache_prop)
> +{
> + /* if online/prsent/health/is_charging is changed, then return true */
> + if (!cache_prop)
> + return true;
> +
> + if (cache_prop->online != prop->online ||
> + cache_prop->present != prop->present ||
> + cache_prop->is_charging != prop->is_charging ||
> + cache_prop->health != prop->health)
> + return true;
> + else
> + return false;
> +
> +}
> +
> +static inline void get_cur_chrgr_prop(struct power_supply *psy,
> + struct psy_charger_props *chrgr_prop)
> +{
> + chrgr_prop->is_charging = psy_is_charging_enabled(psy);
> + chrgr_prop->online = psy_is_online(psy);
> + chrgr_prop->present = psy_is_present(psy);
> + chrgr_prop->cable = psy_cable_type(psy);
> + chrgr_prop->health = PSY_HEALTH(psy);
> + chrgr_prop->tstamp = get_jiffies_64();
> +}
> +
> +static void dump_charger_props(struct psy_charger_props *props)
> +{
> + if (props)
You can drop the NULL pointer check. This should be checked already
by the parent function.
> + pr_debug("%s: present=%d is_charging=%d health=%d online=%d
> cable=%ld tstamp=%ld\n",
> + __func__, props->present, props->is_charging,
> + props->health, props->online, props->cable,
> + props->tstamp);
> +}
> +
> +static void dump_battery_props(struct psy_batt_props *props)
> +{
> + if (props)
You can drop the NULL pointer check. This should be checked already
by the parent function.
> + pr_debug("%s voltage_now=%ld current_now=%ld temperature=%d
> status=%ld health=%d tstamp=%lld",
> + __func__, props->voltage_now, props->current_now,
> + props->temperature, props->status, props->health,
> + props->tstamp);
> +}
> +
> +static int __update_supplied_psy(struct device *dev, void *data)
> +{
> + struct psy_charger_context *charger_context;
> + struct psy_batt_context *batt_context;
> + struct power_supply *psb, *psy;
> + int i;
> +
> + psy = dev_get_drvdata(dev);
> +
> + if (!psy || !psy_is_charger(psy) || !psy->data)
> + return 0;
> +
> + charger_context = psy->data;
> + charger_context->num_supplied_to_psy = 0;
> +
> + for (i = 0; i < psy->num_supplicants; i++) {
> + psb = power_supply_get_by_name(psy->supplied_to[i]);
> + if (!psb)
> + continue;
no warning?
> + if (!psb->data)
> + psb->data = devm_kzalloc(psb->dev,
> + sizeof(struct psy_batt_context), GFP_KERNEL);
devm_kzalloc can fail (recheck psb->data == NULL).
> + batt_context = psb->data;
> + batt_context->supplied_by_psy
> + [batt_context->num_supplied_by_psy++] = psy;
this can go out of bound!
> + charger_context->supplied_to_psy
> + [charger_context->num_supplied_to_psy++] = psb;
this can go out of bound!
> + }
> +
> + return 0;
> +}
> +
> [...]
> +
> +static void cache_successive_samples(long *sample_array, long new_sample)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_CUR_VOLT_SAMPLES - 1; ++i)
> + *(sample_array + i) = *(sample_array + i + 1);
> +
> + *(sample_array + i) = new_sample;
> +}
please use array syntax, e.g.
sample_array[i] = new_sample[i+1];
not using a ring buffer is ok with MAX_CUR_VOLT_SAMPLES being 3. If
this is ever increased a ring buffer should be used, though.
> [...]
> +static inline bool is_batt_prop_changed(struct power_supply *psy)
> +{
> + struct psy_batt_context *batt_context;
> + struct psy_batt_props new_batt_props;
> +
> + if (!psy->data)
> + update_supplied_psy();
> +
> + batt_context = psy->data;
^^
remove one space
> [...]
> +static int process_cable_props(struct psy_cable_props *cap)
> +{
> + struct charger_cable *cable = NULL;
> + unsigned off;
> +
> + pr_info("%s: event:%d, type:%lu, current_mA:%d\n",
> + __func__, cap->chrg_evt, cap->chrg_type, cap->current_mA);
> +
> + off = ffs(cap->chrg_type);
> +
> + if (!off || off >= ARRAY_SIZE(psy_chrgr.cable_list)) {
> + pr_err("%s:%d Invalid cable type\n", __FILE__, __LINE__);
> + return -EINVAL;
> + }
> +
> + cable = &psy_chrgr.cable_list[off - 1];
> +
> + if (cable->psy_cable_type == PSY_CHARGER_CABLE_TYPE_NONE)
> + cable->psy_cable_type = cap->chrg_type;
> +
> + memcpy((void *)&cable->cable_props, (void *)cap,
> + sizeof(cable->cable_props));
Use struct assignment instead of memcpy:
*cable->cable_props = *cap;
> +
> + configure_chrgr_source(psy_chrgr.cable_list);
> +
> + return 0;
> +}
> +
> [...]
> +
> +static int register_usb_notifier(void)
> +{
> + int retval = 0;
> +
> + otg_xceiver = usb_get_phy(USB_PHY_TYPE_USB2);
> + if (!otg_xceiver) {
> + pr_err("failure to get otg transceiver\n");
> + retval = -EIO;
> + goto notifier_reg_failed;
> + }
> + retval = usb_register_notifier(otg_xceiver, &nb);
> + if (retval) {
> + pr_err("failure to register otg notifier\n");
> + goto notifier_reg_failed;
> + }
> +
> +notifier_reg_failed:
> + return retval;
> +}
remove useless goto by returning directly.
> [...]
> +static int get_battery_status(struct power_supply *psy)
> +{
> + int status;
> + struct power_supply *psc;
> + struct psy_batt_context *batt_context;
> + int cnt;
> +
> + if (!psy_is_battery(psy) || (!psy->data))
> + return POWER_SUPPLY_STATUS_UNKNOWN;
> +
> + batt_context = psy->data;
> + status = POWER_SUPPLY_STATUS_DISCHARGING;
> + cnt = batt_context->num_supplied_by_psy;
> +
> + while (cnt--) {
> + psc = batt_context->supplied_by_psy[cnt];
> +
> + if (psy_is_present(psc))
> + status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +
> + if (!(psy_is_charging_can_be_enabled(psc)) ||
> + (!psy_is_health_good(psy)) ||
> + (!psy_is_health_good(psc)))
> + continue;
> +
> + if ((batt_context->algo_stat == PSY_ALGO_STAT_FULL) ||
> + (batt_context->algo_stat == PSY_ALGO_STAT_MAINT))
> + status = POWER_SUPPLY_STATUS_FULL;
> + else if (psy_is_charging_enabled(psc))
> + status = POWER_SUPPLY_STATUS_CHARGING;
> + }
> +
> + pr_debug("%s: Set status=%d for %s\n", __func__, status, psy->name);
> +
> + return status;
> +}
mh. So basically if there's more than one charger for a battery
we will return the status of the one, which was initialized at
last? IMHO the status from the other chargers should be used to
validate the other ones.
> [...]
> +static void update_charger_online(struct power_supply *psy)
> +{
> + int online;
> + struct psy_charger_context *charger_context;
> +
> + online = psy_is_charger_enabled(psy) ? 1 : 0;
online = !!psy_is_charger_enabled(psy)
> + psy_set_charger_online(psy, online);
> + charger_context = psy->data;
> + charger_context->charger_props.online = online;
> +}
> [...]
TODO TODO TODO
> diff --git a/drivers/power/power_supply_charger.h
> b/drivers/power/power_supply_charger.h
> new file mode 100644
> index 0000000..665ab7b
> --- /dev/null
> +++ b/drivers/power/power_supply_charger.h
> @@ -0,0 +1,226 @@
> +/*
> + * Copyright (C) 2012 Intel Corporation
^^^^
update?
> [...]
> +static inline int psy_throttle_action
> + (struct power_supply *psy, unsigned int state)
> +{
> + struct power_supply_charger *psyc;
> +
> + psyc = psy_to_psyc(psy);
> +
> + if (psyc)
> + return ((psyc->throttle_states)+state)->throttle_action;
please use array syntax!
> +
> + /* If undetermined state, better disable charger for safety reasons */
> +
> + return PSY_THROTTLE_DISABLE_CHARGER;
> +}
> +
> [...]
> +
> +static inline int psy_throttle_cc_value(struct power_supply *psy,
> + unsigned int state)
> +{
> + struct power_supply_charger *psyc;
> +
> + psyc = psy_to_psyc(psy);
> +
> + if (psyc)
> + return ((psyc->throttle_states)+state)->throttle_val;
please use array syntax!
> +
> + /* If undetermined state, better set CC as 0 */
> + return 0;
> +}
> +
> [...]
-- Sebastian
signature.asc
Description: Digital signature
