[RFC 0/4] Enable power supply charging control

[RFC 0/4] Enable power supply charging control

[Jenny TC]

Enable power supply based charging control for charger manager.
At present charger manager allows to control charging using
regulator interface. This series of patch extends the charger
manager to setup and monitor charging using power supply based
controls. This patch moves the features introduced in
https://lkml.org/lkml/2014/8/13/355  to charger manager.

Jenny TC (4):
  power_supply: Introduce charging object table
  power: core: Add generic interface to get battery specification.
  power_supply: Introduce charger control interface
  charger-manager: Enable psy based charge control

 drivers/power/charger-manager.c       |  486 +++++++++++++++++++++++++--------
 drivers/power/power_supply_core.c     |   86 ++++++
 include/linux/power/charger-manager.h |   30 +-
 include/linux/power_supply.h          |   67 +++++
 4 files changed, 552 insertions(+), 117 deletions(-)

-- 
1.7.9.5

[RFC 1/4] power_supply: Introduce charging object table

[Jenny TC]

Charging current (CC) and charging voltage (CV) may vary based on
battery temperature. To support CC and CV for different temperature
zones, defined a charging object which holds the properties related
to battery charging.

Signed-off-by: Jenny TC <jenny.tc@intel.com>
---
 include/linux/power_supply.h |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 096dbce..7aada44 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -252,6 +252,33 @@ struct power_supply_info {
 	int use_for_apm;
 };
 
+
+struct psy_temp_mon_table {
+	int temp_max;
+	int temp_min;
+	int charging_current; /* CC */
+	int charging_voltage; /* CV */
+	/* delta voltage at which charging should restart */
+	int maint_voltage_delta;
+};
+
+#define PSY_MAX_BAT_NAME_LEN 8
+#define PSY_MAX_TEMP_ZONE 6
+
+struct psy_charging_obj {
+	char name[PSY_MAX_BAT_NAME_LEN];
+	int battery_type;
+	int temp_max;
+	int temp_min;
+	int full_condition_soc;
+	int full_condition_capacity;
+	int full_condition_voltage;
+	int iterm; /* charge termination current */
+	/* CC/CV table for different temperature range */
+	int temp_mon_count; /* number of entries in temp_mon_table */
+	struct psy_temp_mon_table temp_mon_table[PSY_MAX_TEMP_ZONE];
+};
+
 extern struct atomic_notifier_head power_supply_notifier;
 extern int power_supply_reg_notifier(struct notifier_block *nb);
 extern void power_supply_unreg_notifier(struct notifier_block *nb);
-- 
1.7.9.5

Re: [RFC 1/4] power_supply: Introduce charging object table

[Oliver Neukum]

On Fri, 2015-03-06 at 16:03 +0530, Jenny TC wrote:
> Charging current (CC) and charging voltage (CV) may vary based on
> battery temperature. To support CC and CV for different temperature
> zones, defined a charging object which holds the properties related
> to battery charging.
> 
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> ---
>  include/linux/power_supply.h |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 096dbce..7aada44 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -252,6 +252,33 @@ struct power_supply_info {
>  	int use_for_apm;
>  };
>  
> +
> +struct psy_temp_mon_table {
> +	int temp_max;
> +	int temp_min;
> +	int charging_current; /* CC */
> +	int charging_voltage; /* CV */

In which units?

> +	/* delta voltage at which charging should restart */
> +	int maint_voltage_delta;
> +};
> +
> +#define PSY_MAX_BAT_NAME_LEN 8
> +#define PSY_MAX_TEMP_ZONE 6
> +
> +struct psy_charging_obj {
> +	char name[PSY_MAX_BAT_NAME_LEN];
> +	int battery_type;
> +	int temp_max;
> +	int temp_min;
> +	int full_condition_soc;
> +	int full_condition_capacity;
> +	int full_condition_voltage;
> +	int iterm; /* charge termination current */
> +	/* CC/CV table for different temperature range */
> +	int temp_mon_count; /* number of entries in temp_mon_table */
> +	struct psy_temp_mon_table temp_mon_table[PSY_MAX_TEMP_ZONE];
> +};
> +
>  extern struct atomic_notifier_head power_supply_notifier;
>  extern int power_supply_reg_notifier(struct notifier_block *nb);
>  extern void power_supply_unreg_notifier(struct notifier_block *nb);


-- 
Oliver Neukum <oneukum@suse.de>

Re: [RFC 1/4] power_supply: Introduce charging object table

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

Hi,

On Fri, Mar 06, 2015 at 12:12:50PM +0100, Oliver Neukum wrote:
> On Fri, 2015-03-06 at 16:03 +0530, Jenny TC wrote:
> > +struct psy_temp_mon_table {
> > +	int temp_max;
> > +	int temp_min;
> > +	int charging_current; /* CC */
> > +	int charging_voltage; /* CV */
> 
> In which units?

First few lines from power_supply.h:

/*
 * All voltages, currents, charges, energies, time and temperatures in uV,
 * µA, µAh, µWh, seconds and tenths of degree Celsius unless otherwise
 * stated. It's driver's job to convert its raw values to units in which
 * this class operates.
 */

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 1/4] power_supply: Introduce charging object table

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2150 bytes --]

Hi,

On Fri, Mar 06, 2015 at 04:03:24PM +0530, Jenny TC wrote:
> Charging current (CC) and charging voltage (CV) may vary based on
> battery temperature. To support CC and CV for different temperature
> zones, defined a charging object which holds the properties related
> to battery charging.
> 
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> ---
>  include/linux/power_supply.h |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 096dbce..7aada44 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -252,6 +252,33 @@ struct power_supply_info {
>  	int use_for_apm;
>  };
>  
> +
> +struct psy_temp_mon_table {
> +	int temp_max;
> +	int temp_min;
> +	int charging_current; /* CC */
> +	int charging_voltage; /* CV */
> +	/* delta voltage at which charging should restart */
> +	int maint_voltage_delta;
> +};
> +
> +#define PSY_MAX_BAT_NAME_LEN 8
> +#define PSY_MAX_TEMP_ZONE 6
> +
> +struct psy_charging_obj {

This is not just about charging data, but also about the batteries
thermal limits, technology and full capacity, so how about

struct psy_battery_information {

> +	char name[PSY_MAX_BAT_NAME_LEN];

char *name;

No need for arbitrary length limitation.

> +	int battery_type;
> +	int temp_max;
> +	int temp_min;
> +	int full_condition_soc;

Please be more verbose about the information being stored here.

> +	int full_condition_capacity;
> +	int full_condition_voltage;
> +	int iterm; /* charge termination current */
> +	/* CC/CV table for different temperature range */
> +	int temp_mon_count; /* number of entries in temp_mon_table */
> +	struct psy_temp_mon_table temp_mon_table[PSY_MAX_TEMP_ZONE];

No need to embed this into the struct. Just point to the array and
remove the size limitation.

> +};
> +
>  extern struct atomic_notifier_head power_supply_notifier;
>  extern int power_supply_reg_notifier(struct notifier_block *nb);
>  extern void power_supply_unreg_notifier(struct notifier_block *nb);

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC 2/4] power: core: Add generic interface to get battery specification.

[Jenny TC]

In power supply system, battery specification's dealt as static
information regardless of battery chainging. And it often assumed
fuelgauge's role to hold battery specification even same driver is
used with different batteries. To make subsystem handles above cases
properly, this patch adds helper functions to manager the battery
specification.

Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
Signed-off-by: Jenny TC <jenny.tc@intel.com>
---
 drivers/power/power_supply_core.c |   86 +++++++++++++++++++++++++++++++++++++
 include/linux/power_supply.h      |   12 ++++++
 2 files changed, 98 insertions(+)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 694e8cd..99c3455 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -650,6 +650,92 @@ static void __exit power_supply_class_exit(void)
 subsys_initcall(power_supply_class_init);
 module_exit(power_supply_class_exit);
 
