[PATCH 3/7] [RFC] Battery monitoring class

[PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

Here is battery monitor class. According to first copyright string, we're
maintaining it since 2003. I've took few days and cleaned it up to be
more suitable for mainline inclusion.

It differs from battery class at git://git.infradead.org/battery-2.6.git:

* It's using external power kernel interface, i.e. does not fake external
  powers as batteries. (Same thing David Woodhouse planed last year).

* It have predefined set of attributes, this eliminates code duplication
  by battery drivers. And also gives opportunity to write emulation drivers
  for legacy stuff (APM emulation driver follow).

  If driver can't afford some attribute, it will not appear in sysfs.

* It insists on reusing its predefined attributes *and* their units.
  So, userspace getting expected values for any battery.
  
  Also common units is required for APM/ACPI emulation.
  
  Though our battery class insisting on re-usage, but not forces it. If some
  battery driver can't convert its own raw values (can't imagine why), then
  driver is free to implement its own attributes *and* additional _units
  attribute. Though, this scheme is discouraged.

* LEDs support. Each battery register its trigger, and gadgets with LEDs
  can quickly bind to battery-charging / battery-full triggers.

Here how it looks like from user space:

# ls /sys/class/battery/main-battery/
capacity  max_capacity  max_voltage   min_current  power   subsystem  uevent
current   max_current   min_capacity  min_voltage  status  temp       voltage
# cat /sys/class/battery/main-battery/status
Full
# cat /sys/class/leds/h5400\:green-right/trigger
none h5400-radio timer hwtimer main-battery-charging [main-battery-full]
# cat /sys/class/leds/h5400\:green-right/brightness
255

---
 drivers/Kconfig           |    2 +
 drivers/Makefile          |    1 +
 drivers/battery/Kconfig   |   11 ++
 drivers/battery/Makefile  |    1 +
 drivers/battery/battery.c |  303 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/battery.h   |   98 +++++++++++++++
 6 files changed, 416 insertions(+), 0 deletions(-)
 create mode 100644 drivers/battery/Kconfig
 create mode 100644 drivers/battery/Makefile
 create mode 100644 drivers/battery/battery.c
 create mode 100644 include/linux/battery.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index c546de3..c3a0038 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -56,6 +56,8 @@ source "drivers/w1/Kconfig"
 
 source "drivers/power/Kconfig"
 
+source "drivers/battery/Kconfig"
+
 source "drivers/hwmon/Kconfig"
 
 source "drivers/mfd/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 2bdaae7..7cbfd37 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_RTC_LIB)		+= rtc/
 obj-$(CONFIG_I2C)		+= i2c/
 obj-$(CONFIG_W1)		+= w1/
 obj-$(CONFIG_EXTERNAL_POWER)	+= power/
+obj-$(CONFIG_BATTERY)		+= battery/
 obj-$(CONFIG_HWMON)		+= hwmon/
 obj-$(CONFIG_PHONE)		+= telephony/
 obj-$(CONFIG_MD)		+= md/
