[RFC PATCH 0/3] power: Generic interface to get battery specification.

[RFC PATCH 0/3] power: Generic interface to get battery specification.

[Jonghwa Lee]

This patches contains supporting generic interface to get battery specification
and of-based battery driver which use the interface.

Up to now, power supply subsystem assumes that battery's charartric data is
static and also often left as fuelgauge's role. However, fuelgauge driver or
any power_supply driver can be worked with different battery with different
implementation and battery also can be changed even in runtime.
If so, it needs to notify its change to all related power_supply drivers not
let them notice all with private way. Thus, it tries to introduce generic
interface for management of the battery specification.

In addition to, for the smart battery, this'll help to abstract battery
interface which can be varied with different batteries. (SDQ, MIPI BIF..)

Jonghwa Lee (3):
  power: core: Add generic interface to get battery specification.
  power: core: Add variables related temperature to power_supply_info.
  power: of_battery: Initial support for of-based battery specification
    driver.

 .../bindings/power_supply/of_battery.txt           |   34 +++++++
 drivers/power/Kconfig                              |    7 ++
 drivers/power/Makefile                             |    1 +
 drivers/power/of_battery.c                         |  100 ++++++++++++++++++++
 drivers/power/power_supply_core.c                  |   91 ++++++++++++++++++
 include/linux/power_supply.h                       |    9 ++
 6 files changed, 242 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power_supply/of_battery.txt
 create mode 100644 drivers/power/of_battery.c

-- 
1.7.9.5

[PATCH 1/3] power: core: Add generic interface to get battery specification.

[Jonghwa Lee]

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>
---
 drivers/power/power_supply_core.c |   91 +++++++++++++++++++++++++++++++++++++
 include/linux/power_supply.h      |    7 +++
 2 files changed, 98 insertions(+)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 078afd6..59207d9 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -653,6 +653,97 @@ 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 power_supply_info *info;