+/****************************************************************
+ *                   Battery information interface              *
+ ****************************************************************/
+
+ATOMIC_NOTIFIER_HEAD(psy_battery_info_notifier);
+static LIST_HEAD(psy_battery_info_list);
+
+struct psy_battery_info {
+	struct list_head entry;
+	struct psy_charging_obj *info;
+};
+
+int psy_battery_info_notifier_register(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_register(&psy_battery_info_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(psy_battery_info_notifier_register);
+
+void psy_battery_info_notifier_unregister(struct notifier_block *nb)
+{
+	atomic_notifier_chain_unregister(&psy_battery_info_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(psy_battery_info_notifier_unregister);
+
+struct psy_charging_obj *psy_get_battery_info(const char *name)
+{
+	struct psy_battery_info *battery;
+
+	/* Sanity check */
+	if (!name)
+		goto err_out;
+
+	list_for_each_entry(battery, &psy_battery_info_list, entry) {
+		if (!strcmp(battery->info->name, name))
+			return battery->info;
+	}
+
+err_out:
+	return NULL;
+}
+EXPORT_SYMBOL(psy_get_battery_info);
+
+int psy_register_battery_info(struct psy_charging_obj *info)
+{
+	struct psy_battery_info *battery;
+
+	/* Sanity check */
+	if (!info->name)
+		return -EINVAL;
+
+	/* Check if same data is existed */
+	list_for_each_entry(battery, &psy_battery_info_list, entry)
+		if (!strcmp(battery->info->name, info->name))
+			return -EEXIST;
+
+	battery = kzalloc(sizeof(*battery), GFP_KERNEL);
+	if (!battery)
+		return -ENOMEM;
+
+	battery->info = info;
+	list_add_tail(&battery->entry, &psy_battery_info_list);
+
+	atomic_notifier_call_chain(&psy_battery_info_notifier,
+				PSY_BATT_INFO_REGISTERED, info);
+
+	return 0;
+}
+EXPORT_SYMBOL(psy_register_battery_info);
+
+void psy_unregister_battery_info(struct psy_charging_obj *info)
+{
+	struct psy_battery_info *battery, *tmp;
+
+	list_for_each_entry_safe(battery, tmp, &psy_battery_info_list, entry) {
+		if (battery->info == info) {
+			list_del(&battery->entry);
+			kfree(battery);
+			return;
+		}
+	}
+
+	atomic_notifier_call_chain(&psy_battery_info_notifier,
+				PSY_BATT_INFO_UNREGISTERED, info);
+}
+EXPORT_SYMBOL(psy_unregister_battery_info);
+
 MODULE_DESCRIPTION("Universal power supply monitor class");
 MODULE_AUTHOR("Ian Molton <spyro@f2s.com>, "
 	      "Szabolcs Gyurko, "
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 7aada44..30145d8e 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -279,6 +279,11 @@ struct psy_charging_obj {
 	struct psy_temp_mon_table temp_mon_table[PSY_MAX_TEMP_ZONE];
 };
 
+enum battery_info_notifier_events {
+	PSY_BATT_INFO_REGISTERED,
+	PSY_BATT_INFO_UNREGISTERED,
+};
+
 extern struct atomic_notifier_head power_supply_notifier;
 extern int power_supply_reg_notifier(struct notifier_block *nb);
 extern void power_supply_unreg_notifier(struct notifier_block *nb);
@@ -311,6 +316,13 @@ extern int power_supply_powers(struct power_supply *psy, struct device *dev);
 /* For APM emulation, think legacy userspace. */
 extern struct class *power_supply_class;
 
+/* Battery information helper */
+extern int psy_battery_info_notifier_register(struct notifier_block *);
+extern void psy_battery_info_notifier_unregister(struct notifier_block *);
+extern struct psy_charging_obj *psy_get_battery_info(const char *);
+extern int psy_register_battery_info(struct psy_charging_obj *);
+extern void psy_unregister_battery_info(struct psy_charging_obj *);
+
 static inline bool power_supply_is_amp_property(enum power_supply_property psp)
 {
 	switch (psp) {
-- 
1.7.9.5

Re: [RFC 2/4] power: core: Add generic interface to get battery specification.

[Oliver Neukum]

On Fri, 2015-03-06 at 16:03 +0530, Jenny TC wrote:
> In power supply system, battery specification's dealt as static
> information regardless of battery chainging. And it often assumed
> fuelgauge's role to hold battery specification even same driver is
> used with different batteries. To make subsystem handles above cases
> properly, this patch adds helper functions to manager the battery
> specification.
> 
> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> ---
>  drivers/power/power_supply_core.c |   86 +++++++++++++++++++++++++++++++++++++
>  include/linux/power_supply.h      |   12 ++++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 694e8cd..99c3455 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -650,6 +650,92 @@ static void __exit power_supply_class_exit(void)
>  subsys_initcall(power_supply_class_init);
>  module_exit(power_supply_class_exit);
>  
> +/****************************************************************
> + *                   Battery information interface              *
> + ****************************************************************/
> +
> +ATOMIC_NOTIFIER_HEAD(psy_battery_info_notifier);
> +static LIST_HEAD(psy_battery_info_list);
> +
> +struct psy_battery_info {
> +	struct list_head entry;
> +	struct psy_charging_obj *info;
> +};
> +
> +int psy_battery_info_notifier_register(struct notifier_block *nb)
> +{
> +	return atomic_notifier_chain_register(&psy_battery_info_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(psy_battery_info_notifier_register);
> +
> +void psy_battery_info_notifier_unregister(struct notifier_block *nb)
> +{
> +	atomic_notifier_chain_unregister(&psy_battery_info_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(psy_battery_info_notifier_unregister);
> +
> +struct psy_charging_obj *psy_get_battery_info(const char *name)
> +{
> +	struct psy_battery_info *battery;
> +
> +	/* Sanity check */
> +	if (!name)
> +		goto err_out;
> +
> +	list_for_each_entry(battery, &psy_battery_info_list, entry) {
> +		if (!strcmp(battery->info->name, name))
> +			return battery->info;
> +	}
> +
> +err_out:
> +	return NULL;
> +}
> +EXPORT_SYMBOL(psy_get_battery_info);
> +
> +int psy_register_battery_info(struct psy_charging_obj *info)
> +{
> +	struct psy_battery_info *battery;
> +
> +	/* Sanity check */
> +	if (!info->name)
> +		return -EINVAL;
> +
> +	/* Check if same data is existed */
> +	list_for_each_entry(battery, &psy_battery_info_list, entry)
> +		if (!strcmp(battery->info->name, info->name))
> +			return -EEXIST;
> +
> +	battery = kzalloc(sizeof(*battery), GFP_KERNEL);

That is a race condition. If you check for duplication, you'll
need a lock.

> +	if (!battery)
> +		return -ENOMEM;
> +
> +	battery->info = info;
> +	list_add_tail(&battery->entry, &psy_battery_info_list);
> +
> +	atomic_notifier_call_chain(&psy_battery_info_notifier,
> +				PSY_BATT_INFO_REGISTERED, info);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(psy_register_battery_info);
> +
> +void psy_unregister_battery_info(struct psy_charging_obj *info)
> +{
> +	struct psy_battery_info *battery, *tmp;
> +
> +	list_for_each_entry_safe(battery, tmp, &psy_battery_info_list, entry) {
> +		if (battery->info == info) {
> +			list_del(&battery->entry);
> +			kfree(battery);
> +			return;
> +		}
> +	}
> +
> +	atomic_notifier_call_chain(&psy_battery_info_notifier,
> +				PSY_BATT_INFO_UNREGISTERED, info);
> +}
> +EXPORT_SYMBOL(psy_unregister_battery_info);
> +
>  MODULE_DESCRIPTION("Universal power supply monitor class");
>  MODULE_AUTHOR("Ian Molton <spyro@f2s.com>, "
>  	      "Szabolcs Gyurko, "
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 7aada44..30145d8e 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -279,6 +279,11 @@ struct psy_charging_obj {
>  	struct psy_temp_mon_table temp_mon_table[PSY_MAX_TEMP_ZONE];
>  };
>  
> +enum battery_info_notifier_events {
> +	PSY_BATT_INFO_REGISTERED,
> +	PSY_BATT_INFO_UNREGISTERED,
> +};
> +
>  extern struct atomic_notifier_head power_supply_notifier;
>  extern int power_supply_reg_notifier(struct notifier_block *nb);
>  extern void power_supply_unreg_notifier(struct notifier_block *nb);
> @@ -311,6 +316,13 @@ extern int power_supply_powers(struct power_supply *psy, struct device *dev);
>  /* For APM emulation, think legacy userspace. */
>  extern struct class *power_supply_class;
>  
> +/* Battery information helper */
> +extern int psy_battery_info_notifier_register(struct notifier_block *);
> +extern void psy_battery_info_notifier_unregister(struct notifier_block *);
> +extern struct psy_charging_obj *psy_get_battery_info(const char *);
> +extern int psy_register_battery_info(struct psy_charging_obj *);
> +extern void psy_unregister_battery_info(struct psy_charging_obj *);
> +
>  static inline bool power_supply_is_amp_property(enum power_supply_property psp)
>  {
>  	switch (psp) {


-- 
Oliver Neukum <oneukum@suse.de>

Re: [RFC 2/4] power: core: Add generic interface to get battery specification.

[jonghwa3.lee]

Hi,
2015년 03월 06일 20:16에 Oliver Neukum 이(가) 쓴 글:
>> +
>> +	/* Check if same data is existed */
>> +	list_for_each_entry(battery, &psy_battery_info_list, entry)
>> +		if (!strcmp(battery->info->name, info->name))
>> +			return -EEXIST;
>> +
>> +	battery = kzalloc(sizeof(*battery), GFP_KERNEL);
> 
> That is a race condition. If you check for duplication, you'll
> need a lock.
> 

Yes, you`re right. I`ll apply it.

Thanks,
Jonghwa

[RFC 3/4] power_supply: Introduce charger control interface

[Jenny TC]

Introduce power_supply charger control interfaces to control
charging from charging framework like charger-manager. The interfaces
are similar to the existing power supply get/set interfaces, but
introduce a different set of properties in order to differentiate
itself from other power supply properties which exposed in sysfs

Signed-off-by: Jenny TC <jenny.tc@intel.com>
---
 include/linux/power_supply.h |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 30145d8e..a80a3ef 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -176,6 +176,32 @@ union power_supply_propval {
 struct device;
 struct device_node;
 
+enum psy_charger_control_property {
+	PSY_CHARGER_PROP_ENABLE_CHARGING = 0,
+	PSY_CHARGER_PROP_ENABLE_CHARGER,
+	PSY_CHARGER_PROP_RESET_WDT,
+};
+
+/**
+ * struct power_supply_charger - power supply charger driver
+ * @get_property: get property function to retrieve charger properties defined
+ *	in enum power_supply_charger_property
+ * @set_property: get property function to retrieve charger properties defined
+ *	in enum power_supply_charger_property
+ *
+ * This structure is used by charger drivers to register with power supply
+ * charging driver
+ */
+
+struct power_supply_charger {
+	int (*get_property)(struct power_supply_charger *psyc,
+			    enum psy_charger_control_property pspc,
+			    union power_supply_propval *val);
+	int (*set_property)(struct power_supply_charger *psyc,
+			    enum psy_charger_control_property pspc,
+			    const union power_supply_propval *val);
+};
+
 struct power_supply {
 	const char *name;
 	enum power_supply_type type;
@@ -200,6 +226,8 @@ struct power_supply {
 	void (*external_power_changed)(struct power_supply *psy);
 	void (*set_charged)(struct power_supply *psy);
 
+	struct power_supply_charger *psy_charger;
+
 	/*
 	 * Set if thermal zone should not be created for this power supply.
 	 * For example for virtual supplies forwarding calls to actual
-- 
1.7.9.5

Re: [RFC 3/4] power_supply: Introduce charger control interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]

Hi,

On Fri, Mar 06, 2015 at 04:03:26PM +0530, Jenny TC wrote:
> Introduce power_supply charger control interfaces to control
> charging from charging framework like charger-manager. The interfaces
> are similar to the existing power supply get/set interfaces, but
> introduce a different set of properties in order to differentiate
> itself from other power supply properties which exposed in sysfs
> 
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> ---
>  include/linux/power_supply.h |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 30145d8e..a80a3ef 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -176,6 +176,32 @@ union power_supply_propval {
>  struct device;
>  struct device_node;
>  
> +enum psy_charger_control_property {
> +	PSY_CHARGER_PROP_ENABLE_CHARGING = 0,
> +	PSY_CHARGER_PROP_ENABLE_CHARGER,
> +	PSY_CHARGER_PROP_RESET_WDT,
> +};
> +
> +/**
> + * struct power_supply_charger - power supply charger driver
> + * @get_property: get property function to retrieve charger properties defined
> + *	in enum power_supply_charger_property
> + * @set_property: get property function to retrieve charger properties defined
> + *	in enum power_supply_charger_property
> + *
> + * This structure is used by charger drivers to register with power supply
> + * charging driver
> + */
> +
> +struct power_supply_charger {
> +	int (*get_property)(struct power_supply_charger *psyc,
> +			    enum psy_charger_control_property pspc,
> +			    union power_supply_propval *val);

The charging framework can simply call the same get_property
as used by sysfs. This is already done by all kind of drivers.

> +	int (*set_property)(struct power_supply_charger *psyc,
> +			    enum psy_charger_control_property pspc,
> +			    const union power_supply_propval *val);

I guess this is needed for values, which are supposed to be
writable by the kernel / charging framework, but non-writable
by the sysfs. I suggest to add set_property_kernel() instead
(and make the above properties part of enum power_supply_property)

> +};
> +
>  struct power_supply {
>  	const char *name;
>  	enum power_supply_type type;
> @@ -200,6 +226,8 @@ struct power_supply {
>  	void (*external_power_changed)(struct power_supply *psy);
>  	void (*set_charged)(struct power_supply *psy);
>  
> +	struct power_supply_charger *psy_charger;

Why is this a pointer?

> +
>  	/*
>  	 * Set if thermal zone should not be created for this power supply.
>  	 * For example for virtual supplies forwarding calls to actual

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [RFC 3/4] power_supply: Introduce charger control interface

[Tc, Jenny]

Hi,

> > +struct power_supply_charger {
> > +	int (*get_property)(struct power_supply_charger *psyc,
> > +			    enum psy_charger_control_property pspc,
> > +			    union power_supply_propval *val);
> 
> The charging framework can simply call the same get_property
> as used by sysfs. This is already done by all kind of drivers.

The idea is to separate power supply properties from power supply
charger properties. Existing power supply properties exposes a generic
property of a power supply. But the properties introduced above, is used
to control charging.  But I agree, if the charger properties are moved to
enum power_supply_property{ }, the existing set_property()/get_property()
calls can be used
 
> 
> > +	int (*set_property)(struct power_supply_charger *psyc,
> > +			    enum psy_charger_control_property pspc,
> > +			    const union power_supply_propval *val);
> 
> I guess this is needed for values, which are supposed to be
> writable by the kernel / charging framework, but non-writable
> by the sysfs. I suggest to add set_property_kernel() instead
> (and make the above properties part of enum power_supply_property)
> 

If properties are moved to enum power_supply_property {}, then it's possible
to reuse the set_property() call. property_is_writeable() can be used to block
user space  write access.


> > +};
> > +
> >  struct power_supply {
> >  	const char *name;
> >  	enum power_supply_type type;
> > @@ -200,6 +226,8 @@ struct power_supply {
> >  	void (*external_power_changed)(struct power_supply *psy);
> >  	void (*set_charged)(struct power_supply *psy);
> >
> > +	struct power_supply_charger *psy_charger;
> 
> Why is this a pointer?

This is introduced to access charger properties using power supply object.
If the properties can be accessed using existing set_property/get_property(),
then this is not really needed

-Jenny

Re: [RFC 3/4] power_supply: Introduce charger control interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]

Hi,

On Mon, Mar 09, 2015 at 12:47:18PM +0000, Tc, Jenny wrote:
> > > +struct power_supply_charger {
> > > +	int (*get_property)(struct power_supply_charger *psyc,
> > > +			    enum psy_charger_control_property pspc,
> > > +			    union power_supply_propval *val);
> > 
> > The charging framework can simply call the same get_property
> > as used by sysfs. This is already done by all kind of drivers.
> 
> The idea is to separate power supply properties from power supply
> charger properties. Existing power supply properties exposes a generic
> property of a power supply. But the properties introduced above, is used
> to control charging.  But I agree, if the charger properties are moved to
> enum power_supply_property{ }, the existing set_property()/get_property()
> calls can be used

I think making them part of power_supply_property and re-using
existing functions is sensible.

> > > +	int (*set_property)(struct power_supply_charger *psyc,
> > > +			    enum psy_charger_control_property pspc,
> > > +			    const union power_supply_propval *val);
> > 
> > I guess this is needed for values, which are supposed to be
> > writable by the kernel / charging framework, but non-writable
> > by the sysfs. I suggest to add set_property_kernel() instead
> > (and make the above properties part of enum power_supply_property)
> 
> If properties are moved to enum power_supply_property {}, then it's possible
> to reuse the set_property() call. property_is_writeable() can be used to block
> user space  write access.

Right.

> > > +};
> > > +
> > >  struct power_supply {
> > >  	const char *name;
> > >  	enum power_supply_type type;
> > > @@ -200,6 +226,8 @@ struct power_supply {
> > >  	void (*external_power_changed)(struct power_supply *psy);
> > >  	void (*set_charged)(struct power_supply *psy);
> > >
> > > +	struct power_supply_charger *psy_charger;
> > 
> > Why is this a pointer?
> 
> This is introduced to access charger properties using power supply
> object.

The question was why you choose (1) over (2), considering that the
struct contains only two pointers.

(1) struct power_supply_charger *psy_charger;
(2) struct power_supply_charger psy_charger;

> If the properties can be accessed using existing
> set_property/get_property(), then this is not really needed

Right, so don't worry about my comment :)

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC 4/4] charger-manager: Enable psy based charge control

[Jenny TC]

At present charger manager support only regulator based charging
control. But most of the charger drivers are registered with power
supply subsystem. This patch adds support for power supply based
charging control along with the regulator based control. With the
patch, charging control can be done either using power supply
interface or with regulator interface. The charging is setup
based on battery parameters received through the battery info
handlers.

Signed-off-by: Jenny TC <jenny.tc@intel.com>
---
 drivers/power/charger-manager.c       |  486 +++++++++++++++++++++++++--------
 include/linux/power/charger-manager.h |   30 +-
 2 files changed, 399 insertions(+), 117 deletions(-)

diff --git a/drivers/power/charger-manager.c b/drivers/power/charger-manager.c
index 14b0d85..b455ac2 100644
--- a/drivers/power/charger-manager.c
+++ b/drivers/power/charger-manager.c
@@ -42,6 +42,7 @@ static const char * const default_event_names[] = {
 	[CM_EVENT_BATT_OUT] = "Battery Pulled Out",
 	[CM_EVENT_BATT_OVERHEAT] = "Battery Overheat",
 	[CM_EVENT_BATT_COLD] = "Battery Cold",
+	[CM_EVENT_BATT_TZONE_CHANGE] = "Battery Temp Zone Change",
 	[CM_EVENT_EXT_PWR_IN_OUT] = "External Power Attach/Detach",
 	[CM_EVENT_CHG_START_STOP] = "Charging Start/Stop",
 	[CM_EVENT_OTHERS] = "Other battery events"
@@ -81,6 +82,97 @@ static unsigned long next_polling; /* Next appointed polling time */
 static struct workqueue_struct *cm_wq; /* init at driver add */
 static struct delayed_work cm_monitor_work; /* init at driver add */
 
+static inline int psy_set_charger_contol_prop(struct power_supply *psy,
+			    enum psy_charger_control_property pscp,
+			    const union power_supply_propval *val)
+{
+	struct power_supply_charger *psyc;
+
+	psyc = psy->psy_charger;
+
+	if (psyc && psyc->set_property)
+		return psyc->set_property(psyc, pscp, val);
+
+	return -EINVAL;
+}
+
+static inline int psy_get_charger_contol_prop(struct power_supply *psy,
+			    enum psy_charger_control_property pscp,
+			    union power_supply_propval *val)
+{
+	struct power_supply_charger *psyc;
+
+	psyc = psy->psy_charger;
+
+	if (psyc && psyc->get_property)
+		return psyc->get_property(psyc, pscp, val);
+
+	return -EINVAL;
+}
+
+static inline int psy_enable_charger(struct power_supply *psy, bool enable)
+{
+	union power_supply_propval prop_val;
+
+	prop_val.intval = enable;
+	return psy_set_charger_contol_prop(psy,
+				PSY_CHARGER_PROP_ENABLE_CHARGER, &prop_val);
+}
+
+static inline int psy_is_charger_enabled(struct power_supply *psy)
+{
+	union power_supply_propval prop_val;
+	int ret;
+
+	ret = psy_get_charger_contol_prop(psy,
+				PSY_CHARGER_PROP_ENABLE_CHARGER, &prop_val);
+	if (ret)
+		return ret;
+
+	return prop_val.intval;
+}
+
+static inline int psy_set_inlimit(struct power_supply *psy, int inlimit)
+{
+	union power_supply_propval prop_val;
+
+	prop_val.intval = inlimit;
+	return power_supply_set_property(psy,
+				POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+				&prop_val);
+}
+
+static int enable_charger(struct charger_controller *charger, bool enable)
+{
+	if (charger->regulator_control) {
+		if (enable)
+			return regulator_enable(charger->regulator_control);
+		else
+			return regulator_disable(charger->regulator_control);
+	}
+
+	return psy_enable_charger(charger->psy_control, enable);
+}
+
+static int is_charger_enabled(struct charger_controller *charger)
+{
+	if (charger->regulator_control)
+		return regulator_is_enabled(charger->regulator_control);
+
+	return psy_is_charger_enabled(charger->psy_control);
+}
+
+static int set_input_current(struct charger_cable *cable)
+{
+	if (cable->charger->regulator_control)
+		return regulator_set_current_limit(
+					cable->charger->regulator_control,
+					cable->min_uA, cable->max_uA);
+
+	return psy_set_inlimit(cable->charger->psy_control,
+					cable->max_uA / 1000);
+}
+
 /**
  * is_batt_present - See if the battery presents in place.
  * @cm: the Charger Manager representing the battery.
@@ -328,6 +420,140 @@ static bool is_polling_required(struct charger_manager *cm)
 	return false;
 }
 
+static int get_temp_zone_index(struct psy_charging_obj *cobj, int temp)
+{
+	int i, tzone_count;
+
+	tzone_count = cobj->temp_mon_count > PSY_MAX_TEMP_ZONE ?
+				PSY_MAX_TEMP_ZONE : cobj->temp_mon_count;
+
+	for (i = 0; i < tzone_count; i++) {
+		if (temp <=  cobj->temp_mon_table[i].temp_max &&
+				temp > cobj->temp_mon_table[i].temp_min)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int cm_get_battery_temperature_by_psy(struct charger_manager *cm,
+					int *temp)
+{
+	struct power_supply *fuel_gauge;
+
+	fuel_gauge = power_supply_get_by_name(cm->desc->psy_fuel_gauge);
+	if (!fuel_gauge)
+		return -ENODEV;
+
+	return fuel_gauge->get_property(fuel_gauge,
+				POWER_SUPPLY_PROP_TEMP,
+				(union power_supply_propval *)temp);
+}
+
+static int cm_get_battery_temperature(struct charger_manager *cm,
+					int *temp)
+{
+	int ret;
+
+	if (!cm->desc->measure_battery_temp)
+		return -ENODEV;
+
+#ifdef CONFIG_THERMAL
+	if (cm->tzd_batt) {
+		ret = thermal_zone_get_temp(cm->tzd_batt,
+					(unsigned long *)temp);
+		if (!ret)
+			/* Calibrate temperature unit */
+			*temp /= 100;
+	} else
+#endif
+	{
+		/* if-else continued from CONFIG_THERMAL */
+		ret = cm_get_battery_temperature_by_psy(cm, temp);
+	}
+
+	return ret;
+}
+
+
+
+static inline int get_cc_cv(struct charger_manager *cm,
+		int *cc, int *cv)
+
+{
+	int temp, ret, tzone_index;
+	struct psy_charging_obj *cobj;
+
+	ret = cm_get_battery_temperature(cm, &temp);
+	if (ret < 0)
+		return ret;
+
+	cobj = cm->battery_info;
+	tzone_index = get_temp_zone_index(cobj, temp);
+
+	if (tzone_index < 0)
+		return tzone_index;
+
+	*cc = cobj->temp_mon_table[tzone_index].charging_current;
+	*cv = cobj->temp_mon_table[tzone_index].charging_voltage;
+
+	return 0;
+}
+
+static inline int configure_charger_params(struct charger_manager *cm,
+	struct charger_controller *charger)
+{
+	int ret = 0, cc = 0, cv = 0;
+	union power_supply_propval val;
+
+	if (charger->regulator_control)
+		return 0;
+
+	if (charger->psy_control && cm->battery_info) {
+		ret = get_cc_cv(cm, &cc, &cv);
+		if (ret < 0)
+			return ret;
+
+		val.intval = cc;
+		ret = power_supply_set_property(charger->psy_control,
+				POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
+				&val);
+
+		if (ret)
+			return ret;
+
+		val.intval = cv;
+		ret = power_supply_set_property(charger->psy_control,
+				POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
+				&val);
+		if (ret)
+			return ret;
+
+		val.intval = cm->battery_info->iterm;
+		ret = power_supply_set_property(charger->psy_control,
+				POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
+				&val);
+
+		if (ret)
+			return ret;
+
+		val.intval = cm->battery_info->temp_max;
+		ret = power_supply_set_property(charger->psy_control,
+				POWER_SUPPLY_PROP_TEMP_MAX,
+				&val);
+		if (ret)
+			return ret;
+
+		val.intval = cm->battery_info->temp_min;
+		ret = power_supply_set_property(charger->psy_control,
+				POWER_SUPPLY_PROP_TEMP_MIN,
+				&val);
+	}
+
+	return ret;
+}
+
+
 /**
  * try_charger_enable - Enable/Disable chargers altogether
  * @cm: the Charger Manager representing the battery.
@@ -341,7 +567,10 @@ static bool is_polling_required(struct charger_manager *cm)
 static int try_charger_enable(struct charger_manager *cm, bool enable)
 {
 	int err = 0, i;
+	const char *charger_name;
 	struct charger_desc *desc = cm->desc;
+	struct charger_controller *charger;
+	struct regulator *reg_ctrl;
 
 	/* Ignore if it's redundent command */
 	if (enable == cm->charger_enabled)
@@ -358,14 +587,24 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
 		cm->charging_start_time = ktime_to_ms(ktime_get());
 		cm->charging_end_time = 0;
 
-		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
-			if (desc->charger_regulators[i].externally_control)
+		for (i = 0 ; i < desc->num_charger_controllers ; i++) {
+
+			charger =  &desc->charger_controllers[i];
+
+			if (charger->externally_control)
 				continue;
 
-			err = regulator_enable(desc->charger_regulators[i].consumer);
+			err = enable_charger(charger, true);
+			charger_name = charger->regulator_control ?
+					charger->regulator_name :
+					charger->psy_name;
 			if (err < 0) {
 				dev_warn(cm->dev, "Cannot enable %s regulator\n",
-					 desc->charger_regulators[i].regulator_name);
+						charger_name);
+			} else if (configure_charger_params(cm, charger) < 0) {
+				err = enable_charger(charger, false);
+				dev_warn(cm->dev, "Failed to configure %s\n",
+						charger_name);
 			}
 		}
 	} else {
@@ -376,14 +615,19 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
 		cm->charging_start_time = 0;
 		cm->charging_end_time = ktime_to_ms(ktime_get());
 
-		for (i = 0 ; i < desc->num_charger_regulators ; i++) {
-			if (desc->charger_regulators[i].externally_control)
+		for (i = 0 ; i < desc->num_charger_controllers ; i++) {
+
+			charger =  &desc->charger_controllers[i];
+			if (charger->externally_control)
 				continue;
 
-			err = regulator_disable(desc->charger_regulators[i].consumer);
+			err = enable_charger(charger, false);
 			if (err < 0) {
+				charger_name = charger->regulator_control ?
+						charger->regulator_name :
+						charger->psy_name;
 				dev_warn(cm->dev, "Cannot disable %s regulator\n",
-					 desc->charger_regulators[i].regulator_name);
+						charger_name);
 			}
 		}
 
@@ -391,13 +635,15 @@ static int try_charger_enable(struct charger_manager *cm, bool enable)
 		 * Abnormal battery state - Stop charging forcibly,
 		 * even if charger was enabled at the other places
 		 */
-		for (i = 0; i < desc->num_charger_regulators; i++) {
-			if (regulator_is_enabled(
-				    desc->charger_regulators[i].consumer)) {
-				regulator_force_disable(
-					desc->charger_regulators[i].consumer);
+		for (i = 0; i < desc->num_charger_controllers; i++) {
+			if (!desc->charger_controllers[i].regulator_control)
+				continue;
+			reg_ctrl =
+				desc->charger_controllers[i].regulator_control;
+			if (regulator_is_enabled(reg_ctrl)) {
+				regulator_force_disable(reg_ctrl);
 				dev_warn(cm->dev, "Disable regulator(%s) forcibly\n",
-					 desc->charger_regulators[i].regulator_name);
+				  desc->charger_controllers[i].regulator_name);
 			}
 		}
 	}
@@ -571,48 +817,11 @@ static int check_charging_duration(struct charger_manager *cm)
 	return ret;
 }
 
-static int cm_get_battery_temperature_by_psy(struct charger_manager *cm,
-					int *temp)
-{
-	struct power_supply *fuel_gauge;
-
-	fuel_gauge = power_supply_get_by_name(cm->desc->psy_fuel_gauge);
-	if (!fuel_gauge)
-		return -ENODEV;
-
-	return fuel_gauge->get_property(fuel_gauge,
-				POWER_SUPPLY_PROP_TEMP,
-				(union power_supply_propval *)temp);
-}
-
-static int cm_get_battery_temperature(struct charger_manager *cm,
-					int *temp)
-{
-	int ret;
-
-	if (!cm->desc->measure_battery_temp)
-		return -ENODEV;
-
-#ifdef CONFIG_THERMAL
-	if (cm->tzd_batt) {
-		ret = thermal_zone_get_temp(cm->tzd_batt, (unsigned long *)temp);
-		if (!ret)
-			/* Calibrate temperature unit */
-			*temp /= 100;
-	} else
-#endif
-	{
-		/* if-else continued from CONFIG_THERMAL */
-		ret = cm_get_battery_temperature_by_psy(cm, temp);
-	}
-
-	return ret;
-}
 
 static int cm_check_thermal_status(struct charger_manager *cm)
 {
 	struct charger_desc *desc = cm->desc;
-	int temp, upper_limit, lower_limit;
+	int temp, upper_limit, lower_limit, tzone_index = -1;
 	int ret = 0;
 
 	ret = cm_get_battery_temperature(cm, &temp);
@@ -638,6 +847,13 @@ static int cm_check_thermal_status(struct charger_manager *cm)
 		ret = CM_EVENT_BATT_OVERHEAT;
 	else if (temp < lower_limit)
 		ret = CM_EVENT_BATT_COLD;
+	else if (cm->battery_info)
+		tzone_index = get_temp_zone_index(cm->battery_info, temp);
+
+	if (tzone_index > 0 && cm->temp_zone != tzone_index) {
+		ret = CM_EVENT_BATT_TZONE_CHANGE;
+		cm->temp_zone = tzone_index;
+	}
 
 	return ret;
 }
@@ -656,14 +872,15 @@ static bool _cm_monitor(struct charger_manager *cm)
 	temp_alrt = cm_check_thermal_status(cm);
 
 	/* It has been stopped already */
-	if (temp_alrt && cm->emergency_stop)
+	if (temp_alrt &&  temp_alrt != CM_EVENT_BATT_TZONE_CHANGE
+			&& cm->emergency_stop)
 		return false;
 
 	/*
 	 * Check temperature whether overheat or cold.
 	 * If temperature is out of range normal state, stop charging.
 	 */
-	if (temp_alrt) {
+	 if (temp_alrt && temp_alrt != CM_EVENT_BATT_TZONE_CHANGE) {
 		cm->emergency_stop = temp_alrt;
 		if (!try_charger_enable(cm, false))
 			uevent_notify(cm, default_event_names[temp_alrt]);
@@ -698,6 +915,16 @@ static bool _cm_monitor(struct charger_manager *cm)
 		fullbatt_vchk(&cm->fullbatt_vchk_work.work);
 	} else {
 		cm->emergency_stop = 0;
+		/*
+		 * Check temperature zone changed or not
+		 */
+		if (temp_alrt == CM_EVENT_BATT_TZONE_CHANGE) {
+			/*
+			 * Set charger_enabled = false to force charger enable
+			 * and configure charger with new params
+			 */
+			cm->charger_enabled = false;
+		}
 		if (is_ext_pwr_online(cm)) {
 			if (!try_charger_enable(cm, true))
 				uevent_notify(cm, "CHARGING");
@@ -1107,18 +1334,22 @@ static void charger_extcon_work(struct work_struct *work)
 	struct charger_cable *cable =
 			container_of(work, struct charger_cable, wq);
 	int ret;
+	const char *charger_name;
+
+	charger_name = cable->charger->regulator_name ?
+				cable->charger->regulator_name :
+				cable->charger->psy_name;
 
 	if (cable->attached && cable->min_uA != 0 && cable->max_uA != 0) {
-		ret = regulator_set_current_limit(cable->charger->consumer,
-					cable->min_uA, cable->max_uA);
+		ret = set_input_current(cable);
 		if (ret < 0) {
 			pr_err("Cannot set current limit of %s (%s)\n",
-			       cable->charger->regulator_name, cable->name);
+				charger_name, cable->name);
 			return;
 		}
 
 		pr_info("Set current limit of %s : %duA ~ %duA\n",
-			cable->charger->regulator_name,
+			charger_name,
 			cable->min_uA, cable->max_uA);
 	}
 
@@ -1206,21 +1437,32 @@ static int charger_extcon_init(struct charger_manager *cm,
 static int charger_manager_register_extcon(struct charger_manager *cm)
 {
 	struct charger_desc *desc = cm->desc;
-	struct charger_regulator *charger;
+	struct charger_controller *charger;
 	int ret = 0;
 	int i;
 	int j;
 
-	for (i = 0; i < desc->num_charger_regulators; i++) {
-		charger = &desc->charger_regulators[i];
+	for (i = 0; i < desc->num_charger_controllers; i++) {
+		charger = &desc->charger_controllers[i];
 
-		charger->consumer = regulator_get(cm->dev,
+		if (charger->regulator_name) {
+			charger->regulator_control = regulator_get(cm->dev,
+						charger->regulator_name);
+			if (IS_ERR(charger->regulator_control)) {
+				dev_err(cm->dev, "Cannot find charger control(%s)\n",
 					charger->regulator_name);
-		if (IS_ERR(charger->consumer)) {
-			dev_err(cm->dev, "Cannot find charger(%s)\n",
-				charger->regulator_name);
-			return PTR_ERR(charger->consumer);
+				return PTR_ERR(charger->regulator_control);
+			}
+		} else if (charger->psy_name) {
+			charger->psy_control = power_supply_get_by_name(
+							charger->psy_name);
+			if (IS_ERR_OR_NULL(charger->psy_control)) {
+				dev_err(cm->dev, "Cannot find charger control(%s)\n",
+					charger->psy_name);
+				return -EINVAL;
+			}
 		}
+
 		charger->cm = cm;
 
 		for (j = 0; j < charger->num_cables; j++) {
@@ -1245,8 +1487,8 @@ err:
 static ssize_t charger_name_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	struct charger_regulator *charger
-		= container_of(attr, struct charger_regulator, attr_name);
+	struct charger_controller *charger
+		= container_of(attr, struct charger_controller, attr_name);
 
 	return sprintf(buf, "%s\n", charger->regulator_name);
 }
@@ -1254,12 +1496,12 @@ static ssize_t charger_name_show(struct device *dev,
 static ssize_t charger_state_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	struct charger_regulator *charger
-		= container_of(attr, struct charger_regulator, attr_state);
+	struct charger_controller *charger
+		= container_of(attr, struct charger_controller, attr_state);
 	int state = 0;
 
 	if (!charger->externally_control)
-		state = regulator_is_enabled(charger->consumer);
+		state = is_charger_enabled(charger);
 
 	return sprintf(buf, "%s\n", state ? "enabled" : "disabled");
 }
@@ -1267,8 +1509,8 @@ static ssize_t charger_state_show(struct device *dev,
 static ssize_t charger_externally_control_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	struct charger_regulator *charger = container_of(attr,
-			struct charger_regulator, attr_externally_control);
+	struct charger_controller *charger = container_of(attr,
+			struct charger_controller, attr_externally_control);
 
 	return sprintf(buf, "%d\n", charger->externally_control);
 }
@@ -1277,8 +1519,8 @@ static ssize_t charger_externally_control_store(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
 {
-	struct charger_regulator *charger
-		= container_of(attr, struct charger_regulator,
+	struct charger_controller *charger
+		= container_of(attr, struct charger_controller,
 					attr_externally_control);
 	struct charger_manager *cm = charger->cm;
 	struct charger_desc *desc = cm->desc;
@@ -1298,9 +1540,9 @@ static ssize_t charger_externally_control_store(struct device *dev,
 		return count;
 	}
 
-	for (i = 0; i < desc->num_charger_regulators; i++) {
-		if (&desc->charger_regulators[i] != charger &&
-			!desc->charger_regulators[i].externally_control) {
+	for (i = 0; i < desc->num_charger_controllers; i++) {
+		if (&desc->charger_controllers[i] != charger &&
+			!desc->charger_controllers[i].externally_control) {
 			/*
 			 * At least, one charger is controlled by
 			 * charger-manager
@@ -1343,7 +1585,7 @@ static ssize_t charger_externally_control_store(struct device *dev,
 static int charger_manager_register_sysfs(struct charger_manager *cm)
 {
 	struct charger_desc *desc = cm->desc;
-	struct charger_regulator *charger;
+	struct charger_controller *charger;
 	int chargers_externally_control = 1;
 	char buf[11];
 	char *str;
@@ -1351,8 +1593,8 @@ static int charger_manager_register_sysfs(struct charger_manager *cm)
 	int i;
 
 	/* Create sysfs entry to control charger(regulator) */
-	for (i = 0; i < desc->num_charger_regulators; i++) {
-		charger = &desc->charger_regulators[i];
+	for (i = 0; i < desc->num_charger_controllers; i++) {
+		charger = &desc->charger_controllers[i];
 
 		snprintf(buf, 10, "charger.%d", i);
 		str = devm_kzalloc(cm->dev,
@@ -1389,7 +1631,7 @@ static int charger_manager_register_sysfs(struct charger_manager *cm)
 		charger->attr_externally_control.store
 				= charger_externally_control_store;
 
-		if (!desc->charger_regulators[i].externally_control ||
+		if (!desc->charger_controllers[i].externally_control ||
 				!chargers_externally_control)
 			chargers_externally_control = 0;
 
@@ -1449,11 +1691,19 @@ static int cm_init_thermal_data(struct charger_manager *cm,
 	}
 #endif
 	if (cm->desc->measure_battery_temp) {
-		/* NOTICE : Default allowable minimum charge temperature is 0 */
-		if (!desc->temp_max)
-			desc->temp_max = CM_DEFAULT_CHARGE_TEMP_MAX;
-		if (!desc->temp_diff)
+
+		if (!cm->battery_info) {
+			if (!desc->temp_max)
+				desc->temp_max = CM_DEFAULT_CHARGE_TEMP_MAX;
+			if (!desc->temp_diff)
+				desc->temp_diff = CM_DEFAULT_RECHARGE_TEMP_DIFF;
+		} else {
+			desc->temp_max = cm->battery_info->temp_max;
+			desc->temp_min = cm->battery_info->temp_min;
 			desc->temp_diff = CM_DEFAULT_RECHARGE_TEMP_DIFF;
+		}
+
+		/* NOTICE : Default allowable minimum charge temperature is 0 */
 	}
 
 	return ret;
@@ -1530,18 +1780,18 @@ static struct charger_desc *of_cm_parse_desc(struct device *dev)
 				&desc->discharging_max_duration_ms);
 
 	/* battery charger regualtors */
-	desc->num_charger_regulators = of_get_child_count(np);
-	if (desc->num_charger_regulators) {
-		struct charger_regulator *chg_regs;
+	desc->num_charger_controllers = of_get_child_count(np);
+	if (desc->num_charger_controllers) {
+		struct charger_controller *chg_regs;
 		struct device_node *child;
 
 		chg_regs = devm_kzalloc(dev, sizeof(*chg_regs)
-					* desc->num_charger_regulators,
+					* desc->num_charger_controllers,
 					GFP_KERNEL);
 		if (!chg_regs)
 			return ERR_PTR(-ENOMEM);
 
-		desc->charger_regulators = chg_regs;
+		desc->charger_controllers = chg_regs;
 
 		for_each_child_of_node(np, child) {
 			struct charger_cable *cables;
@@ -1582,6 +1832,20 @@ static struct charger_desc *of_cm_parse_desc(struct device *dev)
 	return desc;
 }
 
+static int battery_info_handler(struct notifier_block *self,
+				   unsigned long event, void *info)
+{
+	struct charger_manager *cm =
+		container_of(self, struct charger_manager, batt_info_nb);
+
+	if (event == PSY_BATT_INFO_UNREGISTERED)
+		cm->battery_info = NULL;
+	else if (event == PSY_BATT_INFO_REGISTERED)
+		cm->battery_info = (struct psy_charging_obj *) info;
+
+	return NOTIFY_OK;
+}
+
 static inline struct charger_desc *cm_get_drv_data(struct platform_device *pdev)
 {
 	if (pdev->dev.of_node)
@@ -1643,8 +1907,8 @@ static int charger_manager_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "Ignoring full-battery full capacity threshold as it is not supplied\n");
 	}
 
-	if (!desc->charger_regulators || desc->num_charger_regulators < 1) {
-		dev_err(&pdev->dev, "charger_regulators undefined\n");
+	if (!desc->charger_controllers || desc->num_charger_controllers < 1) {
+		dev_err(&pdev->dev, "charger_controllers undefined\n");
 		return -EINVAL;
 	}
 
@@ -1704,6 +1968,10 @@ static int charger_manager_probe(struct platform_device *pdev)
 		strncpy(cm->psy_name_buf, desc->psy_name, PSY_NAME_MAX);
 	cm->charger_psy.name = cm->psy_name_buf;
 
+	if (!power_supply_get_property(fuel_gauge, POWER_SUPPLY_PROP_MODEL_NAME,
+						&val))
+		cm->battery_info = psy_get_battery_info(val.strval);
+
 	/* Allocate for psy properties because they may vary */
 	cm->charger_psy.properties = devm_kzalloc(&pdev->dev,
 				sizeof(enum power_supply_property)
@@ -1753,7 +2021,6 @@ static int charger_manager_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Cannot initialize extcon device\n");
 		goto err_reg_extcon;
 	}
-
 	/* Register sysfs entry for charger(regulator) */
 	ret = charger_manager_register_sysfs(cm);
 	if (ret < 0) {
@@ -1773,6 +2040,8 @@ static int charger_manager_probe(struct platform_device *pdev)
 	 */
 	device_init_wakeup(&pdev->dev, true);
 	device_set_wakeup_capable(&pdev->dev, false);
+	cm->batt_info_nb.notifier_call = battery_info_handler;
+	power_supply_reg_notifier(&cm->batt_info_nb);
 
 	/*
 	 * Charger-manager have to check the charging state right after
@@ -1786,26 +2055,27 @@ static int charger_manager_probe(struct platform_device *pdev)
 	return 0;
 
 err_reg_sysfs:
-	for (i = 0; i < desc->num_charger_regulators; i++) {
-		struct charger_regulator *charger;
+	for (i = 0; i < desc->num_charger_controllers; i++) {
+		struct charger_controller *charger;
 
-		charger = &desc->charger_regulators[i];
+		charger = &desc->charger_controllers[i];
 		sysfs_remove_group(&cm->charger_psy.dev->kobj,
 				&charger->attr_g);
 	}
 err_reg_extcon:
-	for (i = 0; i < desc->num_charger_regulators; i++) {
-		struct charger_regulator *charger;
+	for (i = 0; i < desc->num_charger_controllers; i++) {
+		struct charger_controller *charger;
 
-		charger = &desc->charger_regulators[i];
+		charger = &desc->charger_controllers[i];
 		for (j = 0; j < charger->num_cables; j++) {
 			struct charger_cable *cable = &charger->cables[j];
 			/* Remove notifier block if only edev exists */
 			if (cable->extcon_dev.edev)
 				extcon_unregister_interest(&cable->extcon_dev);
 		}
-
-		regulator_put(desc->charger_regulators[i].consumer);
+		if (desc->charger_controllers[i].regulator_control)
+			regulator_put(
+			   desc->charger_controllers[i].regulator_control);
 	}
 
 	power_supply_unregister(&cm->charger_psy);
@@ -1828,17 +2098,19 @@ static int charger_manager_remove(struct platform_device *pdev)
 	cancel_work_sync(&setup_polling);
 	cancel_delayed_work_sync(&cm_monitor_work);
 
-	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
-		struct charger_regulator *charger
-				= &desc->charger_regulators[i];
+	for (i = 0 ; i < desc->num_charger_controllers ; i++) {
+		struct charger_controller *charger
+				= &desc->charger_controllers[i];
 		for (j = 0 ; j < charger->num_cables ; j++) {
 			struct charger_cable *cable = &charger->cables[j];
 			extcon_unregister_interest(&cable->extcon_dev);
 		}
 	}
 
-	for (i = 0 ; i < desc->num_charger_regulators ; i++)
-		regulator_put(desc->charger_regulators[i].consumer);
+	for (i = 0 ; i < desc->num_charger_controllers ; i++)
+		if (desc->charger_controllers[i].regulator_control)
+			regulator_put(
+			   desc->charger_controllers[i].regulator_control);
 
 	power_supply_unregister(&cm->charger_psy);
 
diff --git a/include/linux/power/charger-manager.h b/include/linux/power/charger-manager.h
index 416ebeb..928cb2d 100644
--- a/include/linux/power/charger-manager.h
+++ b/include/linux/power/charger-manager.h
@@ -40,6 +40,7 @@ enum cm_event_types {
 	CM_EVENT_BATT_OUT,
 	CM_EVENT_BATT_OVERHEAT,
 	CM_EVENT_BATT_COLD,
+	CM_EVENT_BATT_TZONE_CHANGE,
 	CM_EVENT_EXT_PWR_IN_OUT,
 	CM_EVENT_CHG_START_STOP,
 	CM_EVENT_OTHERS,
@@ -58,7 +59,7 @@ enum cm_event_types {
  * @attached: the state of charger cable.
  *	true: the charger cable is attached
  *	false: the charger cable is detached
- * @charger: the instance of struct charger_regulator.
+ * @charger: the instance of struct charger_controller.
  * @cm: the Charger Manager representing the battery.
  */
 struct charger_cable {
@@ -73,7 +74,7 @@ struct charger_cable {
 	/* The state of charger cable */
 	bool attached;
 
-	struct charger_regulator *charger;
+	struct charger_controller *charger;
 
 	/*
 	 * Set min/max current of regulator to protect over-current issue
@@ -86,9 +87,9 @@ struct charger_cable {
 };
 
 /**
- * struct charger_regulator
+ * struct charger_controller
  * @regulator_name: the name of regulator for using charger.
- * @consumer: the regulator consumer for the charger.
+ * @regulator_control: the regulator regulator_control for the charger.
  * @externally_control:
  *	Set if the charger-manager cannot control charger,
  *	the charger will be maintained with disabled state.
@@ -104,10 +105,12 @@ struct charger_cable {
  * @attr_externally_control: "externally_control" sysfs entry
  * @attrs: Arrays pointing to attr_name/state/externally_control for attr_g
  */
-struct charger_regulator {
+struct charger_controller {
 	/* The name of regulator for charging */
 	const char *regulator_name;
-	struct regulator *consumer;
+	struct regulator *regulator_control;
+	const char *psy_name;
+	struct power_supply *psy_control;
 
 	/* charger never on when system is on */
 	int externally_control;
@@ -150,8 +153,8 @@ struct charger_regulator {
  * @battery_present:
  *	Specify where information for existance of battery can be obtained
  * @psy_charger_stat: the names of power-supply for chargers
- * @num_charger_regulator: the number of entries in charger_regulators
- * @charger_regulators: array of charger regulators
+ * @num_charger_controller: the number of entries in charger_controllers
+ * @charger_controllers: array of charger regulators
  * @psy_fuel_gauge: the name of power-supply for fuel gauge
  * @thermal_zone : the name of thermal zone for battery
  * @temp_min : Minimum battery temperature for charging.
@@ -184,8 +187,8 @@ struct charger_desc {
 
 	const char **psy_charger_stat;
 
-	int num_charger_regulators;
-	struct charger_regulator *charger_regulators;
+	int num_charger_controllers;
+	struct charger_controller *charger_controllers;
 
 	const char *psy_fuel_gauge;
 
@@ -217,6 +220,8 @@ struct charger_desc {
  * @fullbatt_vchk_work: work queue for full battery check
  * @emergency_stop:
  *	When setting true, stop charging
+ * @temp_zone: Current battery Temperature zone as defined in
+ *	charging object
  * @psy_name_buf: the name of power-supply-class for charger manager
  * @charger_psy: power_supply for charger manager
  * @status_save_ext_pwr_inserted:
@@ -240,10 +245,15 @@ struct charger_manager {
 	struct delayed_work fullbatt_vchk_work;
 
 	int emergency_stop;
+	int temp_zone;
 
 	char psy_name_buf[PSY_NAME_MAX + 1];
 	struct power_supply charger_psy;
 
+	struct psy_charging_obj *battery_info;
+
+	struct notifier_block batt_info_nb;
+
 	u64 charging_start_time;
 	u64 charging_end_time;
 };
-- 
1.7.9.5

Re: [RFC 4/4] charger-manager: Enable psy based charge control

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]

Hi,

On Fri, Mar 06, 2015 at 04:03:27PM +0530, Jenny TC wrote:
> At present charger manager support only regulator based charging
> control. But most of the charger drivers are registered with power
> supply subsystem. This patch adds support for power supply based
> charging control along with the regulator based control. With the
> patch, charging control can be done either using power supply
> interface or with regulator interface. The charging is setup
> based on battery parameters received through the battery info
> handlers.

[...]

(so far I only skipped over the patch)

[...]

> @@ -1704,6 +1968,10 @@ static int charger_manager_probe(struct platform_device *pdev)
>  		strncpy(cm->psy_name_buf, desc->psy_name, PSY_NAME_MAX);
>  	cm->charger_psy.name = cm->psy_name_buf;
>  
> +	if (!power_supply_get_property(fuel_gauge, POWER_SUPPLY_PROP_MODEL_NAME,
> +						&val))
> +		cm->battery_info = psy_get_battery_info(val.strval);
> +
>  	/* Allocate for psy properties because they may vary */
>  	cm->charger_psy.properties = devm_kzalloc(&pdev->dev,
>  				sizeof(enum power_supply_property)

We are currently splitting battery data from fuel gauge data, so
acquiring the battery using the fuel gauge's MODEL_NAME is not very
nice.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [RFC 4/4] charger-manager: Enable psy based charge control

[Tc, Jenny]

Hi,
 
> On Fri, Mar 06, 2015 at 04:03:27PM +0530, Jenny TC wrote:
> > At present charger manager support only regulator based charging
> > control. But most of the charger drivers are registered with power
> > supply subsystem. This patch adds support for power supply based
> > charging control along with the regulator based control. With the
> > patch, charging control can be done either using power supply
> > interface or with regulator interface. The charging is setup
> > based on battery parameters received through the battery info
> > handlers.
> 
> [...]
> 
> (so far I only skipped over the patch)

Appreciate if you could review entire patch. I will submit next patch set
addressing all your comments.

> [...]
> 
> > @@ -1704,6 +1968,10 @@ static int charger_manager_probe(struct
> platform_device *pdev)
> >  		strncpy(cm->psy_name_buf, desc->psy_name, PSY_NAME_MAX);
> >  	cm->charger_psy.name = cm->psy_name_buf;
> >
> > +	if (!power_supply_get_property(fuel_gauge,
> POWER_SUPPLY_PROP_MODEL_NAME,
> > +						&val))
> > +		cm->battery_info = psy_get_battery_info(val.strval);
> > +
> >  	/* Allocate for psy properties because they may vary */
> >  	cm->charger_psy.properties = devm_kzalloc(&pdev->dev,
> >  				sizeof(enum power_supply_property)
> 
> We are currently splitting battery data from fuel gauge data, so
> acquiring the battery using the fuel gauge's MODEL_NAME is not very
> nice.


Will enhance struct charger_desc{ ..} to support list of battery model name
supported. This can be used to query battery_info using psy_get_battery_info()

-Jenny

RE: [RFC 1/4] power_supply: Introduce charging object table

[Tc, Jenny]

Hi,

> > +#define PSY_MAX_BAT_NAME_LEN 8
> > +#define PSY_MAX_TEMP_ZONE 6
> > +
> > +struct psy_charging_obj {
> 
> This is not just about charging data, but also about the batteries
> thermal limits, technology and full capacity, so how about
> 
> struct psy_battery_information {

Ok, Agree.
> 
> > +	char name[PSY_MAX_BAT_NAME_LEN];
> 
> char *name;
> 
> No need for arbitrary length limitation.

The length limitation is introduced to form a packed structure so that
the data can be read directly from memory without parsing.

> 
> > +	int battery_type;
> > +	int temp_max;
> > +	int temp_min;
> > +	int full_condition_soc;
> 
> Please be more verbose about the information being stored here.

Ok..Agree.
> 
> > +	int full_condition_capacity;
> > +	int full_condition_voltage;
> > +	int iterm; /* charge termination current */
> > +	/* CC/CV table for different temperature range */
> > +	int temp_mon_count; /* number of entries in temp_mon_table */
> > +	struct psy_temp_mon_table temp_mon_table[PSY_MAX_TEMP_ZONE];
> 
> No need to embed this into the struct. Just point to the array and
> remove the size limitation.

This is to make sure that the array is packed so that the data can be
read directly from memory without parsing.

-Jenny

Re: [RFC 1/4] power_supply: Introduce charging object table

['Sebastian Reichel']

[-- Attachment #1: Type: text/plain, Size: 415 bytes --]

Hi,

On Mon, Mar 09, 2015 at 12:26:21PM +0000, Tc, Jenny wrote:
> > > +	char name[PSY_MAX_BAT_NAME_LEN];
> > 
> > char *name;
> > 
> > No need for arbitrary length limitation.
> 
> The length limitation is introduced to form a packed structure so that
> the data can be read directly from memory without parsing.

This reason is ok for a device driver, but not for a kernel
subsystem.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [RFC 1/4] power_supply: Introduce charging object table

[Tc, Jenny]

> 
> On Mon, Mar 09, 2015 at 12:26:21PM +0000, Tc, Jenny wrote:
> > > > +	char name[PSY_MAX_BAT_NAME_LEN];
> > >
> > > char *name;
> > >
> > > No need for arbitrary length limitation.
> >
> > The length limitation is introduced to form a packed structure so that
> > the data can be read directly from memory without parsing.
> 
> This reason is ok for a device driver, but not for a kernel
> subsystem.

Agree, will fix this. Same for psy_temp_mon_table temp_mon_table

-Jenny