diff --git a/drivers/battery/Kconfig b/drivers/battery/Kconfig
new file mode 100644
index 0000000..c386593
--- /dev/null
+++ b/drivers/battery/Kconfig
@@ -0,0 +1,11 @@
+
+menu "Battery support"
+
+config BATTERY
+	tristate "Battery monitoring support"
+	select EXTERNAL_POWER
+	help
+	  Say Y here to enable generic battery status reporting in
+	  the /sys filesystem.
+
+endmenu
diff --git a/drivers/battery/Makefile b/drivers/battery/Makefile
new file mode 100644
index 0000000..a2239cb
--- /dev/null
+++ b/drivers/battery/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_BATTERY)              += battery.o
diff --git a/drivers/battery/battery.c b/drivers/battery/battery.c
new file mode 100644
index 0000000..32b8288
--- /dev/null
+++ b/drivers/battery/battery.c
@@ -0,0 +1,303 @@
+/*
+ *  Universal battery monitor class
+ *
+ *  Copyright (c) 2007  Anton Vorontsov <cbou@mail.ru>
+ *  Copyright (c) 2004  Szabolcs Gyurko
+ *  Copyright (c) 2003  Ian Molton <spyro@f2s.com>
+ *
+ *  Modified: 2004, Oct     Szabolcs Gyurko
+ *
+ *  You may use this code as per GPL version 2
+ *
+ * All voltages, currents, capacities and temperatures in mV, mA, mAh and
+ * tenths of a degree unless otherwise stated. It's driver's job to convert
+ * its raw values to which this class operates. If for some reason driver
+ * can't afford this requirement, then it have to create its own attributes,
+ * plus additional "XYZ_units" for each of them.
+ */
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/battery.h>
+
+/* If we have hwtimer trigger, then use it to blink charging LED */
+#if defined(CONFIG_LEDS_TRIGGER_HWTIMER) || \
+    defined(CONFIG_LEDS_TRIGGER_HWTIMER_MODULE)
+	#define led_trigger_register_charging led_trigger_register_hwtimer
+	#define led_trigger_unregister_charging led_trigger_unregister_hwtimer
+#else
+	#define led_trigger_register_charging led_trigger_register_simple
+	#define led_trigger_unregister_charging led_trigger_unregister_simple
+#endif
+
+struct class *battery_class;
+
+static void battery_external_power_changed(struct power_supplicant *pst,
+                                           struct power_supply *psy)
+{
+	struct battery *bat = container_of(pst, struct battery, pst);
+	pr_debug("%s\n", __FUNCTION__);
+	if (bat->external_power_changed)
+		bat->external_power_changed(bat);
+	return;
+}
+
+int battery_is_external_power_supplied(struct battery *bat)
+{
+	pr_debug("%s\n", __FUNCTION__);
+	return power_supplicant_am_i_supplied(&bat->pst);
+}
+
+void battery_status_changed(struct battery *bat)
+{
+	pr_debug("%s\n", __FUNCTION__);
+	#ifdef CONFIG_LEDS_TRIGGERS
+	switch(bat->get_status(bat))
+	{
+		case BATTERY_STATUS_FULL:
+			led_trigger_event(bat->charging_trig, LED_OFF);
+			led_trigger_event(bat->full_trig, LED_FULL);
+			break;
+		case BATTERY_STATUS_CHARGING:
+			led_trigger_event(bat->charging_trig, LED_FULL);
+			led_trigger_event(bat->full_trig, LED_OFF);
+			break;
+		default:
+			led_trigger_event(bat->charging_trig, LED_OFF);
+			led_trigger_event(bat->full_trig, LED_OFF);
+			break;
+	}
+	#endif /* CONFIG_LEDS_TRIGGERS */
+	return;
+}
+
+static char *status_text[] = {
+	"Unknown", "Charging", "Discharging", "Not charging", "Full"
+};
+
+static ssize_t battery_show_status(struct device *dev,
+                                   struct device_attribute *attr, char *buf)
+{
+	struct battery *bat = dev_get_drvdata(dev);
+	int status = 0;
+	if (bat->get_status) {
+		status = bat->get_status(bat);
+		if (status > 4)
+			status = 0;
+		return sprintf(buf, "%s\n", status_text[status]);
+	}
+	return 0;
+}
+
+/*
+ * This is because the name "current" breaks the device attr macro.
+ * The "current" word resolvs to "(get_current())" so instead of
+ * "current" "(get_current())" appears in the sysfs.
+ *
+ * The source of this definition is the device.h which calls __ATTR
+ * macro in sysfs.h which calls the __stringify macro.
+ *
+ * Only modification that the name is not tried to be resolved
+ * (as a macro let's say).
+ */
+
+#define BATTERY_INT_ATTR(_name)                                         \
+static ssize_t battery_show_##_name(struct device *dev,                 \
+                                   struct device_attribute *attr,       \
+                                   char *buf) {                         \
+	struct battery *bat = dev_get_drvdata(dev);                     \
+	return sprintf(buf, "%d\n", bat->get_##_name(bat));             \
+}                                                                       \
+static struct device_attribute dev_attr_##_name = {                     \
+	.attr = { .name = #_name, .mode = 0444, .owner = THIS_MODULE }, \
+	.show = battery_show_##_name,                                   \
+	.store = NULL,                                                  \
+}
+
+static DEVICE_ATTR(status, 0444, battery_show_status, NULL);
+BATTERY_INT_ATTR(min_voltage);
+BATTERY_INT_ATTR(min_current);
+BATTERY_INT_ATTR(min_capacity);
+BATTERY_INT_ATTR(max_voltage);
+BATTERY_INT_ATTR(max_current);
+BATTERY_INT_ATTR(max_capacity);
+BATTERY_INT_ATTR(temp);
+BATTERY_INT_ATTR(voltage);
+BATTERY_INT_ATTR(current);
+BATTERY_INT_ATTR(capacity);
+
+static int battery_create_attrs(struct battery *bat)
+{
+	int rc;
+
+	#define create_bat_attr_conditional(name)                    \
+	if(bat->get_##name) {                                        \
+		rc = device_create_file(bat->dev, &dev_attr_##name); \
+		if (rc) goto name##_failed;                          \
+	}
+
+	create_bat_attr_conditional(status);
+	create_bat_attr_conditional(min_voltage);
+	create_bat_attr_conditional(min_current);
+	create_bat_attr_conditional(min_capacity);
+	create_bat_attr_conditional(max_voltage);
+	create_bat_attr_conditional(max_current);
+	create_bat_attr_conditional(max_capacity);
+	create_bat_attr_conditional(temp);
+	create_bat_attr_conditional(voltage);
+	create_bat_attr_conditional(current);
+	create_bat_attr_conditional(capacity);
+
+	#define remove_bat_attr_conditional(name)               \
+	if(bat->get_##name)                                     \
+		device_remove_file(bat->dev, &dev_attr_##name);
+
+	goto success;
+
+capacity_failed:     remove_bat_attr_conditional(current);
+current_failed:      remove_bat_attr_conditional(voltage);
+voltage_failed:      remove_bat_attr_conditional(temp);
+temp_failed:         remove_bat_attr_conditional(max_capacity);
+max_capacity_failed: remove_bat_attr_conditional(max_current);
+max_current_failed:  remove_bat_attr_conditional(max_voltage);
+max_voltage_failed:  remove_bat_attr_conditional(min_capacity);
+min_capacity_failed: remove_bat_attr_conditional(min_current);
+min_current_failed:  remove_bat_attr_conditional(min_voltage);
+min_voltage_failed:  remove_bat_attr_conditional(status);
+status_failed:
+success:
+	return rc;
+}
+
+static void battery_remove_attrs(struct battery *bat)
+{
+	remove_bat_attr_conditional(capacity);
+	remove_bat_attr_conditional(current);
+	remove_bat_attr_conditional(voltage);
+	remove_bat_attr_conditional(temp);
+	remove_bat_attr_conditional(max_capacity);
+	remove_bat_attr_conditional(max_current);
+	remove_bat_attr_conditional(max_voltage);
+	remove_bat_attr_conditional(min_capacity);
+	remove_bat_attr_conditional(min_current);
+	remove_bat_attr_conditional(min_voltage);
+	remove_bat_attr_conditional(status);
+	return;
+}
+
+int battery_register(struct device *parent, struct battery *bat)
+{
+	int rc = 0;
+
+	bat->dev = device_create(battery_class, parent, 0, "%s", bat->name);
+	if(IS_ERR(bat->dev)) {
+		rc = PTR_ERR(bat->dev);
+		goto dev_create_failed;
+	}
+
+	dev_set_drvdata(bat->dev, bat);
+
+	rc = battery_create_attrs(bat);
+	if (rc)
+		goto create_bat_attrs_failed;
+
+	bat->pst.name = bat->name;
+	bat->pst.power_supply_changed = battery_external_power_changed;
+	rc = power_supplicant_register(&bat->pst);
+	if (rc)
+		goto power_supplicant_failed;
+
+	#ifdef CONFIG_LEDS_TRIGGERS
+	bat->charging_trig_name = kmalloc(strlen(bat->name) +
+	                                  sizeof("-charging"), GFP_KERNEL);
+	if (!bat->charging_trig_name) {
+		rc = -ENOMEM;
+		goto charging_trig_name_failed;
+	}
+
+	bat->full_trig_name = kmalloc(strlen(bat->name) +
+	                              sizeof("-full"), GFP_KERNEL);
+	if (!bat->full_trig_name) {
+		rc = -ENOMEM;
+		goto full_trig_name_failed;
+	}
+
+	strcpy(bat->charging_trig_name, bat->name);
+	strcat(bat->charging_trig_name, "-charging");
+	strcpy(bat->full_trig_name, bat->name);
+	strcat(bat->full_trig_name, "-full");
+
+	led_trigger_register_charging(bat->charging_trig_name,
+	                              &bat->charging_trig);
+	led_trigger_register_simple(bat->full_trig_name,
+	                            &bat->full_trig);
+	#endif /* CONFIG_LEDS_TRIGGERS */
+
+	goto success;
+
+#ifdef CONFIG_LEDS_TRIGGERS
+full_trig_name_failed:
+	kfree(bat->charging_trig_name);
+charging_trig_name_failed:
+#endif
+	power_supplicant_unregister(&bat->pst);
+power_supplicant_failed:
+	battery_remove_attrs(bat);
+create_bat_attrs_failed:
+	device_unregister(bat->dev);
+dev_create_failed:
+success:
+	return rc;
+}
+
+void battery_unregister(struct battery *bat)
+{
+	power_supplicant_unregister(&bat->pst);
+	battery_remove_attrs(bat);
+	device_unregister(bat->dev);
+
+	#ifdef CONFIG_LEDS_TRIGGERS
+	led_trigger_unregister_charging(bat->charging_trig);
+	led_trigger_unregister_simple(bat->full_trig);
+	kfree(bat->full_trig_name);
+	kfree(bat->charging_trig_name);
+	#endif
+
+	return;
+}
+
+static int __init battery_class_init(void)
+{
+	battery_class = class_create(THIS_MODULE, "battery");
+
+	if (IS_ERR(battery_class))
+		return PTR_ERR(battery_class);
+
+	return 0;
+}
+
+static void __exit battery_class_exit(void)
+{
+	class_destroy(battery_class);
+	return;
+}
+
+EXPORT_SYMBOL_GPL(battery_register);
+EXPORT_SYMBOL_GPL(battery_unregister);
+EXPORT_SYMBOL_GPL(battery_status_changed);
+EXPORT_SYMBOL_GPL(battery_is_external_power_supplied);
+
+/* exported for the APM Power driver, APM emulation */
+EXPORT_SYMBOL_GPL(battery_class);
+
+subsys_initcall(battery_class_init);
+module_exit(battery_class_exit);
+
+MODULE_DESCRIPTION("Universal battery monitor class");
+MODULE_AUTHOR("Ian Molton <spyro@f2s.com>, "
+              "Szabolcs Gyurko, "
+              "Anton Vorontsov <cbou@mail.ru>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/battery.h b/include/linux/battery.h
new file mode 100644
index 0000000..a687781
--- /dev/null
+++ b/include/linux/battery.h
@@ -0,0 +1,98 @@
+/*
+ *  Universal battery monitor class
+ *
+ *  Copyright (c) 2007  Anton Vorontsov <cbou@mail.ru>
+ *  Copyright (c) 2004  Szabolcs Gyurko
+ *  Copyright (c) 2003  Ian Molton <spyro@f2s.com>
+ *
+ *  Modified: 2004, Oct     Szabolcs Gyurko
+ *
+ *  You may use this code as per GPL version 2
+ *
+ * All voltages, currents, capacities and temperatures in mV, mA, mAh and
+ * tenths of a degree unless otherwise stated. It's driver's job to convert
+ * its raw values to which this class operates. If for some reason driver
+ * can't afford this requirement, then it have to create its own attributes,
+ * plus additional "XYZ_units" for each of them.
+ */
+
+#ifndef _LINUX_BATTERY_H
+#define _LINUX_BATTERY_H
+
+#include <linux/device.h>
+#include <linux/external_power.h>
+#include <linux/leds.h>
+
+#define BATTERY_STATUS_UNKNOWN      0
+#define BATTERY_STATUS_CHARGING     1
+#define BATTERY_STATUS_DISCHARGING  2
+#define BATTERY_STATUS_NOT_CHARGING 3
+#define BATTERY_STATUS_FULL         4
+
+/* 
+ * For systems where the charger determines the maximum battery capacity
+ * the min and max fields should be used to present these values to user
+ * space. Unused/uknown fields can be NULL and will not appear in sysfs.
+ */
+
+struct battery {
+	struct device *dev;
+	char *name;
+
+	/* For APM emulation, think legacy userspace. */
+	int main_battery;
+
+	/* executed in userspace, feel free to sleep */
+	int (*get_min_voltage)(struct battery *bat);
+	int (*get_min_current)(struct battery *bat);
+	int (*get_min_capacity)(struct battery *bat);
+	int (*get_max_voltage)(struct battery *bat);
+	int (*get_max_current)(struct battery *bat);
+	int (*get_max_capacity)(struct battery *bat);
+	int (*get_temp)(struct battery *bat);
+	int (*get_voltage)(struct battery *bat);
+	int (*get_current)(struct battery *bat);
+	int (*get_capacity)(struct battery *bat);
+	int (*get_status)(struct battery *bat);
+
+	/* drivers should not sleep inside it, you'll get there from ISRs */
+	void (*external_power_changed)(struct battery *bat);
+
+	/* private */
+	struct power_supplicant pst;
+
+	#ifdef CONFIG_LEDS_TRIGGERS
+	struct led_trigger *charging_trig;
+	char *charging_trig_name;
+	struct led_trigger *full_trig;
+	char *full_trig_name;
+	#endif
+};
+
+/* 
+ * This is recommended structure to specify static battery parameters.
+ * Generic one, parametrizable for different batteries. Battery device
+ * itself does bot use it, but that's what implementing most drivers,
+ * should try reuse for consistency.
+ */
+
+struct battery_info {
+	char *name;
+	int min_voltage;
+	int max_voltage;
+	int min_current;
+	int max_current;
+	int min_capacity;
+	int max_capacity;
+	int main_battery;
+};
+
+extern void battery_status_changed(struct battery *bat);
+extern int battery_is_external_power_supplied(struct battery *bat);
+extern int battery_register(struct device *parent, struct battery *bat);
+extern void battery_unregister(struct battery *bat);
+
+/* For APM emulation, think legacy userspace. */
+extern struct class *battery_class;
+
+#endif
-- 
1.5.0.5-dirty

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Randy Dunlap]

On Thu, 12 Apr 2007 03:25:03 +0400 Anton Vorontsov wrote:

> Here is battery monitor class. According to first copyright string, we're
> maintaining it since 2003. I've took few days and cleaned it up to be
> more suitable for mainline inclusion.
> 
> ---
>  drivers/Kconfig           |    2 +
>  drivers/Makefile          |    1 +
>  drivers/battery/Kconfig   |   11 ++
>  drivers/battery/Makefile  |    1 +
>  drivers/battery/battery.c |  303 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/battery.h   |   98 +++++++++++++++
>  6 files changed, 416 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/battery/Kconfig
>  create mode 100644 drivers/battery/Makefile
>  create mode 100644 drivers/battery/battery.c
>  create mode 100644 include/linux/battery.h
> 
> diff --git a/drivers/battery/battery.c b/drivers/battery/battery.c
> new file mode 100644
> index 0000000..32b8288
> --- /dev/null
> +++ b/drivers/battery/battery.c
> @@ -0,0 +1,303 @@
> +
> +void battery_status_changed(struct battery *bat)
> +{
> +	pr_debug("%s\n", __FUNCTION__);
> +	#ifdef CONFIG_LEDS_TRIGGERS

Please don't indent preprocessor controls (ifdef/endif etc.).

> +	switch(bat->get_status(bat))
> +	{
> +		case BATTERY_STATUS_FULL:
> +			led_trigger_event(bat->charging_trig, LED_OFF);
> +			led_trigger_event(bat->full_trig, LED_FULL);
> +			break;
> +		case BATTERY_STATUS_CHARGING:
> +			led_trigger_event(bat->charging_trig, LED_FULL);
> +			led_trigger_event(bat->full_trig, LED_OFF);
> +			break;
> +		default:
> +			led_trigger_event(bat->charging_trig, LED_OFF);
> +			led_trigger_event(bat->full_trig, LED_OFF);
> +			break;

Place 'switch' and 'case' at the same indent level.  This prevents
the "double-indent" for the code statements.

> +	}
> +	#endif /* CONFIG_LEDS_TRIGGERS */
> +	return;
> +}
> +
> +static char *status_text[] = {
> +	"Unknown", "Charging", "Discharging", "Not charging", "Full"
> +};
> +
> +static ssize_t battery_show_status(struct device *dev,
> +                                   struct device_attribute *attr, char *buf)
> +{
> +	struct battery *bat = dev_get_drvdata(dev);
> +	int status = 0;

We usually try to place a blank line between local data and code.

> +	if (bat->get_status) {
> +		status = bat->get_status(bat);
> +		if (status > 4)
> +			status = 0;
> +		return sprintf(buf, "%s\n", status_text[status]);
> +	}
> +	return 0;
> +}
> +
> +static int battery_create_attrs(struct battery *bat)
> +{
> +	int rc;
> +
> +	#define create_bat_attr_conditional(name)                    \
> +	if(bat->get_##name) {                                        \

	space after "if"

> +		rc = device_create_file(bat->dev, &dev_attr_##name); \
> +		if (rc) goto name##_failed;                          \
> +	}
> +
> +	create_bat_attr_conditional(status);
> +	create_bat_attr_conditional(min_voltage);
> +	create_bat_attr_conditional(min_current);
> +	create_bat_attr_conditional(min_capacity);
> +	create_bat_attr_conditional(max_voltage);
> +	create_bat_attr_conditional(max_current);
> +	create_bat_attr_conditional(max_capacity);
> +	create_bat_attr_conditional(temp);
> +	create_bat_attr_conditional(voltage);
> +	create_bat_attr_conditional(current);
> +	create_bat_attr_conditional(capacity);
> +
> +	#define remove_bat_attr_conditional(name)               \
> +	if(bat->get_##name)                                     \

ditto.

> +		device_remove_file(bat->dev, &dev_attr_##name);
> +
> +	goto success;
> +
> +capacity_failed:     remove_bat_attr_conditional(current);
> +current_failed:      remove_bat_attr_conditional(voltage);
> +voltage_failed:      remove_bat_attr_conditional(temp);
> +temp_failed:         remove_bat_attr_conditional(max_capacity);
> +max_capacity_failed: remove_bat_attr_conditional(max_current);
> +max_current_failed:  remove_bat_attr_conditional(max_voltage);
> +max_voltage_failed:  remove_bat_attr_conditional(min_capacity);
> +min_capacity_failed: remove_bat_attr_conditional(min_current);
> +min_current_failed:  remove_bat_attr_conditional(min_voltage);
> +min_voltage_failed:  remove_bat_attr_conditional(status);

I thought there was a class_remove() or something like that?
but I'm not sure of it.

> +status_failed:
> +success:
> +	return rc;
> +}
> +
> +static void battery_remove_attrs(struct battery *bat)
> +{
> +	remove_bat_attr_conditional(capacity);
> +	remove_bat_attr_conditional(current);
> +	remove_bat_attr_conditional(voltage);
> +	remove_bat_attr_conditional(temp);
> +	remove_bat_attr_conditional(max_capacity);
> +	remove_bat_attr_conditional(max_current);
> +	remove_bat_attr_conditional(max_voltage);
> +	remove_bat_attr_conditional(min_capacity);
> +	remove_bat_attr_conditional(min_current);
> +	remove_bat_attr_conditional(min_voltage);
> +	remove_bat_attr_conditional(status);
> +	return;
> +}
> +
> +int battery_register(struct device *parent, struct battery *bat)
> +{
> +	int rc = 0;
> +
> +	bat->dev = device_create(battery_class, parent, 0, "%s", bat->name);
> +	if(IS_ERR(bat->dev)) {

	space after "if"

> +		rc = PTR_ERR(bat->dev);
> +		goto dev_create_failed;
> +	}
> +
> +	dev_set_drvdata(bat->dev, bat);
> +
> +	rc = battery_create_attrs(bat);
> +	if (rc)
> +		goto create_bat_attrs_failed;
> +
> +	bat->pst.name = bat->name;
> +	bat->pst.power_supply_changed = battery_external_power_changed;
> +	rc = power_supplicant_register(&bat->pst);
> +	if (rc)
> +		goto power_supplicant_failed;
> +
> +	#ifdef CONFIG_LEDS_TRIGGERS

Don't indent the preprocessor lines.  It hides them (too much).

> +	bat->charging_trig_name = kmalloc(strlen(bat->name) +
> +	                                  sizeof("-charging"), GFP_KERNEL);
> +	if (!bat->charging_trig_name) {
> +		rc = -ENOMEM;
> +		goto charging_trig_name_failed;
> +	}
> +
> +	bat->full_trig_name = kmalloc(strlen(bat->name) +
> +	                              sizeof("-full"), GFP_KERNEL);
> +	if (!bat->full_trig_name) {
> +		rc = -ENOMEM;
> +		goto full_trig_name_failed;
> +	}
> +
> +	strcpy(bat->charging_trig_name, bat->name);
> +	strcat(bat->charging_trig_name, "-charging");
> +	strcpy(bat->full_trig_name, bat->name);
> +	strcat(bat->full_trig_name, "-full");
> +
> +	led_trigger_register_charging(bat->charging_trig_name,
> +	                              &bat->charging_trig);
> +	led_trigger_register_simple(bat->full_trig_name,
> +	                            &bat->full_trig);
> +	#endif /* CONFIG_LEDS_TRIGGERS */
> +
> +	goto success;
> +
> +#ifdef CONFIG_LEDS_TRIGGERS
> +full_trig_name_failed:
> +	kfree(bat->charging_trig_name);
> +charging_trig_name_failed:
> +#endif
> +	power_supplicant_unregister(&bat->pst);
> +power_supplicant_failed:
> +	battery_remove_attrs(bat);
> +create_bat_attrs_failed:
> +	device_unregister(bat->dev);
> +dev_create_failed:
> +success:
> +	return rc;
> +}
> +
> +void battery_unregister(struct battery *bat)
> +{
> +	power_supplicant_unregister(&bat->pst);
> +	battery_remove_attrs(bat);
> +	device_unregister(bat->dev);
> +
> +	#ifdef CONFIG_LEDS_TRIGGERS

ifdef/endif not indented, please.

> +	led_trigger_unregister_charging(bat->charging_trig);
> +	led_trigger_unregister_simple(bat->full_trig);
> +	kfree(bat->full_trig_name);
> +	kfree(bat->charging_trig_name);
> +	#endif
> +
> +	return;
> +}
> +
> diff --git a/include/linux/battery.h b/include/linux/battery.h
> new file mode 100644
> index 0000000..a687781
> --- /dev/null
> +++ b/include/linux/battery.h
> @@ -0,0 +1,98 @@
> +
> +/* 
> + * For systems where the charger determines the maximum battery capacity
> + * the min and max fields should be used to present these values to user
> + * space. Unused/uknown fields can be NULL and will not appear in sysfs.

                    unknown

> + */
> +
> +struct battery {
> +	struct device *dev;
> +	char *name;
> +
> +	/* For APM emulation, think legacy userspace. */
> +	int main_battery;
> +
> +	/* executed in userspace, feel free to sleep */
> +	int (*get_min_voltage)(struct battery *bat);
> +	int (*get_min_current)(struct battery *bat);
> +	int (*get_min_capacity)(struct battery *bat);
> +	int (*get_max_voltage)(struct battery *bat);
> +	int (*get_max_current)(struct battery *bat);
> +	int (*get_max_capacity)(struct battery *bat);
> +	int (*get_temp)(struct battery *bat);
> +	int (*get_voltage)(struct battery *bat);
> +	int (*get_current)(struct battery *bat);
> +	int (*get_capacity)(struct battery *bat);
> +	int (*get_status)(struct battery *bat);
> +
> +	/* drivers should not sleep inside it, you'll get there from ISRs */
> +	void (*external_power_changed)(struct battery *bat);
> +
> +	/* private */
> +	struct power_supplicant pst;
> +
> +	#ifdef CONFIG_LEDS_TRIGGERS

ifdef/endif not indented.

> +	struct led_trigger *charging_trig;
> +	char *charging_trig_name;
> +	struct led_trigger *full_trig;
> +	char *full_trig_name;
> +	#endif
> +};
> +
> +/* 

Please check all patches for trailing whitespace and correct that.

> + * This is recommended structure to specify static battery parameters.
> + * Generic one, parametrizable for different batteries. Battery device
> + * itself does bot use it, but that's what implementing most drivers,
> + * should try reuse for consistency.
> + */


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

Hello Randy,

On Wed, Apr 11, 2007 at 07:53:59PM -0700, Randy Dunlap wrote:
> On Thu, 12 Apr 2007 03:25:03 +0400 Anton Vorontsov wrote:
> > +void battery_status_changed(struct battery *bat)
> > +{
> > +	pr_debug("%s\n", __FUNCTION__);
> > +	#ifdef CONFIG_LEDS_TRIGGERS
> 
> Please don't indent preprocessor controls (ifdef/endif etc.).
...
> Place 'switch' and 'case' at the same indent level.  This prevents
> the "double-indent" for the code statements.
...
> We usually try to place a blank line between local data and code.
...
> 	space after "if"
...

Much thanks, will fix.

> > +		device_remove_file(bat->dev, &dev_attr_##name);
> > +
> > +	goto success;
> > +
> > +capacity_failed:     remove_bat_attr_conditional(current);
> > +current_failed:      remove_bat_attr_conditional(voltage);
> > +voltage_failed:      remove_bat_attr_conditional(temp);
> > +temp_failed:         remove_bat_attr_conditional(max_capacity);
> > +max_capacity_failed: remove_bat_attr_conditional(max_current);
> > +max_current_failed:  remove_bat_attr_conditional(max_voltage);
> > +max_voltage_failed:  remove_bat_attr_conditional(min_capacity);
> > +min_capacity_failed: remove_bat_attr_conditional(min_current);
> > +min_current_failed:  remove_bat_attr_conditional(min_voltage);
> > +min_voltage_failed:  remove_bat_attr_conditional(status);
> 
> I thought there was a class_remove() or something like that?
> but I'm not sure of it.

[class_]device_destroy()? Yeah, but it's exactly same thing as
[class_]device_unregister.

But this is really good question. Should I manually clean up attributes,
or sysfs will take care? I.e. whole battery directory removes, and
sysfs removes files in it, or they just leaks?

I've took worst scenario, and done removal manually (I've grep'ed in
drivers/, and almost every driver also doing so).

> ---
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code ***

Thanks for comments!

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Greg KH]

On Thu, Apr 12, 2007 at 03:25:03AM +0400, Anton Vorontsov wrote:
> Here is battery monitor class. According to first copyright string, we're
> maintaining it since 2003. I've took few days and cleaned it up to be
> more suitable for mainline inclusion.
> 
> It differs from battery class at git://git.infradead.org/battery-2.6.git:

Why fork from David's work?  Does he not like these changes for some
reason?

> +static int battery_create_attrs(struct battery *bat)
> +{
> +	int rc;
> +
> +	#define create_bat_attr_conditional(name)                    \
> +	if(bat->get_##name) {                                        \
> +		rc = device_create_file(bat->dev, &dev_attr_##name); \
> +		if (rc) goto name##_failed;                          \
> +	}
> +
> +	create_bat_attr_conditional(status);
> +	create_bat_attr_conditional(min_voltage);
> +	create_bat_attr_conditional(min_current);
> +	create_bat_attr_conditional(min_capacity);
> +	create_bat_attr_conditional(max_voltage);
> +	create_bat_attr_conditional(max_current);
> +	create_bat_attr_conditional(max_capacity);
> +	create_bat_attr_conditional(temp);
> +	create_bat_attr_conditional(voltage);
> +	create_bat_attr_conditional(current);
> +	create_bat_attr_conditional(capacity);

Use an attribute group please.  It's much simpler and will be created at
the proper time so your userspace tools don't have to sit and spin in
order to properly wait for them to show up.

Ok, yes, you want a conditional type of attribute group, like the
new firewire code does.  I have no problem adding that if you like.

thanks,

greg k-h

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Henrique de Moraes Holschuh]

On Wed, 11 Apr 2007, Greg KH wrote:
> Ok, yes, you want a conditional type of attribute group, like the
> new firewire code does.  I have no problem adding that if you like.

Please do.  I want that for ibm-acpi/thinkpad-acpi as well.  Right now I
need to use multiple attribute groups because of that.  Some hwmon drivers
are in the same boat, too.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

Hello Greg,

On Wed, Apr 11, 2007 at 08:43:23PM -0700, Greg KH wrote:
> On Thu, Apr 12, 2007 at 03:25:03AM +0400, Anton Vorontsov wrote:
> > Here is battery monitor class. According to first copyright string, we're
> > maintaining it since 2003. I've took few days and cleaned it up to be
> > more suitable for mainline inclusion.
> > 
> > It differs from battery class at git://git.infradead.org/battery-2.6.git:
> 
> Why fork from David's work?  Does he not like these changes for some
> reason?

It's not a fork, actually. Ian Molton started battery stuff years before
David's work.

Though ours and David's exported API functions are exactly the same
(don't count functions which are unique for our code), but our
implementation a little intersects with David's.

For me it was no matter if I'll take handhelds.org or David's code as a
start point, except that hh.o code familiar to me, and I can test it on
real devices.

Though, you're right, we're in situation when we've two battery classes
now. :-/ And another pity fact is that we also have 8 Mb of patches in
our CVS. It takes a lot of time to to cleanup code for mainline,
especially with limited man-power resources. But we're working hard.

So, this is brief explanation why it took so long for us to show up.

> > +static int battery_create_attrs(struct battery *bat)
> > +{
> > +	int rc;
> > +
> > +	#define create_bat_attr_conditional(name)                    \
> > +	if(bat->get_##name) {                                        \
> > +		rc = device_create_file(bat->dev, &dev_attr_##name); \
> > +		if (rc) goto name##_failed;                          \
> > +	}
> > +
> > +	create_bat_attr_conditional(status);
> > +	create_bat_attr_conditional(min_voltage);
> > +	create_bat_attr_conditional(min_current);
> > +	create_bat_attr_conditional(min_capacity);
> > +	create_bat_attr_conditional(max_voltage);
> > +	create_bat_attr_conditional(max_current);
> > +	create_bat_attr_conditional(max_capacity);
> > +	create_bat_attr_conditional(temp);
> > +	create_bat_attr_conditional(voltage);
> > +	create_bat_attr_conditional(current);
> > +	create_bat_attr_conditional(capacity);
> 
> Use an attribute group please.  It's much simpler and will be created at
> the proper time so your userspace tools don't have to sit and spin in
> order to properly wait for them to show up.
> 
> Ok, yes, you want a conditional type of attribute group, like the
> new firewire code does.  I have no problem adding that if you like.

I'm not sure if it's possible to create that type of conditional
attribute group. Because the condition is "bat->func != NULL", not
attribute' function. And that condition is battery-specific, not class
specific.

But anyway, I guess you're talking about not yet existent API, so I'd be
glad to take a look.

> thanks,
> 
> greg k-h

Thanks for comments!

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Matthew Garrett]

On Thu, Apr 12, 2007 at 03:25:03AM +0400, Anton Vorontsov wrote:
> + * All voltages, currents, capacities and temperatures in mV, mA, mAh and
> + * tenths of a degree unless otherwise stated. It's driver's job to convert
> + * its raw values to which this class operates. If for some reason driver
> + * can't afford this requirement, then it have to create its own attributes,
> + * plus additional "XYZ_units" for each of them.

ACPI batteries can report capacity and rate in either mA or mW. Given 
the lack of a constant voltage, how do you accurately convert between 
the two? Right now, I think this is a loss of functionality over the 
current situation.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

On Thu, Apr 12, 2007 at 02:08:18PM +0100, Matthew Garrett wrote:
> On Thu, Apr 12, 2007 at 03:25:03AM +0400, Anton Vorontsov wrote:
> > + * All voltages, currents, capacities and temperatures in mV, mA, mAh and
> > + * tenths of a degree unless otherwise stated. It's driver's job to convert
> > + * its raw values to which this class operates. If for some reason driver
> > + * can't afford this requirement, then it have to create its own attributes,
> > + * plus additional "XYZ_units" for each of them.
> 
> ACPI batteries can report capacity and rate in either mA or mW. Given

You sure, capacity in mA? Then I don't know. But you can safely
fallback and create your own attribute (just as in David's battery class,
where every battery required to create its own attributes), plus create
capacity_units attribute. So, user space will know your driver's specific
units.

> the lack of a constant voltage, how do you accurately convert between 
> the two? Right now, I think this is a loss of functionality over the 
> current situation.


> 
> -- 
> Matthew Garrett | mjg59@srcf.ucam.org

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Matthew Garrett]

On Thu, Apr 12, 2007 at 06:15:05PM +0400, Anton Vorontsov wrote:
> On Thu, Apr 12, 2007 at 02:08:18PM +0100, Matthew Garrett wrote:
> > ACPI batteries can report capacity and rate in either mA or mW. Given
> 
> You sure, capacity in mA? Then I don't know. But you can safely
> fallback and create your own attribute (just as in David's battery class,
> where every battery required to create its own attributes), plus create
> capacity_units attribute. So, user space will know your driver's specific
> units.

Well, mAh, but yes. Clearly it's possible to add extra attributes, but 
speccing standard attributes that don't entirely cover the most common 
non-embedded battery class seems less than ideal. Why not just require
capacity_units and rate_units attributes?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[Paul Sokolovsky]

Hello Matthew,

Thursday, April 12, 2007, 5:24:30 PM, you wrote:

> On Thu, Apr 12, 2007 at 06:15:05PM +0400, Anton Vorontsov wrote:
>> On Thu, Apr 12, 2007 at 02:08:18PM +0100, Matthew Garrett wrote:
>> > ACPI batteries can report capacity and rate in either mA or mW. Given
>> 
>> You sure, capacity in mA? Then I don't know. But you can safely
>> fallback and create your own attribute (just as in David's battery class,
>> where every battery required to create its own attributes), plus create
>> capacity_units attribute. So, user space will know your driver's specific
>> units.

> Well, mAh, but yes. Clearly it's possible to add extra attributes, but
> speccing standard attributes that don't entirely cover the most common
> non-embedded battery class seems less than ideal. Why not just require
> capacity_units and rate_units attributes?

  Yes, that's apparently the way to go. We just should consider
if mAh and mWh are enough, or we go wider and allow whole collection of
units. Btw, original handhelds.org code used Joules ;-).


-- 
Best regards,
 Paul                            mailto:pmiscml@gmail.com

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[Henrique de Moraes Holschuh]

On Thu, 12 Apr 2007, Paul Sokolovsky wrote:
>   Yes, that's apparently the way to go. We just should consider
> if mAh and mWh are enough, or we go wider and allow whole collection of
> units. Btw, original handhelds.org code used Joules ;-).

FWIW, SBS only mentions mAh and mWh.  AFAIK, all other (meaningful) units
should be able to be converted to either Ah or Wh, assuming enough precision
on the math.  I never heard of any other way to fuel-gauge batteries than
these two main modes (current-based or capacity-based), but I don't work on
the battery field.

That said, you may need to use uWh and uAh instead of mAh and mWh, though.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

On Thu, Apr 12, 2007 at 03:56:30PM -0300, Henrique de Moraes Holschuh wrote:
> On Thu, 12 Apr 2007, Paul Sokolovsky wrote:
> >   Yes, that's apparently the way to go. We just should consider
> > if mAh and mWh are enough, or we go wider and allow whole collection of
> > units. Btw, original handhelds.org code used Joules ;-).
> 
> FWIW, SBS only mentions mAh and mWh.  AFAIK, all other (meaningful) units
> should be able to be converted to either Ah or Wh, assuming enough precision
> on the math.  I never heard of any other way to fuel-gauge batteries than
> these two main modes (current-based or capacity-based), but I don't work on
> the battery field.

Okay, I have an idea:

Let's name attributes with mWh units as {min_,max_,design_,}energy,
and attributes with mAh units as {min_,max_,design_,}charge.

Because both energy and charge represents ""capacity"" in some meanings,
and that's why we bothering with _units attribute. So, lets drop
"capacity"* and use more specific terms? I really don't want string
attributes by default (except status).

If we export attributes with predefined units, userspace developers
could just look into include/linux/battery.h and conclude: "Ah, great,
if battery reporting energy, then it's in mWh, and if battery reporting
charge it's always in mAh".

* Yup, I've read last discussion regarding batteries, and I've seen
  objections against "charge" term, quoting Shem Multinymous:

  "And, for the reasons I explained earlier, I strongly suggest not using
  the term "charge" except when referring to the action of charging.
  Hence:
  s/charge_rate/rate/;  s/charge/capacity/"

  But lets think about it once again? We'll make things much cleaner
  if we'll drop "capacity" at all.

> That said, you may need to use uWh and uAh instead of mAh and mWh, though.

Not sure. Is there any existing chip that can report uAh/uWh? That is
great precision.

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[Henrique de Moraes Holschuh]

On Fri, 13 Apr 2007, Anton Vorontsov wrote:
> Let's name attributes with mWh units as {min_,max_,design_,}energy,
> and attributes with mAh units as {min_,max_,design_,}charge.

[...]

> * Yup, I've read last discussion regarding batteries, and I've seen
>   objections against "charge" term, quoting Shem Multinymous:
> 
>   "And, for the reasons I explained earlier, I strongly suggest not using
>   the term "charge" except when referring to the action of charging.
>   Hence:
>   s/charge_rate/rate/;  s/charge/capacity/"
> 
>   But lets think about it once again? We'll make things much cleaner
>   if we'll drop "capacity" at all.

I stand with Shem on this one.  The people behind the SBS specification
seems to agree... that specification is aimed at *engineers* and still
avoids the obvious trap of using "charge" due to its high potential for
confusion.

I don't even want to know how much of a mess the people writing applets
woudl make of it...

> > That said, you may need to use uWh and uAh instead of mAh and mWh, though.
> 
> Not sure. Is there any existing chip that can report uAh/uWh? That is
> great precision.

The way things are going, it should be feasible for small embedded systems
quite soon.  Refer to the previous thread.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

On Thu, Apr 12, 2007 at 09:51:12PM -0300, Henrique de Moraes Holschuh wrote:
> On Fri, 13 Apr 2007, Anton Vorontsov wrote:
> > Let's name attributes with mWh units as {min_,max_,design_,}energy,
> > and attributes with mAh units as {min_,max_,design_,}charge.
> 
> [...]
> 
> > * Yup, I've read last discussion regarding batteries, and I've seen
> >   objections against "charge" term, quoting Shem Multinymous:
> > 
> >   "And, for the reasons I explained earlier, I strongly suggest not using
> >   the term "charge" except when referring to the action of charging.
> >   Hence:
> >   s/charge_rate/rate/;  s/charge/capacity/"
> > 
> >   But lets think about it once again? We'll make things much cleaner
> >   if we'll drop "capacity" at all.
> 
> I stand with Shem on this one.  The people behind the SBS specification
> seems to agree... that specification is aimed at *engineers* and still
> avoids the obvious trap of using "charge" due to its high potential for
> confusion.
> 
> I don't even want to know how much of a mess the people writing applets
> woudl make of it...

:-(

Okay, term "charge" is out of scope, I guess. But can we use "capacity"
for xAh, and "energy" for xWh? I just trying to separate these terms
somehow, and avoid "_units" stuff.

> 
> > > That said, you may need to use uWh and uAh instead of mAh and mWh, though.
> > 
> > Not sure. Is there any existing chip that can report uAh/uWh? That is
> > great precision.
> 
> The way things are going, it should be feasible for small embedded systems
> quite soon.  Refer to the previous thread.

I see... is it also applicable to currents and voltages? I.e. should we
use uA and uV from the start?

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[Pavel Machek]

Hi!

> > > > That said, you may need to use uWh and uAh instead of mAh and mWh, though.
> > > 
> > > Not sure. Is there any existing chip that can report uAh/uWh? That is
> > > great precision.
> > 
> > The way things are going, it should be feasible for small embedded systems
> > quite soon.  Refer to the previous thread.
> 
> I see... is it also applicable to currents and voltages? I.e. should we
> use uA and uV from the start?

AFAICT, mobile phone in standby can eat less than 1000 uW... so uA/uV
would indeed be nice.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[Shem Multinymous]

Hi,

On 4/12/07, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
> On Fri, 13 Apr 2007, Anton Vorontsov wrote:
> > * Yup, I've read last discussion regarding batteries, and I've seen
> >   objections against "charge" term, quoting Shem Multinymous:
> >
> >   "And, for the reasons I explained earlier, I strongly suggest not using
> >   the term "charge" except when referring to the action of charging.
> >   Hence:
> >   s/charge_rate/rate/;  s/charge/capacity/"
> >
> >   But lets think about it once again? We'll make things much cleaner
> >   if we'll drop "capacity" at all.
>
> I stand with Shem on this one.  The people behind the SBS specification
> seems to agree... that specification is aimed at *engineers* and still
> avoids the obvious trap of using "charge" due to its high potential for
> confusion.
>
> I don't even want to know how much of a mess the people writing applets
> woudl make of it...

With fixed-units files, having *_energy and *_capacity isn't too clear
either... Nor is it consistent with SBS, since SBS uses "capacity" to
refer to either energy or charge, depending on a units attribute.

As a compromise, how about using "energy" and "charge" for quantities,
and "charging" (i.e., a verb) when referring to the operation?

BTW,  tp_smapi uses "charge" and "charging" interchangeably; that was
a  mistake.

  Shem

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

On Thu, Apr 12, 2007 at 10:34:06PM -0400, Shem Multinymous wrote:
> Hi,
> 
> On 4/12/07, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
> >On Fri, 13 Apr 2007, Anton Vorontsov wrote:
> >> * Yup, I've read last discussion regarding batteries, and I've seen
> >>   objections against "charge" term, quoting Shem Multinymous:
> >>
> >>   "And, for the reasons I explained earlier, I strongly suggest not using
> >>   the term "charge" except when referring to the action of charging.
> >>   Hence:
> >>   s/charge_rate/rate/;  s/charge/capacity/"
> >>
> >>   But lets think about it once again? We'll make things much cleaner
> >>   if we'll drop "capacity" at all.
> >
> >I stand with Shem on this one.  The people behind the SBS specification
> >seems to agree... that specification is aimed at *engineers* and still
> >avoids the obvious trap of using "charge" due to its high potential for
> >confusion.
> >
> >I don't even want to know how much of a mess the people writing applets
> >woudl make of it...
> 
> With fixed-units files, having *_energy and *_capacity isn't too clear
> either... Nor is it consistent with SBS, since SBS uses "capacity" to
> refer to either energy or charge, depending on a units attribute.
> 
> As a compromise, how about using "energy" and "charge" for quantities,
> and "charging" (i.e., a verb) when referring to the operation?

It would be great compromise! Please please please!

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[Henrique de Moraes Holschuh]

On Fri, 13 Apr 2007, Anton Vorontsov wrote:
> > With fixed-units files, having *_energy and *_capacity isn't too clear
> > either... Nor is it consistent with SBS, since SBS uses "capacity" to
> > refer to either energy or charge, depending on a units attribute.
> > 
> > As a compromise, how about using "energy" and "charge" for quantities,
> > and "charging" (i.e., a verb) when referring to the operation?
> 
> It would be great compromise! Please please please!

I can live with it, although I'd rather just use the units (zero margin of
error or confusion).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Shem Multinymous]

Hi Anton,

A few comments on the ever-contentious choice of battery attributes:

On 4/11/07, Anton Vorontsov <cbou@mail.ru> wrote:
> + * All voltages, currents, capacities and temperatures in mV, mA, mAh and
> + * tenths of a degree unless otherwise stated. It's driver's job to convert
> + * its raw values to which this class operates. If for some reason driver
> + * can't afford this requirement, then it have to create its own attributes,
> + * plus additional "XYZ_units" for each of them.
> + */

Many (most, I believe) laptop batteries report capacity in mWh
(energy), not mAh (charge). You can't convert between the two without
a detailed, up-to-date physical model of the battery. This is too
common to relegate to a non-standardized "XYZ_units" extension.


> +BATTERY_INT_ATTR(min_voltage);
> +BATTERY_INT_ATTR(min_current);
> +BATTERY_INT_ATTR(min_capacity);
> +BATTERY_INT_ATTR(max_voltage);
> +BATTERY_INT_ATTR(max_current);
> +BATTERY_INT_ATTR(max_capacity);
> +BATTERY_INT_ATTR(temp);
> +BATTERY_INT_ATTR(voltage);
> +BATTERY_INT_ATTR(current);
> +BATTERY_INT_ATTR(capacity);

I suggest adding "remaining operating time" and "remaining charging
time". You can try deducing these from the above attributes, but in
practice this gives very inaccurate predictions. On laptops (e.g.,
ThinkPad) the BIOS or EC often provides much better estimates, using a
more accurate physical model.

I also see you omitted a host of other common attributes, like design
capacity, cycle count, model string, and temperatures. There was an
extensive LKML discussion of the choice and naming of attributes, in
the context of David Woodhouse's patch; there are futher observations
there. Also, here's the list of attributes in my tp_smapi ThinkPad
driver: http://thinkwiki.org/wiki/tp_smapi#Battery_charge_control_features

Does "max capaxity" correspond to what's usually denoted "last full capacity".

I understand you allow adding custom attributes, but they're of
limited use if generic userspace tools don't know their name and
semantics. It's important to standardize all reasonably common
attributes.

  Shem

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

Hello Shem,

On Thu, Apr 12, 2007 at 11:00:07AM -0400, Shem Multinymous wrote:
> Hi Anton,
> 
> A few comments on the ever-contentious choice of battery attributes:
> 
> On 4/11/07, Anton Vorontsov <cbou@mail.ru> wrote:
> >+ * All voltages, currents, capacities and temperatures in mV, mA, mAh and
> >+ * tenths of a degree unless otherwise stated. It's driver's job to 
> >convert
> >+ * its raw values to which this class operates. If for some reason driver
> >+ * can't afford this requirement, then it have to create its own 
> >attributes,
> >+ * plus additional "XYZ_units" for each of them.
> >+ */
> 
> Many (most, I believe) laptop batteries report capacity in mWh
> (energy), not mAh (charge). You can't convert between the two without
> a detailed, up-to-date physical model of the battery. This is too
> common to relegate to a non-standardized "XYZ_units" extension.

I see. Okay, I'll try to cook something regarding capacity units.

> >+BATTERY_INT_ATTR(min_voltage);
> >+BATTERY_INT_ATTR(min_current);
> >+BATTERY_INT_ATTR(min_capacity);
> >+BATTERY_INT_ATTR(max_voltage);
> >+BATTERY_INT_ATTR(max_current);
> >+BATTERY_INT_ATTR(max_capacity);
> >+BATTERY_INT_ATTR(temp);
> >+BATTERY_INT_ATTR(voltage);
> >+BATTERY_INT_ATTR(current);
> >+BATTERY_INT_ATTR(capacity);
> 
> I suggest adding "remaining operating time" and "remaining charging
> time". You can try deducing these from the above attributes, but in
> practice this gives very inaccurate predictions. On laptops (e.g.,
> ThinkPad) the BIOS or EC often provides much better estimates, using a
> more accurate physical model.

Yes, sure. Feel free to add these attributes to the "standard" ones,
along with your drivers. See (1).

> 
> I also see you omitted a host of other common attributes, like design
> capacity, cycle count, model string, and temperatures. There was an
> extensive LKML discussion of the choice and naming of attributes, in
> the context of David Woodhouse's patch; there are futher observations
> there. Also, here's the list of attributes in my tp_smapi ThinkPad
> driver: http://thinkwiki.org/wiki/tp_smapi#Battery_charge_control_features
> 
> Does "max capaxity" correspond to what's usually denoted "last full 
> capacity".

Yup.

> I understand you allow adding custom attributes, but they're of
> limited use if generic userspace tools don't know their name and
> semantics. It's important to standardize all reasonably common
> attributes.

(1) You're free to add your attribute to "standard" ones in
include/linux/battery.h if it's proven to widely used. Batteries which
do not have such attribute will not even notice that addition, so you
will not have to grep and modify other drivers.

So, that is plain matter of code duplication. If two or more battery
drivers have same attributes, then we can put it in battery.h.
If only one driver using it, then code duplication impossible, and
driver should keep this attribute private.

In handhelds.org tree we have two battery drivers with attributes you
see currently. Second driver is not ready for mainline inclusion yet
(it depends on ADC framework, which we'll present soon), thus I've not
posted it.


Also, APM emulation driver should learn how to use one attribute, or
another depending on which available/better. But this is only technical
question, like

if (battery have attribute1)
	do precise maths of something, because we have mWh;
else
	do approx maths of something, because we have only mAh.
else
	n/a.

>  Shem

Thanks for comments!

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Shem Multinymous]

Hi Anton,

On 4/12/07, Anton Vorontsov <cbou@mail.ru> wrote:
> On Thu, Apr 12, 2007 at 11:00:07AM -0400, Shem Multinymous wrote:
> > I suggest adding "remaining operating time" and "remaining charging
> > time". You can try deducing these from the above attributes, but in
> > practice this gives very inaccurate predictions. On laptops (e.g.,
> > ThinkPad) the BIOS or EC often provides much better estimates, using a
> > more accurate physical model.
>
> Yes, sure. Feel free to add these attributes to the "standard" ones,
> along with your drivers. See (1).

That's a sound way to go around i. We just need to be careful about
naming conventions. For example, "*_charge" rather than "*_capacity,
so that we can later add "*_energy" analogously.

But specifically about {operating,charge} time remaining readouts, it
seems important to have them there from the beginning and have all
drivers implement them. Otherwise, userspace will just go ahead and
implement its own crude computation, so by the time when new
attributes and drivers are introduced, userspace will be full of bad
code. I've seen this happening with the tp_smapi ThinkPad driver -- I
introduced those attributes at a recent version, and then needed to
encourage userspace utility authors to dump their extrapolation
calcultions and use the attribute instead.

The rest of the missing attributes are not as imporant in this
respect, because userspace can't try to estimate them.

  Shem

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

On Thu, Apr 12, 2007 at 03:25:03AM +0400, Anton Vorontsov wrote:
> Here is battery monitor class. According to first copyright string, we're
> maintaining it since 2003. I've took few days and cleaned it up to be
> more suitable for mainline inclusion.
> 
> It differs from battery class at git://git.infradead.org/battery-2.6.git:
> 
> * It's using external power kernel interface, i.e. does not fake external
>   powers as batteries. (Same thing David Woodhouse planed last year).
> 
> * It have predefined set of attributes, this eliminates code duplication
>   by battery drivers. And also gives opportunity to write emulation drivers
>   for legacy stuff (APM emulation driver follow).
> 
>   If driver can't afford some attribute, it will not appear in sysfs.
> 
> * It insists on reusing its predefined attributes *and* their units.
>   So, userspace getting expected values for any battery.
>   
>   Also common units is required for APM/ACPI emulation.
>   
>   Though our battery class insisting on re-usage, but not forces it. If some
>   battery driver can't convert its own raw values (can't imagine why), then
>   driver is free to implement its own attributes *and* additional _units
>   attribute. Though, this scheme is discouraged.
> 
> * LEDs support. Each battery register its trigger, and gadgets with LEDs
>   can quickly bind to battery-charging / battery-full triggers.
> 
> Here how it looks like from user space:
> 
> # ls /sys/class/battery/main-battery/
> capacity  max_capacity  max_voltage   min_current  power   subsystem  uevent
> current   max_current   min_capacity  min_voltage  status  temp       voltage
> # cat /sys/class/battery/main-battery/status
> Full
> # cat /sys/class/leds/h5400\:green-right/trigger
> none h5400-radio timer hwtimer main-battery-charging [main-battery-full]
> # cat /sys/class/leds/h5400\:green-right/brightness
> 255
> 

Changes:

- Cleanups based on comments from Randy Dunlap.

- Attribute creation scheme changed drastically. No more tons of

  macro-created functions. Compiled code should be much smaller.
  Also adding new "standard" attributes is trivial task now (matter of
  adding two lines, one in battery.c and another in battery.h).

- charge (as quantity) in mAh, energy in mWh.


I'll convert mXh to uXh a bit later, if there will no further objections
against uXh. Also I'd like to hear if there any objections on
mA/mV -> uA/uV conversion. I think we'd better keep all units at the
same order/precision.


Subject: [PATCH] [take2] Battery monitoring class


Signed-off-by: Anton Vorontsov <cbou@mail.ru>
---
 drivers/Kconfig           |    2 +
 drivers/Makefile          |    1 +
 drivers/battery/Kconfig   |   11 ++
 drivers/battery/Makefile  |    1 +
 drivers/battery/battery.c |  290 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/battery.h   |  113 ++++++++++++++++++
 6 files changed, 418 insertions(+), 0 deletions(-)
 create mode 100644 drivers/battery/Kconfig
 create mode 100644 drivers/battery/Makefile
 create mode 100644 drivers/battery/battery.c
 create mode 100644 include/linux/battery.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index c546de3..c3a0038 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -56,6 +56,8 @@ source "drivers/w1/Kconfig"
 
 source "drivers/power/Kconfig"
 
+source "drivers/battery/Kconfig"
+
 source "drivers/hwmon/Kconfig"
 
 source "drivers/mfd/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 2bdaae7..7cbfd37 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_RTC_LIB)		+= rtc/
 obj-$(CONFIG_I2C)		+= i2c/
 obj-$(CONFIG_W1)		+= w1/
 obj-$(CONFIG_EXTERNAL_POWER)	+= power/
+obj-$(CONFIG_BATTERY)		+= battery/
 obj-$(CONFIG_HWMON)		+= hwmon/
 obj-$(CONFIG_PHONE)		+= telephony/
 obj-$(CONFIG_MD)		+= md/
diff --git a/drivers/battery/Kconfig b/drivers/battery/Kconfig
new file mode 100644
index 0000000..c386593
--- /dev/null
+++ b/drivers/battery/Kconfig
@@ -0,0 +1,11 @@
+
+menu "Battery support"
+
+config BATTERY
+	tristate "Battery monitoring support"
+	select EXTERNAL_POWER
+	help
+	  Say Y here to enable generic battery status reporting in
+	  the /sys filesystem.
+
+endmenu
diff --git a/drivers/battery/Makefile b/drivers/battery/Makefile
new file mode 100644
index 0000000..a2239cb
--- /dev/null
+++ b/drivers/battery/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_BATTERY)              += battery.o
diff --git a/drivers/battery/battery.c b/drivers/battery/battery.c
new file mode 100644
index 0000000..6c87fe3
--- /dev/null
+++ b/drivers/battery/battery.c
@@ -0,0 +1,290 @@
+/*
+ *  Universal battery monitor class
+ *
+ *  Copyright (c) 2007  Anton Vorontsov <cbou@mail.ru>
+ *  Copyright (c) 2004  Szabolcs Gyurko
+ *  Copyright (c) 2003  Ian Molton <spyro@f2s.com>
+ *
+ *  Modified: 2004, Oct     Szabolcs Gyurko
+ *
+ *  You may use this code as per GPL version 2
+ *
+ * All voltages, currents, charges, energies and temperatures in mV, mA,
+ * mAh, mWh and tenths of a Celsius degree unless otherwise stated. It's
+ * driver's job to convert its raw values to which this class operates. If
+ * for some reason driver can't afford this requirement, then it have to
+ * create its own attributes, plus additional "XYZ_units" for each of them.
+ */
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/battery.h>
+
+/* If we have hwtimer trigger, then use it to blink charging LED */
+#if defined(CONFIG_LEDS_TRIGGER_HWTIMER) ||                  \
+               (defined(CONFIG_BATTERY_MODULE) &&            \
+                defined(CONFIG_LEDS_TRIGGER_HWTIMER_MODULE))
+	#define led_trigger_register_charging led_trigger_register_hwtimer
+	#define led_trigger_unregister_charging led_trigger_unregister_hwtimer
+#else
+	#define led_trigger_register_charging led_trigger_register_simple
+	#define led_trigger_unregister_charging led_trigger_unregister_simple
+#endif
+
+struct class *battery_class;
+
+static void battery_external_power_changed(struct power_supplicant *pst,
+                                           struct power_supply *psy)
+{
+	struct battery *bat = container_of(pst, struct battery, pst);
+
+	pr_debug("%s\n", __FUNCTION__);
+	if (bat->external_power_changed)
+		bat->external_power_changed(bat);
+
+	return;
+}
+
+int battery_is_external_power_supplied(struct battery *bat)
+{
+	pr_debug("%s\n", __FUNCTION__);
+	return power_supplicant_am_i_supplied(&bat->pst);
+}
+
+void battery_status_changed(struct battery *bat)
+{
+	void *value;
+
+	pr_debug("%s\n", __FUNCTION__);
+
+	value = bat->get_property(bat, BATTERY_PROP_STATUS);
+	if (!value)
+		return;
+
+#ifdef CONFIG_LEDS_TRIGGERS
+	switch(*(int *)value) {
+	case BATTERY_STATUS_FULL:
+		led_trigger_event(bat->charging_trig, LED_OFF);
+		led_trigger_event(bat->full_trig, LED_FULL);
+		break;
+	case BATTERY_STATUS_CHARGING:
+		led_trigger_event(bat->charging_trig, LED_FULL);
+		led_trigger_event(bat->full_trig, LED_OFF);
+		break;
+	default:
+		led_trigger_event(bat->charging_trig, LED_OFF);
+		led_trigger_event(bat->full_trig, LED_OFF);
+		break;
+	}
+#endif /* CONFIG_LEDS_TRIGGERS */
+	return;
+}
+
+/*
+ * This is because the name "current" breaks the device attr macro.
+ * The "current" word resolvs to "(get_current())" so instead of
+ * "current" "(get_current())" appears in the sysfs.
+ *
+ * The source of this definition is the device.h which calls __ATTR
+ * macro in sysfs.h which calls the __stringify macro.
+ *
+ * Only modification that the name is not tried to be resolved
+ * (as a macro let's say).
+ */
+
+#define BATTERY_ATTR(_name)                                             \
+{                                                                       \
+	.attr = { .name = #_name, .mode = 0444, .owner = THIS_MODULE }, \
+	.show = battery_show_property,                                  \
+	.store = NULL,                                                  \
+}
+
+static struct device_attribute battery_attrs[];
+
+static ssize_t battery_show_property(struct device *dev,
+                                     struct device_attribute *attr,
+                                     char *buf) {
+	static char *status_text[] = {
+		"Unknown", "Charging", "Discharging", "Not charging", "Full"
+	};
+	struct battery *bat = dev_get_drvdata(dev);
+	const off_t off = attr - battery_attrs;
+	void *value = bat->get_property(bat, off);
+
+	if (!value)
+		return sprintf(buf, "Driver can't report this property, "
+		               "but claimed it can. Fix it.\n");
+
+	if (off == BATTERY_PROP_STATUS)
+		return sprintf(buf, "%s\n", status_text[*(int *)value]);
+	else
+		return sprintf(buf, "%d\n", *(int *)value);
+}
+
+/* Must be in the same order as BATTERY_PROP_*, defined in battery.h */
+static struct device_attribute battery_attrs[] = {
+	BATTERY_ATTR(status),
+	BATTERY_ATTR(min_voltage),
+	BATTERY_ATTR(max_voltage),
+	BATTERY_ATTR(voltage),
+	BATTERY_ATTR(min_current),
+	BATTERY_ATTR(max_current),
+	BATTERY_ATTR(current),
+	BATTERY_ATTR(design_charge),
+	BATTERY_ATTR(min_charge),
+	BATTERY_ATTR(max_charge),
+	BATTERY_ATTR(charge),
+	BATTERY_ATTR(design_energy),
+	BATTERY_ATTR(min_energy),
+	BATTERY_ATTR(max_energy),
+	BATTERY_ATTR(energy),
+	BATTERY_ATTR(temp),
+};
+
+static int battery_create_attrs(struct battery *bat)
+{
+	int rc = 0;
+	int i;
+
+	for (i = 0; i < bat->num_properties; i++) {
+		rc = device_create_file(bat->dev,
+		            &battery_attrs[bat->properties[i]]);
+		if (rc)
+			goto failed;
+	}
+
+	goto succeed;
+
+failed:
+	while (i--)
+		device_remove_file(bat->dev,
+		           &battery_attrs[bat->properties[i]]);
+succeed:
+	return rc;
+}
+
+static void battery_remove_attrs(struct battery *bat)
+{
+	int i;
+
+	for (i = 0; i < bat->num_properties; i++)
+		device_remove_file(bat->dev,
+		           &battery_attrs[bat->properties[i]]);
+	return;
+}
+
+int battery_register(struct device *parent, struct battery *bat)
+{
+	int rc = 0;
+
+	bat->dev = device_create(battery_class, parent, 0, "%s", bat->name);
+	if (IS_ERR(bat->dev)) {
+		rc = PTR_ERR(bat->dev);
+		goto dev_create_failed;
+	}
+
+	dev_set_drvdata(bat->dev, bat);
+
+	rc = battery_create_attrs(bat);
+	if (rc)
+		goto create_bat_attrs_failed;
+
+	bat->pst.name = bat->name;
+	bat->pst.power_supply_changed = battery_external_power_changed;
+	rc = power_supplicant_register(&bat->pst);
+	if (rc)
+		goto power_supplicant_failed;
+
+#ifdef CONFIG_LEDS_TRIGGERS
+	bat->charging_trig_name = kmalloc(strlen(bat->name) +
+	                                  sizeof("-charging"), GFP_KERNEL);
+	if (!bat->charging_trig_name) {
+		rc = -ENOMEM;
+		goto charging_trig_name_failed;
+	}
+
+	bat->full_trig_name = kmalloc(strlen(bat->name) +
+	                              sizeof("-full"), GFP_KERNEL);
+	if (!bat->full_trig_name) {
+		rc = -ENOMEM;
+		goto full_trig_name_failed;
+	}
+
+	strcpy(bat->charging_trig_name, bat->name);
+	strcat(bat->charging_trig_name, "-charging");
+	strcpy(bat->full_trig_name, bat->name);
+	strcat(bat->full_trig_name, "-full");
+
+	led_trigger_register_charging(bat->charging_trig_name,
+	                              &bat->charging_trig);
+	led_trigger_register_simple(bat->full_trig_name,
+	                            &bat->full_trig);
+#endif /* CONFIG_LEDS_TRIGGERS */
+
+	goto success;
+
+#ifdef CONFIG_LEDS_TRIGGERS
+full_trig_name_failed:
+	kfree(bat->charging_trig_name);
+charging_trig_name_failed:
+#endif
+	power_supplicant_unregister(&bat->pst);
+power_supplicant_failed:
+	battery_remove_attrs(bat);
+create_bat_attrs_failed:
+	device_unregister(bat->dev);
+dev_create_failed:
+success:
+	return rc;
+}
+
+void battery_unregister(struct battery *bat)
+{
+	power_supplicant_unregister(&bat->pst);
+	battery_remove_attrs(bat);
+	device_unregister(bat->dev);
+
+#ifdef CONFIG_LEDS_TRIGGERS
+	led_trigger_unregister_charging(bat->charging_trig);
+	led_trigger_unregister_simple(bat->full_trig);
+	kfree(bat->full_trig_name);
+	kfree(bat->charging_trig_name);
+#endif
+	return;
+}
+
+static int __init battery_class_init(void)
+{
+	battery_class = class_create(THIS_MODULE, "battery");
+
+	if (IS_ERR(battery_class))
+		return PTR_ERR(battery_class);
+
+	return 0;
+}
+
+static void __exit battery_class_exit(void)
+{
+	class_destroy(battery_class);
+	return;
+}
+
+EXPORT_SYMBOL_GPL(battery_register);
+EXPORT_SYMBOL_GPL(battery_unregister);
+EXPORT_SYMBOL_GPL(battery_status_changed);
+EXPORT_SYMBOL_GPL(battery_is_external_power_supplied);
+
+/* exported for the APM Power driver, APM emulation */
+EXPORT_SYMBOL_GPL(battery_class);
+
+subsys_initcall(battery_class_init);
+module_exit(battery_class_exit);
+
+MODULE_DESCRIPTION("Universal battery monitor class");
+MODULE_AUTHOR("Ian Molton <spyro@f2s.com>, "
+              "Szabolcs Gyurko, "
+              "Anton Vorontsov <cbou@mail.ru>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/battery.h b/include/linux/battery.h
new file mode 100644
index 0000000..2dcc8ed
--- /dev/null
+++ b/include/linux/battery.h
@@ -0,0 +1,113 @@
+/*
+ *  Universal battery monitor class
+ *
+ *  Copyright (c) 2007  Anton Vorontsov <cbou@mail.ru>
+ *  Copyright (c) 2004  Szabolcs Gyurko
+ *  Copyright (c) 2003  Ian Molton <spyro@f2s.com>
+ *
+ *  Modified: 2004, Oct     Szabolcs Gyurko
+ *
+ *  You may use this code as per GPL version 2
+ *
+ * All voltages, currents, charges, energies and temperatures in mV, mA,
+ * mAh, mWh and tenths of a Celsius degree unless otherwise stated. It's
+ * driver's job to convert its raw values to which this class operates. If
+ * for some reason driver can't afford this requirement, then it have to
+ * create its own attributes, plus additional "XYZ_units" for each of them.
+ */
+
+#ifndef _LINUX_BATTERY_H
+#define _LINUX_BATTERY_H
+
+#include <linux/device.h>
+#include <linux/external_power.h>
+#include <linux/leds.h>
+
+#define BATTERY_STATUS_UNKNOWN      0
+#define BATTERY_STATUS_CHARGING     1
+#define BATTERY_STATUS_DISCHARGING  2
+#define BATTERY_STATUS_NOT_CHARGING 3
+#define BATTERY_STATUS_FULL         4
+
+/*
+ * For systems where the charger determines the maximum battery capacity
+ * the min and max fields should be used to present these values to user
+ * space. Unused/unknown fields can be NULL and will not appear in sysfs.
+ */
+
+enum battery_property {
+	BATTERY_PROP_STATUS = 0,
+	BATTERY_PROP_MIN_VOLTAGE,
+	BATTERY_PROP_MAX_VOLTAGE,
+	BATTERY_PROP_VOLTAGE,
+	BATTERY_PROP_MIN_CURRENT,
+	BATTERY_PROP_MAX_CURRENT,
+	BATTERY_PROP_CURRENT,
+	BATTERY_PROP_DESIGN_CHARGE,
+	BATTERY_PROP_MIN_CHARGE,
+	BATTERY_PROP_MAX_CHARGE,
+	BATTERY_PROP_CHARGE,
+	BATTERY_PROP_DESIGN_ENERGY,
+	BATTERY_PROP_MIN_ENERGY,
+	BATTERY_PROP_MAX_ENERGY,
+	BATTERY_PROP_ENERGY,
+	BATTERY_PROP_TEMP,
+};
+
+struct battery {
+	struct device *dev;
+	char *name;
+	int *properties;
+	int num_properties;
+
+	/* For APM emulation, think legacy userspace. */
+	int main_battery;
+
+	/* executed in userspace, feel free to sleep */
+	void *(*get_property)(struct battery *bat, enum battery_property);
+
+	/* drivers should not sleep inside it, you'll get there from ISRs */
+	void (*external_power_changed)(struct battery *bat);
+
+	/* private */
+	struct power_supplicant pst;
+
+#ifdef CONFIG_LEDS_TRIGGERS
+	struct led_trigger *charging_trig;
+	char *charging_trig_name;
+	struct led_trigger *full_trig;
+	char *full_trig_name;
+#endif
+};
+
+/*
+ * This is recommended structure to specify static battery parameters.
+ * Generic one, parametrizable for different batteries. Battery device
+ * itself does bot use it, but that's what implementing most drivers,
+ * should try reuse for consistency.
+ */
+
+struct battery_info {
+	char *name;
+	int min_voltage;
+	int max_voltage;
+	int min_current;
+	int max_current;
+	int design_charge;
+	int min_charge;
+	int max_charge;
+	int design_energy;
+	int min_energy;
+	int max_energy;
+	int main_battery;
+};
+
+extern void battery_status_changed(struct battery *bat);
+extern int battery_is_external_power_supplied(struct battery *bat);
+extern int battery_register(struct device *parent, struct battery *bat);
+extern void battery_unregister(struct battery *bat);
+
+/* For APM emulation, think legacy userspace. */
+extern struct class *battery_class;
+
+#endif
-- 
1.5.0.5-dirty

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

On Fri, Apr 13, 2007 at 05:49:39PM +0400, Anton Vorontsov wrote:
> I'll convert mXh to uXh a bit later, if there will no further objections
> against uXh. Also I'd like to hear if there any objections on
> mA/mV -> uA/uV conversion. I think we'd better keep all units at the
> same order/precision.

Okay, would it make sense to use "long" instead of "int" after "milli" to
"micro" conversion? On 32 bit machines int gives +-2147483648 limit. So
2147 volts/amperes/...

Though 2147 amperes is unrealistic for batteries, but if used in
calculations it could be dangerous.

For example:
di->life_sec = -((di->accum_current_uAh - di->empty_uAh) *
                 3600) / di->current_uA;

It can be also solved (and I voting for it) by typecasting to long
in the driver itself.

Would it also make sense to use int64_t instead of long? And how should
it passed to printk in portable way? I guess printk (vsprintf) does not
support PRIx notation as defined in /usr/include/inttypes.h ?

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Pavel Machek]

Hi!

> > I'll convert mXh to uXh a bit later, if there will no further objections
> > against uXh. Also I'd like to hear if there any objections on
> > mA/mV -> uA/uV conversion. I think we'd better keep all units at the
> > same order/precision.
> 
> Okay, would it make sense to use "long" instead of "int" after "milli" to
> "micro" conversion? On 32 bit machines int gives +-2147483648 limit. So
> 2147 volts/amperes/...

long == int on 32bit machines.

> Though 2147 amperes is unrealistic for batteries, but if used in
> calculations it could be dangerous.

Let the one doing calculations handle that ;-).
								Pavel

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

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Pavel Machek]

Hi!

> * It insists on reusing its predefined attributes *and* their units.
>   So, userspace getting expected values for any battery.
>   
>   Also common units is required for APM/ACPI emulation.
>   
>   Though our battery class insisting on re-usage, but not forces it. If some
>   battery driver can't convert its own raw values (can't imagine why), then
>   driver is free to implement its own attributes *and* additional _units
>   attribute. Though, this scheme is discouraged.
> 
> * LEDs support. Each battery register its trigger, and gadgets with LEDs
>   can quickly bind to battery-charging / battery-full triggers.
> 
> Here how it looks like from user space:
> 
> # ls /sys/class/battery/main-battery/
> capacity  max_capacity  max_voltage   min_current  power   subsystem  uevent
> current   max_current   min_capacity  min_voltage  status  temp       voltage
> # cat /sys/class/battery/main-battery/status
> Full
> # cat /sys/class/leds/h5400\:green-right/trigger
> none h5400-radio timer hwtimer main-battery-charging [main-battery-full]
> # cat /sys/class/leds/h5400\:green-right/brightness
> 255

Can we get few lines in Documentation? I guess min_capacity is
shutdown capacity at current temperature, but its surely non-obvious.

Will min_capacity increase as batery gets old? Or will max_capacity
decrease? (Should we introduce design_capacity for ACPI systems that
know the difference?)

What is min_current? Granularity of amper meter?

And min_voltage is shutdown voltage?

Otherwise it looks good to me. Something like this is really needed.

> + * All voltages, currents, capacities and temperatures in mV, mA, mAh and
> + * tenths of a degree unless otherwise stated. It's driver's job to convert

tenths of degree Celsius?

							Pavel

> +#define BATTERY_STATUS_UNKNOWN      0
> +#define BATTERY_STATUS_CHARGING     1
> +#define BATTERY_STATUS_DISCHARGING  2
> +#define BATTERY_STATUS_NOT_CHARGING 3
> +#define BATTERY_STATUS_FULL         4

Perhaps we need STATUS_ERROR? At least on some machines it is
different from STATUS_NOT_CHARGING.

> +	/* private */
> +	struct power_supplicant pst;
> +
> +	#ifdef CONFIG_LEDS_TRIGGERS
> +	struct led_trigger *charging_trig;
> +	char *charging_trig_name;
> +	struct led_trigger *full_trig;
> +	char *full_trig_name;
> +	#endif
> +};

#ifdefs need to be at column 0?

> +/* 
> + * This is recommended structure to specify static battery parameters.
> + * Generic one, parametrizable for different batteries. Battery device
> + * itself does bot use it, but that's what implementing most drivers,

'does not'?

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

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

Hello Pavel,

On Sun, Apr 15, 2007 at 07:56:56PM +0000, Pavel Machek wrote:
> Hi!
> 
> > * It insists on reusing its predefined attributes *and* their units.
> >   So, userspace getting expected values for any battery.
> >   
> >   Also common units is required for APM/ACPI emulation.
> >   
> >   Though our battery class insisting on re-usage, but not forces it. If some
> >   battery driver can't convert its own raw values (can't imagine why), then
> >   driver is free to implement its own attributes *and* additional _units
> >   attribute. Though, this scheme is discouraged.
> > 
> > * LEDs support. Each battery register its trigger, and gadgets with LEDs
> >   can quickly bind to battery-charging / battery-full triggers.
> > 
> > Here how it looks like from user space:
> > 
> > # ls /sys/class/battery/main-battery/
> > capacity  max_capacity  max_voltage   min_current  power   subsystem  uevent
> > current   max_current   min_capacity  min_voltage  status  temp       voltage
> > # cat /sys/class/battery/main-battery/status
> > Full
> > # cat /sys/class/leds/h5400\:green-right/trigger
> > none h5400-radio timer hwtimer main-battery-charging [main-battery-full]
> > # cat /sys/class/leds/h5400\:green-right/brightness
> > 255
> 
> Can we get few lines in Documentation?

Yes, sure.

> I guess min_capacity is
> shutdown capacity at current temperature, but its surely non-obvious.
> 
> Will min_capacity increase as batery gets old? Or will max_capacity
> decrease? 

min_capacity and max_capacity depend on temperature. But some drivers
can/want to interpolate, others can't/don't want. It's driver's matter.

ds2760_battery just remembers last capacity when current was < 10 mA
(i.e. battery charged full at given temperature) as max_capacity, and
takes this as reference for calculations.

> (Should we introduce design_capacity for ACPI systems that
> know the difference?)

Yup, I've introduced it in [take2] (it's in this thread, thus it's hard
to find). Will resend it as separate thread soon, along with few other
changes.

> What is min_current? Granularity of amper meter?
> 
> And min_voltage is shutdown voltage?

Yes, should be. But, for example, ds2760 battery can't remember this
value in its eeprom (iirc), thus this value passed by platform code,
i.e. in our case (iPaqs) it's machine dependent value. Probably we
should not even export this attribute in ds2760_battery driver, as it
does not take any part in calculations. Plus this attribute highly
depend on battery chemistry and temperature. Thus it's hardly anyhow
useful, except if hardware itself reports it (not ds2760 case).

> Otherwise it looks good to me. Something like this is really needed.

Thanks!

> > + * All voltages, currents, capacities and temperatures in mV, mA, mAh and
> > + * tenths of a degree unless otherwise stated. It's driver's job to convert
> 
> tenths of degree Celsius?

Yup, fixed in [take2].

> > +#define BATTERY_STATUS_UNKNOWN      0
> > +#define BATTERY_STATUS_CHARGING     1
> > +#define BATTERY_STATUS_DISCHARGING  2
> > +#define BATTERY_STATUS_NOT_CHARGING 3
> > +#define BATTERY_STATUS_FULL         4
> 
> Perhaps we need STATUS_ERROR? At least on some machines it is
> different from STATUS_NOT_CHARGING.

I'm unsure about this. BATTERY_STATUS_* is mostly about charging process
status (should I rename it to more verbose BATTERY_CHARGING_STATUS_, or just
mention it in Documentation?).

For errors things it might be better to create "health" attribute.

> > +	/* private */
> > +	struct power_supplicant pst;
> > +
> > +	#ifdef CONFIG_LEDS_TRIGGERS
> > +	struct led_trigger *charging_trig;
> > +	char *charging_trig_name;
> > +	struct led_trigger *full_trig;
> > +	char *full_trig_name;
> > +	#endif
> > +};
> 
> #ifdefs need to be at column 0?

Yup, fixed in [take2].

> > +/* 
> > + * This is recommended structure to specify static battery parameters.
> > + * Generic one, parametrizable for different batteries. Battery device
> > + * itself does bot use it, but that's what implementing most drivers,
> 
> 'does not'?

Thanks, will fix!

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


Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Ondrej Zajicek]

On Thu, Apr 12, 2007 at 03:25:03AM +0400, Anton Vorontsov wrote:
> Here is battery monitor class. According to first copyright string, we're
> maintaining it since 2003. I've took few days and cleaned it up to be
> more suitable for mainline inclusion.

Just some ideas:

- what about using exponents in values?
For example file "voltage" could contain "123 -3" to represent 123 mV.
Exponents could be hardcoded in drivers according to device's range
(so there is no complication in it), but interface is usable in great
range of values. And it is pretty easy to use from userspace.

- interface should allow to present values which are some monotonic
functions of common physical properties. For example when we know
where are some raw data from sensor, but we don't know where are
calibration tables to be able to compute value in some standard unit
(as V for voltage) - in this case it is better to show raw data 
(or raw data after some transformation which makes them monotonic)
and specify that this is raw data than show nothing. 

- it would be nice to know whether presented value is from some
measurement or it is (inaccurate) estimation.

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@mail.cz, jabber: santiago@njs.netlab.cz)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

Hi,

On Mon, Apr 16, 2007 at 12:08:54AM +0200, Ondrej Zajicek wrote:
> On Thu, Apr 12, 2007 at 03:25:03AM +0400, Anton Vorontsov wrote:
> > Here is battery monitor class. According to first copyright string, we're
> > maintaining it since 2003. I've took few days and cleaned it up to be
> > more suitable for mainline inclusion.
> 
> Just some ideas:
> 
> - what about using exponents in values?
> For example file "voltage" could contain "123 -3" to represent 123 mV.
> Exponents could be hardcoded in drivers according to device's range
> (so there is no complication in it), but interface is usable in great
> range of values. And it is pretty easy to use from userspace.

No, sorry. Common units is main goal of that class. If you're saying
"energy" you always know that it's uWh. It's better for both userspace
(don't bother to parse anything from kernel) and kernel itself.

No need to invent new kernel<->userspace protocols, no need to do
useless string manipulations in kernel itself.

> - interface should allow to present values which are some monotonic
> functions of common physical properties. For example when we know
> where are some raw data from sensor, but we don't know where are
> calibration tables to be able to compute value in some standard unit
> (as V for voltage) - in this case it is better to show raw data 
> (or raw data after some transformation which makes them monotonic)
> and specify that this is raw data than show nothing. 
> 
> - it would be nice to know whether presented value is from some
> measurement or it is (inaccurate) estimation.

Current battery class assumes values are not averaged. I.e. momentary
values. In general, it's userspace' job to collect statistics. Though,
if hardware can report only average values, it's just okay to use
usual attributes.

Also, if you your battery can collect and report its approximated values
in additional to momentary values, you're free to add _AVG attributes
to standard ones and use them.

> -- 
> Elen sila lumenn' omentielvo
> 
> Ondrej 'SanTiago' Zajicek (email: santiago@mail.cz, jabber: santiago@njs.netlab.cz)
> OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> "To err is human -- to blame it on a computer is even more so."

Thanks for comments!

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Henrique de Moraes Holschuh]

On Mon, 16 Apr 2007, Anton Vorontsov wrote:
> Current battery class assumes values are not averaged. I.e. momentary
> values. In general, it's userspace' job to collect statistics. Though,
> if hardware can report only average values, it's just okay to use
> usual attributes.

What about SBS-style battery firmware, which can report *both* ?  This
includes just about all ThinkPads in the last five years, so we are talking
about a damn big lot of machines...

I'd really appreciate if there is a standard way to communicate both.  And
it is probably a good idea to define what should be averaged, and what
should be instantaneous when that matters, that way userspace actually has a
chance at not doing something braindamaged.

Actually, IMHO, every attribute and alarm from SBS should be somehow
losslessly translatable to standard class attributes from day one, unless it
is something that makes no sense at all (and there is precious little of
that in the latest version of SBS, thankfully...).

> Also, if you your battery can collect and report its approximated values
> in additional to momentary values, you're free to add _AVG attributes
> to standard ones and use them.

No, that won't help much.  IMO, we want the sanest set of standard
attributes we can get, and weird as it might be, average reporting are
common properties of battery control firmware on laptops (maybe because of
SBS, but still...).

We don't have to get it perfect at the first try, but I really think we are
getting a bit too far from "as good as we can make it" at the first attempt
if we don't take the SBS into account properly, given the ammount of
circuits and firmware out there that are shaped along the SBS guidelines.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

On Sun, Apr 15, 2007 at 09:57:22PM -0300, Henrique de Moraes Holschuh wrote:
> On Mon, 16 Apr 2007, Anton Vorontsov wrote:
> > Current battery class assumes values are not averaged. I.e. momentary
> > values. In general, it's userspace' job to collect statistics. Though,
> > if hardware can report only average values, it's just okay to use
> > usual attributes.
> 
> What about SBS-style battery firmware, which can report *both* ?  This
> includes just about all ThinkPads in the last five years, so we are talking
> about a damn big lot of machines...

Sure, no problem. I'm taking away my words "if hardware can report
only average values, it's just okay to use usual attributes", and replacing
them by "if hardware can report only average values, it should use _AVG
attributes".

So, this scheme should work now:

1. If userspace (nice GUI app) seeing _avg values, it will use these.
2. If userspace seeing no _avg values, it is using its own statistics
   mechanism, i.e. collecting information from momentary attributes.
3. If userspace seeing both, it can decide, most probably it will decide
   to use hardware averaged values, i.e. _avg.

> I'd really appreciate if there is a standard way to communicate both.  And
> it is probably a good idea to define what should be averaged, and what
> should be instantaneous when that matters, that way userspace actually has a
> chance at not doing something braindamaged.
> 
> Actually, IMHO, every attribute and alarm from SBS should be somehow
> losslessly translatable to standard class attributes from day one, unless it
> is something that makes no sense at all (and there is precious little of
> that in the latest version of SBS, thankfully...).
> 
> > Also, if you your battery can collect and report its approximated values
> > in additional to momentary values, you're free to add _AVG attributes
> > to standard ones and use them.
> 
> No, that won't help much.  IMO, we want the sanest set of standard
> attributes we can get, and weird as it might be, average reporting are
> common properties of battery control firmware on laptops (maybe because of
> SBS, but still...).

Why that won't help? Is there something else besides momentary and
hardware-averaged values?

> We don't have to get it perfect at the first try, but I really think we are
> getting a bit too far from "as good as we can make it" at the first attempt
> if we don't take the SBS into account properly, given the ammount of
> circuits and firmware out there that are shaped along the SBS guidelines.

I guess the only question for you is whether we would use, "attr" as momentary
and "attr_avg" as averaged by hardware value, or will use "attr" as averaged
values, and something alike "attr_now" for momentary? And you voting for
"attr" being averaged and "attr_now" momentary.

Actually, I don't see much difference, except default assumption of "attr"
being averaged/momentary.

Though, if it's real issue for you or SBS conformity, I can surely rename
attrs.

> -- 
>   "One disk to rule them all, One disk to find them. One disk to bring
>   them all and in the darkness grind them. In the Land of Redmond
>   where the shadows lie." -- The Silicon Valley Tarot
>   Henrique Holschuh
> 

And again, much thanks for your comments! They're really helpful.

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [PATCH 3/7] [RFC] Battery monitoring class

[Henrique de Moraes Holschuh]

> > > Also, if you your battery can collect and report its approximated values
> > > in additional to momentary values, you're free to add _AVG attributes
> > > to standard ones and use them.
> > 
> > No, that won't help much.  IMO, we want the sanest set of standard
> > attributes we can get, and weird as it might be, average reporting are
> > common properties of battery control firmware on laptops (maybe because of
> > SBS, but still...).
> 
> Why that won't help? Is there something else besides momentary and
> hardware-averaged values?

It wouldn't help much to not have standard generic attributes for the
averaged measurements.

That said, yes, there are relative measurements (percent), and time
measurements (some battery firmware can tell you how much *time* it will
take to stop recharing, empty the battery, etc).

> I guess the only question for you is whether we would use, "attr" as momentary
> and "attr_avg" as averaged by hardware value, or will use "attr" as averaged
> values, and something alike "attr_now" for momentary? And you voting for
> "attr" being averaged and "attr_now" momentary.

I don't care either way, as long as it is *documented* what is averaged, and
what is instantaneous.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[ian]

On Sun, 2007-04-15 at 21:57 -0300, Henrique de Moraes Holschuh wrote:
> 
> No, that won't help much.  IMO, we want the sanest set of standard
> attributes we can get, and weird as it might be, average reporting are
> common properties of battery control firmware on laptops (maybe
> because of SBS, but still...).

We need to think very carefully here.

charge, current, capacity, etc. are properties all batteries have, and
the current values can all be sampled instantaneously.

funky values processed by 'black box' firmware are not universal
properties of all batteries.

IOW, battery class should be for 'simple' battery types only.

perhaps rename it to simple battery class to make it distinct?

Userspace is the place to put the complications, in any case, and I see
nothing wrong with having both a simple battery class AND other
proprietary battery class an SBS battery class. (or a
toshiba_proprietary_bios battery class or whatever).

Perhaps we need a 'libbattery.so' so that userspace can have a nice
consistent interface?

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[Anton Vorontsov]

On Mon, Apr 16, 2007 at 03:32:54AM +0100, ian wrote:
> On Sun, 2007-04-15 at 21:57 -0300, Henrique de Moraes Holschuh wrote:
> > 
> > No, that won't help much.  IMO, we want the sanest set of standard
> > attributes we can get, and weird as it might be, average reporting are
> > common properties of battery control firmware on laptops (maybe
> > because of SBS, but still...).
> 
> We need to think very carefully here.
> 
> charge, current, capacity, etc. are properties all batteries have, and
> the current values can all be sampled instantaneously.
> 
> funky values processed by 'black box' firmware are not universal
> properties of all batteries.
> 
> IOW, battery class should be for 'simple' battery types only.
> 
> perhaps rename it to simple battery class to make it distinct?
> 
> Userspace is the place to put the complications, in any case, and I see
> nothing wrong with having both a simple battery class AND other
> proprietary battery class an SBS battery class. (or a
> toshiba_proprietary_bios battery class or whatever).
> 
> Perhaps we need a 'libbattery.so' so that userspace can have a nice
> consistent interface?

Why? With current battery class we can do whatever everyone needs. No
need for wrappers.

Adding new properties is cheap and easy. Simple batteries using only
"simple" properties/attributes, and complicated batteries using
complicated attributes.

Because of your original design, simple batteries are stay simple, and
no noticing that there is some "complicated" attributes exists at all.
That's indeed great characteristic of that *universal* battery class.

For example, ds2760 is not really "simple" monitoring chip. ADC battery
is (it's in -hh tree so far). So, ds2760 is somewhere in between SBS
design, and dumb ADC batteries.

So, my another purposal, which I very like now:
Let's do self-documented properties. current_now, current_avg, e.t.c.
No more just "current", lets remove any ambiguousness!

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [Kernel-discuss] Re: [PATCH 3/7] [RFC] Battery monitoring class

[ian]

On Mon, 2007-04-16 at 07:12 +0400, Anton Vorontsov wrote:

> Why? With current battery class we can do whatever everyone needs. No
> need for wrappers.

<cut>

> Because of your original design, simple batteries are stay simple, and
> no noticing that there is some "complicated" attributes exists at all.
> That's indeed great characteristic of that *universal* battery class.

Indeed. Im just trying to make sure we dont bloat an otherwise very
simple class. A word of caution only.