Battery class driver.

Battery class driver.

[David Woodhouse]

At git://git.infradead.org/battery-2.6.git there is an initial
implementation of a battery class, along with a driver which makes use
of it. The patch is below, and also viewable at 
http://git.infradead.org/?p=battery-2.6.git;a=commitdiff;h=master;hp=linus

I don't like the sysfs interaction much -- is it really necessary for me
to provide a separate function for each attribute, rather than a single
function which handles them all and is given the individual attribute as
an argument? That seems strange and bloated.

I'm half tempted to ditch the sysfs attributes and just use a single
seq_file, in fact.

The idea is that all batteries should be presented to userspace through
this class instead of through the existing mess of PMU/APM/ACPI and even
APM _emulation_.

I think I probably want to make AC power a separate 'device' too, rather
than an attribute of any given battery. And when there are multiple
power supplies, there should be multiple such devices. So maybe it
should be a 'power supply' class, not a battery class at all?

Comments? 


commit 42fe507a262b2a2879ca62740c5312778ae78627
Author: David Woodhouse <dwmw2@infradead.org>
Date:   Mon Oct 23 18:14:54 2006 +0100

    [BATTERY] Add support for OLPC battery
    
    Signed-off-by: David Woodhouse <dwmw2@infradead.org>

commit 6cbec3b84e3ce737b4217788841ea10a28a5e340
Author: David Woodhouse <dwmw2@infradead.org>
Date:   Mon Oct 23 18:14:14 2006 +0100

    [BATTERY] Add initial implementation of battery class
    
    I really don't like the sysfs interaction, and I don't much like the
    internal interaction with the battery drivers either. In fact, there
    isn't much I _do_ like, but it's good enough as a straw man.
    
    Signed-off-by: David Woodhouse <dwmw2@infradead.org>

--- drivers/Makefile~	2006-10-19 19:51:32.000000000 +0100
+++ drivers/Makefile	2006-10-23 15:27:47.000000000 +0100
@@ -30,6 +30,7 @@ obj-$(CONFIG_PARPORT)		+= parport/
 obj-y				+= base/ block/ misc/ mfd/ net/ media/
 obj-$(CONFIG_NUBUS)		+= nubus/
 obj-$(CONFIG_ATM)		+= atm/
+obj-$(CONFIG_BATTERY_CLASS)	+= battery/
 obj-$(CONFIG_PPC_PMAC)		+= macintosh/
 obj-$(CONFIG_XEN)		+= xen/
 obj-$(CONFIG_IDE)		+= ide/
--- drivers/Kconfig~	2006-09-20 04:42:06.000000000 +0100
+++ drivers/Kconfig	2006-10-23 15:27:42.000000000 +0100
@@ -28,6 +28,8 @@ source "drivers/ieee1394/Kconfig"
 
 source "drivers/message/i2o/Kconfig"
 
+source "drivers/battery/Kconfig"
+
 source "drivers/macintosh/Kconfig"
 
 source "drivers/net/Kconfig"
--- /dev/null	2006-10-01 17:20:05.827666723 +0100
+++ drivers/battery/Makefile	2006-10-23 16:53:20.000000000 +0100
@@ -0,0 +1,4 @@
+# Battery code
+obj-$(CONFIG_BATTERY_CLASS)		+= battery-class.o
+
+obj-$(CONFIG_OLPC_BATTERY)		+= olpc-battery.o
--- /dev/null	2006-10-01 17:20:05.827666723 +0100
+++ drivers/battery/Kconfig	2006-10-23 16:52:42.000000000 +0100
@@ -0,0 +1,22 @@
+
+menu "Battery support"
+
+config BATTERY_CLASS
+       tristate "Battery support"
+       help
+         Say Y to enable battery class support. This allows a battery
+	 information to be presented in a uniform manner for all types
+	 of batteries.
+
+	 Battery information from APM and ACPI is not yet available by
+	 this method, but should soon be. If you use APM or ACPI, say
+	 'N', although saying 'Y' would be harmless.
+
+config OLPC_BATTERY
+       tristate "One Laptop Per Child battery"
+       depends on BATTERY_CLASS && X86_32
+       help
+         Say Y to enable support for the battery on the $100 laptop.
+
+
+endmenu
--- /dev/null	2006-10-01 17:20:05.827666723 +0100
+++ drivers/battery/battery-class.c	2006-10-23 17:59:04.000000000 +0100
@@ -0,0 +1,286 @@
+/*
+ * Battery class core
+ *
+ *	© 2006 David Woodhouse <dwmw2@infradead.org>
+ *
+ * Based on LED Class support, by John Lenz and Richard Purdie:
+ *
+ *	© 2005 John Lenz <lenz@cs.wisc.edu>
+ *	© 2005-2006 Richard Purdie <rpurdie@openedhand.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/battery.h>
+#include <linux/spinlock.h>
+#include <linux/err.h>
+
+static struct class *battery_class;
+
+/* OMFG we can't just have a single 'show' routine which is given the
+   'class_attribute' as an argument -- we have to have 20-odd copies
+   of almost identical routines */
+
+static ssize_t battery_attribute_show_int(struct class_device *dev, char *buf, int attr)
+{
+        struct battery_classdev *battery_cdev = class_get_devdata(dev);
+        ssize_t ret = 0;
+	long value;
+
+	ret =  battery_cdev->query_long(battery_cdev, attr, &value);
+	if (ret)
+		return ret;
+
+	sprintf(buf, "%ld\n", value);
+        ret = strlen(buf) + 1;
+
+        return ret;
+}  
+
+static ssize_t battery_attribute_show_milli(struct class_device *dev, char *buf, int attr)
+{
+        struct battery_classdev *battery_cdev = class_get_devdata(dev);
+        ssize_t ret = 0;
+	long value;
+
+	ret = battery_cdev->query_long(battery_cdev, attr, &value);
+	if (ret)
+		return ret;
+
+	sprintf(buf, "%ld.%03ld\n", value/1000, value % 1000);
+        ret = strlen(buf) + 1;
+        return ret;
+}  
+
+static ssize_t battery_attribute_show_string(struct class_device *dev, char *buf, int attr)
+{
+        struct battery_classdev *battery_cdev = class_get_devdata(dev);
+        ssize_t ret = 0;
+
+	ret = battery_cdev->query_str(battery_cdev, attr, buf, PAGE_SIZE-1);
+	if (ret)
+		return ret;
+
+	strcat(buf, "\n");
+        ret = strlen(buf) + 1;
+        return ret;
+}  
+
+static ssize_t battery_attribute_show_status(struct class_device *dev, char *buf)
+{
+        struct battery_classdev *battery_cdev = class_get_devdata(dev);
+        ssize_t ret = 0;
+	unsigned long status;
+
+	status = battery_cdev->status(battery_cdev, ~BAT_STAT_AC);
+	if (status & BAT_STAT_ERROR)
+		return -EIO;
+
+	if (status & BAT_STAT_PRESENT)
+		sprintf(buf, "present");
+	else
+		sprintf(buf, "absent");
+
+	if (status & BAT_STAT_LOW)
+		strcat(buf, ",low");
+
+	if (status & BAT_STAT_FULL)
+		strcat(buf, ",full");
+
+	if (status & BAT_STAT_CHARGING)
+		strcat(buf, ",charging");
+
+	if (status & BAT_STAT_DISCHARGING)
+		strcat(buf, ",discharging");
+
+	if (status & BAT_STAT_OVERTEMP)
+		strcat(buf, ",overtemp");
+
+	if (status & BAT_STAT_FIRE)
+		strcat(buf, ",on-fire");
+
+	if (status & BAT_STAT_CHARGE_DONE)
+		strcat(buf, ",charge-done");
+
+	strcat(buf, "\n");
+        ret = strlen(buf) + 1;
+        return ret;
+}
+
+static ssize_t battery_attribute_show_ac_status(struct class_device *dev, char *buf)
+{
+        struct battery_classdev *battery_cdev = class_get_devdata(dev);
+        ssize_t ret = 0;
+	unsigned long status;
+
+	status = battery_cdev->status(battery_cdev, BAT_STAT_AC);
+	if (status & BAT_STAT_ERROR)
+		return -EIO;
+
+	if (status & BAT_STAT_AC)
+		sprintf(buf, "on-line");
+	else
+		sprintf(buf, "off-line");
+
+	strcat(buf, "\n");
+        ret = strlen(buf) + 1;
+        return ret;
+}  
+
+/* Ew. We can't even use CLASS_DEVICE_ATTR() if we want one named 'current' */
+#define BATTERY_DEVICE_ATTR(_name, _attr, _type) \
+static ssize_t battery_attr_show_##_attr(struct class_device *dev, char *buf)	\
+{										\
+	return battery_attribute_show_##_type(dev, buf, BAT_INFO_##_attr);	\
+}										\
+static struct class_device_attribute class_device_attr_##_attr  = {		\
+        .attr = { .name = _name, .mode = 0444, .owner = THIS_MODULE },		\
+	.show = battery_attr_show_##_attr };
+
+static CLASS_DEVICE_ATTR(status,0444,battery_attribute_show_status, NULL);
+static CLASS_DEVICE_ATTR(ac,0444,battery_attribute_show_ac_status, NULL);
+BATTERY_DEVICE_ATTR("temp1",TEMP1,milli);
+BATTERY_DEVICE_ATTR("temp1_name",TEMP1_NAME,string);
+BATTERY_DEVICE_ATTR("temp2",TEMP2,milli);
+BATTERY_DEVICE_ATTR("temp2_name",TEMP2_NAME,string);
+BATTERY_DEVICE_ATTR("voltage",VOLTAGE,milli);
+BATTERY_DEVICE_ATTR("voltage_design",VOLTAGE_DESIGN,milli);
+BATTERY_DEVICE_ATTR("current",CURRENT,milli);
+BATTERY_DEVICE_ATTR("charge_rate",CHARGE_RATE,milli);
+BATTERY_DEVICE_ATTR("charge_max",CHARGE_MAX,milli);
+BATTERY_DEVICE_ATTR("charge_last",CHARGE_LAST,milli);
+BATTERY_DEVICE_ATTR("charge_low",CHARGE_LOW,milli);
+BATTERY_DEVICE_ATTR("charge_warn",CHARGE_WARN,milli);
+BATTERY_DEVICE_ATTR("charge_unit",CHARGE_UNITS,string);
+BATTERY_DEVICE_ATTR("charge_percent",CHARGE_PCT,int);
+BATTERY_DEVICE_ATTR("time_remaining",TIME_REMAINING,int);
+BATTERY_DEVICE_ATTR("manufacturer",MANUFACTURER,string);
+BATTERY_DEVICE_ATTR("technology",TECHNOLOGY,string);
+BATTERY_DEVICE_ATTR("model",MODEL,string);
+BATTERY_DEVICE_ATTR("serial",SERIAL,string);
+BATTERY_DEVICE_ATTR("type",TYPE,string);
+BATTERY_DEVICE_ATTR("oem_info",OEM_INFO,string);
+
+#define REGISTER_ATTR(_attr)							\
+	if (battery_cdev->capabilities & (1<<BAT_INFO_##_attr))			\
+		class_device_create_file(battery_cdev->class_dev,		\
+					 &class_device_attr_##_attr);
+#define UNREGISTER_ATTR(_attr)							\
+	if (battery_cdev->capabilities & (1<<BAT_INFO_##_attr))			\
+		class_device_remove_file(battery_cdev->class_dev,		\
+					 &class_device_attr_##_attr);
+/**
+ * battery_classdev_register - register a new object of battery_classdev class.
+ * @dev: The device to register.
+ * @battery_cdev: the battery_classdev structure for this device.
+ */
+int battery_classdev_register(struct device *parent, struct battery_classdev *battery_cdev)
+{
+        battery_cdev->class_dev = class_device_create(battery_class, NULL, 0,
+						      parent, "%s", battery_cdev->name);
+
+        if (unlikely(IS_ERR(battery_cdev->class_dev)))
+                return PTR_ERR(battery_cdev->class_dev);
+
+        class_set_devdata(battery_cdev->class_dev, battery_cdev);
+
+        /* register the attributes */
+        class_device_create_file(battery_cdev->class_dev,
+                                &class_device_attr_status);
+
+        if (battery_cdev->status_cap & (1<<BAT_STAT_AC))
+		class_device_create_file(battery_cdev->class_dev, &class_device_attr_ac);
+
+	REGISTER_ATTR(TEMP1);
+	REGISTER_ATTR(TEMP1_NAME);
+	REGISTER_ATTR(TEMP2);
+	REGISTER_ATTR(TEMP2_NAME);
+	REGISTER_ATTR(VOLTAGE);
+	REGISTER_ATTR(VOLTAGE_DESIGN);
+	REGISTER_ATTR(CHARGE_PCT);
+	REGISTER_ATTR(CURRENT);
+	REGISTER_ATTR(CHARGE_RATE);
+	REGISTER_ATTR(CHARGE_MAX);
+	REGISTER_ATTR(CHARGE_LAST);
+	REGISTER_ATTR(CHARGE_LOW);
+	REGISTER_ATTR(CHARGE_WARN);
+	REGISTER_ATTR(CHARGE_UNITS);
+	REGISTER_ATTR(TIME_REMAINING);
+	REGISTER_ATTR(MANUFACTURER);
+	REGISTER_ATTR(TECHNOLOGY);
+	REGISTER_ATTR(MODEL);
+	REGISTER_ATTR(SERIAL);
+	REGISTER_ATTR(TYPE);
+	REGISTER_ATTR(OEM_INFO);
+
+        printk(KERN_INFO "Registered battery device: %s\n",
+                        battery_cdev->class_dev->class_id);
+
+        return 0;
+}
+EXPORT_SYMBOL_GPL(battery_classdev_register);
+
+/**
+ * battery_classdev_unregister - unregisters a object of battery_properties class.
+ * @battery_cdev: the battery device to unreigister
+ *
+ * Unregisters a previously registered via battery_classdev_register object.
+ */
+void battery_classdev_unregister(struct battery_classdev *battery_cdev)
+{
+        class_device_remove_file(battery_cdev->class_dev,
+				 &class_device_attr_status);
+
+        if (battery_cdev->status_cap & (1<<BAT_STAT_AC))
+		class_device_remove_file(battery_cdev->class_dev, &class_device_attr_ac);
+
+	UNREGISTER_ATTR(TEMP1);
+	UNREGISTER_ATTR(TEMP1_NAME);
+	UNREGISTER_ATTR(TEMP2);
+	UNREGISTER_ATTR(TEMP2_NAME);
+	UNREGISTER_ATTR(VOLTAGE);
+	UNREGISTER_ATTR(VOLTAGE_DESIGN);
+	UNREGISTER_ATTR(CHARGE_PCT);
+	UNREGISTER_ATTR(CURRENT);
+	UNREGISTER_ATTR(CHARGE_RATE);
+	UNREGISTER_ATTR(CHARGE_MAX);
+	UNREGISTER_ATTR(CHARGE_LAST);
+	UNREGISTER_ATTR(CHARGE_LOW);
+	UNREGISTER_ATTR(CHARGE_WARN);
+	UNREGISTER_ATTR(CHARGE_UNITS);
+	UNREGISTER_ATTR(TIME_REMAINING);
+	UNREGISTER_ATTR(MANUFACTURER);
+	UNREGISTER_ATTR(TECHNOLOGY);
+	UNREGISTER_ATTR(MODEL);
+	UNREGISTER_ATTR(SERIAL);
+	UNREGISTER_ATTR(TYPE);
+	UNREGISTER_ATTR(OEM_INFO);
+
+        class_device_unregister(battery_cdev->class_dev);
+}
+EXPORT_SYMBOL_GPL(battery_classdev_unregister);
+
+static int __init battery_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_exit(void)
+{
+        class_destroy(battery_class);
+}
+
+subsys_initcall(battery_init);
+module_exit(battery_exit);
+
+MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Battery class interface");
--- /dev/null	2006-10-01 17:20:05.827666723 +0100
+++ drivers/battery/olpc-battery.c	2006-10-23 17:56:38.000000000 +0100
@@ -0,0 +1,198 @@
+/*
+ * Battery driver for One Laptop Per Child ($100 laptop) board.
+ *
+ *	© 2006 David Woodhouse <dwmw2@infradead.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/battery.h>
+#include <linux/spinlock.h>
+#include <linux/err.h>
+#include <asm/io.h>
+
+#define wBAT_VOLTAGE		0xf900 /* *9.76/32,    mV */
+#define wBAT_CURRENT		0xf902 /* *15.625/120, mA */
+#define wBAT_TEMP		0xf906 /* *256/1000,   °C */
+#define wAMB_TEMP		0xf908 /* *256/1000,   °C */
+#define SOC			0xf910 /*      percentage */
+#define sMBAT_STATUS		0xfaa4
+#define		sBAT_PRESENT		1
+#define		sBAT_FULL		2
+#define		sBAT_DESTROY		4
+#define		sBAT_LOW			32
+#define		sBAT_DISCHG		64
+#define sMCHARGE_STATUS		0xfaa5
+#define		sBAT_CHARGE		1
+#define		sBAT_OVERTEMP		4
+#define		sBAT_NiMH		8
+#define sPOWER_FLAG		0xfa40
+#define		ADAPTER_IN		1
+
+static int lock_ec(void)
+{
+	unsigned long timeo = jiffies + HZ/20;
+
+	while (1) {
+                unsigned char lock = inb(0x6c) & 0x80;
+                if (!lock)
+                        return 0;
+		if (time_after(jiffies, timeo))
+			return 1;
+                yield();
+        }
+}
+
+static void unlock_ec(void)
+{
+        outb(0xff, 0x6c);
+}
+
+unsigned char read_ec_byte(unsigned short adr)
+{
+        outb(adr >> 8, 0x381);
+        outb(adr, 0x382);
+        return inb(0x383);
+}
+
+unsigned short read_ec_word(unsigned short adr)
+{
+        return (read_ec_byte(adr) << 8) | read_ec_byte(adr+1);
+}
+
+unsigned long olpc_bat_status(struct battery_classdev *cdev, unsigned long mask)
+{
+	unsigned long result = 0;
+	unsigned short tmp;
+
+	if (lock_ec()) {
+		printk(KERN_ERR "Failed to lock EC for battery access\n");
+		return BAT_STAT_ERROR;
+	}
+
+	if (mask & BAT_STAT_AC) {
+		if (read_ec_byte(sPOWER_FLAG) & ADAPTER_IN)
+			result |= BAT_STAT_AC;
+	}
+	if (mask & (BAT_STAT_PRESENT|BAT_STAT_FULL|BAT_STAT_FIRE|BAT_STAT_LOW|BAT_STAT_DISCHARGING)) {
+		tmp = read_ec_byte(sMBAT_STATUS);
+		
+		if (tmp & sBAT_PRESENT)
+			result |= BAT_STAT_PRESENT;
+		if (tmp & sBAT_FULL)
+			result |= BAT_STAT_FULL;
+		if (tmp & sBAT_DESTROY)
+			result |= BAT_STAT_FIRE;
+		if (tmp & sBAT_LOW)
+			result |= BAT_STAT_LOW;
+		if (tmp & sBAT_DISCHG)
+			result |= BAT_STAT_DISCHARGING;
+	}
+	if (mask & (BAT_STAT_CHARGING|BAT_STAT_OVERTEMP)) {
+		tmp = read_ec_byte(sMCHARGE_STATUS);
+		if (tmp & sBAT_CHARGE)
+			result |= BAT_STAT_CHARGING;
+		if (tmp & sBAT_OVERTEMP)
+			result |= BAT_STAT_OVERTEMP;
+	}
+	unlock_ec();
+	return result;
+}
+
+int olpc_bat_query_long(struct battery_classdev *dev, int attr, long *result)
+{
+	int ret = 0;
+
+	if (lock_ec())
+		return -EIO;
+
+	if (!(read_ec_byte(sMBAT_STATUS) & sBAT_PRESENT)) {
+		ret = -ENODEV;
+	} else if (attr == BAT_INFO_VOLTAGE) {
+		*result = read_ec_word(wBAT_VOLTAGE) * 9760 / 32000;
+	} else if (attr == BAT_INFO_CURRENT) {
+		*result = read_ec_word(wBAT_CURRENT) * 15625 / 120000;
+	} else if (attr == BAT_INFO_TEMP1) {
+		*result = read_ec_word(wBAT_TEMP) * 1000 / 256;
+	} else if (attr == BAT_INFO_TEMP2) {
+		*result = read_ec_word(wAMB_TEMP) * 1000 / 256;
+	} else if (attr == BAT_INFO_CHARGE_PCT) {
+		*result = read_ec_byte(SOC);
+	} else 
+		ret = -EINVAL;
+
+	unlock_ec();
+	return ret;
+}
+
+int olpc_bat_query_str(struct battery_classdev *dev, int attr, char *str, int len)
+{
+	int ret = 0;
+
+	if (attr == BAT_INFO_TYPE) {
+		snprintf(str, len, "OLPC");
+	} else if (attr == BAT_INFO_TEMP1_NAME) {
+		snprintf(str, len, "battery");
+	} else if (attr == BAT_INFO_TEMP2_NAME) {
+		snprintf(str, len, "ambient");
+	} else if (!(read_ec_byte(sMBAT_STATUS) & sBAT_PRESENT)) {
+		ret = -ENODEV;
+	} else if (attr == BAT_INFO_TECHNOLOGY) {
+		if (lock_ec())
+			ret = -EIO;
+		else {
+			unsigned short tmp = read_ec_byte(sMCHARGE_STATUS);
+			if (tmp & sBAT_NiMH)
+				snprintf(str, len, "NiMH");
+			else
+				snprintf(str, len, "unknown");
+		}
+		unlock_ec();
+	} else {
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static struct battery_classdev olpc_bat = {
+	.name = "OLPC",
+	.capabilities = (1<<BAT_INFO_VOLTAGE) |
+			(1<<BAT_INFO_CURRENT) |
+			(1<<BAT_INFO_TEMP1) |
+			(1<<BAT_INFO_TEMP2) |
+			(1<<BAT_INFO_CHARGE_PCT) |
+			(1<<BAT_INFO_TYPE) |
+			(1<<BAT_INFO_TECHNOLOGY) |
+			(1<<BAT_INFO_TEMP1_NAME) |
+			(1<<BAT_INFO_TEMP2_NAME),
+	.status_cap = BAT_STAT_AC | BAT_STAT_PRESENT | BAT_STAT_LOW | 
+		      BAT_STAT_FULL | BAT_STAT_CHARGING| BAT_STAT_DISCHARGING |
+		      BAT_STAT_OVERTEMP | BAT_STAT_FIRE,
+	.status = olpc_bat_status,
+	.query_long = olpc_bat_query_long,
+	.query_str = olpc_bat_query_str,
+};
+
+void __exit olpc_bat_exit(void)
+{
+	battery_classdev_unregister(&olpc_bat);
+}
+
+int __init olpc_bat_init(void)
+{
+	battery_classdev_register(NULL, &olpc_bat);
+	return 0;
+}
+
+module_init(olpc_bat_init);
+module_exit(olpc_bat_exit);
+
+MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Battery class interface");
--- /dev/null	2006-10-01 17:20:05.827666723 +0100
+++ include/linux/battery.h	2006-10-23 17:11:28.000000000 +0100
@@ -0,0 +1,84 @@
+/*
+ * Driver model for batteries
+ *
+ *	© 2006 David Woodhouse <dwmw2@infradead.org>
+ *
+ * Based on LED Class support, by John Lenz and Richard Purdie:
+ *
+ *	© 2005 John Lenz <lenz@cs.wisc.edu>
+ *	© 2005-2006 Richard Purdie <rpurdie@openedhand.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#ifndef __LINUX_BATTERY_H__
+#define __LINUX_BATTERY_H__
+
+struct device;
+struct class_device;
+
+/*
+ * Battery Core
+ */
+#define BAT_STAT_AC		(1<<0)
+#define BAT_STAT_PRESENT	(1<<1)
+#define BAT_STAT_LOW		(1<<2)
+#define BAT_STAT_FULL		(1<<3)
+#define BAT_STAT_CHARGING	(1<<4)
+#define BAT_STAT_DISCHARGING	(1<<5)
+#define BAT_STAT_OVERTEMP	(1<<6)
+#define BAT_STAT_FIRE		(1<<7)
+#define BAT_STAT_CHARGE_DONE	(1<<8)
+
+#define BAT_STAT_ERROR		(1<<31)
+
+#define BAT_INFO_TEMP1		(0) /* °C/1000 */
+#define BAT_INFO_TEMP1_NAME	(1) /* string */
+
+#define BAT_INFO_TEMP2		(2) /* °C/1000 */
+#define BAT_INFO_TEMP2_NAME	(3) /* string */
+
+#define BAT_INFO_VOLTAGE	(4) /* mV */
+#define BAT_INFO_VOLTAGE_DESIGN	(5) /* mV */
+
+#define BAT_INFO_CURRENT	(6) /* mA */
+
+#define BAT_INFO_CHARGE_RATE	(7) /* BAT_INFO_CHARGE_UNITS */
+#define BAT_INFO_CHARGE		(8) /* BAT_INFO_CHARGE_UNITS */
+#define BAT_INFO_CHARGE_MAX	(9) /* BAT_INFO_CHARGE_UNITS  */
+#define BAT_INFO_CHARGE_LAST	(10) /* BAT_INFO_CHARGE_UNITS  */
+#define BAT_INFO_CHARGE_LOW	(11) /* BAT_INFO_CHARGE_UNITS  */
+#define BAT_INFO_CHARGE_WARN	(12) /* BAT_INFO_CHARGE_UNITS  */
+#define BAT_INFO_CHARGE_UNITS	(13) /* string */
+#define BAT_INFO_CHARGE_PCT	(14) /* % */
+
+#define BAT_INFO_TIME_REMAINING	(15) /* seconds */
+
+#define BAT_INFO_MANUFACTURER	(16) /* string */
+#define BAT_INFO_TECHNOLOGY	(17) /* string */
+#define BAT_INFO_MODEL		(18) /* string */
+#define BAT_INFO_SERIAL		(19) /* string */
+#define BAT_INFO_TYPE		(20) /* string */
+#define BAT_INFO_OEM_INFO	(21) /* string */
+
+struct battery_classdev {
+	const char		*name;
+	/* Capabilities of this battery driver */
+	unsigned long		 capabilities, status_cap;
+
+	/* Query functions */
+	unsigned long		(*status)(struct battery_classdev *, unsigned long mask);
+	int			(*query_long)(struct battery_classdev *, int attr, long *result);
+	int			(*query_str)(struct battery_classdev *, int attr, char *str, ssize_t len);
+
+	struct class_device	*class_dev;
+	struct list_head	 node;	/* Battery Device list */
+};
+
+extern int battery_classdev_register(struct device *parent,
+				     struct battery_classdev *battery_cdev);
+extern void battery_classdev_unregister(struct battery_classdev *battery_cdev);
+
+#endif /* __LINUX_BATTERY_H__ */


-- 
dwmw2

Re: Battery class driver.

[Matthew Garrett]

On Mon, Oct 23, 2006 at 07:20:33PM +0100, David Woodhouse wrote:

> +BATTERY_DEVICE_ATTR("temp1",TEMP1,milli);
> +BATTERY_DEVICE_ATTR("temp1_name",TEMP1_NAME,string);
> +BATTERY_DEVICE_ATTR("temp2",TEMP2,milli);
> +BATTERY_DEVICE_ATTR("temp2_name",TEMP2_NAME,string);
> +BATTERY_DEVICE_ATTR("voltage",VOLTAGE,milli);
> +BATTERY_DEVICE_ATTR("voltage_design",VOLTAGE_DESIGN,milli);
> +BATTERY_DEVICE_ATTR("current",CURRENT,milli);
> +BATTERY_DEVICE_ATTR("charge_rate",CHARGE_RATE,milli);
> +BATTERY_DEVICE_ATTR("charge_max",CHARGE_MAX,milli);
> +BATTERY_DEVICE_ATTR("charge_last",CHARGE_LAST,milli);
> +BATTERY_DEVICE_ATTR("charge_low",CHARGE_LOW,milli);
> +BATTERY_DEVICE_ATTR("charge_warn",CHARGE_WARN,milli);
> +BATTERY_DEVICE_ATTR("charge_unit",CHARGE_UNITS,string);
> +BATTERY_DEVICE_ATTR("charge_percent",CHARGE_PCT,int);
> +BATTERY_DEVICE_ATTR("time_remaining",TIME_REMAINING,int);
> +BATTERY_DEVICE_ATTR("manufacturer",MANUFACTURER,string);
> +BATTERY_DEVICE_ATTR("technology",TECHNOLOGY,string);
> +BATTERY_DEVICE_ATTR("model",MODEL,string);
> +BATTERY_DEVICE_ATTR("serial",SERIAL,string);
> +BATTERY_DEVICE_ATTR("type",TYPE,string);
> +BATTERY_DEVICE_ATTR("oem_info",OEM_INFO,string);

Without commenting on the rest:

The tp_smapi code also provides average current and power, the charge 
cycle count, the date of first use, the date of manufacture and controls 
for altering the charge behaviour of the battery. Are these things that 
should go in the generic class?

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

Re: Battery class driver.

[David Woodhouse]

On Mon, 2006-10-23 at 19:26 +0100, Matthew Garrett wrote:
> Without commenting on the rest:
> 
> The tp_smapi code also provides average current and power, the charge 
> cycle count, the date of first use, the date of manufacture and controls 
> for altering the charge behaviour of the battery. Are these things that 
> should go in the generic class?

Yes, almost certainly. I went looking for what ACPI, PMU and of course
OLPC provided, but missed the others.

-- 
dwmw2

Re: Battery class driver.

[Greg KH]

On Mon, Oct 23, 2006 at 07:20:33PM +0100, David Woodhouse wrote:
> At git://git.infradead.org/battery-2.6.git there is an initial
> implementation of a battery class, along with a driver which makes use
> of it. The patch is below, and also viewable at 
> http://git.infradead.org/?p=battery-2.6.git;a=commitdiff;h=master;hp=linus
> 
> I don't like the sysfs interaction much -- is it really necessary for me
> to provide a separate function for each attribute, rather than a single
> function which handles them all and is given the individual attribute as
> an argument? That seems strange and bloated.

It is, but no one has asked for it to be changed to be like the struct
device attributes are.  In fact, why not just use the struct device
attributes here instead?  That will be much easier and keep me from
having to convert your code over to use it in the future :)

> I'm half tempted to ditch the sysfs attributes and just use a single
> seq_file, in fact.

Ick, no.  You should use the hwmon interface, and standardize on a
proper battery api just like those developers have standardized on other
sensor apis that are exported to userspace.  Take a look at
Documentation/hwmon/sysfs-interface for an example of what it should
look like.

> The idea is that all batteries should be presented to userspace through
> this class instead of through the existing mess of PMU/APM/ACPI and even
> APM _emulation_.

Yes, I agree this should be done in this manner.

> I think I probably want to make AC power a separate 'device' too, rather
> than an attribute of any given battery. And when there are multiple
> power supplies, there should be multiple such devices. So maybe it
> should be a 'power supply' class, not a battery class at all?

That sounds good to me.

Jean, I know you had some ideas with regards to this in the past.

thanks,

greg k-h

Re: Battery class driver.

[Greg KH]

On Mon, Oct 23, 2006 at 11:30:48AM -0700, Greg KH wrote:
> On Mon, Oct 23, 2006 at 07:20:33PM +0100, David Woodhouse wrote:
> > I'm half tempted to ditch the sysfs attributes and just use a single
> > seq_file, in fact.
> 
> Ick, no.  You should use the hwmon interface, and standardize on a
> proper battery api just like those developers have standardized on other
> sensor apis that are exported to userspace.  Take a look at
> Documentation/hwmon/sysfs-interface for an example of what it should
> look like.

Ok, nevermind, it looks like your code does do something much like this,
which is great.  Just make sure your units are the same as the other
hwmon drivers and everything should be fine.

thanks,

greg k-h

Re: Battery class driver.

[David Woodhouse]

On Mon, 2006-10-23 at 11:30 -0700, Greg KH wrote:
> On Mon, Oct 23, 2006 at 07:20:33PM +0100, David Woodhouse wrote:
> > At git://git.infradead.org/battery-2.6.git there is an initial
> > implementation of a battery class, along with a driver which makes use
> > of it. The patch is below, and also viewable at 
> > http://git.infradead.org/?p=battery-2.6.git;a=commitdiff;h=master;hp=linus
> > 
> > I don't like the sysfs interaction much -- is it really necessary for me
> > to provide a separate function for each attribute, rather than a single
> > function which handles them all and is given the individual attribute as
> > an argument? That seems strange and bloated.
> 
> It is, but no one has asked for it to be changed to be like the struct
> device attributes are.  In fact, why not just use the struct device
> attributes here instead?  That will be much easier and keep me from
> having to convert your code over to use it in the future :)

Heh, OK. I'll look at that. Thanks.

> > I'm half tempted to ditch the sysfs attributes and just use a single
> > seq_file, in fact.
> 
> Ick, no.  You should use the hwmon interface, and standardize on a
> proper battery api just like those developers have standardized on other
> sensor apis that are exported to userspace.  

Er, yes. The whole point in this is so we can standardise on a proper
battery API. I'm only really supposed to be adding support for the
battery on the $100 laptop, but I just couldn't bring myself to do yet
another different battery driver without trying to bring in some sanity.

I sincerely hope that those responsible for the various other different
userspace interfaces for PMU, ACPI, etc. are all hanging their heads in
shame at this point :)

-- 
dwmw2

Re: Battery class driver.

[Dan Williams]

Adding Richard Hughes and David Zeuthen, since they will actually have
to talk to your batter class and driver.

On Mon, 2006-10-23 at 19:32 +0100, David Woodhouse wrote:
> At git://git.infradead.org/battery-2.6.git there is an initial
> implementation of a battery class, along with a driver which makes use
> of it. The patch is below, and also viewable at 
> http://git.infradead.org/?p=battery-2.6.git;a=commitdiff;h=master;hp=linus
> 
> I don't like the sysfs interaction much -- is it really necessary for me
> to provide a separate function for each attribute, rather than a single
> function which handles them all and is given the individual attribute as
> an argument? That seems strange and bloated.
> 
> I'm half tempted to ditch the sysfs attributes and just use a single
> seq_file, in fact.
> 
> The idea is that all batteries should be presented to userspace through
> this class instead of through the existing mess of PMU/APM/ACPI and even
> APM _emulation_.
> 
> I think I probably want to make AC power a separate 'device' too, rather
> than an attribute of any given battery. And when there are multiple
> power supplies, there should be multiple such devices. So maybe it
> should be a 'power supply' class, not a battery class at all?
> 
> Comments? 
> 
> 
> commit 42fe507a262b2a2879ca62740c5312778ae78627
> Author: David Woodhouse <dwmw2@infradead.org>
> Date:   Mon Oct 23 18:14:54 2006 +0100
> 
>     [BATTERY] Add support for OLPC battery
>     
>     Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> 
> commit 6cbec3b84e3ce737b4217788841ea10a28a5e340
> Author: David Woodhouse <dwmw2@infradead.org>
> Date:   Mon Oct 23 18:14:14 2006 +0100
> 
>     [BATTERY] Add initial implementation of battery class
>     
>     I really don't like the sysfs interaction, and I don't much like the
>     internal interaction with the battery drivers either. In fact, there
>     isn't much I _do_ like, but it's good enough as a straw man.
>     
>     Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> 
> --- drivers/Makefile~	2006-10-19 19:51:32.000000000 +0100
> +++ drivers/Makefile	2006-10-23 15:27:47.000000000 +0100
> @@ -30,6 +30,7 @@ obj-$(CONFIG_PARPORT)		+= parport/
>  obj-y				+= base/ block/ misc/ mfd/ net/ media/
>  obj-$(CONFIG_NUBUS)		+= nubus/
>  obj-$(CONFIG_ATM)		+= atm/
> +obj-$(CONFIG_BATTERY_CLASS)	+= battery/
>  obj-$(CONFIG_PPC_PMAC)		+= macintosh/
>  obj-$(CONFIG_XEN)		+= xen/
>  obj-$(CONFIG_IDE)		+= ide/
> --- drivers/Kconfig~	2006-09-20 04:42:06.000000000 +0100
> +++ drivers/Kconfig	2006-10-23 15:27:42.000000000 +0100
> @@ -28,6 +28,8 @@ source "drivers/ieee1394/Kconfig"
>  
>  source "drivers/message/i2o/Kconfig"
>  
> +source "drivers/battery/Kconfig"
> +
>  source "drivers/macintosh/Kconfig"
>  
>  source "drivers/net/Kconfig"
> --- /dev/null	2006-10-01 17:20:05.827666723 +0100
> +++ drivers/battery/Makefile	2006-10-23 16:53:20.000000000 +0100
> @@ -0,0 +1,4 @@
> +# Battery code
> +obj-$(CONFIG_BATTERY_CLASS)		+= battery-class.o
> +
> +obj-$(CONFIG_OLPC_BATTERY)		+= olpc-battery.o
> --- /dev/null	2006-10-01 17:20:05.827666723 +0100
> +++ drivers/battery/Kconfig	2006-10-23 16:52:42.000000000 +0100
> @@ -0,0 +1,22 @@
> +
> +menu "Battery support"
> +
> +config BATTERY_CLASS
> +       tristate "Battery support"
> +       help
> +         Say Y to enable battery class support. This allows a battery
> +	 information to be presented in a uniform manner for all types
> +	 of batteries.
> +
> +	 Battery information from APM and ACPI is not yet available by
> +	 this method, but should soon be. If you use APM or ACPI, say
> +	 'N', although saying 'Y' would be harmless.
> +
> +config OLPC_BATTERY
> +       tristate "One Laptop Per Child battery"
> +       depends on BATTERY_CLASS && X86_32
> +       help
> +         Say Y to enable support for the battery on the $100 laptop.
> +
> +
> +endmenu
> --- /dev/null	2006-10-01 17:20:05.827666723 +0100
> +++ drivers/battery/battery-class.c	2006-10-23 17:59:04.000000000 +0100
> @@ -0,0 +1,286 @@
> +/*
> + * Battery class core
> + *
> + *	© 2006 David Woodhouse <dwmw2@infradead.org>
> + *
> + * Based on LED Class support, by John Lenz and Richard Purdie:
> + *
> + *	© 2005 John Lenz <lenz@cs.wisc.edu>
> + *	© 2005-2006 Richard Purdie <rpurdie@openedhand.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/battery.h>
> +#include <linux/spinlock.h>
> +#include <linux/err.h>
> +
> +static struct class *battery_class;
> +
> +/* OMFG we can't just have a single 'show' routine which is given the
> +   'class_attribute' as an argument -- we have to have 20-odd copies
> +   of almost identical routines */
> +
> +static ssize_t battery_attribute_show_int(struct class_device *dev, char *buf, int attr)
> +{
> +        struct battery_classdev *battery_cdev = class_get_devdata(dev);
> +        ssize_t ret = 0;
> +	long value;
> +
> +	ret =  battery_cdev->query_long(battery_cdev, attr, &value);
> +	if (ret)
> +		return ret;
> +
> +	sprintf(buf, "%ld\n", value);
> +        ret = strlen(buf) + 1;
> +
> +        return ret;
> +}  
> +
> +static ssize_t battery_attribute_show_milli(struct class_device *dev, char *buf, int attr)
> +{
> +        struct battery_classdev *battery_cdev = class_get_devdata(dev);
> +        ssize_t ret = 0;
> +	long value;
> +
> +	ret = battery_cdev->query_long(battery_cdev, attr, &value);
> +	if (ret)
> +		return ret;
> +
> +	sprintf(buf, "%ld.%03ld\n", value/1000, value % 1000);
> +        ret = strlen(buf) + 1;
> +        return ret;
> +}  
> +
> +static ssize_t battery_attribute_show_string(struct class_device *dev, char *buf, int attr)
> +{
> +        struct battery_classdev *battery_cdev = class_get_devdata(dev);
> +        ssize_t ret = 0;
> +
> +	ret = battery_cdev->query_str(battery_cdev, attr, buf, PAGE_SIZE-1);
> +	if (ret)
> +		return ret;
> +
> +	strcat(buf, "\n");
> +        ret = strlen(buf) + 1;
> +        return ret;
> +}  
> +
> +static ssize_t battery_attribute_show_status(struct class_device *dev, char *buf)
> +{
> +        struct battery_classdev *battery_cdev = class_get_devdata(dev);
> +        ssize_t ret = 0;
> +	unsigned long status;
> +
> +	status = battery_cdev->status(battery_cdev, ~BAT_STAT_AC);
> +	if (status & BAT_STAT_ERROR)
> +		return -EIO;
> +
> +	if (status & BAT_STAT_PRESENT)
> +		sprintf(buf, "present");
> +	else
> +		sprintf(buf, "absent");
> +
> +	if (status & BAT_STAT_LOW)
> +		strcat(buf, ",low");
> +
> +	if (status & BAT_STAT_FULL)
> +		strcat(buf, ",full");
> +
> +	if (status & BAT_STAT_CHARGING)
> +		strcat(buf, ",charging");
> +
> +	if (status & BAT_STAT_DISCHARGING)
> +		strcat(buf, ",discharging");
> +
> +	if (status & BAT_STAT_OVERTEMP)
> +		strcat(buf, ",overtemp");
> +
> +	if (status & BAT_STAT_FIRE)
> +		strcat(buf, ",on-fire");
> +
> +	if (status & BAT_STAT_CHARGE_DONE)
> +		strcat(buf, ",charge-done");
> +
> +	strcat(buf, "\n");
> +        ret = strlen(buf) + 1;
> +        return ret;
> +}
> +
> +static ssize_t battery_attribute_show_ac_status(struct class_device *dev, char *buf)
> +{
> +        struct battery_classdev *battery_cdev = class_get_devdata(dev);
> +        ssize_t ret = 0;
> +	unsigned long status;
> +
> +	status = battery_cdev->status(battery_cdev, BAT_STAT_AC);
> +	if (status & BAT_STAT_ERROR)
> +		return -EIO;
> +
> +	if (status & BAT_STAT_AC)
> +		sprintf(buf, "on-line");
> +	else
> +		sprintf(buf, "off-line");
> +
> +	strcat(buf, "\n");
> +        ret = strlen(buf) + 1;
> +        return ret;
> +}  
> +
> +/* Ew. We can't even use CLASS_DEVICE_ATTR() if we want one named 'current' */
> +#define BATTERY_DEVICE_ATTR(_name, _attr, _type) \
> +static ssize_t battery_attr_show_##_attr(struct class_device *dev, char *buf)	\
> +{										\
> +	return battery_attribute_show_##_type(dev, buf, BAT_INFO_##_attr);	\
> +}										\
> +static struct class_device_attribute class_device_attr_##_attr  = {		\
> +        .attr = { .name = _name, .mode = 0444, .owner = THIS_MODULE },		\
> +	.show = battery_attr_show_##_attr };
> +
> +static CLASS_DEVICE_ATTR(status,0444,battery_attribute_show_status, NULL);
> +static CLASS_DEVICE_ATTR(ac,0444,battery_attribute_show_ac_status, NULL);
> +BATTERY_DEVICE_ATTR("temp1",TEMP1,milli);
> +BATTERY_DEVICE_ATTR("temp1_name",TEMP1_NAME,string);
> +BATTERY_DEVICE_ATTR("temp2",TEMP2,milli);
> +BATTERY_DEVICE_ATTR("temp2_name",TEMP2_NAME,string);
> +BATTERY_DEVICE_ATTR("voltage",VOLTAGE,milli);
> +BATTERY_DEVICE_ATTR("voltage_design",VOLTAGE_DESIGN,milli);
> +BATTERY_DEVICE_ATTR("current",CURRENT,milli);
> +BATTERY_DEVICE_ATTR("charge_rate",CHARGE_RATE,milli);
> +BATTERY_DEVICE_ATTR("charge_max",CHARGE_MAX,milli);
> +BATTERY_DEVICE_ATTR("charge_last",CHARGE_LAST,milli);
> +BATTERY_DEVICE_ATTR("charge_low",CHARGE_LOW,milli);
> +BATTERY_DEVICE_ATTR("charge_warn",CHARGE_WARN,milli);
> +BATTERY_DEVICE_ATTR("charge_unit",CHARGE_UNITS,string);
> +BATTERY_DEVICE_ATTR("charge_percent",CHARGE_PCT,int);
> +BATTERY_DEVICE_ATTR("time_remaining",TIME_REMAINING,int);
> +BATTERY_DEVICE_ATTR("manufacturer",MANUFACTURER,string);
> +BATTERY_DEVICE_ATTR("technology",TECHNOLOGY,string);
> +BATTERY_DEVICE_ATTR("model",MODEL,string);
> +BATTERY_DEVICE_ATTR("serial",SERIAL,string);
> +BATTERY_DEVICE_ATTR("type",TYPE,string);
> +BATTERY_DEVICE_ATTR("oem_info",OEM_INFO,string);
> +
> +#define REGISTER_ATTR(_attr)							\
> +	if (battery_cdev->capabilities & (1<<BAT_INFO_##_attr))			\
> +		class_device_create_file(battery_cdev->class_dev,		\
> +					 &class_device_attr_##_attr);
> +#define UNREGISTER_ATTR(_attr)							\
> +	if (battery_cdev->capabilities & (1<<BAT_INFO_##_attr))			\
> +		class_device_remove_file(battery_cdev->class_dev,		\
> +					 &class_device_attr_##_attr);
> +/**
> + * battery_classdev_register - register a new object of battery_classdev class.
> + * @dev: The device to register.
> + * @battery_cdev: the battery_classdev structure for this device.
> + */
> +int battery_classdev_register(struct device *parent, struct battery_classdev *battery_cdev)
> +{
> +        battery_cdev->class_dev = class_device_create(battery_class, NULL, 0,
> +						      parent, "%s", battery_cdev->name);
> +
> +        if (unlikely(IS_ERR(battery_cdev->class_dev)))
> +                return PTR_ERR(battery_cdev->class_dev);
> +
> +        class_set_devdata(battery_cdev->class_dev, battery_cdev);
> +
> +        /* register the attributes */
> +        class_device_create_file(battery_cdev->class_dev,
> +                                &class_device_attr_status);
> +
> +        if (battery_cdev->status_cap & (1<<BAT_STAT_AC))
> +		class_device_create_file(battery_cdev->class_dev, &class_device_attr_ac);
> +
> +	REGISTER_ATTR(TEMP1);
> +	REGISTER_ATTR(TEMP1_NAME);
> +	REGISTER_ATTR(TEMP2);
> +	REGISTER_ATTR(TEMP2_NAME);
> +	REGISTER_ATTR(VOLTAGE);
> +	REGISTER_ATTR(VOLTAGE_DESIGN);
> +	REGISTER_ATTR(CHARGE_PCT);
> +	REGISTER_ATTR(CURRENT);
> +	REGISTER_ATTR(CHARGE_RATE);
> +	REGISTER_ATTR(CHARGE_MAX);
> +	REGISTER_ATTR(CHARGE_LAST);
> +	REGISTER_ATTR(CHARGE_LOW);
> +	REGISTER_ATTR(CHARGE_WARN);
> +	REGISTER_ATTR(CHARGE_UNITS);
> +	REGISTER_ATTR(TIME_REMAINING);
> +	REGISTER_ATTR(MANUFACTURER);
> +	REGISTER_ATTR(TECHNOLOGY);
> +	REGISTER_ATTR(MODEL);
> +	REGISTER_ATTR(SERIAL);
> +	REGISTER_ATTR(TYPE);
> +	REGISTER_ATTR(OEM_INFO);
> +
> +        printk(KERN_INFO "Registered battery device: %s\n",
> +                        battery_cdev->class_dev->class_id);
> +
> +        return 0;
> +}
> +EXPORT_SYMBOL_GPL(battery_classdev_register);
> +
> +/**
> + * battery_classdev_unregister - unregisters a object of battery_properties class.
> + * @battery_cdev: the battery device to unreigister
> + *
> + * Unregisters a previously registered via battery_classdev_register object.
> + */
> +void battery_classdev_unregister(struct battery_classdev *battery_cdev)
> +{
> +        class_device_remove_file(battery_cdev->class_dev,
> +				 &class_device_attr_status);
> +
> +        if (battery_cdev->status_cap & (1<<BAT_STAT_AC))
> +		class_device_remove_file(battery_cdev->class_dev, &class_device_attr_ac);
> +
> +	UNREGISTER_ATTR(TEMP1);
> +	UNREGISTER_ATTR(TEMP1_NAME);
> +	UNREGISTER_ATTR(TEMP2);
> +	UNREGISTER_ATTR(TEMP2_NAME);
> +	UNREGISTER_ATTR(VOLTAGE);
> +	UNREGISTER_ATTR(VOLTAGE_DESIGN);
> +	UNREGISTER_ATTR(CHARGE_PCT);
> +	UNREGISTER_ATTR(CURRENT);
> +	UNREGISTER_ATTR(CHARGE_RATE);
> +	UNREGISTER_ATTR(CHARGE_MAX);
> +	UNREGISTER_ATTR(CHARGE_LAST);
> +	UNREGISTER_ATTR(CHARGE_LOW);
> +	UNREGISTER_ATTR(CHARGE_WARN);
> +	UNREGISTER_ATTR(CHARGE_UNITS);
> +	UNREGISTER_ATTR(TIME_REMAINING);
> +	UNREGISTER_ATTR(MANUFACTURER);
> +	UNREGISTER_ATTR(TECHNOLOGY);
> +	UNREGISTER_ATTR(MODEL);
> +	UNREGISTER_ATTR(SERIAL);
> +	UNREGISTER_ATTR(TYPE);
> +	UNREGISTER_ATTR(OEM_INFO);
> +
> +        class_device_unregister(battery_cdev->class_dev);
> +}
> +EXPORT_SYMBOL_GPL(battery_classdev_unregister);
> +
> +static int __init battery_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_exit(void)
> +{
> +        class_destroy(battery_class);
> +}
> +
> +subsys_initcall(battery_init);
> +module_exit(battery_exit);
> +
> +MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Battery class interface");
> --- /dev/null	2006-10-01 17:20:05.827666723 +0100
> +++ drivers/battery/olpc-battery.c	2006-10-23 17:56:38.000000000 +0100
> @@ -0,0 +1,198 @@
> +/*
> + * Battery driver for One Laptop Per Child ($100 laptop) board.
> + *
> + *	© 2006 David Woodhouse <dwmw2@infradead.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/battery.h>
> +#include <linux/spinlock.h>
> +#include <linux/err.h>
> +#include <asm/io.h>
> +
> +#define wBAT_VOLTAGE		0xf900 /* *9.76/32,    mV */
> +#define wBAT_CURRENT		0xf902 /* *15.625/120, mA */
> +#define wBAT_TEMP		0xf906 /* *256/1000,   °C */
> +#define wAMB_TEMP		0xf908 /* *256/1000,   °C */
> +#define SOC			0xf910 /*      percentage */
> +#define sMBAT_STATUS		0xfaa4
> +#define		sBAT_PRESENT		1
> +#define		sBAT_FULL		2
> +#define		sBAT_DESTROY		4
> +#define		sBAT_LOW			32
> +#define		sBAT_DISCHG		64
> +#define sMCHARGE_STATUS		0xfaa5
> +#define		sBAT_CHARGE		1
> +#define		sBAT_OVERTEMP		4
> +#define		sBAT_NiMH		8
> +#define sPOWER_FLAG		0xfa40
> +#define		ADAPTER_IN		1
> +
> +static int lock_ec(void)
> +{
> +	unsigned long timeo = jiffies + HZ/20;
> +
> +	while (1) {
> +                unsigned char lock = inb(0x6c) & 0x80;
> +                if (!lock)
> +                        return 0;
> +		if (time_after(jiffies, timeo))
> +			return 1;
> +                yield();
> +        }
> +}
> +
> +static void unlock_ec(void)
> +{
> +        outb(0xff, 0x6c);
> +}
> +
> +unsigned char read_ec_byte(unsigned short adr)
> +{
> +        outb(adr >> 8, 0x381);
> +        outb(adr, 0x382);
> +        return inb(0x383);
> +}
> +
> +unsigned short read_ec_word(unsigned short adr)
> +{
> +        return (read_ec_byte(adr) << 8) | read_ec_byte(adr+1);
> +}
> +
> +unsigned long olpc_bat_status(struct battery_classdev *cdev, unsigned long mask)
> +{
> +	unsigned long result = 0;
> +	unsigned short tmp;
> +
> +	if (lock_ec()) {
> +		printk(KERN_ERR "Failed to lock EC for battery access\n");
> +		return BAT_STAT_ERROR;
> +	}
> +
> +	if (mask & BAT_STAT_AC) {
> +		if (read_ec_byte(sPOWER_FLAG) & ADAPTER_IN)
> +			result |= BAT_STAT_AC;
> +	}
> +	if (mask & (BAT_STAT_PRESENT|BAT_STAT_FULL|BAT_STAT_FIRE|BAT_STAT_LOW|BAT_STAT_DISCHARGING)) {
> +		tmp = read_ec_byte(sMBAT_STATUS);
> +		
> +		if (tmp & sBAT_PRESENT)
> +			result |= BAT_STAT_PRESENT;
> +		if (tmp & sBAT_FULL)
> +			result |= BAT_STAT_FULL;
> +		if (tmp & sBAT_DESTROY)
> +			result |= BAT_STAT_FIRE;
> +		if (tmp & sBAT_LOW)
> +			result |= BAT_STAT_LOW;
> +		if (tmp & sBAT_DISCHG)
> +			result |= BAT_STAT_DISCHARGING;
> +	}
> +	if (mask & (BAT_STAT_CHARGING|BAT_STAT_OVERTEMP)) {
> +		tmp = read_ec_byte(sMCHARGE_STATUS);
> +		if (tmp & sBAT_CHARGE)
> +			result |= BAT_STAT_CHARGING;
> +		if (tmp & sBAT_OVERTEMP)
> +			result |= BAT_STAT_OVERTEMP;
> +	}
> +	unlock_ec();
> +	return result;
> +}
> +
> +int olpc_bat_query_long(struct battery_classdev *dev, int attr, long *result)
> +{
> +	int ret = 0;
> +
> +	if (lock_ec())
> +		return -EIO;
> +
> +	if (!(read_ec_byte(sMBAT_STATUS) & sBAT_PRESENT)) {
> +		ret = -ENODEV;
> +	} else if (attr == BAT_INFO_VOLTAGE) {
> +		*result = read_ec_word(wBAT_VOLTAGE) * 9760 / 32000;
> +	} else if (attr == BAT_INFO_CURRENT) {
> +		*result = read_ec_word(wBAT_CURRENT) * 15625 / 120000;
> +	} else if (attr == BAT_INFO_TEMP1) {
> +		*result = read_ec_word(wBAT_TEMP) * 1000 / 256;
> +	} else if (attr == BAT_INFO_TEMP2) {
> +		*result = read_ec_word(wAMB_TEMP) * 1000 / 256;
> +	} else if (attr == BAT_INFO_CHARGE_PCT) {
> +		*result = read_ec_byte(SOC);
> +	} else 
> +		ret = -EINVAL;
> +
> +	unlock_ec();
> +	return ret;
> +}
> +
> +int olpc_bat_query_str(struct battery_classdev *dev, int attr, char *str, int len)
> +{
> +	int ret = 0;
> +
> +	if (attr == BAT_INFO_TYPE) {
> +		snprintf(str, len, "OLPC");
> +	} else if (attr == BAT_INFO_TEMP1_NAME) {
> +		snprintf(str, len, "battery");
> +	} else if (attr == BAT_INFO_TEMP2_NAME) {
> +		snprintf(str, len, "ambient");
> +	} else if (!(read_ec_byte(sMBAT_STATUS) & sBAT_PRESENT)) {
> +		ret = -ENODEV;
> +	} else if (attr == BAT_INFO_TECHNOLOGY) {
> +		if (lock_ec())
> +			ret = -EIO;
> +		else {
> +			unsigned short tmp = read_ec_byte(sMCHARGE_STATUS);
> +			if (tmp & sBAT_NiMH)
> +				snprintf(str, len, "NiMH");
> +			else
> +				snprintf(str, len, "unknown");
> +		}
> +		unlock_ec();
> +	} else {
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static struct battery_classdev olpc_bat = {
> +	.name = "OLPC",
> +	.capabilities = (1<<BAT_INFO_VOLTAGE) |
> +			(1<<BAT_INFO_CURRENT) |
> +			(1<<BAT_INFO_TEMP1) |
> +			(1<<BAT_INFO_TEMP2) |
> +			(1<<BAT_INFO_CHARGE_PCT) |
> +			(1<<BAT_INFO_TYPE) |
> +			(1<<BAT_INFO_TECHNOLOGY) |
> +			(1<<BAT_INFO_TEMP1_NAME) |
> +			(1<<BAT_INFO_TEMP2_NAME),
> +	.status_cap = BAT_STAT_AC | BAT_STAT_PRESENT | BAT_STAT_LOW | 
> +		      BAT_STAT_FULL | BAT_STAT_CHARGING| BAT_STAT_DISCHARGING |
> +		      BAT_STAT_OVERTEMP | BAT_STAT_FIRE,
> +	.status = olpc_bat_status,
> +	.query_long = olpc_bat_query_long,
> +	.query_str = olpc_bat_query_str,
> +};
> +
> +void __exit olpc_bat_exit(void)
> +{
> +	battery_classdev_unregister(&olpc_bat);
> +}
> +
> +int __init olpc_bat_init(void)
> +{
> +	battery_classdev_register(NULL, &olpc_bat);
> +	return 0;
> +}
> +
> +module_init(olpc_bat_init);
> +module_exit(olpc_bat_exit);
> +
> +MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Battery class interface");
> --- /dev/null	2006-10-01 17:20:05.827666723 +0100
> +++ include/linux/battery.h	2006-10-23 17:11:28.000000000 +0100
> @@ -0,0 +1,84 @@
> +/*
> + * Driver model for batteries
> + *
> + *	© 2006 David Woodhouse <dwmw2@infradead.org>
> + *
> + * Based on LED Class support, by John Lenz and Richard Purdie:
> + *
> + *	© 2005 John Lenz <lenz@cs.wisc.edu>
> + *	© 2005-2006 Richard Purdie <rpurdie@openedhand.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#ifndef __LINUX_BATTERY_H__
> +#define __LINUX_BATTERY_H__
> +
> +struct device;
> +struct class_device;
> +
> +/*
> + * Battery Core
> + */
> +#define BAT_STAT_AC		(1<<0)
> +#define BAT_STAT_PRESENT	(1<<1)
> +#define BAT_STAT_LOW		(1<<2)
> +#define BAT_STAT_FULL		(1<<3)
> +#define BAT_STAT_CHARGING	(1<<4)
> +#define BAT_STAT_DISCHARGING	(1<<5)
> +#define BAT_STAT_OVERTEMP	(1<<6)
> +#define BAT_STAT_FIRE		(1<<7)
> +#define BAT_STAT_CHARGE_DONE	(1<<8)
> +
> +#define BAT_STAT_ERROR		(1<<31)
> +
> +#define BAT_INFO_TEMP1		(0) /* °C/1000 */
> +#define BAT_INFO_TEMP1_NAME	(1) /* string */
> +
> +#define BAT_INFO_TEMP2		(2) /* °C/1000 */
> +#define BAT_INFO_TEMP2_NAME	(3) /* string */
> +
> +#define BAT_INFO_VOLTAGE	(4) /* mV */
> +#define BAT_INFO_VOLTAGE_DESIGN	(5) /* mV */
> +
> +#define BAT_INFO_CURRENT	(6) /* mA */
> +
> +#define BAT_INFO_CHARGE_RATE	(7) /* BAT_INFO_CHARGE_UNITS */
> +#define BAT_INFO_CHARGE		(8) /* BAT_INFO_CHARGE_UNITS */
> +#define BAT_INFO_CHARGE_MAX	(9) /* BAT_INFO_CHARGE_UNITS  */
> +#define BAT_INFO_CHARGE_LAST	(10) /* BAT_INFO_CHARGE_UNITS  */
> +#define BAT_INFO_CHARGE_LOW	(11) /* BAT_INFO_CHARGE_UNITS  */
> +#define BAT_INFO_CHARGE_WARN	(12) /* BAT_INFO_CHARGE_UNITS  */
> +#define BAT_INFO_CHARGE_UNITS	(13) /* string */
> +#define BAT_INFO_CHARGE_PCT	(14) /* % */
> +
> +#define BAT_INFO_TIME_REMAINING	(15) /* seconds */
> +
> +#define BAT_INFO_MANUFACTURER	(16) /* string */
> +#define BAT_INFO_TECHNOLOGY	(17) /* string */
> +#define BAT_INFO_MODEL		(18) /* string */
> +#define BAT_INFO_SERIAL		(19) /* string */
> +#define BAT_INFO_TYPE		(20) /* string */
> +#define BAT_INFO_OEM_INFO	(21) /* string */
> +
> +struct battery_classdev {
> +	const char		*name;
> +	/* Capabilities of this battery driver */
> +	unsigned long		 capabilities, status_cap;
> +
> +	/* Query functions */
> +	unsigned long		(*status)(struct battery_classdev *, unsigned long mask);
> +	int			(*query_long)(struct battery_classdev *, int attr, long *result);
> +	int			(*query_str)(struct battery_classdev *, int attr, char *str, ssize_t len);
> +
> +	struct class_device	*class_dev;
> +	struct list_head	 node;	/* Battery Device list */
> +};
> +
> +extern int battery_classdev_register(struct device *parent,
> +				     struct battery_classdev *battery_cdev);
> +extern void battery_classdev_unregister(struct battery_classdev *battery_cdev);
> +
> +#endif /* __LINUX_BATTERY_H__ */
> 
> 
> -- 
> dwmw2
> _______________________________________________
> Devel mailing list
> Devel@laptop.org
> http://mailman.laptop.org/mailman/listinfo/devel

Re: Battery class driver.

[Richard Hughes]

On Mon, 2006-10-23 at 15:18 -0400, Dan Williams wrote:
> Adding Richard Hughes and David Zeuthen, since they will actually have
> to talk to your batter class and driver.

Cool, thanks.

> On Mon, 2006-10-23 at 19:32 +0100, David Woodhouse wrote:
> > At git://git.infradead.org/battery-2.6.git there is an initial
> > implementation of a battery class, along with a driver which makes use
> > of it. The patch is below, and also viewable at 
> > http://git.infradead.org/?p=battery-2.6.git;a=commitdiff;h=master;hp=linus
> > 
> > I don't like the sysfs interaction much -- is it really necessary for me
> > to provide a separate function for each attribute, rather than a single
> > function which handles them all and is given the individual attribute as
> > an argument? That seems strange and bloated.
> > 
> > I'm half tempted to ditch the sysfs attributes and just use a single
> > seq_file, in fact.

I can't comment on the kernel implementation side of it much, but see
below.

> > The idea is that all batteries should be presented to userspace through
> > this class instead of through the existing mess of PMU/APM/ACPI and even
> > APM _emulation_.

Ohh yes. This would make the battery code in HAL so much better. There
is so much legacy crud for batteries that we have to wade through in
HAL. A battery kernel class is a very good idea IMO.

> > I think I probably want to make AC power a separate 'device' too, rather
> > than an attribute of any given battery. And when there are multiple
> > power supplies, there should be multiple such devices. So maybe it
> > should be a 'power supply' class, not a battery class at all?

No, I think the distinction between batteries and ac_adapter is large
enough to have different classes of devices. You may have many
batteries, but you'll only ever have one ac_adapter. I'm not sure it's
an obvious abstraction to make.

> > Comments? 

How are battery change notifications delivered to userspace? I know acpi
is using the input layer for buttons in the future (very sane IMO), so
using sysfs events for each property changing would probably be nice.

Comments on your patch:

> +#define BAT_INFO_TEMP2		(2) /* °C/1000 */
Temperature expressed in degrees C/1000? - what if the temperature goes
below 0? What about just using mK (kelvin / 1000) - I don't know what is
used in the kernel elsewhere tho. Also, are you allowed the ° sign in
kernel source now?

> +#define BAT_INFO_CURRENT	(6) /* mA */
Can't this also be expressed in mW according to the ACPI spec?

> +#define BAT_STAT_FIRE		(1<<7)
I know there is precedent for "FIRE" but maybe CRITICAL or DANGER might
be better chosen words. We can reserve the word FIRE for when the faulty
battery really is going to explode...

Richard.

> > commit 42fe507a262b2a2879ca62740c5312778ae78627
> > Author: David Woodhouse <dwmw2@infradead.org>
> > Date:   Mon Oct 23 18:14:54 2006 +0100
> > 
> >     [BATTERY] Add support for OLPC battery
> >     
> >     Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> > 
> > commit 6cbec3b84e3ce737b4217788841ea10a28a5e340
> > Author: David Woodhouse <dwmw2@infradead.org>
> > Date:   Mon Oct 23 18:14:14 2006 +0100
> > 
> >     [BATTERY] Add initial implementation of battery class
> >     
> >     I really don't like the sysfs interaction, and I don't much like the
> >     internal interaction with the battery drivers either. In fact, there
> >     isn't much I _do_ like, but it's good enough as a straw man.
> >     
> >     Signed-off-by: David Woodhouse <dwmw2@infradead.org>

Re: Battery class driver.

[Roland Dreier]

 > No, I think the distinction between batteries and ac_adapter is large
 > enough to have different classes of devices. You may have many
 > batteries, but you'll only ever have one ac_adapter. I'm not sure it's
 > an obvious abstraction to make.

Speaking from ignorance here, but what about (big) systems that have
multiple power supplies and multiple rails of AC power?  Would it make
sense to use the same abstraction for that as well as laptop AC adaptors?

Re: Battery class driver.

[David Woodhouse]

On Mon, 2006-10-23 at 20:58 +0100, Richard Hughes wrote:
> No, I think the distinction between batteries and ac_adapter is large
> enough to have different classes of devices. You may have many
> batteries, but you'll only ever have one ac_adapter. I'm not sure it's
> an obvious abstraction to make.

∃ machines with more than one individual AC adapter. Which want
individual notification when one of them goes down.

You're right that they don't necessarily fit into the 'battery' class,
but I'm not entirely sure if it's worth putting them elsewhere, because
the information about them is usually available in the same place as the
information about the batteries, at least in the laptop case.

The simple case of AC adapters, where we have only 'present' or
'absent', is a subset of what batteries can do. If you have more
complicated monitoring, then it's _also_ going to bear a remarkable
similarity to what you get from batteries -- you'll be able to monitor
temperatures, voltage, current, etc. So they're not _that_ much out of
place in a 'power supply' class.

It makes _less_ sense, imho, to have 'ac present' as a property of a
battery -- which is what I've done for now.

> How are battery change notifications delivered to userspace? I know acpi
> is using the input layer for buttons in the future (very sane IMO), so
> using sysfs events for each property changing would probably be nice.

For selected properties, yes. I wouldn't want it happening every time
the current draw changes by a millivolt but for 'battery removed' or 'ac
power applied' events it makes some sense.

For sane hardware where we get an interrupt on such changes, that's fine
-- but I'm wary of having to implement that by making the kernel poll
for them; especially if/when there's nothing in userspace which cares. 

> Comments on your patch:
> 
> > +#define BAT_INFO_TEMP2		(2) /* °C/1000 */
> Temperature expressed in degrees C/1000? - what if the temperature goes
> below 0? 

It's signed.

> What about just using mK (kelvin / 1000) - I don't know what is
> used in the kernel elsewhere tho. Also, are you allowed the ° sign in
> kernel source now?

Welcome to the 21st century.

> > +#define BAT_INFO_CURRENT	(6) /* mA */
> Can't this also be expressed in mW according to the ACPI spec?

No, it can't. The Watt is not a unit of current.

I intended the ACPI 'present rate' to map to the 'charge_rate' property,
which is why we have the 'charge_unit' property. I don't like that much,
but it seems necessary unless we're going to do something like separate
'charge_rate_mA' and 'charge_rate_mW' properties.

Actually, I suspect that on reflection I would prefer that latter
option. DavidZ?

> > +#define BAT_STAT_FIRE		(1<<7)
> I know there is precedent for "FIRE" but maybe CRITICAL or DANGER might
> be better chosen words. We can reserve the word FIRE for when the faulty
> battery really is going to explode...

Yes, feasibly. I don't quite know what the 'destroy' bit in the OLPC
embedded controller is supposed to mean, and 'FIRE' seemed as good as
anything else.

-- 
dwmw2

Re: Battery class driver.

[Jean Delvare]

On Mon, 23 Oct 2006 11:30:48 -0700, Greg KH wrote:
> On Mon, Oct 23, 2006 at 07:20:33PM +0100, David Woodhouse wrote:
> > At git://git.infradead.org/battery-2.6.git there is an initial
> > implementation of a battery class, along with a driver which makes use
> > of it. The patch is below, and also viewable at 
> > http://git.infradead.org/?p=battery-2.6.git;a=commitdiff;h=master;hp=linus
> > 
> > I don't like the sysfs interaction much -- is it really necessary for me
> > to provide a separate function for each attribute, rather than a single
> > function which handles them all and is given the individual attribute as
> > an argument? That seems strange and bloated.
> 
> It is, but no one has asked for it to be changed to be like the struct
> device attributes are.  In fact, why not just use the struct device
> attributes here instead?  That will be much easier and keep me from
> having to convert your code over to use it in the future :)
> 
> > I'm half tempted to ditch the sysfs attributes and just use a single
> > seq_file, in fact.
> 
> Ick, no.  You should use the hwmon interface, and standardize on a
> proper battery api just like those developers have standardized on other
> sensor apis that are exported to userspace.  Take a look at
> Documentation/hwmon/sysfs-interface for an example of what it should
> look like.
> 
> > The idea is that all batteries should be presented to userspace through
> > this class instead of through the existing mess of PMU/APM/ACPI and even
> > APM _emulation_.
> 
> Yes, I agree this should be done in this manner.
> 
> > I think I probably want to make AC power a separate 'device' too, rather
> > than an attribute of any given battery. And when there are multiple
> > power supplies, there should be multiple such devices. So maybe it
> > should be a 'power supply' class, not a battery class at all?
> 
> That sounds good to me.
> 
> Jean, I know you had some ideas with regards to this in the past.

Did I? I don't remember 8]

Anyway I don't have much to add over what you said. The hwmon interface
proved to be good, so the battery interface should look the same. Sysfs
files, one integer value per file, using standard names and units, with
"small units" (mV, mW etc...) so that you have enough resolution in all
present and future cases.

-- 
Jean Delvare

Re: Battery class driver.

[David Zeuthen]

Hi,

On Mon, 2006-10-23 at 19:20 +0100, David Woodhouse wrote:
> The idea is that all batteries should be presented to userspace through
> this class instead of through the existing mess of PMU/APM/ACPI and even
> APM _emulation_.

Yea, that would be nice, we've got code in HAL to handle all these three
(and more) and provide one coherent interface. The battery class
abstraction should probably be flexible enough to handle things such as

 - UPS's
 - Bluetooth devices with batteries
 - USB wireless mice / keyboards
 - ... and so on.

if someone wants to write a kernel-space driver for these some day. We
handle the latter in HAL using some user space code and to me it's still
an open question (and not really a very interesting one) whether you
want the code kernel- or user-side.

As an aside, you probably want some kind of device symlink for the
"battery class" instance in sysfs such that it points to the "physical"
device. For ACPI/PMU/APM etc. I guess it points to somewhere
in /sys/devices/platform; for a Bluetooth device it might point to the
Bluetooth device in sysfs (cf. Marcel's patch to do this) and so on.
Otherwise it's hard for user space to figure out what the battery is
used for. Perhaps the driver itself can contribute with some kind of
sysfs file 'usage' that can assume values "laptop_battery", "ups" etc.,
dunno if that's a good idea.

I think that it requires some degree of documentation for the files you
export including docs on the ranges. Probably worth sticking to *some*
convention such as if you can't provide a given value then the file
simply isn't there instead of just providing some magic value like
charge=0 if you don't know the charge. That probably means you need some
kind of 'sentinel' for each value that will make the generic battery
class code omit presenting the file. 

(I'm not sure whether there are any general sysfs guidelines on this so
forgive me if there is already.)

Please also avoid putting more than on value in a single file, e.g. I
think we want to avoid e.g. this

 # cat /sys/class/battery/OLPC/state
 present,low,charging

and rather have is_present, is_charging, is_discharging and so on; i.e.
one value per file even if we're talking about flags. It's just less
messy this way.

> I think I probably want to make AC power a separate 'device' too, rather
> than an attribute of any given battery. And when there are multiple
> power supplies, there should be multiple such devices. So maybe it
> should be a 'power supply' class, not a battery class at all?

So I happen to think they are different beasts. Some batteries may not
be rechargable by the host / device (think USB wireless mice with normal
AAA batteries) and some power supply units may not have a battery (think
big iron with multiple PSU's). I think, in general, they have different
characteristics (they share some though) so perhaps it would be good to
have different abstractions? I'm not a domain expert here though.

How do we plan to get updates to user space? The ACPI code today
provides updates via the ACPI socket but that is broken on some hardware
so essentially HAL polls by reading /proc/acpi/battery/BAT0/state some
every 30 secs on, and, on some boxen, that generates a SMBIOS trap or
some other expensive operation. That's wrong.

So I think the generic battery class code should cache everything which
has the nice side effect that any stupid user space app doesn't bring
the system to it's knees just because it's re-reading the same file over
and over again. 

So, perhaps the battery class should provide a file called 'timestamp'
or something that is only writable by the super user. If you read from
that file it gives the time when the information was last updated. If
you write to the file it will force the driver query the hardware and
update the other files. Reading any other file than 'timestamp' will
just read cached information. 

The mechanism to notify user space that something have been updated
would be either to make the timestamp file pollable or use an uevent or
something else (no input drivers please). 

If the hardware is able to generate an interrupt when certain data on
the battery has changed the driver simply updates the timestamp file.

With this scheme, user space simply does this

 10 poll on /sys/class/battery/BAT0/timestamp with timeout=30sec
 20 if timeout, write to 'timestamp' file to force polling
 30 read values from sysfs and update graphics for battery icon etc.
 40 goto 10

and we only poll when the hardware don't provide such interrupts /
hardware is broken so it don't provide interrupts.

Specifically, user space can decide to make the timeout infinite or
decide not to poll under certain conditions etc. etc.

How about that?

     David

Re: Battery class driver.

[Greg KH]

On Mon, Oct 23, 2006 at 06:15:03PM -0400, David Zeuthen wrote:
> 
> Hi,
> 
> On Mon, 2006-10-23 at 19:20 +0100, David Woodhouse wrote:
> > The idea is that all batteries should be presented to userspace through
> > this class instead of through the existing mess of PMU/APM/ACPI and even
> > APM _emulation_.
> 
> Yea, that would be nice, we've got code in HAL to handle all these three
> (and more) and provide one coherent interface. The battery class
> abstraction should probably be flexible enough to handle things such as
> 
>  - UPS's
>  - Bluetooth devices with batteries
>  - USB wireless mice / keyboards
>  - ... and so on.
> 
> if someone wants to write a kernel-space driver for these some day. We
> handle the latter in HAL using some user space code and to me it's still
> an open question (and not really a very interesting one) whether you
> want the code kernel- or user-side.
> 
> As an aside, you probably want some kind of device symlink for the
> "battery class" instance in sysfs such that it points to the "physical"
> device. For ACPI/PMU/APM etc. I guess it points to somewhere
> in /sys/devices/platform; for a Bluetooth device it might point to the
> Bluetooth device in sysfs (cf. Marcel's patch to do this) and so on.
> Otherwise it's hard for user space to figure out what the battery is
> used for. Perhaps the driver itself can contribute with some kind of
> sysfs file 'usage' that can assume values "laptop_battery", "ups" etc.,
> dunno if that's a good idea.

By using a real device for the battery, you will get this for free,
along with a symlink back from the sysfs class if you so desire.  I know
HAL can handle this properly, as we are doing this in the next opensuse
release (10.2) for almost all class devices.

Actually, using the hwmon class is probably the best, as you are doing
much the same thing.  I don't think a new class is probably needed here.

> I think that it requires some degree of documentation for the files you
> export including docs on the ranges. Probably worth sticking to *some*
> convention such as if you can't provide a given value then the file
> simply isn't there instead of just providing some magic value like
> charge=0 if you don't know the charge. That probably means you need some
> kind of 'sentinel' for each value that will make the generic battery
> class code omit presenting the file. 

Documentation/hwmon/sysfs-interface is the proper format for documenting
this to start with.  Documentation/ABI is probably the best place for it
to end up in eventually.

> (I'm not sure whether there are any general sysfs guidelines on this so
> forgive me if there is already.)

Yes, see above :)

> Please also avoid putting more than on value in a single file, e.g. I
> think we want to avoid e.g. this
> 
>  # cat /sys/class/battery/OLPC/state
>  present,low,charging
> 
> and rather have is_present, is_charging, is_discharging and so on; i.e.
> one value per file even if we're talking about flags. It's just less
> messy this way.

Agreed.

> > I think I probably want to make AC power a separate 'device' too, rather
> > than an attribute of any given battery. And when there are multiple
> > power supplies, there should be multiple such devices. So maybe it
> > should be a 'power supply' class, not a battery class at all?
> 
> So I happen to think they are different beasts. Some batteries may not
> be rechargable by the host / device (think USB wireless mice with normal
> AAA batteries) and some power supply units may not have a battery (think
> big iron with multiple PSU's). I think, in general, they have different
> characteristics (they share some though) so perhaps it would be good to
> have different abstractions? I'm not a domain expert here though.
> 
> How do we plan to get updates to user space? The ACPI code today
> provides updates via the ACPI socket but that is broken on some hardware
> so essentially HAL polls by reading /proc/acpi/battery/BAT0/state some
> every 30 secs on, and, on some boxen, that generates a SMBIOS trap or
> some other expensive operation. That's wrong.
> 
> So I think the generic battery class code should cache everything which
> has the nice side effect that any stupid user space app doesn't bring
> the system to it's knees just because it's re-reading the same file over
> and over again. 
> 
> So, perhaps the battery class should provide a file called 'timestamp'
> or something that is only writable by the super user. If you read from
> that file it gives the time when the information was last updated. If
> you write to the file it will force the driver query the hardware and
> update the other files. Reading any other file than 'timestamp' will
> just read cached information. 

You can poll the sysfs file, which means you just sleep until something
changes in it and then you wake up and read it.  Sound accepatble?

thanks,

greg k-h

Re: Battery class driver.

[David Zeuthen]

On Mon, 2006-10-23 at 15:59 -0700, Greg KH wrote:
> > So, perhaps the battery class should provide a file called 'timestamp'
> > or something that is only writable by the super user. If you read from
> > that file it gives the time when the information was last updated. If
> > you write to the file it will force the driver query the hardware and
> > update the other files. Reading any other file than 'timestamp' will
> > just read cached information. 
> 
> You can poll the sysfs file, which means you just sleep until something
> changes in it and then you wake up and read it.  Sound accepatble?

Yea, however I still think there needs to be a 'timestamp' file so 

 a) we don't need to poll every file in /sys/class/battery/BAT0
   (if we had to do that, we would (potentially) wake up N times!); and 

 b) if the kernel ensures that the 'timestamp' file is updated last,
    we get atomic updates (which might matter for drawing pretty graphs,
    guestimating remaining time etc.); and

 c) it provides some mechanism to instruct the driver to go read the
    values from the hardware (if that is what user space wants)
    nonwithstanding that the hardware / driver delivers asynchronous
    updates once in a while via an IRQ.

    Notably user space can see _when_ the values from the hardware was
    retrieved the last time which makes it easier to work around with
    hardware / drivers that don't provide asynchronous updates even
    when they are supposed to (if there is some bug with e.g. the ACPI
    stack).

This implies that the battery class should probably cache the values as
to make the platform driver as simple as possible. I've got a bad
feeling things like caching is badly frowned upon in kernel space but I
thought I'd ask for it anyway :-). I hope I've stated my case :-)

(Anyway, the point is that I want to avoid having an open file
descriptor for each and every file in /sys/class/battery/BAT0 to put it
into the huge poll() stmt in my main loop (granted with things like glib
it's dead simple), that's all.. Caching might also avoid excessive round
trips to the hardware but one can argue both way in most cases.)

So.. how all this relates to hwmon I'm not sure.. looking briefly at
Documentation/hwmon/sysfs-interface no such thing seems to be available,
I'm not sure how libsensors get by here but the problem is sorta
similar. I do think batteries in itself deserves it's own abstraction
instead of using hwmon but I'm no expert on this.

Btw, an OLPC specific feature is also to instruct the Embedded
Controller (EC) to stop delivering IRQ's on battery/ac_adapter changes.
This is so the host CPU won't be woken up when e.g. in e-book reader
mode (or whatever) where the host CPU is supposed to be turned off to
save juice. 
This is simple right now as the EC currently doesn't deliver any IRQ's
at all [1] and I guess a simple sysfs file in the OLPC platform device
will let us do that once we actually get IRQ delivery. Thus, I'm not
sure "stop IRQ delivery" belongs in the battery class proper. This is
also why we need device link to the OLPC platform device.

     David

[1] : but see http://dev.laptop.org/ticket/224

Re: Battery class driver.

[Shem Multinymous]

Hi David,

On 2006-10-23, David Woodhouse wrote:
> At git://git.infradead.org/battery-2.6.git there is an initial
> implementation of a battery class

Excellent. It's about time the messy, inconsistent /proc battery ABI
gets cleaned up.


> I think I probably want to make AC power a separate 'device' too, rather
> than an attribute of any given battery. And when there are multiple
> power supplies, there should be multiple such devices. So maybe it
> should be a 'power supply' class, not a battery class at all?

It should be seprate, and probably a different class since most
attributes don't make sense for AC.


> +static ssize_t battery_attribute_show_int(struct class_device *dev, char *buf, int attr)
> +{
> +        struct battery_classdev *battery_cdev = class_get_devdata(dev);
> +        ssize_t ret = 0;
> +	long value;

There's some tab-vs-space noise here and elsewhere.


> +static ssize_t battery_attribute_show_milli(struct class_device *dev, char *buf, int attr)
> +{
> +        struct battery_classdev *battery_cdev = class_get_devdata(dev);
> +        ssize_t ret = 0;
> +	long value;
> +
> +	ret = battery_cdev->query_long(battery_cdev, attr, &value);
> +	if (ret)
> +		return ret;
> +
> +	sprintf(buf, "%ld.%03ld\n", value/1000, value % 1000);
> +        ret = strlen(buf) + 1;
> +        return ret;
> +}

The sysfs spec (Documentation/hwmon/sysfs-interface) prescribes milli
as integer, not simulated floating point ("All sysfs values are fixed
point numbers"). Better follow that.


> +static ssize_t battery_attribute_show_status(struct class_device *dev, char *buf)
> +{
> +        struct battery_classdev *battery_cdev = class_get_devdata(dev);
> +        ssize_t ret = 0;
> +	unsigned long status;
> +
> +	status = battery_cdev->status(battery_cdev, ~BAT_STAT_AC);
> +	if (status & BAT_STAT_ERROR)
> +		return -EIO;
> +
> +	if (status & BAT_STAT_PRESENT)
> +		sprintf(buf, "present");
> +	else
> +		sprintf(buf, "absent");
> +
> +	if (status & BAT_STAT_LOW)
> +		strcat(buf, ",low");

I suggest a more sysfs-ish approach:

"present" attribute, containing 0 or 1.
"state" attribute, containing one of "charging","discharging" and "idle".
"low" attribute, containing 0 or 1.
Et cetera.


> +BATTERY_DEVICE_ATTR("temp1",TEMP1,milli);
> +BATTERY_DEVICE_ATTR("temp1_name",TEMP1_NAME,string);
> +BATTERY_DEVICE_ATTR("temp2",TEMP2,milli);
> +BATTERY_DEVICE_ATTR("temp2_name",TEMP2_NAME,string);
> +BATTERY_DEVICE_ATTR("voltage",VOLTAGE,milli);
> +BATTERY_DEVICE_ATTR("voltage_design",VOLTAGE_DESIGN,milli);
> +BATTERY_DEVICE_ATTR("current",CURRENT,milli);

The Smart Battery System spec
(http://www.sbs-forum.org/specs/sbdat110.pdf) defines two current
readouts: "Current()" for instantaneous value, and "AverageCurrent()"
for a one-minute rolling average.

On ThinkPads, the value reported by ACPI is actually AverageCurrent().
Both values can be read by accessing the embedded controller (the
tp_smapi out-of-tree driver does this). Both values are useful and can
differ considerably.

I suggest having both "current_now" and "current_avg", as done in tp_smapi.


> +BATTERY_DEVICE_ATTR("charge_rate",CHARGE_RATE,milli);

There's a terminology issue here. "Charge" is problematic for two reasons:
First, it's inaccurate: many batteries report not electric charge
(coulomb) but energy (watt-hour), and there are probably devices out
there that report only percent.
Second, it's confusing to have "charge" both as a quantity and as an action.

The SBS spec takes pain to always refer to "capacity" or "charge
capacity" as a generic term for energy or electric charge. I think
"capacity" is a reasonable generic term, and tp_smapi follows it.

For this attribute, I suggest using "rate_{now,avg}", and defining it
to use the same units as the capacity readouts.

In addition, there should be "power_{now,avg}" attributes which are
always in watts regardless of the capacity readouts. This does not
necessarily duplicate voltage * current_{now,avg}, because the
hardware may be able to perform accurate integration over time.


> +BATTERY_DEVICE_ATTR("charge_max",CHARGE_MAX,milli);

SBS uses "DesignCapacity()" and tp_smapi uses "design_capacity".
I suggest "capacity_design".


> +BATTERY_DEVICE_ATTR("charge_last",CHARGE_LAST,milli);

SBS uses "FullChargeCapacity()" and tp_smapi uses last_full_capacity.
I suggest "capacity_last_full".


> +BATTERY_DEVICE_ATTR("charge_low",CHARGE_LOW,milli);
> +BATTERY_DEVICE_ATTR("charge_warn",CHARGE_WARN,milli);
> +BATTERY_DEVICE_ATTR("charge_unit",CHARGE_UNITS,string);

s/charge/capacity/.
BTW, the SBS defines the possible capacity units (CAPACITY_MODE bit)
as mAh and 10mWh. I guess the latter should be normalized by drivers
to mWh where applicable.

> +BATTERY_DEVICE_ATTR("charge_percent",CHARGE_PCT,int);

Percent of what? SBS distinguishes between "RelativeStateOfCharge()"
(percent of last full capacity") and "AbsoluteStateOfCharge()"
(percent of design capacity).

I suggest defining it to be the former and calling it "capacity_percent".


> +BATTERY_DEVICE_ATTR("time_remaining",TIME_REMAINING,int);

Separate attributes are needed for "time to full charge" and
"remaining time to full discharge". They're completely different
things (and sysfs doesn't let you read the time and status
atomically).

SBS defines "RunTimeToEmpty()", "AverageTimeToEmpty()" and
"AverageTimeToFull()", which using my suggested convention would
become:

time_to_empty_now
time_to_empty_avg
time_to_full

I guess SBS considered charging rate to be stable enough not to bother
with average vs. instantaneous rates. However, with the OLPC hand/foot
crank in mind, maybe we should have the full set:

time_to_empty_now
time_to_empty_avg
time_to_full_now
time_to_full_avg


> +BATTERY_DEVICE_ATTR("type",TYPE,string);

What's this?


> +BATTERY_DEVICE_ATTR("oem_info",OEM_INFO,string);

Tthe ThinkPad embedded controller battery interface also provides a
"barcode" data, which is 13-character submodel identifer. Not sure if
it should get a separate attribute, or appended to "model". FWIW,
tp_smapi does the former.

There are also the following, defined in SBS and exposed by tp_smapi:
cycle_count
date_manufactured

And ThinkPads also have this non-SBS extension:
date_first_used

Do you intend to also encompass battery control commands?
See here for what ThinkPads can do:
http://thinkwiki.org/wiki/Tp_smapi#Battery_charge_control_features

  Shem

Re: Battery class driver.

[Shem Multinymous]

Hi,

On 10/24/06, David Zeuthen <davidz@redhat.com> wrote:

> How do we plan to get updates to user space?

There was a long LKML thread ("Generic battery interface") about this in July.
This a general issue in sysfs, applicably whenever sysfs is used to
communicate device data (as opposed to system configuration). We need
a generic solution. Here are a few of the considerations that came up
in that thread:

- Efficiency on both poll-based and event-based hardware data sources.
- Avoiding unnecessary timer interrupts on tickless kernels.
- Letting multiple userspace clients poll the same data source at
different rates, without causing duplicate queries or unnecessary
process wakeups.
- Avoiding duplicate queries on poll-based hardware that provides
several attributes simultaneously.

Here's my latest proposed solution in that thread (but see the dozens
of messages before and after for context...):
http://lkml.org/lkml/2006/7/30/193


> The ACPI code today
> provides updates via the ACPI socket but that is broken on some hardware
> so essentially HAL polls by reading /proc/acpi/battery/BAT0/state some
> every 30 secs on, and, on some boxen, that generates a SMBIOS trap or
> some other expensive operation. That's wrong.

30 seconds? I've seen battery applets that poll 1sec intervals (that's
actually useful when you tweak power saving). And for things like the
hdaps accelerometer driver, we're at the 50HZ region.


> So, perhaps the battery class should provide a file called 'timestamp'
> or something that is only writable by the super user. If you read from
> that file it gives the time when the information was last updated. If
> you write to the file it will force the driver query the hardware and
> update the other files. Reading any other file than 'timestamp' will
> just read cached information.
>
> The mechanism to notify user space that something have been updated
> would be either to make the timestamp file pollable or use an uevent or
> something else (no input drivers please).
>
> If the hardware is able to generate an interrupt when certain data on
> the battery has changed the driver simply updates the timestamp file.
>
> With this scheme, user space simply does this
>
>  10 poll on /sys/class/battery/BAT0/timestamp with timeout=30sec
>  20 if timeout, write to 'timestamp' file to force polling
>  30 read values from sysfs and update graphics for battery icon etc.
>  40 goto 10
>
> and we only poll when the hardware don't provide such interrupts /
> hardware is broken so it don't provide interrupts.
>
> Specifically, user space can decide to make the timeout infinite or
> decide not to poll under certain conditions etc. etc.

This is a very interesting approach; I don't recall anything like this
from on the older thread.
There are a few problems though:
You can't require reading battery status to be a root-only operation.
When mutiple userspace apps poll the battery, you'll get race
conditions on timestamp, and even in the best case all the apps will
be woken up at the poll rate of the most-frequently-polling app (think
of that happening at 50HZ for hdaps...).

  Shem

Re: Battery class driver.

[Shem Multinymous]

On 10/24/06, David Zeuthen <davidz@redhat.com> wrote:

>  b) if the kernel ensures that the 'timestamp' file is updated last,
>     we get atomic updates

Atomic updates require either double-buffering the data (twice worse
than mere caching...) or locking away access during update (in which
case the order doesn't mater).

But yes, lack of atomicity in sysfs is a big issue. We don't seem to
have any ABI convention for providing atomic snapshots of those
dozen-small-atribute directories.


>     Notably user space can see _when_ the values from the hardware was
>     retrieved the last time


> This is a good thing, but is orthogonal to the how-do-we-poll issue.


> So.. how all this relates to hwmon I'm not sure.. looking briefly at
> Documentation/hwmon/sysfs-interface no such thing seems to be available,

Yes, hwmon ignores merrily ignores this issue, as do all other
data-through-sysfs drivers I've looked at. Except for the patched
hdaps driver in the tp_smapi package, which has a (very) rudimentary
solution via caching and a configurable in-kernel polling timer.

  Shem

Re: Battery class driver.

[Matthew Garrett]

On Tue, Oct 24, 2006 at 04:56:30AM +0200, Shem Multinymous wrote:

> 30 seconds? I've seen battery applets that poll 1sec intervals (that's
> actually useful when you tweak power saving). And for things like the
> hdaps accelerometer driver, we're at the 50HZ region.

Reading the battery status has the potential to call an SMI that might 
take an arbitrary period of time to return, and we found that having 
querying at around the 1 second mark tended to result in noticable 
system performace degredation.

Possibly it would be useful if the kernel could keep track of how long 
certain queries take? That would let userspace calibrate itself without 
having to worry about whether it was preempted or not.

> You can't require reading battery status to be a root-only operation.

We certainly can. Whether we want to is another matter :) I tend to 
agree that moving to a setup that makes it harder for command-line users 
to read the battery status would be a regression.

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

Re: Battery class driver.

[Benjamin Herrenschmidt]

> The tp_smapi code also provides average current and power, the charge 
> cycle count, the date of first use, the date of manufacture and controls 
> for altering the charge behaviour of the battery. Are these things that 
> should go in the generic class?

Also, should we create files for things that the backend doesn't
provide ? Seems like a waste of memory to me.

Ben

Re: Battery class driver.

[Benjamin Herrenschmidt]

> I sincerely hope that those responsible for the various other different
> userspace interfaces for PMU, ACPI, etc. are all hanging their heads in
> shame at this point :)

/me looks at ACPI ... :)

Ben.

Re: Battery class driver.

[Benjamin Herrenschmidt]

> No, I think the distinction between batteries and ac_adapter is large
> enough to have different classes of devices. You may have many
> batteries, but you'll only ever have one ac_adapter. I'm not sure it's
> an obvious abstraction to make.

No you won't :) You can have several power supplies, you can have UPS
too, and limited power budget depending on the "status" of these things
(for example, on some blades, we slow things down when one of the power
supply fails to limit our load on the remaining one(s), though that's
currently done outside of linux).

Ben.

> > > Comments? 
> 
> How are battery change notifications delivered to userspace? I know acpi
> is using the input layer for buttons in the future (very sane IMO), so
> using sysfs events for each property changing would probably be nice.
> 
> Comments on your patch:
> 
> > +#define BAT_INFO_TEMP2		(2) /* °C/1000 */
> Temperature expressed in degrees C/1000? - what if the temperature goes
> below 0? What about just using mK (kelvin / 1000) - I don't know what is
> used in the kernel elsewhere tho. Also, are you allowed the ° sign in
> kernel source now?
> 
> > +#define BAT_INFO_CURRENT	(6) /* mA */
> Can't this also be expressed in mW according to the ACPI spec?
> 
> > +#define BAT_STAT_FIRE		(1<<7)
> I know there is precedent for "FIRE" but maybe CRITICAL or DANGER might
> be better chosen words. We can reserve the word FIRE for when the faulty
> battery really is going to explode...
> 
> Richard.
> 
> > > commit 42fe507a262b2a2879ca62740c5312778ae78627
> > > Author: David Woodhouse <dwmw2@infradead.org>
> > > Date:   Mon Oct 23 18:14:54 2006 +0100
> > > 
> > >     [BATTERY] Add support for OLPC battery
> > >     
> > >     Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> > > 
> > > commit 6cbec3b84e3ce737b4217788841ea10a28a5e340
> > > Author: David Woodhouse <dwmw2@infradead.org>
> > > Date:   Mon Oct 23 18:14:14 2006 +0100
> > > 
> > >     [BATTERY] Add initial implementation of battery class
> > >     
> > >     I really don't like the sysfs interaction, and I don't much like the
> > >     internal interaction with the battery drivers either. In fact, there
> > >     isn't much I _do_ like, but it's good enough as a straw man.
> > >     
> > >     Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> 

Re: Battery class driver.

[Benjamin Herrenschmidt]

> It makes _less_ sense, imho, to have 'ac present' as a property of a
> battery -- which is what I've done for now.

Then maybe it's better to call it "in_use" meaning that the system is
currently drawing power from that battery rather than "ac_present"...

Ben.

Re: Battery class driver.

[Benjamin Herrenschmidt]

On Tue, 2006-10-24 at 04:27 +0100, Matthew Garrett wrote:
> On Tue, Oct 24, 2006 at 04:56:30AM +0200, Shem Multinymous wrote:
> 
> > 30 seconds? I've seen battery applets that poll 1sec intervals (that's
> > actually useful when you tweak power saving). And for things like the
> > hdaps accelerometer driver, we're at the 50HZ region.
> 
> Reading the battery status has the potential to call an SMI that might 
> take an arbitrary period of time to return, and we found that having 
> querying at around the 1 second mark tended to result in noticable 
> system performace degredation.
> 
> Possibly it would be useful if the kernel could keep track of how long 
> certain queries take? That would let userspace calibrate itself without 
> having to worry about whether it was preempted or not.
> 
> > You can't require reading battery status to be a root-only operation.
> 
> We certainly can. Whether we want to is another matter :) I tend to 
> agree that moving to a setup that makes it harder for command-line users 
> to read the battery status would be a regression.

I think it's up to the backend to poll more slowly and cache the results
on those machines then.

Ben.

Re: Battery class driver.

[Matthew Garrett]

On Tue, Oct 24, 2006 at 01:48:27PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2006-10-24 at 04:27 +0100, Matthew Garrett wrote:

> > Reading the battery status has the potential to call an SMI that might 
> > take an arbitrary period of time to return, and we found that having 
> > querying at around the 1 second mark tended to result in noticable 
> > system performace degredation.

> I think it's up to the backend to poll more slowly and cache the results
> on those machines then.

The kernel backend or the userspace backend? We need to decide on 
terminology :) There's no good programmatic way of determining how long 
a query will take other than doing it and looking at the result. I guess 
we could do that at boot time.

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

Re: Battery class driver.

[Benjamin Herrenschmidt]

On Tue, 2006-10-24 at 04:53 +0100, Matthew Garrett wrote:
> On Tue, Oct 24, 2006 at 01:48:27PM +1000, Benjamin Herrenschmidt wrote:
> > On Tue, 2006-10-24 at 04:27 +0100, Matthew Garrett wrote:
> 
> > > Reading the battery status has the potential to call an SMI that might 
> > > take an arbitrary period of time to return, and we found that having 
> > > querying at around the 1 second mark tended to result in noticable 
> > > system performace degredation.
> 
> > I think it's up to the backend to poll more slowly and cache the results
> > on those machines then.
> 
> The kernel backend or the userspace backend? We need to decide on 
> terminology :) 

The kernel. Userspace don't have to know the details of how hard it is
for the backend to fetch the data imho.

> There's no good programmatic way of determining how long 
> a query will take other than doing it and looking at the result. I guess 
> we could do that at boot time.

Ben.

Re: Battery class driver.

[Shem Multinymous]

On 10/24/06, Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> The kernel backend or the userspace backend? We need to decide on
> terminology :) There's no good programmatic way of determining how long
> a query will take other than doing it and looking at the result. I guess
> we could do that at boot time.

This is up to the kernel driver. Most drivers have fairly accurate
knowledge about the hardware they read, in terms of both the cost of
reading and the rate of change that's worth tracking.

The important thing is to define an ABI convention that lets userspace
tell the driver when it wants the next refresh (via either David's
timestamp or my suggested ioctl). The driver can then make its
informed decision on how to reasonably fulfill the request.

  Shem

Re: Battery class driver.

[Richard Hughes]

On Mon, 2006-10-23 at 21:48 +0100, David Woodhouse wrote:
> On Mon, 2006-10-23 at 20:58 +0100, Richard Hughes wrote:
> > No, I think the distinction between batteries and ac_adapter is large
> > enough to have different classes of devices. You may have many
> > batteries, but you'll only ever have one ac_adapter. I'm not sure it's
> > an obvious abstraction to make.
> 
> ∃ machines with more than one individual AC adapter. Which want
> individual notification when one of them goes down.
> 
> You're right that they don't necessarily fit into the 'battery' class,
> but I'm not entirely sure if it's worth putting them elsewhere, because
> the information about them is usually available in the same place as the
> information about the batteries, at least in the laptop case.

Sure, makes sense I guess.

> The simple case of AC adapters, where we have only 'present' or
> 'absent', is a subset of what batteries can do. If you have more
> complicated monitoring, then it's _also_ going to bear a remarkable
> similarity to what you get from batteries -- you'll be able to monitor
> temperatures, voltage, current, etc. So they're not _that_ much out of
> place in a 'power supply' class.

Sure, psu could be a nice class. We would need buy-in from ACPI, APM and
PMU maintainers to avoid just creating *another* standard that HAL has
to read.

> It makes _less_ sense, imho, to have 'ac present' as a property of a
> battery -- which is what I've done for now.

Agree.

> > How are battery change notifications delivered to userspace? I know acpi
> > is using the input layer for buttons in the future (very sane IMO), so
> > using sysfs events for each property changing would probably be nice.
> 
> For selected properties, yes. I wouldn't want it happening every time
> the current draw changes by a millivolt but for 'battery removed' or 'ac
> power applied' events it makes some sense.

Maybe still send events for large changes, like > whole % changes in
value. Then HAL hasn't got to poll at all.

> For sane hardware where we get an interrupt on such changes, that's fine
> -- but I'm wary of having to implement that by making the kernel poll
> for them; especially if/when there's nothing in userspace which cares. 

HAL and gnome-power-manager? There should only be a few changing values
on charging and discharging, and one every percentage point change isn't
a lot.

> > Comments on your patch:
> > 
> > > +#define BAT_INFO_TEMP2		(2) /* °C/1000 */
> > Temperature expressed in degrees C/1000? - what if the temperature goes
> > below 0? 
> 
> It's signed.

Sure, n/p.

> > What about just using mK (kelvin / 1000) - I don't know what is
> > used in the kernel elsewhere tho. Also, are you allowed the ° sign in
> > kernel source now?
> 
> Welcome to the 21st century.

Fair play. :-)

> > > +#define BAT_INFO_CURRENT	(6) /* mA */
> > Can't this also be expressed in mW according to the ACPI spec?
> 
> No, it can't. The Watt is not a unit of current.

Ahh, current as in electron flow, rather than current power use,
apologies.

> I intended the ACPI 'present rate' to map to the 'charge_rate' property,
> which is why we have the 'charge_unit' property. I don't like that much,
> but it seems necessary unless we're going to do something like separate
> 'charge_rate_mA' and 'charge_rate_mW' properties.

Not sure how to best do this for the kernel - maybe just expose the
value and the format separately. In HAL we normalise the rate to mWh
anyway using the rate in mAh and reported voltage.

> Actually, I suspect that on reflection I would prefer that latter
> option. DavidZ?
> 
> > > +#define BAT_STAT_FIRE		(1<<7)
> > I know there is precedent for "FIRE" but maybe CRITICAL or DANGER might
> > be better chosen words. We can reserve the word FIRE for when the faulty
> > battery really is going to explode...
> 
> Yes, feasibly. I don't quite know what the 'destroy' bit in the OLPC
> embedded controller is supposed to mean, and 'FIRE' seemed as good as
> anything else.

Then maybe just set present to false as a destroyed battery isn't much
use anyway...

Richard.

Re: Battery class driver.

[Pavel Machek]

Hi!

> At git://git.infradead.org/battery-2.6.git there is an initial
> implementation of a battery class, along with a driver which makes use
> of it. The patch is below, and also viewable at 
> http://git.infradead.org/?p=battery-2.6.git;a=commitdiff;h=master;hp=linus

Thanks a lot for this work.

> I don't like the sysfs interaction much -- is it really necessary for me
> to provide a separate function for each attribute, rather than a single
> function which handles them all and is given the individual attribute as
> an argument? That seems strange and bloated.
> 
> I'm half tempted to ditch the sysfs attributes and just use a single
> seq_file, in fact.

No, please don't.

							Pavel
-- 
Thanks for all the (sleeping) penguins.