+};
+
+enum battery_info_notifier_events {
+	PSY_BATT_INFO_REGISTERED,
+	PSY_BATT_INFO_UNREGISTERED,
+};
+
+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 power_supply_info *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 power_supply_info *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 power_supply_info *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 f3dea41..99306aa 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -276,6 +276,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 power_supply_info *psy_get_battery_info(const char *);
+extern int psy_register_battery_info(struct power_supply_info *);
+extern void psy_unregister_battery_info(struct power_supply_info *);
+
 static inline bool power_supply_is_amp_property(enum power_supply_property psp)
 {
 	switch (psp) {
-- 
1.7.9.5

RE: [PATCH 1/3] power: core: Add generic interface to get battery specification.

[Tc, Jenny]

> +ATOMIC_NOTIFIER_HEAD(psy_battery_info_notifier);

 Isn't it good to reuse the existing power_supply_notifier for this?

> +enum battery_info_notifier_events {
> +	PSY_BATT_INFO_REGISTERED,
> +	PSY_BATT_INFO_UNREGISTERED,
> +};

If we use the power_supply_notifier, then this can be moved to
enum power_supply_notifier_events

-Jenny

Re: [PATCH 1/3] power: core: Add generic interface to get battery specification.

[jonghwa3.lee]

On 2014년 11월 08일 19:13, Tc, Jenny wrote:

>> +ATOMIC_NOTIFIER_HEAD(psy_battery_info_notifier);
> 
>  Isn't it good to reuse the existing power_supply_notifier for this?
> 
>> +enum battery_info_notifier_events {
>> +	PSY_BATT_INFO_REGISTERED,
>> +	PSY_BATT_INFO_UNREGISTERED,
>> +};
> 
> If we use the power_supply_notifier, then this can be moved to
> enum power_supply_notifier_events
> 


It doesn't use power_supply_notifier, rather than it uses newly introduced
notifier for battery information. Intention of making of new notifier block here
is to extinguish event from power_supply_changed which might be noisy for
battery information consumer. However, If it looks wasteful code, it's not a big
deal to use existed power_supply_notifier.

Thanks,
Jonghwa

RE: [PATCH 1/3] power: core: Add generic interface to get battery specification.

[Tc, Jenny]

> 
> >> +ATOMIC_NOTIFIER_HEAD(psy_battery_info_notifier);
> >
> >  Isn't it good to reuse the existing power_supply_notifier for this?
> >
> >> +enum battery_info_notifier_events {
> >> +	PSY_BATT_INFO_REGISTERED,
> >> +	PSY_BATT_INFO_UNREGISTERED,
> >> +};
> >
> > If we use the power_supply_notifier, then this can be moved to
> > enum power_supply_notifier_events
> >
> 
> 
> It doesn't use power_supply_notifier, rather than it uses newly introduced
> notifier for battery information. Intention of making of new notifier block here
> is to extinguish event from power_supply_changed which might be noisy for
> battery information consumer. However, If it looks wasteful code, it's not a big
> deal to use existed power_supply_notifier.

Using the power_supply_notifier helps to get all power supply notifications
(power_supply_chnaged, battery info register/unregister, ..) using a single notifier. The
consumers can ignore the unwanted events.

[PATCH 2/3] power: core: Add variables related temperature to power_supply_info.

[Jonghwa Lee]

To represent thermal limitation of power_supply, it adds 'temperature_max,
temperature_min' to power_supply_info structure.

Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
---
 include/linux/power_supply.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 99306aa..0c8588c 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -241,6 +241,8 @@ struct power_supply_info {
 	int charge_empty_design;
 	int energy_full_design;
 	int energy_empty_design;
+	int temperature_max;
+	int temperature_min;
 	int use_for_apm;
 };
 
-- 
1.7.9.5

RE: [PATCH 2/3] power: core: Add variables related temperature to power_supply_info.

[Tc, Jenny]

> @@ -241,6 +241,8 @@ struct power_supply_info {
>  	int charge_empty_design;
>  	int energy_full_design;
>  	int energy_empty_design;
> +	int temperature_max;
> +	int temperature_min;
>  	int use_for_apm;
>  };


The CC,CV and restart threshold would vary based on the battery temperature
So I would suggest to have temperature zone table as part  of battery info
along with other attributes.

int iterm; //charge termination current (used to stop charging)
int temp_zone_count; // number of temperature zone tables present
struct batt_temp_mon_table temp_mon_tbl[MAX_TEMP_MON_TABLE]; //temperature zone table array

struct  batt_temp_mon_table {
	 short int temp_max;
	 short int cc;
	 short int cv;
	 short int vbat_vchk_drop_uv;
	 short int temp_min;
};

-Jenny

Re: [PATCH 2/3] power: core: Add variables related temperature to power_supply_info.

[jonghwa3.lee]

On 2014년 11월 10일 20:16, Tc, Jenny wrote:

>> @@ -241,6 +241,8 @@ struct power_supply_info {
>>  	int charge_empty_design;
>>  	int energy_full_design;
>>  	int energy_empty_design;
>> +	int temperature_max;
>> +	int temperature_min;
>>  	int use_for_apm;
>>  };
> 
> 
> The CC,CV and restart threshold would vary based on the battery temperature
> So I would suggest to have temperature zone table as part  of battery info
> along with other attributes.
> 
> int iterm; //charge termination current (used to stop charging)
> int temp_zone_count; // number of temperature zone tables present
> struct batt_temp_mon_table temp_mon_tbl[MAX_TEMP_MON_TABLE]; //temperature zone table array
> 
> struct  batt_temp_mon_table {
> 	 short int temp_max;
> 	 short int cc;
> 	 short int cv;
> 	 short int vbat_vchk_drop_uv;
> 	 short int temp_min;
> };
> 


IMO, throttling cc/cv according the temperature can be done via thermal fw
interface. However voltage drop and charging termination current can be added here.


Jonghwa

> -Jenny
> 
> 

RE: [PATCH 2/3] power: core: Add variables related temperature to power_supply_info.

[Tc, Jenny]

> > The CC,CV and restart threshold would vary based on the battery temperature
> > So I would suggest to have temperature zone table as part  of battery info
> > along with other attributes.
> >
> > int iterm; //charge termination current (used to stop charging)
> > int temp_zone_count; // number of temperature zone tables present
> > struct batt_temp_mon_table temp_mon_tbl[MAX_TEMP_MON_TABLE];
> //temperature zone table array
> >
> > struct  batt_temp_mon_table {
> > 	 short int temp_max;
> > 	 short int cc;
> > 	 short int cv;
> > 	 short int vbat_vchk_drop_uv;
> > 	 short int temp_min;
> > };
> >
> 
> 
> IMO, throttling cc/cv according the temperature can be done via thermal fw
> interface. However voltage drop and charging termination current can be added here.

The CC/CV for each battery temperature zone is defined as part of battery spec. This is
as per the JEITA/PSE standards. So IMO, this is a battery charging information
(charging object) rather than a thermal throttling information.

Also the battery information may not fit into a standard format. Different standards have
different format for charging object. So I would suggest to make it flexible enough to
support different charging object format. For example MIPI BIF charging object format
(https://members.mipi.org/wg/BIF/document/11518) and MIPI BIF Rule based charging algorithm
(http://mipi.org/sites/default/files/mipi_BIF_rule-based-charging_white-paper_1.pdf)
has different charging object format. This is why the patch  https://lkml.org/lkml/2014/8/13/355
has option to support different charging objects and different charging algorithms.

-Jenny

Re: [PATCH 2/3] power: core: Add variables related temperature to power_supply_info.

[Pavel Machek]

On Tue 2014-11-11 04:56:31, Tc, Jenny wrote:
> > > The CC,CV and restart threshold would vary based on the battery temperature
> > > So I would suggest to have temperature zone table as part  of battery info
> > > along with other attributes.
> > >
> > > int iterm; //charge termination current (used to stop charging)
> > > int temp_zone_count; // number of temperature zone tables present
> > > struct batt_temp_mon_table temp_mon_tbl[MAX_TEMP_MON_TABLE];
> > //temperature zone table array
> > >
> > > struct  batt_temp_mon_table {
> > > 	 short int temp_max;
> > > 	 short int cc;
> > > 	 short int cv;
> > > 	 short int vbat_vchk_drop_uv;
> > > 	 short int temp_min;
> > > };
> > >
> > 
> > 
> > IMO, throttling cc/cv according the temperature can be done via thermal fw
> > interface. However voltage drop and charging termination current can be added here.
> 
> The CC/CV for each battery temperature zone is defined as part of battery spec. This is
> as per the JEITA/PSE standards. So IMO, this is a battery charging information
> (charging object) rather than a thermal throttling information.
> 
> Also the battery information may not fit into a standard format. Different standards have
> different format for charging object. So I would suggest to make it flexible enough to
> support different charging object format. For example MIPI BIF charging object format
> (https://members.mipi.org/wg/BIF/document/11518) and MIPI BIF Rule based charging algorithm
> (http://mipi.org/sites/default/files/mipi_BIF_rule-based-charging_white-paper_1.pdf)
> has different charging object format. This is why the patch  https://lkml.org/lkml/2014/8/13/355
> has option to support different charging objects and different charging algorithms.

Yes, and this is also why your patches are not being
merged. Overengineered, too complex. Citing standards will not improve
the patches.

And yes, adding cc/cv to the thermal interface seems like a good idea
to me.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH 2/3] power: core: Add variables related temperature to power_supply_info.

[Tc, Jenny]

> > The CC/CV for each battery temperature zone is defined as part of battery spec.
> This is
> > as per the JEITA/PSE standards. So IMO, this is a battery charging information
> > (charging object) rather than a thermal throttling information.
> >
> > Also the battery information may not fit into a standard format. Different standards
> have
> > different format for charging object. So I would suggest to make it flexible enough to
> > support different charging object format. For example MIPI BIF charging object
> format
> > (https://members.mipi.org/wg/BIF/document/11518) and MIPI BIF Rule based
> charging algorithm
> > (http://mipi.org/sites/default/files/mipi_BIF_rule-based-charging_white-paper_1.pdf)
> > has different charging object format. This is why the patch
> https://lkml.org/lkml/2014/8/13/355
> > has option to support different charging objects and different charging algorithms.
> 
> Yes, and this is also why your patches are not being
> merged. Overengineered, too complex. Citing standards will not improve
> the patches.
> 
> And yes, adding cc/cv to the thermal interface seems like a good idea
> to me.

Sorry to disagree with you. IMHO it's a charging profile and not a thermal profile. The
cc/cv information is defined as part of battery spec. If the intention here is to provide a
place for battery info, then cc/cv should be part of battery info. The latest charger chips
allows to configure CC/CV for different temperature zone. IMHO adding these information
to thermal profile doesn't seems to be the right approach since the thermal subsystem
need to be aware of the charging subsystem constraints.

The standards were cited to point where the industry is moving. Anyway let the maintainer
take a final call -  should we align with industry standards or stick to legacy charging
methodologies?

-Jenny

[PATCH 3/3] power: of_battery: Initial support for of-based battery specification driver.

[Jonghwa Lee]

This driver supports battery specification through the generic interface of
power_supply subsystem. All data are parsed from device tree and it exactly
matches with power_supply_info's content.

Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
---
 .../bindings/power_supply/of_battery.txt           |   34 +++++++
 drivers/power/Kconfig                              |    7 ++
 drivers/power/Makefile                             |    1 +
 drivers/power/of_battery.c                         |  100 ++++++++++++++++++++
 4 files changed, 142 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power_supply/of_battery.txt
 create mode 100644 drivers/power/of_battery.c

diff --git a/Documentation/devicetree/bindings/power_supply/of_battery.txt b/Documentation/devicetree/bindings/power_supply/of_battery.txt
new file mode 100644
index 0000000..5ccd2a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/of_battery.txt
@@ -0,0 +1,34 @@
+of_battery bindings
+~~~~~~~~~~~~~~~~~~~
+
+This documentation describes how to define data in device tree
+to use of_battery driver. All properties are just matched with
+power_supply_info's.
+
+Required properties :
+ - battery-name : battery's name for identifying.
+
+Optional properties :
+ - voltage_max : Maximum voltage of the battery (uV).
+ - voltage_min : Minimum voltage of the battery (uV).
+ - charge_full : Full capacity of the battery (uAh).
+ - charge_empty : Empty capacity of the battery (uAh).
+ - energy_full : Full capacity of the battery (uWh).
+ - energy_empty : Empty capacity of the battery (uWh).
+ - temperature_max : Maximum temperature for the battery (deci 'C)
+ - temperature_min : Minium temperature for the battery (deci 'C)
+
+Example:
+	battery {
+		main-battery {
+			battery-name = "battery";
+			voltage-max = <4350000>;
+			charge-full = <2500000>;
+			temperature_max = <600>;
+			temperature_min = <(-100)>;
+		};
+		backup-battery {
+			battery-name = "coin-battery";
+			voltage-max = <3000000>;
+		};
+	};
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 73cfcdf..c324ac6 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -36,6 +36,13 @@ config GENERIC_ADC_BATTERY
 	  Say Y here to enable support for the generic battery driver
 	  which uses IIO framework to read adc.
 
+config BATTERY_OF
+	tristate "Generic of-based battery specification driver"
+	depends on OF
+	help
+	  Say Y here to enable support for the of-based generic battery
+	  specification driver which gives static battery data.
+
 config MAX8925_POWER
 	tristate "MAX8925 battery charger support"
 	depends on MFD_MAX8925
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index dfa8942..38a83aa 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -6,6 +6,7 @@ power_supply-$(CONFIG_LEDS_TRIGGERS)	+= power_supply_leds.o
 
 obj-$(CONFIG_POWER_SUPPLY)	+= power_supply.o
 obj-$(CONFIG_GENERIC_ADC_BATTERY)	+= generic-adc-battery.o
+obj-$(CONFIG_BATTERY_OF)	+= of_battery.o
 
 obj-$(CONFIG_PDA_POWER)		+= pda_power.o
 obj-$(CONFIG_APM_POWER)		+= apm_power.o
diff --git a/drivers/power/of_battery.c b/drivers/power/of_battery.c
new file mode 100644
index 0000000..3c66765
--- /dev/null
+++ b/drivers/power/of_battery.c
@@ -0,0 +1,100 @@
+/*
+ * OF-based battery specification driver
+ *
+ * This driver supports generic interface to get static battery data.
+ *
+ * Copyright © 2014 Jonghwa Lee <jonghwa3.lee@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/power_supply.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+
+static struct power_supply_info *of_get_battery_info(struct device_node *np)
+{
+	struct power_supply_info *info;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	if (of_property_read_string(np, "battery-name", &info->name)) {
+		kfree(info);
+		return ERR_PTR(-EINVAL);
+	}
+
+	of_property_read_u32(np, "voltage_max", &info->voltage_max_design);
+	of_property_read_u32(np, "voltage_min", &info->voltage_min_design);
+	of_property_read_u32(np, "charge_full", &info->charge_full_design);
+	of_property_read_u32(np, "charge_empty", &info->charge_empty_design);
+	of_property_read_u32(np, "energy_full", &info->energy_full_design);
+	of_property_read_u32(np, "energy_empty", &info->energy_empty_design);
+	of_property_read_u32(np, "temperature_max", &info->temperature_max);
+	of_property_read_u32(np, "temperature_min", &info->temperature_min);
+
+	return info;
+}
+
+static int __init psy_of_battery_init(void)
+{
+	struct power_supply_info *info;
+	struct device_node *np, *child;
+	int ret;
+
+	np = of_find_node_by_name(NULL, "battery");
+	if (!np)
+		return -ENODEV;
+
+	for_each_child_of_node(np, child) {
+		info = of_get_battery_info(child);
+		if (IS_ERR(info)) {
+			pr_err("Failed to get battery information (%ld)\n",
+				PTR_ERR(info));
+			continue;
+		}
+		ret = psy_register_battery_info(info);
+		if (ret) {
+			pr_err("Failed to register battery information (%d)\n",
+				ret);
+			kfree(info);
+			continue;
+		}
+	}
+
+	return 0;
+}
+module_init(psy_of_battery_init);
+
+static void __exit psy_of_battery_exit(void)
+{
+	struct power_supply_info *info;
+	struct device_node *np, *child;
+	const char *name;
+
+	np = of_find_node_by_name(NULL, "battery");
+	if (!np)
+		return;
+
+	for_each_child_of_node(np, child) {
+		if (!of_property_read_string(np, "battery-name", &name))
+			continue;
+
+		info = psy_get_battery_info(name);
+		if (IS_ERR(info))
+			continue;
+
+		psy_unregister_battery_info(info);
+	}
+}
+module_exit(psy_of_battery_exit);
+
+MODULE_AUTHOR("Jonghwa Lee <jonhgwa3.lee@samsung.com>");
+MODULE_DESCRIPTION("of battery specification driver");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

Re: [PATCH 3/3] power: of_battery: Initial support for of-based battery specification driver.

[Pavel Machek]

On Tue 2014-10-07 19:58:38, Jonghwa Lee wrote:
> This driver supports battery specification through the generic interface of
> power_supply subsystem. All data are parsed from device tree and it exactly
> matches with power_supply_info's content.
> 
> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

> +++ b/drivers/power/of_battery.c
> @@ -0,0 +1,100 @@
> +/*
> + * OF-based battery specification driver
> + *
> + * This driver supports generic interface to get static battery data.
> + *
> + * Copyright © 2014 Jonghwa Lee <jonghwa3.lee@samsung.com>

I'd replace it with (c).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC PATCH 0/3] power: Generic interface to get battery specification.

[Sebastian Reichel]

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

Hi Jonghwa,

On Tue, Oct 07, 2014 at 07:58:35PM +0900, Jonghwa Lee wrote:
> This patches contains supporting generic interface to get battery specification
> and of-based battery driver which use the interface.
> 
> Up to now, power supply subsystem assumes that battery's charartric data is
> static and also often left as fuelgauge's role. However, fuelgauge driver or
> any power_supply driver can be worked with different battery with different
> implementation and battery also can be changed even in runtime.
> If so, it needs to notify its change to all related power_supply drivers not
> let them notice all with private way. Thus, it tries to introduce generic
> interface for management of the battery specification.
> 
> In addition to, for the smart battery, this'll help to abstract battery
> interface which can be varied with different batteries. (SDQ, MIPI BIF..)

Sorry it took me so long. Here are my thoughts regarding this
patchset:

1. Please introduce drivers/power/battery/*

As far as I understand it we will get another driver for getting the
information dynamically via MIPI BIF, SDQ, ... and drivers/power is
already quite messy. Next step is moving battery gauges and battery
chargers into their own directories, but than can come after
cleaning up the core.

2. Please use "-" instead of "_" in DT property names

3. We should add a compatible string for the battery. Maybe
something like "simple-battery"

4. I suggest to rename charge_full/empty to current_full/empty, so
that its about generic battery metadata and not charging metadata.

5. I assume temperature_max/temperature_min is used for non-charging
related safety trips (e.g. battery over temperature_max starts an
emergency shutdown).

6. Temperature based charging is most likely something we need in
the future. There are at least 3 temperature zones normally (too
cold for charging [but ok for general use]), (normal mode) and (too
hot for charging [but ok for general use]). Having a slow charging
mode before complete turnoff seems logic to me :)

Since its kind of hard to keep working on big patchsets (and also
not very nice for reviewing) I suggest to start with the basic
battery stuff and add the charging information afterwards.

-- Sebastian

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