Date:	Fri, 18 Jul 2014 04:18:36 +0200
From:	Sebastian Reichel <sre@...nel.org>
To:	Jenny TC <jenny.tc@...el.com>
Cc:	linux-kernel@...r.kernel.org,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	Pavel Machek <pavel@....cz>,
	Anton Vorontsov <anton.vorontsov@...aro.org>,
	David Woodhouse <dwmw2@...radead.org>,
	David Cohen <david.a.cohen@...ux.intel.com>,
	Pallala Ramakrishna <ramakrishna.pallala@...el.com>,
	myungjoo.ham@...sung.com
Subject: Re: [PATCH 2/4] power_supply: Introduce generic psy charging driver

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.tc@...el.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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists