LinuxLists.cc - Battery class driver.

[permalink] [raw]

Subject: Re: Battery class driver.

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

2006-10-23 18:31:14

[permalink] [raw]

Subject: Re: Battery class driver.

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

2006-10-23 18:33:46

[permalink] [raw]

Subject: Re: Battery class driver.

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

2006-10-23 18:50:26

[permalink] [raw]

Subject: Re: Battery class driver.

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

2006-10-23 19:17:26

by Dan Williams

[permalink] [raw]

Subject: Re: Battery class driver.

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 <[email protected]>
> Date: Mon Oct 23 18:14:54 2006 +0100
>
> [BATTERY] Add support for OLPC battery
>
> Signed-off-by: David Woodhouse <[email protected]>
>
> commit 6cbec3b84e3ce737b4217788841ea10a28a5e340
> Author: David Woodhouse <[email protected]>
> 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 <[email protected]>
>
> --- 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 <[email protected]>
> + *
> + * Based on LED Class support, by John Lenz and Richard Purdie:
> + *
> + * © 2005 John Lenz <[email protected]>
> + * © 2005-2006 Richard Purdie <[email protected]>
> + *
> + * 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 <[email protected]>");
> +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 <[email protected]>
> + *
> + * 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 <[email protected]>");
> +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 <[email protected]>
> + *
> + * Based on LED Class support, by John Lenz and Richard Purdie:
> + *
> + * © 2005 John Lenz <[email protected]>
> + * © 2005-2006 Richard Purdie <[email protected]>
> + *
> + * 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
> [email protected]
> http://mailman.laptop.org/mailman/listinfo/devel

2006-10-23 19:58:37

[permalink] [raw]

Subject: Re: Battery class driver.

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 <[email protected]>
> > Date: Mon Oct 23 18:14:54 2006 +0100
> >
> > [BATTERY] Add support for OLPC battery
> >
> > Signed-off-by: David Woodhouse <[email protected]>
> >
> > commit 6cbec3b84e3ce737b4217788841ea10a28a5e340
> > Author: David Woodhouse <[email protected]>
> > 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 <[email protected]>

2006-10-23 20:10:38

by Roland Dreier

[permalink] [raw]

Subject: Re: Battery class driver.

> 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?

2006-10-23 20:48:34

[permalink] [raw]

Subject: Re: Battery class driver.

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

2006-10-23 21:04:22

[permalink] [raw]

Subject: Re: Battery class driver.

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

2006-10-23 22:15:40

[permalink] [raw]

Subject: Re: Battery class driver.

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

2006-10-23 23:00:57

[permalink] [raw]

Subject: Re: Battery class driver.

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

2006-10-24 01:31:36

[permalink] [raw]

Subject: Re: Battery class driver.

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

2006-10-24 02:04:08

[permalink] [raw]

Subject: Re: Battery class driver.

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

2006-10-24 02:56:33

[permalink] [raw]

Subject: Re: Battery class driver.

Hi,

On 10/24/06, David Zeuthen <[email protected]> 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

2006-10-24 03:04:20

[permalink] [raw]

Subject: Re: Battery class driver.

On 10/24/06, David Zeuthen <[email protected]> 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

2006-10-24 03:27:14

by Matthew Garrett

[permalink] [raw]

Subject: Re: Battery class driver.

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 | [email protected]

2006-10-24 03:41:49

[permalink] [raw]

Subject: Re: Battery class driver.

> 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 <[email protected]>
> > > Date: Mon Oct 23 18:14:54 2006 +0100
> > >
> > > [BATTERY] Add support for OLPC battery
> > >
> > > Signed-off-by: David Woodhouse <[email protected]>
> > >
> > > commit 6cbec3b84e3ce737b4217788841ea10a28a5e340
> > > Author: David Woodhouse <[email protected]>
> > > 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 <[email protected]>
>

2006-10-24 03:41:33

[permalink] [raw]

Subject: Re: Battery class driver.

> 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

2006-10-24 03:43:34

[permalink] [raw]

Subject: Re: Battery class driver.

> 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.

2006-10-24 03:44:34

[permalink] [raw]

Subject: Re: Battery class driver.

> 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.

2006-10-24 03:48:47

[permalink] [raw]

Subject: Re: Battery class driver.

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.

2006-10-24 03:53:55

by Matthew Garrett

[permalink] [raw]

Subject: Re: Battery class driver.

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 | [email protected]

2006-10-24 05:45:26

[permalink] [raw]

Subject: Re: Battery class driver.

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.

2006-10-24 11:09:58

[permalink] [raw]

Subject: Re: Battery class driver.

On 10/24/06, Matthew Garrett <[email protected]> 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

2006-10-24 17:19:05

[permalink] [raw]

Subject: Re: Battery class driver.

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.

2006-10-25 07:43:58

[permalink] [raw]

Subject: [PATCH v2] Re: Battery class driver.

On Tue, 2006-10-24 at 18:18 +0100, Richard Hughes wrote:
> We would need buy-in from ACPI, APM and PMU maintainers to avoid just
> creating *another* standard that HAL has to read.

That's the whole point in what I'm doing, yes. And that's why the owners
of that code are Cc'd.

> > > 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.

Yes, perhaps. I'm still reluctant to make the kernel poll when the
hardware isn't sensible enough to provide interrupts -- I think we
should leave that to be implemented later, if at all. Let's get the
basic reporting implemented first.

> > 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.

My point is that if HAL isn't running, we shouldn't bother to poll. If
HAL _is_ running, then _it_ could poll, if the hardware isn't capable of
generating events. But let's leave that on the TODO list for now.

> > > > +#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.

Indeed. That's one of the measurements I get from the OLPC battery
controller.

> > 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.

In that case, perhaps we should do the same in the kernel. But I think I
prefer to expose the 'raw' data.

> > 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...

Hm, I've now seen it say 'destroy', after leaving it on battery
overnight till it shut down. When I applied power again, the battery
voltage claimed to be about 2½ volts and the 'destroy' flag was set. It
did seem to charge it up again though -- and it definitely wasn't on
fire :)

Updated patch in git://git.infradead.org/battery-2.6.git and below. I've
redone it to be more like hwmon, as suggested. Individual battery
'drivers' are now responsible for creating their own sysfs files
directly, instead of it being done by the core. I've added a list of
strings to battery.h which are the _only_ names I want to see in sysfs,
with specified units. Obviously we can continue to extend those -- and
one of the things I plan is to remove 'charge_units' and provide both
'design_charge' and 'design_energy' (also {energy,charge}_last,
_*_thresh etc.) to cover the mWh vs. mAh cases.

I haven't (yet) changed from a single 'status' file to multiple
'is_flag_0' 'is_flag_1' 'is_flag_2' files. I really don't like that idea
much -- it doesn't seem any more sensible than exposing each bit of the
voltage value through a separate file. These flags are _read_ together,
and _used_ together. I'd rather show it as a hex value 'flags' than
split it up. But I still think that the current 'present,charging,low'
is best.

commit 0aa9acc1c47dbb3c285d61bb55a1a363433c129f
Author: David Woodhouse <[email protected]>
Date: Tue Oct 24 16:14:59 2006 +0100

[BATTERY] Update OLPC battery driver

- Use platform_device
- Implement all available status fields

Signed-off-by: David Woodhouse <[email protected]>

commit 0513b2f5473f1e8bb8880c9dee8e1d63af86336e
Author: David Woodhouse <[email protected]>
Date: Tue Oct 24 09:56:46 2006 +0100

[BATTERY] Switch to using device_attribute, from the hardware driver

... instead of the core battery class. Various other cleanups.

Signed-off-by: David Woodhouse <[email protected]>

commit 74e2a48a77347d348e9ed21576863d0d9b51c690
Author: Greg KH <[email protected]>
Date: Tue Oct 24 09:39:31 2006 +0100

[BATTERY] Convert to struct device instead of class_device

From: Greg KH <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>

commit 42fe507a262b2a2879ca62740c5312778ae78627
Author: David Woodhouse <[email protected]>
Date: Mon Oct 23 18:14:54 2006 +0100

[BATTERY] Add support for OLPC battery

Signed-off-by: David Woodhouse <[email protected]>

commit 6cbec3b84e3ce737b4217788841ea10a28a5e340
Author: David Woodhouse <[email protected]>
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 <[email protected]>
diff --git a/drivers/Kconfig b/drivers/Kconfig
index f394634..1dc5dbe 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -34,6 +34,8 @@ source "drivers/ieee1394/Kconfig"

source "drivers/message/i2o/Kconfig"

+source "drivers/battery/Kconfig"
+
source "drivers/macintosh/Kconfig"

source "drivers/net/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 4ac14da..cd091c9 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -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_IDE) += ide/
obj-$(CONFIG_FC4) += fc4/
diff --git a/drivers/battery/Kconfig b/drivers/battery/Kconfig
new file mode 100644
index 0000000..9c2389a
--- /dev/null
+++ b/drivers/battery/Kconfig
@@ -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
diff --git a/drivers/battery/Makefile b/drivers/battery/Makefile
new file mode 100644
index 0000000..eeb5333
--- /dev/null
+++ b/drivers/battery/Makefile
@@ -0,0 +1,4 @@
+# Battery code
+obj-$(CONFIG_BATTERY_CLASS) += battery-class.o
+
+obj-$(CONFIG_OLPC_BATTERY) += olpc-battery.o
diff --git a/drivers/battery/battery-class.c b/drivers/battery/battery-class.c
new file mode 100644
index 0000000..60325c4
--- /dev/null
+++ b/drivers/battery/battery-class.c
@@ -0,0 +1,177 @@
+/*
+ * Battery class core
+ *
+ * © 2006 David Woodhouse <[email protected]>
+ *
+ * Based on LED Class support, by John Lenz and Richard Purdie:
+ *
+ * © 2005 John Lenz <[email protected]>
+ * © 2005-2006 Richard Purdie <[email protected]>
+ *
+ * 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 <linux/idr.h>
+
+static struct class *battery_class;
+
+static DEFINE_IDR(battery_idr);
+static DEFINE_SPINLOCK(idr_lock);
+
+ssize_t battery_attribute_show_status(char *buf, unsigned long status)
+{
+ ssize_t ret = 0;
+
+ 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_CRITICAL)
+ strcat(buf, ",critical");
+
+ if (status & BAT_STAT_CHARGE_DONE)
+ strcat(buf, ",charge-done");
+
+ strcat(buf, "\n");
+ ret = strlen(buf) + 1;
+ return ret;
+}
+EXPORT_SYMBOL_GPL(battery_attribute_show_status);
+
+ssize_t battery_attribute_show_ac_status(char *buf, unsigned long status)
+{
+ return 1 + sprintf(buf, "o%s-line\n", status?"n":"ff");
+}
+EXPORT_SYMBOL_GPL(battery_attribute_show_ac_status);
+
+static ssize_t battery_attribute_show_name(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct battery_dev *battery_dev = dev_get_drvdata(dev);
+ return 1 + sprintf(buf, "%s\n", battery_dev->name);
+}
+
+static const char *dev_types[] = { "battery", "ac" };
+
+static ssize_t battery_attribute_show_type(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct battery_dev *battery_dev = dev_get_drvdata(dev);
+
+ return 1 + sprintf(buf, "%s\n", dev_types[battery_dev->type]);
+}
+
+static DEVICE_ATTR(name, 0444, battery_attribute_show_name, NULL);
+static DEVICE_ATTR(type, 0444, battery_attribute_show_type, NULL);
+
+/**
+ * battery_dev_register - register a new object of battery_dev class.
+ * @dev: The device to register.
+ * @battery_dev: the battery_dev structure for this device.
+ */
+int battery_device_register(struct device *parent, struct battery_dev *battery_dev)
+{
+ int err;
+
+ if (battery_dev->type < PWRDEV_TYPE_BATTERY ||
+ battery_dev->type > PWRDEV_TYPE_AC)
+ return -EINVAL;
+
+ do {
+ if (unlikely(!idr_pre_get(&battery_idr, GFP_KERNEL)))
+ return -ENOMEM;
+
+ spin_lock(&idr_lock);
+ err = idr_get_new(&battery_idr, NULL, &battery_dev->id);
+ spin_unlock(&idr_lock);
+ } while(err == -EAGAIN);
+
+ if (unlikely(err))
+ return err;
+
+ battery_dev->dev = device_create(battery_class, parent, 0,
+ "%d", battery_dev->id);
+
+ if (unlikely(IS_ERR(battery_dev->dev))) {
+ spin_lock(&idr_lock);
+ idr_remove(&battery_idr, battery_dev->id);
+ spin_unlock(&idr_lock);
+ return PTR_ERR(battery_dev->dev);
+ }
+
+ dev_set_drvdata(battery_dev->dev, battery_dev);
+
+ /* register the attributes */
+ device_create_file(battery_dev->dev, &dev_attr_type);
+ device_create_file(battery_dev->dev, &dev_attr_name);
+
+ dev_info(battery_dev->dev, "Registered power source\n");
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(battery_device_register);
+
+/**
+ * battery_dev_unregister - unregisters a object of battery_properties class.
+ * @battery_dev: the battery device to unreigister
+ *
+ * Unregisters a previously registered via battery_dev_register object.
+ */
+void battery_device_unregister(struct battery_dev *battery_dev)
+{
+ device_remove_file(battery_dev->dev, &dev_attr_type);
+ device_remove_file(battery_dev->dev, &dev_attr_name);
+
+ device_unregister(battery_dev->dev);
+
+ spin_lock(&idr_lock);
+ idr_remove(&battery_idr, battery_dev->id);
+ spin_unlock(&idr_lock);
+}
+EXPORT_SYMBOL_GPL(battery_device_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 <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Battery class interface");
diff --git a/drivers/battery/olpc-battery.c b/drivers/battery/olpc-battery.c
new file mode 100644
index 0000000..2057b7c
--- /dev/null
+++ b/drivers/battery/olpc-battery.c
@@ -0,0 +1,335 @@
+/*
+ * Battery driver for One Laptop Per Child ($100 laptop) board.
+ *
+ * © 2006 David Woodhouse <[email protected]>
+ *
+ * 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 <linux/platform_device.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
+
+/*********************************************************************
+ * EC locking and access
+ *********************************************************************/
+
+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)) {
+ printk(KERN_ERR "Failed to lock EC for battery access\n");
+ return 1;
+ }
+ yield();
+ }
+}
+
+static void unlock_ec(void)
+{
+ outb(0xff, 0x6c);
+}
+
+static 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);
+}
+
+/*********************************************************************
+ * Status flags
+ *********************************************************************/
+static ssize_t olpc_ac_status_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ unsigned long status;
+
+ if (lock_ec())
+ return -EIO;
+
+ if (!(read_ec_byte(sMBAT_STATUS) & sBAT_PRESENT)) {
+ unlock_ec();
+ return -ENODEV;
+ }
+
+ status = read_ec_byte(sPOWER_FLAG) & ADAPTER_IN;
+
+ unlock_ec();
+
+ return battery_attribute_show_ac_status(buf, status);
+}
+
+static struct device_attribute dev_attr_ac_status =
+ __ATTR(status, 0444, olpc_ac_status_show, NULL);
+
+static ssize_t olpc_bat_status_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ unsigned long status = 0;
+ unsigned short tmp;
+
+ if (lock_ec())
+ return -EIO;
+
+ tmp = read_ec_byte(sMBAT_STATUS);
+
+ if (tmp & sBAT_PRESENT)
+ status |= BAT_STAT_PRESENT;
+ if (tmp & sBAT_FULL)
+ status |= BAT_STAT_FULL;
+ if (tmp & sBAT_DESTROY)
+ status |= BAT_STAT_CRITICAL;
+ if (tmp & sBAT_LOW)
+ status |= BAT_STAT_LOW;
+ if (tmp & sBAT_DISCHG)
+ status |= BAT_STAT_DISCHARGING;
+
+ tmp = read_ec_byte(sMCHARGE_STATUS);
+ if (tmp & sBAT_CHARGE)
+ status |= BAT_STAT_CHARGING;
+ if (tmp & sBAT_OVERTEMP)
+ status |= BAT_STAT_OVERTEMP;
+
+ unlock_ec();
+ return battery_attribute_show_status(buf, status);
+}
+static struct device_attribute dev_attr_bat_status =
+ __ATTR(status, 0444, olpc_bat_status_show, NULL);
+
+/*********************************************************************
+ * Integer attributes
+ *********************************************************************/
+
+struct olpc_bat_attr_int {
+ struct device_attribute dev_attr;
+ unsigned short adr;
+ int mul, div;
+};
+
+#define ATTR_INT(_name, _adr, _mul, _div) { \
+ .dev_attr.attr.name = _name, \
+ .dev_attr.attr.mode = 0444, \
+ .dev_attr.show = olpc_bat_attr_int_show, \
+ .adr = _adr, \
+ .mul = _mul, \
+ .div = _div, \
+}
+
+static int olpc_bat_attr_int_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct olpc_bat_attr_int *battr = (void *)attr;
+ long value;
+
+ if (lock_ec())
+ return -EIO;
+
+ if (!(read_ec_byte(sMBAT_STATUS) & sBAT_PRESENT)) {
+ unlock_ec();
+ return -ENODEV;
+ }
+
+ if (battr->adr == SOC) {
+ value = read_ec_byte(battr->adr);
+ } else {
+ value = (signed short)read_ec_word(battr->adr);
+
+ value *= battr->mul;
+ value /= battr->div;
+ }
+ unlock_ec();
+
+ return 1 + sprintf(buf, "%ld\n", value);
+}
+
+static struct olpc_bat_attr_int attrs_int[] = {
+ ATTR_INT(BAT_INFO_VOLTAGE, wBAT_VOLTAGE, 9760, 32000),
+ ATTR_INT(BAT_INFO_CURRENT, wBAT_CURRENT, 15625, 120000),
+ ATTR_INT(BAT_INFO_TEMP1, wBAT_TEMP, 1000, 256),
+ ATTR_INT(BAT_INFO_TEMP2, wAMB_TEMP, 1000, 256),
+ ATTR_INT(BAT_INFO_CHARGE_PCT, SOC, 1, 1)
+};
+
+
+/*********************************************************************
+ * String attributes
+ *********************************************************************/
+
+struct olpc_bat_attr_str {
+ struct device_attribute dev_attr;
+ char *str;
+};
+
+#define ATTR_STR(_name, _str) { \
+ .dev_attr.attr.name = _name, \
+ .dev_attr.attr.mode = 0444, \
+ .dev_attr.show = olpc_bat_attr_str_show, \
+ .str = (char *)_str, \
+}
+
+static int olpc_bat_attr_str_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct olpc_bat_attr_str *sattr = (void *)attr;
+ unsigned short tmp;
+ int ret = 0;
+
+ /* Static strings are simple */
+ if ((unsigned long)sattr->str > PAGE_SIZE) {
+ return 1 + sprintf(buf, "%s\n", sattr->str);
+ }
+
+ if (lock_ec())
+ ret = -EIO;
+
+ if (!(read_ec_byte(sMBAT_STATUS) & sBAT_PRESENT))
+ ret = -ENODEV;
+ else switch((unsigned long)sattr->str) {
+ case 1:
+ tmp = read_ec_byte(sMCHARGE_STATUS);
+ if (tmp & sBAT_NiMH)
+ ret = 1 + sprintf(buf, "NiMH\n");
+ else
+ ret = 1 + sprintf(buf, "unknown\n");
+ break;
+ default:
+ printk(KERN_ERR "Unknown string type %p\n", sattr->str);
+ }
+
+ unlock_ec();
+ return ret;
+}
+
+static struct olpc_bat_attr_str attrs_str[] = {
+ ATTR_STR(BAT_INFO_TEMP1_NAME, "battery"),
+ ATTR_STR(BAT_INFO_TEMP2_NAME, "ambient"),
+ ATTR_STR(BAT_INFO_TECHNOLOGY, 1),
+};
+
+/*********************************************************************
+ * Initialisation
+ *********************************************************************/
+
+static struct battery_dev olpc_bat = {
+ .name = "OLPC battery",
+ .type = PWRDEV_TYPE_BATTERY,
+};
+
+static struct battery_dev olpc_ac = {
+ .name = "OLPC AC",
+ .type = PWRDEV_TYPE_AC,
+};
+static struct platform_device *bat_plat_dev;
+
+int __init olpc_bat_init(void)
+{
+ int ret = -ENODEV;
+ unsigned short tmp;
+ int i;
+
+ if (!request_region(0x380, 4, "battery"))
+ return -EIO;
+
+ if (lock_ec())
+ goto out_rel;
+
+ tmp = read_ec_word(0xfe92);
+ unlock_ec();
+
+ if (tmp != 0x380) {
+ /* Doesn't look like OLPC EC, but unlock anyway */
+ return -ENODEV;
+ }
+
+ bat_plat_dev = platform_device_register_simple("olpc-battery", 0, NULL, 0);
+ if (IS_ERR(bat_plat_dev))
+ goto out_rel;
+
+ ret = battery_device_register(&bat_plat_dev->dev, &olpc_bat);
+ if (ret)
+ goto out_plat;
+
+ ret = battery_device_register(&bat_plat_dev->dev, &olpc_ac);
+ if (ret) {
+ battery_device_unregister(&olpc_bat);
+ out_plat:
+ platform_device_unregister(bat_plat_dev);
+ out_rel:
+ release_region(0x380, 4);
+ return ret;
+ }
+ for (i=0; i < ARRAY_SIZE(attrs_int); i++)
+ device_create_file(olpc_bat.dev, &attrs_int[i].dev_attr);
+ for (i=0; i < ARRAY_SIZE(attrs_str); i++)
+ device_create_file(olpc_bat.dev, &attrs_str[i].dev_attr);
+
+ device_create_file(olpc_bat.dev, &dev_attr_bat_status);
+ device_create_file(olpc_ac.dev, &dev_attr_ac_status);
+
+ return ret;
+}
+
+void __exit olpc_bat_exit(void)
+{
+ int i;
+
+ device_remove_file(olpc_bat.dev, &dev_attr_bat_status);
+ device_remove_file(olpc_ac.dev, &dev_attr_ac_status);
+
+ for (i=0; i < ARRAY_SIZE(attrs_int); i++)
+ device_remove_file(olpc_bat.dev, &attrs_int[i].dev_attr);
+ for (i=0; i < ARRAY_SIZE(attrs_str); i++)
+ device_remove_file(olpc_bat.dev, &attrs_str[i].dev_attr);
+ battery_device_unregister(&olpc_ac);
+ battery_device_unregister(&olpc_bat);
+ platform_device_unregister(bat_plat_dev);
+ release_region(0x380, 4);
+}
+
+
+module_init(olpc_bat_init);
+module_exit(olpc_bat_exit);
+
+MODULE_AUTHOR("David Woodhouse <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Battery class interface");
diff --git a/include/linux/battery.h b/include/linux/battery.h
new file mode 100644
index 0000000..6148d5f
--- /dev/null
+++ b/include/linux/battery.h
@@ -0,0 +1,93 @@
+/*
+ * Driver model for batteries
+ *
+ * © 2006 David Woodhouse <[email protected]>
+ *
+ * Based on LED Class support, by John Lenz and Richard Purdie:
+ *
+ * © 2005 John Lenz <[email protected]>
+ * © 2005-2006 Richard Purdie <[email protected]>
+ *
+ * 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 PWRDEV_TYPE_BATTERY 0
+#define PWRDEV_TYPE_AC 1
+
+#define BAT_STAT_PRESENT (1<<0)
+#define BAT_STAT_LOW (1<<1)
+#define BAT_STAT_FULL (1<<2)
+#define BAT_STAT_CHARGING (1<<3)
+#define BAT_STAT_DISCHARGING (1<<4)
+#define BAT_STAT_OVERTEMP (1<<5)
+#define BAT_STAT_CRITICAL (1<<6)
+#define BAT_STAT_FIRE (1<<7)
+#define BAT_STAT_CHARGE_DONE (1<<8)
+
+/* Thou shalt not export any attributes in sysfs except these, and
+ with these units: */
+#define BAT_INFO_STATUS "status" /* For AC: "on-line" or "off-line" */
+ /* For battery: ... */
+
+#define BAT_INFO_TEMP1 "temp1" /* °C/1000 */
+#define BAT_INFO_TEMP1_NAME "temp1_name" /* string */
+
+#define BAT_INFO_TEMP2 "temp2" /* °C/1000 */
+#define BAT_INFO_TEMP2_NAME "temp2_name" /* string */
+
+#define BAT_INFO_VOLTAGE "voltage" /* mV */
+#define BAT_INFO_VOLTAGE_DESIGN "design_voltage" /* mV */
+
+#define BAT_INFO_CURRENT "current" /* mA */
+#define BAT_INFO_AVG_CURRENT "current_avg" /* mA */
+
+#define BAT_INFO_CHARGE_RATE "charge_rate" /* CHARGE_UNITS */
+#define BAT_INFO_CHARGE "charge" /* CHARGE_UNITS*h */
+#define BAT_INFO_CHARGE_MAX "design_charge" /* CHARGE_UNITS*h */
+#define BAT_INFO_CHARGE_LAST "charge_last" /* CHARGE_UNITS*h */
+#define BAT_INFO_CHARGE_LOW "charge_low_thresh" /* CHARGE_UNITS*h */
+#define BAT_INFO_CHARGE_WARN "charge_warn_thresh" /* CHARGE_UNITS*h */
+#define BAT_INFO_CHARGE_UNITS "charge_units" /* string */
+
+#define BAT_INFO_CHARGE_PCT "charge_percentage" /* integer */
+
+#define BAT_INFO_TIME_REMAINING "time_remaining" /* seconds */
+
+#define BAT_INFO_MANUFACTURER "manufacturer" /* string */
+#define BAT_INFO_TECHNOLOGY "technology" /* string */
+#define BAT_INFO_MODEL "model" /* string */
+#define BAT_INFO_SERIAL "serial" /* string */
+#define BAT_INFO_OEM_INFO "oem_info" /* string */
+
+#define BAT_INFO_CHARGE_COUNT "charge_count" /* integer */
+#define BAT_INFO_MFR_DATE "manufacture_date" /* YYYY[-MM[-DD]] */
+#define BAT_INFO_FIRST_USE "first_use" /* YYYY[-MM[-DD]] */
+
+struct battery_dev {
+
+ int id;
+ int type;
+ const char *name;
+
+ struct device *dev;
+};
+
+int battery_device_register(struct device *parent,
+ struct battery_dev *battery_cdev);
+void battery_device_unregister(struct battery_dev *battery_cdev);
+
+
+ssize_t battery_attribute_show_status(char *buf, unsigned long status);
+ssize_t battery_attribute_show_ac_status(char *buf, unsigned long status);
+#endif /* __LINUX_BATTERY_H__ */

--
dwmw2

2006-10-25 09:54:33

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Hi David,

On 10/25/06, David Woodhouse <[email protected]> wrote:
> > > 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.

What about current and power? These change very quickly.

BTW, your patch doesn't address the instantaneous vs. average readout
issue in the Smart Battery Data Specification and ThinkPads. Nor a
number of other issues I brought up earlier.

> My point is that if HAL isn't running, we shouldn't bother to poll. If
> HAL _is_ running, then _it_ could poll, if the hardware isn't capable of
> generating events. But let's leave that on the TODO list for now.

Agreed.

> one of the things I plan is to remove 'charge_units' and provide both
> 'design_charge' and 'design_energy' (also {energy,charge}_last,
> _*_thresh etc.) to cover the mWh vs. mAh cases.

You can't do this conversion, since the voltage is not constant.
Typically the voltage drops when the charge goes down, so you'll be
grossly overestimating the available energy it. And the effect varies
with battery chemistry and condition.

So there's no choice, you must use a generic term + units (and I
suggest following the Smart Battery Data Specification's term
"capacity"). This also takes care of the inevitable hardware that
reports in other units, such as normalized percent.

> I'd rather show it as a hex value 'flags' than
> split it up. But I still think that the current 'present,charging,low'
> is best.

Then please make space-separated rather than comma-separated. That's
easier to parse in shell and C.

Shem

2006-10-25 12:11:40

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Wed, 2006-10-25 at 11:54 +0200, Shem Multinymous wrote:
> What about current and power? These change very quickly.

I don't think we'd want to generate events for those, except perhaps if
they exceed certain thresholds.

> BTW, your patch doesn't address the instantaneous vs. average readout
> issue in the Smart Battery Data Specification and ThinkPads. Nor a
> number of other issues I brought up earlier.

True; it's a work-in-progress, and was actually sent just as I was
boarding a plane. I shall attempt to go back through the messages I've
received and address anything else that's pending.

If you can summarise the bits I've missed in the meantime that would be
wonderfully useful -- especially if you could do so in 'diff -u' form :)

> > one of the things I plan is to remove 'charge_units' and provide both
> > 'design_charge' and 'design_energy' (also {energy,charge}_last,
> > _*_thresh etc.) to cover the mWh vs. mAh cases.
>
> You can't do this conversion, since the voltage is not constant.
> Typically the voltage drops when the charge goes down, so you'll be
> grossly overestimating the available energy it. And the effect varies
> with battery chemistry and condition.

Absolutely. I don't want to do the conversion -- I want to present the
raw data. I was just a question of whether I provide 'capacity' and
'units' properties, or whether I provide 'capacity_mWh' and
'capacity_mAh' properties (only one of which, presumably, would be
available for any given battery). Likewise for the rates, thresholds,
etc.

> > I'd rather show it as a hex value 'flags' than
> > split it up. But I still think that the current 'present,charging,low'
> > is best.
>
> Then please make space-separated rather than comma-separated. That's
> easier to parse in shell and C.

Ok. Committed to git://git.infradead.org/battery-2.6.git

--
dwmw2

2006-10-25 14:34:38

[permalink] [raw]

Subject: Re: Battery class driver.

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.

2006-10-25 14:42:38

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Hi,

On 10/25/06, David Woodhouse <[email protected]> wrote:
> If you can summarise the bits I've missed in the meantime that would be
> wonderfully useful

OK. Looking at the current git snapshot:

current_now is missing.

time_remainig should be split into:
time_to_empty_now
time_to_empty_avg
time_to_full
or even
time_to_empty_now
time_to_empty_avg
time_to_full_now
time_to_full_avg

s/charge_count/cycle_count/, that's the standard name and used by the SBS spec.

Why the reversed order, for example, in design_charge vs. charge_last?
Following hwmon style, I think it should be
s/design_charge/charge_design/
s/manufacture_date/date_manufactured/
s/first_use/date_first_used/
s/design_voltage/voltage_design/

s/charge_last/charge_last_full/ seems less ambiguous.

s/^charge$/charge_left/ follows SBS and seems better.

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

It would be nice to have power_{now,avg}, always in mW regardless of
the capacity units.

I take it you don't want to deal with battery control actions for now.

> > > one of the things I plan is to remove 'charge_units' and provide both
> > > 'design_charge' and 'design_energy' (also {energy,charge}_last,
> > > _*_thresh etc.) to cover the mWh vs. mAh cases.
> >
> > You can't do this conversion, since the voltage is not constant.
> > Typically the voltage drops when the charge goes down, so you'll be
> > grossly overestimating the available energy it. And the effect varies
> > with battery chemistry and condition.
>
> Absolutely. I don't want to do the conversion -- I want to present the
> raw data. I was just a question of whether I provide 'capacity' and
> 'units' properties, or whether I provide 'capacity_mWh' and
> 'capacity_mAh' properties (only one of which, presumably, would be
> available for any given battery). Likewise for the rates, thresholds,
> etc.

I think using one set of files and units string makes more sense, for
several reasons:
Reduces the number of attributes and kernel code duplication.
Can handle weird power sources that use other units.
Simpler userspace code. One can do
$ cd /sys/foo; echo `cat capacity_left` out of `cat capacity_last`
`cat capaity_units` left.
instead of checking multiple sets of files for valid values.

The great majority of apps don't care about the physical values, but
just need something that they can parse as a relative quantity and
something to show the user. The generic units scheme provides both. We
have current_*, voltage etc. for those that do care, but there's no
need to duplicate the whole set of _thresholds, _last_full, _design
etc.

Shem

2006-10-25 22:25:56

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Wed, 2006-10-25 at 16:42 +0200, Shem Multinymous wrote:
> Hi,
>
> On 10/25/06, David Woodhouse <[email protected]> wrote:
> > If you can summarise the bits I've missed in the meantime that would be
> > wonderfully useful
>
> OK. Looking at the current git snapshot:
> < ... >

Ok, thanks for the feedback -- I think I've done all that. It wasn't in
'diff -u' form so I may have misapplied it. Please confirm.

> I take it you don't want to deal with battery control actions for now.

They're simple enough to add. We can do it when the tp driver gets
converted over.

> I think using one set of files and units string makes more sense, for
> several reasons:

Yeah, OK. Colour me convinced. I'll leave it as I had it.

--
dwmw2

2006-10-25 23:39:04

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Hi,

On 10/26/06, David Woodhouse <[email protected]> wrote:
> Ok, thanks for the feedback -- I think I've done all that. It wasn't in
> 'diff -u' form so I may have misapplied it. Please confirm.

Looks perfect. And I agree that current_{,now} etc. is better than my
suggestion of current_{avg,now}.

(I used free text instead of diff -u so I can give the rationale;
should have included both, I guess).

> > I take it you don't want to deal with battery control actions for now.
>
> They're simple enough to add. We can do it when the tp driver gets
> converted over.

Sure. But It will require some thought. For example, the interface
will need to encompass the non-symmetric pair of force commands on
ThinkPads:
"discharge the battery until further notice" vs.
"don't charge the battery for N minutes".

Also, ThinkPads express the start/stop charging thresholds in
percent, whereas I imagine some other hardware will represent it as
capacity.

What other examples do we have of machines with battery control commands?

Shem

2006-10-26 09:55:16

by FeRD

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

Shem Multinymous wrote:
> Hi,
>
> On 10/25/06, David Woodhouse <[email protected]> wrote:
>> If you can summarise the bits I've missed in the meantime that would be
>> wonderfully useful
>
> OK. Looking at the current git snapshot:
[...]
> Why the reversed order, for example, in design_charge vs. charge_last?
> Following hwmon style, I think it should be
> s/design_charge/charge_design/
> s/manufacture_date/date_manufactured/
> s/first_use/date_first_used/
> s/design_voltage/voltage_design/
>
> s/charge_last/charge_last_full/ seems less ambiguous.
>
> s/^charge$/charge_left/ follows SBS and seems better.
Can I propose a further

s/left/remaining/

...flung at the above? Best to avoid "left" as anything but the opposite
of "right", if disambiguation is the goal.

-FeRD

2006-10-28 05:12:32

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Hi!

> I haven't (yet) changed from a single 'status' file to multiple
> 'is_flag_0' 'is_flag_1' 'is_flag_2' files. I really don't like that idea
> much -- it doesn't seem any more sensible than exposing each bit of the
> voltage value through a separate file. These flags are _read_ together,
> and _used_ together. I'd rather show it as a hex value 'flags' than
> split it up. But I still think that the current 'present,charging,low'
> is best.

Please do this change. sysfs *is* one file per value.. if at least to
be consistent with rest of code.

> @@ -0,0 +1,177 @@
> +/*
> + * Battery class core
> + *
> + * ?? 2006 David Woodhouse <[email protected]>
> + *
> + * Based on LED Class support, by John Lenz and Richard Purdie:
> + *
> + * ?? 2005 John Lenz <[email protected]>
> + * ?? 2005-2006 Richard Purdie <[email protected]>

Could we get something ascii here? I'm not sure what you see instead
of copyright... but I see ??. I could not find it in source, but if
you use non-ascii character in file, please fix that, too.

> +ssize_t battery_attribute_show_ac_status(char *buf, unsigned long status)
> +{
> + return 1 + sprintf(buf, "o%s-line\n", status?"n":"ff");
> +}

I guess ac_online should show 0/1...

> + if (unlikely(err))
> + return err;
> +
> + battery_dev->dev = device_create(battery_class, parent, 0,

space/tab problem?

--
Thanks for all the (sleeping) penguins.

2006-10-28 12:16:03

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Thu, 2006-10-26 at 01:39 +0200, Shem Multinymous wrote:
> > They're simple enough to add. We can do it when the tp driver gets
> > converted over.
>
> Sure. But It will require some thought. For example, the interface
> will need to encompass the non-symmetric pair of force commands on
> ThinkPads:
> "discharge the battery until further notice" vs.
> "don't charge the battery for N minutes".
>
> Also, ThinkPads express the start/stop charging thresholds in
> percent, whereas I imagine some other hardware will represent it as
> capacity.

Hm. Again we have the question of whether to export 'threshold_pct' vs.
'threshold_abs', or whether to have a separate string property which
says what the 'unit' of the threshold is. I don't care much -- I'll do
whatever DavidZ prefers.

The git tree now has support for battery information available from the
PMU on Apple laptops. I _really_ don't like the way I have to register a
fake platform_device just to be able to get sensible attribute
callbacks. I suspect it should go back to being a class_device and we
should fix the class_device attribute functions. Greg?

--
dwmw2

2006-10-28 13:22:11

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Sat, 2006-10-28 at 13:15 +0100, David Woodhouse wrote:
>
> Hm. Again we have the question of whether to export 'threshold_pct'
> vs.'threshold_abs', or whether to have a separate string property
> which says what the 'unit' of the threshold is. I don't care much --
> I'll do whatever DavidZ prefers.

Unit is easier to process in HAL in my opinion.

Richard.

2006-10-28 14:34:54

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On 10/28/06, Richard Hughes <[email protected]> wrote:
> On Sat, 2006-10-28 at 13:15 +0100, David Woodhouse wrote:
> >
> > Hm. Again we have the question of whether to export 'threshold_pct'
> > vs.'threshold_abs', or whether to have a separate string property
> > which says what the 'unit' of the threshold is. I don't care much --
> > I'll do whatever DavidZ prefers.
>
> Unit is easier to process in HAL in my opinion.

That's harder for modifiable attributes, because apps need to know the
minimum and maximum values (e.g., for sane GUI). So it's either
multiple sets, or strings with fixed semantics (say, "percent" and
"capacity"), or adding *_min and *_max read-only attributes.

Speaking of which, battery.h says this:

* Thou shalt not export any attributes in sysfs except these, and
with these units: */

Drivers *will* want to violate this. For example, the "inhibit
charging for N minutes" command on ThinkPads seems too arcane to be
worthy of generalization. I would add a more sensible boolean
"charging_inhibit" attribute to battery.h, and let the ThinkPad driver
implement it as well as it can. The driver will then expose a
non-stadard "charging_inhibit_minutes" attribute to reveal the finer
level of access to those who care.

Shem

2006-10-28 14:36:40

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Sat, 2006-10-28 at 16:34 +0200, Shem Multinymous wrote:
> * Thou shalt not export any attributes in sysfs except these, and
> with these units: */
>
> Drivers *will* want to violate this. For example, the "inhibit
> charging for N minutes" command on ThinkPads seems too arcane to be
> worthy of generalization. I would add a more sensible boolean
> "charging_inhibit" attribute to battery.h, and let the ThinkPad driver
> implement it as well as it can. The driver will then expose a
> non-stadard "charging_inhibit_minutes" attribute to reveal the finer
> level of access to those who care.

If it makes enough sense that it's worth exporting it to userspace at
all, then it can go into battery.h.

--
dwmw2

2006-10-28 14:57:07

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Sat, 2006-10-28 at 15:36 +0100, David Woodhouse wrote:
> On Sat, 2006-10-28 at 16:34 +0200, Shem Multinymous wrote:
> > * Thou shalt not export any attributes in sysfs except these, and
> > with these units: */
> >
> > Drivers *will* want to violate this. For example, the "inhibit
> > charging for N minutes" command on ThinkPads seems too arcane to be
> > worthy of generalization. I would add a more sensible boolean
> > "charging_inhibit" attribute to battery.h, and let the ThinkPad driver
> > implement it as well as it can. The driver will then expose a
> > non-stadard "charging_inhibit_minutes" attribute to reveal the finer
> > level of access to those who care.
>
> If it makes enough sense that it's worth exporting it to userspace at
> all, then it can go into battery.h.

If it's non-standard please make sure to prefix the name with something
unique e.g.

x_thinkpad_charging_inhibit [1]

to avoid collisions, e.g. two drivers using the same name but the
semantics are different. Over time the battery class can standardize on
this (if a feature becomes sufficiently common) and drivers can move
over if they want to.

David

[1] : The x_ bit is inspired by how non standard email headers are
handled e.g. X-Mailer and whatever. It's a very useful hint to user
space people (and users in general) that some sysfs attribute is
non-standard. I was going to suggest to user a full reverse DNS style
naming scheme but I guess something that is unique enough will do.

2006-10-28 15:09:42

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Sat, 2006-10-28 at 14:22 +0100, Richard Hughes wrote:
> On Sat, 2006-10-28 at 13:15 +0100, David Woodhouse wrote:
> >
> > Hm. Again we have the question of whether to export 'threshold_pct'
> > vs.'threshold_abs', or whether to have a separate string property
> > which says what the 'unit' of the threshold is. I don't care much --
> > I'll do whatever DavidZ prefers.
>
> Unit is easier to process in HAL in my opinion.

What about just prepending the unit to the 'threshold' file? Then user
space can expect the contents of said file to be of the form "%d %s". I
don't think that violates the "only one value per file" sysfs mantra.

David

2006-10-28 15:31:41

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Sat, 2006-10-28 at 11:09 -0400, David Zeuthen wrote:
> What about just prepending the unit to the 'threshold' file?
^^^^

Uh, I meant appending. Haven't had coffee just yet.

David

2006-10-28 18:12:44

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Hi David,

On 10/28/06, David Zeuthen <[email protected]> wrote:
> What about just prepending the unit to the 'threshold' file? Then user
> space can expect the contents of said file to be of the form "%d %s". I
> don't think that violates the "only one value per file" sysfs mantra.

The tp_smapi battery driver did just this ("16495 mW"). But I dropped
it in a recent version when Pavel pointed out the rest of sysfs, hwmon
included, uses undecorated integers.
Consistency aside, it seems reasonable and convenient. You have to
decree that writes to the attributes (where relevant) don't include
the units, of course, so no one will expect the kernel to parse that.

There's an issue here if a drunk driver decides to specify (say)
capacity_remaining in mWh and capacity_last_full in mAa, which will
confuse anyone comparing those attributest. So don't do that.

Jean, what's your opinion on letting hwmon-ish attributes specify
units as "%d %s" where these are hardware-dependent?

Shem

2006-10-28 18:53:05

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Hi!
> > If it makes enough sense that it's worth exporting it to userspace at
> > all, then it can go into battery.h.
>
> If it's non-standard please make sure to prefix the name with something
> unique e.g.
>
> x_thinkpad_charging_inhibit [1]

> to avoid collisions, e.g. two drivers using the same name but the

You were clearly exposed to harmful dose of smtp.

This is ugly, and unneccessary: kernel is centrally controlled. We
*will* want to merge such attributes into something standard.

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

2006-10-28 18:55:36

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Hi!

> > > Hm. Again we have the question of whether to export 'threshold_pct'
> > > vs.'threshold_abs', or whether to have a separate string property
> > > which says what the 'unit' of the threshold is. I don't care much --
> > > I'll do whatever DavidZ prefers.
> >
> > Unit is easier to process in HAL in my opinion.
>
> What about just prepending the unit to the 'threshold' file? Then user
> space can expect the contents of said file to be of the form "%d %s". I
> don't think that violates the "only one value per file" sysfs mantra.

Bad idea... I bet someone will just ignore the units part, because all
the machines he seen had mW there. Just put it into the name:

power_avg_mV

Pavel

--
Thanks for all the (sleeping) penguins.

2006-10-28 19:50:14

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Sat, 2006-10-28 at 18:52 +0000, Pavel Machek wrote:
> You were clearly exposed to harmful dose of smtp.

Actually I got the idea when thinking of .desktop files but whatever.

> This is ugly, and unneccessary: kernel is centrally controlled. We
> *will* want to merge such attributes into something standard.

Uh, such standards don't happen overnight as this thread painfully
demonstrates, i.e. there is not yet any "standard" for handling
batteries until dwmw2 actually stepped up. That alone says something.
And we're at 2.6.19 about 15 years into development of Linux?

You may or may not like it... but battery class drivers will have such
non-standard things. I'm merely suggesting to tag these as non-standard
so it's bloody evident they are non-standard. For the record, I also
think that making non standard attributes ugly will help accelerate us
in standardizing on it. You can also easier grep through the sources to
find offending code when you do decide to standardize it.

Playing the "kernel is centrally controlled" card doesn't work here. Do
not pass start. Do not collect $200. (It works in a helluva lot of other
situations though.)

David

2006-10-28 19:54:11

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Sat, 2006-10-28 at 18:55 +0000, Pavel Machek wrote:
> Bad idea... I bet someone will just ignore the units part, because all
> the machines he seen had mW there.

Sure, user space can do silly things. Just ask davej. Let's try to
assume they don't.

> Just put it into the name:
>
> power_avg_mV

Bad idea... it means user space will have to try to open different files
and what happens when someone introduces a new unit? Ideally I'd like
the unit to be part of the payload of the sysfs file. Second to that I
think having the unit in a separate file is preferable.

David

2006-10-28 21:05:30

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Sat 2006-10-28 15:53:56, David Zeuthen wrote:
> On Sat, 2006-10-28 at 18:55 +0000, Pavel Machek wrote:
> > Bad idea... I bet someone will just ignore the units part, because all
> > the machines he seen had mW there.
>
> Sure, user space can do silly things. Just ask davej. Let's try to
> assume they don't.
>
> > Just put it into the name:
> >
> > power_avg_mV
>
> Bad idea... it means user space will have to try to open different files
> and what happens when someone introduces a new unit? Ideally I'd like
> the unit to be part of the payload of the sysfs file. Second to that I
> think having the unit in a separate file is preferable.

Introducing new unit *should* be hard. You know, when you introduce
new unit, you automatically break all the userspace.

Having separate files is actually a *feature*. It allows you to
introduce new units while providing backwards compatibility.

Imagine going from mV to uV... With voltage_mV, you can have both
voltage_mV and voltage_uV. In your system, you'd have to change value
from mV to uV, breaking all the userspace....
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2006-10-28 21:10:33

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Sat 2006-10-28 15:48:54, David Zeuthen wrote:
> On Sat, 2006-10-28 at 18:52 +0000, Pavel Machek wrote:
> > This is ugly, and unneccessary: kernel is centrally controlled. We
> > *will* want to merge such attributes into something standard.
>
> Uh, such standards don't happen overnight as this thread painfully
> demonstrates, i.e. there is not yet any "standard" for handling
> batteries until dwmw2 actually stepped up. That alone says something.
> And we're at 2.6.19 about 15 years into development of Linux?

And we have sys_open. Not sys_x_ext2_open.

> You may or may not like it... but battery class drivers will have such
> non-standard things. I'm merely suggesting to tag these as non-standard
> so it's bloody evident they are non-standard. For the record, I also
> think that making non standard attributes ugly will help accelerate us
> in standardizing on it. You can also easier grep through the sources to
> find offending code when you do decide to standardize it.

You can simply _not merge offending code in the first place_.

"Lets design it so that stupid things can be grepped for" is stupid,
when we can simply "not allow stupid things to be merged".
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2006-10-28 21:54:36

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Sat, 28 Oct 2006, Pavel Machek wrote:
> > > Just put it into the name:
> > >
> > > power_avg_mV
> >
> > Bad idea... it means user space will have to try to open different files
> > and what happens when someone introduces a new unit? Ideally I'd like
> > the unit to be part of the payload of the sysfs file. Second to that I
> > think having the unit in a separate file is preferable.
>
> Introducing new unit *should* be hard. You know, when you introduce
> new unit, you automatically break all the userspace.

Well, I just wish whatever is done for battery is also done the same way for
ACPI when it moves to sysfs, and if at all possible, also to hwmon: we *are*
supposed to move stuff like ACPI temperatures to sysfs using hwmon
conventions, AFAIK.

That said, wearing a userspace app writer hat, I'd really prefer if it is
named in such a way that I can always extract the unit, like:

power_avg:mV or
power_avg[mV]

or whatever (and I'd prefer :mV a lot more than [mV], it is much cleaner).
LED seems already to be using ":" for such qualifiers (they use it for the
colors).

I can't just trust that the last _foo is the unit, as it might be something
that doesn't have an unit (it is the status quo in hwmon, for example). If
the kernel has this unit handling thing clearly defined, I can write a
generic application that displays all battery attributes beautifully,
instantly aware of the units (and even doing the intelligent thing if you
have both power_avg in uV and mV...)

> Having separate files is actually a *feature*. It allows you to
> introduce new units while providing backwards compatibility.

Agreed.

> Imagine going from mV to uV... With voltage_mV, you can have both
> voltage_mV and voltage_uV. In your system, you'd have to change value
> from mV to uV, breaking all the userspace....

I believe there is a school that says that "this is why userspace is
supposed to use a single library helper which will have the knowledge on how
to deal with this".

I am not defending such a library approach. But if the sysfs interface does
not have the units anywhere, it better be strictly versioned and export
that information somewhere, so that such a library is actually doable in a
sane and robust way.

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

2006-10-31 07:56:40

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Sat, Oct 28, 2006 at 08:12:41PM +0200, Shem Multinymous wrote:
> Hi David,
>
> On 10/28/06, David Zeuthen <[email protected]> wrote:
> >What about just prepending the unit to the 'threshold' file? Then user
> >space can expect the contents of said file to be of the form "%d %s". I
> >don't think that violates the "only one value per file" sysfs mantra.
>
> The tp_smapi battery driver did just this ("16495 mW"). But I dropped
> it in a recent version when Pavel pointed out the rest of sysfs, hwmon
> included, uses undecorated integers.
> Consistency aside, it seems reasonable and convenient. You have to
> decree that writes to the attributes (where relevant) don't include
> the units, of course, so no one will expect the kernel to parse that.
>
> There's an issue here if a drunk driver decides to specify (say)
> capacity_remaining in mWh and capacity_last_full in mAa, which will
> confuse anyone comparing those attributest. So don't do that.
>
> Jean, what's your opinion on letting hwmon-ish attributes specify
> units as "%d %s" where these are hardware-dependent?

No, the sysfs files should just always keep the same units as
documented. It's easier all around that way.

thanks,

greg k-h

2006-10-31 08:06:59

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On 10/28/2006, an anonymous coward wrote:
>On 10/28/06, David Zeuthen <[email protected]> wrote:
>> What about just prepending the unit to the 'threshold' file? Then user
>> space can expect the contents of said file to be of the form "%d %s". I
>> don't think that violates the "only one value per file" sysfs mantra.
>
>The tp_smapi battery driver did just this ("16495 mW"). But I dropped
>it in a recent version when Pavel pointed out the rest of sysfs, hwmon
>included, uses undecorated integers.
>Consistency aside, it seems reasonable and convenient. You have to
>decree that writes to the attributes (where relevant) don't include
>the units, of course, so no one will expect the kernel to parse that.

But what value should then be written? One in an absolute aribtrary unit?
That would make reads and writes to the sysfs files inconsistent, in
direct violation of the sysfs standard. Or in the same unit read from
the file? It means that userspace must first read from the file, parse
the unit, then convert the value to be written. This doesn't match my
definition of "convenient".

>There's an issue here if a drunk driver decides to specify (say)
>capacity_remaining in mWh and capacity_last_full in mAa, which will
>confuse anyone comparing those attributest. So don't do that.
>
>Jean, what's your opinion on letting hwmon-ish attributes specify
>units as "%d %s" where these are hardware-dependent?

I fail to see any benefit in doing so, while I see several problems (see
above) and potential for confusion. So my opinion is: please don't do
that.

Thanks,
--
Jean

2006-10-31 13:28:30

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Hi Greg,

On 10/31/06, Greg KH <[email protected]> wrote:
> > On 10/28/06, David Zeuthen <[email protected]> wrote:
> > >What about just prepending the unit to the 'threshold' file? Then user
> > >space can expect the contents of said file to be of the form "%d %s". I
> > >don't think that violates the "only one value per file" sysfs mantra.
> >
> > The tp_smapi battery driver did just this ("16495 mW"). But I dropped
> > it in a recent version when Pavel pointed out the rest of sysfs, hwmon
> > included, uses undecorated integers.
> > Consistency aside, it seems reasonable and convenient. You have to
> > decree that writes to the attributes (where relevant) don't include
> > the units, of course, so no one will expect the kernel to parse that.
> >
> > There's an issue here if a drunk driver decides to specify (say)
> > capacity_remaining in mWh and capacity_last_full in mAa, which will
> > confuse anyone comparing those attributest. So don't do that.
> >
> > Jean, what's your opinion on letting hwmon-ish attributes specify
> > units as "%d %s" where these are hardware-dependent?
>
> No, the sysfs files should just always keep the same units as
> documented. It's easier all around that way.

It sure is easier, but we're discussinng the case where units change
in runtime; what do we document then? Plug in a different battery and
you get reports in mA and mAh insted of mW and mWh.

The suggestions so far were:
1. Append units string to the content of such attribute:
/sys/.../capacity_remaining reads "16495 mW".
2. Add a seprate *_units attribute saying what are units for other attribute:
/sys/.../capacity_units gives the units for
/sys/.../capacity_{remaining,last_full,design,min,...}.
3. Append the units to the attribute names:
capacity_{remaining,last_full,design_min,...}:mV.

Shem

2006-10-31 13:42:14

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Hi Jean,

On 10/31/06, Jean Delvare <[email protected]> wrote:
> On 10/28/2006, someone who remains multinymous wrote:
> >On 10/28/06, David Zeuthen <[email protected]> wrote:
> >> What about just prepending the unit to the 'threshold' file? Then user
> >> space can expect the contents of said file to be of the form "%d %s". I
> >> don't think that violates the "only one value per file" sysfs mantra.

> >Jean, what's your opinion on letting hwmon-ish attributes specify
> >units as "%d %s" where these are hardware-dependent?
>
> I fail to see any benefit in doing so, while I see several problems (see
> above) and potential for confusion. So my opinion is: please don't do
> that.

Well, we have to do *something* about those devices that don't have
fixed units (see my mail to Greg from a few minutes ago), so which
alternative do you prefer?

> >The tp_smapi battery driver did just this ("16495 mW"). But I dropped
> >it in a recent version when Pavel pointed out the rest of sysfs, hwmon
> >included, uses undecorated integers.
> >Consistency aside, it seems reasonable and convenient. You have to
> >decree that writes to the attributes (where relevant) don't include
> >the units, of course, so no one will expect the kernel to parse that.
>
> But what value should then be written? One in an absolute aribtrary unit?
> That would make reads and writes to the sysfs files inconsistent, in
> direct violation of the sysfs standard. Or in the same unit read from
> the file? It means that userspace must first read from the file, parse
> the unit, then convert the value to be written. This doesn't match my
> definition of "convenient".

The latter. Yes, writing is not as convenient as reading; but the same
drawback exists in the other two suggestions for dynamic units - you
still have to read another attribute, or parse a filename, to get the
units.

The "capacity_remaining:mV" option at least saves you the parsing if
your're only interested in mV readouts. But battery gauge applets, for
examples, shouldn't be hardcoded to specific units, so they will have
to do the filename parsing dance.

Shem

2006-10-31 13:51:36

by Xavier Bestel

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Tue, 2006-10-31 at 15:42 +0200, Shem Multinymous wrote:
> > >Jean, what's your opinion on letting hwmon-ish attributes specify
> > >units as "%d %s" where these are hardware-dependent?
> >
> > I fail to see any benefit in doing so, while I see several problems (see
> > above) and potential for confusion. So my opinion is: please don't do
> > that.
>
> Well, we have to do *something* about those devices that don't have
> fixed units (see my mail to Greg from a few minutes ago), so which
> alternative do you prefer?

How about converting on the fly ?

Xav

2006-10-31 14:06:37

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On 10/31/06, Xavier Bestel <[email protected]> wrote:
> > Well, we have to do *something* about those devices that don't have
> > fixed units (see my mail to Greg from a few minutes ago), so which
> > alternative do you prefer?
>
> How about converting on the fly ?

In the case at hand we have mWh and mAh, which measure different
physical quantities. You can't convert between them unless you have
intimate knowledge of the battery's chemistry and condition, which we
don't.

And it would be nice to also allow for power supply devices that use
other, incompatible units like "percent" or "minutes" or "hand crank
revolutions".

Shem

2006-11-01 13:41:25

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Tue, 2006-10-31 at 16:06 +0200, Shem Multinymous wrote:
>
> In the case at hand we have mWh and mAh, which measure different
> physical quantities. You can't convert between them unless you have
> intimate knowledge of the battery's chemistry and condition, which we
> don't.

I'm thinking specifically for ACPI at the moment.

When ACPI can't work out a value, i.e. it's not known, it returns a
value of 0xFFFFFFFF. This can happen either for a split second on
disconnect, or if the hardware really doesn't know the value.

With the battery class driver, how would that be conveyed? Would the
sysfs file be deleted in this case, or would the value of the sysfs key
be something like "<invalid>".

This is something I'm just thinking about for the HAL backend, and
whatever is decided should probably be documented.

Richard.

2006-11-01 13:54:54

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Wed, 2006-11-01 at 13:26 +0000, Richard Hughes wrote:
> With the battery class driver, how would that be conveyed? Would the
> sysfs file be deleted in this case, or would the value of the sysfs
> key be something like "<invalid>".

I'd be inclined to make the read return -EINVAL.

--
dwmw2

2006-11-01 14:36:45

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Wed, 01 Nov 2006, David Woodhouse wrote:
> On Wed, 2006-11-01 at 13:26 +0000, Richard Hughes wrote:
> > With the battery class driver, how would that be conveyed? Would the
> > sysfs file be deleted in this case, or would the value of the sysfs
> > key be something like "<invalid>".
>
> I'd be inclined to make the read return -EINVAL.

-EIO for transient errors (e.g. access to the embedded controller/battery
charger/whatever fails at that instant), -EINVAL for "not supported"
(missing ACPI method, attribute not supported in the specific hardware)?

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

2006-11-01 16:36:41

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On 11/1/06, Henrique de Moraes Holschuh <[email protected]> wrote:
> On Wed, 01 Nov 2006, David Woodhouse wrote:
> > On Wed, 2006-11-01 at 13:26 +0000, Richard Hughes wrote:
> > > With the battery class driver, how would that be conveyed? Would the
> > > sysfs file be deleted in this case, or would the value of the sysfs
> > > key be something like "<invalid>".
> >
> > I'd be inclined to make the read return -EINVAL.
>
> -EIO for transient errors (e.g. access to the embedded controller/battery
> charger/whatever fails at that instant), -EINVAL for "not supported"
> (missing ACPI method, attribute not supported in the specific hardware)?

Shouldn't it be -EIO or -EBUSY for transient errors (depending on
type), and -ENXIO when not provided by hardware?
The -EINVAL is more appropriate for bad user-supplied values (out of
range etc.) to writable attributes.

Shem

2006-11-01 16:55:55

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Wed, 01 Nov 2006, Shem Multinymous wrote:
> On 11/1/06, Henrique de Moraes Holschuh <[email protected]> wrote:
> >On Wed, 01 Nov 2006, David Woodhouse wrote:
> >> On Wed, 2006-11-01 at 13:26 +0000, Richard Hughes wrote:
> >> > With the battery class driver, how would that be conveyed? Would the
> >> > sysfs file be deleted in this case, or would the value of the sysfs
> >> > key be something like "<invalid>".
> >>
> >> I'd be inclined to make the read return -EINVAL.
> >
> >-EIO for transient errors (e.g. access to the embedded controller/battery
> >charger/whatever fails at that instant), -EINVAL for "not supported"
> >(missing ACPI method, attribute not supported in the specific hardware)?
>
> Shouldn't it be -EIO or -EBUSY for transient errors (depending on
> type), and -ENXIO when not provided by hardware?

Sounds good to me, but I haven't seen much stuff returning -ENXIO. Still,
AFAIK usually when you don't support something in sysfs, you don't provide
the entry at all so ENXIO should be quite rare.

> The -EINVAL is more appropriate for bad user-supplied values (out of
> range etc.) to writable attributes.

Yes, that makes a lot of sense.

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

2006-11-01 19:32:09

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Tue, Oct 31, 2006 at 03:28:27PM +0200, Shem Multinymous wrote:
> Hi Greg,
>
> On 10/31/06, Greg KH <[email protected]> wrote:
> >> On 10/28/06, David Zeuthen <[email protected]> wrote:
> >> >What about just prepending the unit to the 'threshold' file? Then user
> >> >space can expect the contents of said file to be of the form "%d %s". I
> >> >don't think that violates the "only one value per file" sysfs mantra.
> >>
> >> The tp_smapi battery driver did just this ("16495 mW"). But I dropped
> >> it in a recent version when Pavel pointed out the rest of sysfs, hwmon
> >> included, uses undecorated integers.
> >> Consistency aside, it seems reasonable and convenient. You have to
> >> decree that writes to the attributes (where relevant) don't include
> >> the units, of course, so no one will expect the kernel to parse that.
> >>
> >> There's an issue here if a drunk driver decides to specify (say)
> >> capacity_remaining in mWh and capacity_last_full in mAa, which will
> >> confuse anyone comparing those attributest. So don't do that.
> >>
> >> Jean, what's your opinion on letting hwmon-ish attributes specify
> >> units as "%d %s" where these are hardware-dependent?
> >
> >No, the sysfs files should just always keep the same units as
> >documented. It's easier all around that way.
>
> It sure is easier, but we're discussinng the case where units change
> in runtime; what do we document then? Plug in a different battery and
> you get reports in mA and mAh insted of mW and mWh.

Then you should just get different sysfs files then. One that describes
power and one that describes current.

> The suggestions so far were:
> 1. Append units string to the content of such attribute:
> /sys/.../capacity_remaining reads "16495 mW".
> 2. Add a seprate *_units attribute saying what are units for other
> attribute:
> /sys/.../capacity_units gives the units for
> /sys/.../capacity_{remaining,last_full,design,min,...}.
> 3. Append the units to the attribute names:
> capacity_{remaining,last_full,design_min,...}:mV.

No, again, one for power and one for current. Two different files
depending on the type of battery present. That way there is no need to
worry about unit issues.

thanks,

greg k-h

2006-11-01 19:33:32

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Wed, Nov 01, 2006 at 01:26:17PM +0000, Richard Hughes wrote:
> On Tue, 2006-10-31 at 16:06 +0200, Shem Multinymous wrote:
> >
> > In the case at hand we have mWh and mAh, which measure different
> > physical quantities. You can't convert between them unless you have
> > intimate knowledge of the battery's chemistry and condition, which we
> > don't.
>
> I'm thinking specifically for ACPI at the moment.
>
> When ACPI can't work out a value, i.e. it's not known, it returns a
> value of 0xFFFFFFFF. This can happen either for a split second on
> disconnect, or if the hardware really doesn't know the value.
>
> With the battery class driver, how would that be conveyed? Would the
> sysfs file be deleted in this case, or would the value of the sysfs key
> be something like "<invalid>".

No, the sysfs file should just not be present.

thanks,

greg k-h

2006-11-01 19:53:16

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Hi Greg,

On 11/1/06, Greg KH <[email protected]> wrote:
> > The suggestions so far were:
> > 1. Append units string to the content of such attribute:
> > /sys/.../capacity_remaining reads "16495 mW".
> > 2. Add a seprate *_units attribute saying what are units for other
> > attribute:
> > /sys/.../capacity_units gives the units for
> > /sys/.../capacity_{remaining,last_full,design,min,...}.
> > 3. Append the units to the attribute names:
> > capacity_{remaining,last_full,design_min,...}:mV.
>
> No, again, one for power and one for current. Two different files
> depending on the type of battery present. That way there is no need to
> worry about unit issues.

I'm missing something. How is that different from option 3 above?
BTW, please note that we're talking about a large set of files that
use these units (remaining, last full, design capacity, alarm
thresholds, etc.), and not just a single attribute.

This particular alternative indeed seems cleanest for the kernel side.
The drawback is that someone in userspace who doesn't care about units
but just wants to show a status report or compute the amount of
remaining fooergy divided by the amount of a fooergy when fully
charged, like your typical battery applet, will need to parse
filenames (or try out a fixed and possibly partial list) to find out
which attribute files contain the numbers.

Shem

2006-11-01 20:53:57

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Wed, Nov 01, 2006 at 09:53:12PM +0200, Shem Multinymous wrote:
> Hi Greg,
>
> On 11/1/06, Greg KH <[email protected]> wrote:
> >> The suggestions so far were:
> >> 1. Append units string to the content of such attribute:
> >> /sys/.../capacity_remaining reads "16495 mW".
> >> 2. Add a seprate *_units attribute saying what are units for other
> >> attribute:
> >> /sys/.../capacity_units gives the units for
> >> /sys/.../capacity_{remaining,last_full,design,min,...}.
> >> 3. Append the units to the attribute names:
> >> capacity_{remaining,last_full,design_min,...}:mV.
> >
> >No, again, one for power and one for current. Two different files
> >depending on the type of battery present. That way there is no need to
> >worry about unit issues.
>
> I'm missing something. How is that different from option 3 above?

No silly ":mV" on the file name.

> BTW, please note that we're talking about a large set of files that
> use these units (remaining, last full, design capacity, alarm
> thresholds, etc.), and not just a single attribute.

Sure, what's wrong with:
capacity_remaining_power
capacity_last_full_power
capacity_design_min_power
if you can read that from the battery, and:
capacity_remaining_current
capacity_last_full_current
capacity_design_min_current
if you can read that instead.

> This particular alternative indeed seems cleanest for the kernel side.
> The drawback is that someone in userspace who doesn't care about units
> but just wants to show a status report or compute the amount of
> remaining fooergy divided by the amount of a fooergy when fully
> charged, like your typical battery applet, will need to parse
> filenames (or try out a fixed and possibly partial list) to find out
> which attribute files contain the numbers.

If the file isn't there, they the attribute isn't present. It doesn't
get easier than that.

And of course user apps will have to change, but only once. And then
they will work with all types of batterys, unlike today.

thanks,

greg k-h

2006-11-01 21:27:45

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Hi!

> >> The suggestions so far were:
> >> 1. Append units string to the content of such attribute:
> >> /sys/.../capacity_remaining reads "16495 mW".
> >> 2. Add a seprate *_units attribute saying what are units for other
> >> attribute:
> >> /sys/.../capacity_units gives the units for
> >> /sys/.../capacity_{remaining,last_full,design,min,...}.
> >> 3. Append the units to the attribute names:
> >> capacity_{remaining,last_full,design_min,...}:mV.
> >
> >No, again, one for power and one for current. Two different files
> >depending on the type of battery present. That way there is no need to
> >worry about unit issues.
>
> I'm missing something. How is that different from option 3 above?
> BTW, please note that we're talking about a large set of files that
> use these units (remaining, last full, design capacity, alarm
> thresholds, etc.), and not just a single attribute.
>
> This particular alternative indeed seems cleanest for the kernel side.
> The drawback is that someone in userspace who doesn't care about units
> but just wants to show a status report or compute the amount of
> remaining fooergy divided by the amount of a fooergy when fully
> charged, like your typical battery applet, will need to parse
> filenames (or try out a fixed and possibly partial list) to find out
> which attribute files contain the numbers.

That's okay, we want userspace to use common library, and doing

echo $[`cat capacity_remaining:*` / `cat capacity_total:*`]

is not exactly rocket science. If greg does not like units suffixes,
that's okay, too, I'm sure handy wildcard match will be possible.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2006-11-01 21:32:48

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Wed, 2006-11-01 at 22:27 +0100, Pavel Machek wrote:
> > The drawback is that someone in userspace who doesn't care about
> units
> > but just wants to show a status report or compute the amount of
> > remaining fooergy divided by the amount of a fooergy when fully
> > charged, like your typical battery applet, will need to parse
> > filenames (or try out a fixed and possibly partial list) to find out
> > which attribute files contain the numbers.
>
> That's okay, we want userspace to use common library, and doing
>
> echo $[`cat capacity_remaining:*` / `cat capacity_total:*`]
>
> is not exactly rocket science. If greg does not like units suffixes,
> that's okay, too, I'm sure handy wildcard match will be possible.

I'm guessing adding the new code to HAL will allow most stuff to keep
working without any changes. I think battstat-applet defaults to using
HAL, and I'm sure gnome-power-manager does. :-)

Richard.

2006-11-01 23:55:53

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

On Wed, 01 Nov 2006, Greg KH wrote:
> On Wed, Nov 01, 2006 at 09:53:12PM +0200, Shem Multinymous wrote:
> > Hi Greg,
> >
> > On 11/1/06, Greg KH <[email protected]> wrote:
> > >> The suggestions so far were:
> > >> 1. Append units string to the content of such attribute:
> > >> /sys/.../capacity_remaining reads "16495 mW".
> > >> 2. Add a seprate *_units attribute saying what are units for other
> > >> attribute:
> > >> /sys/.../capacity_units gives the units for
> > >> /sys/.../capacity_{remaining,last_full,design,min,...}.
> > >> 3. Append the units to the attribute names:
> > >> capacity_{remaining,last_full,design_min,...}:mV.
> > >
> > >No, again, one for power and one for current. Two different files
> > >depending on the type of battery present. That way there is no need to
> > >worry about unit issues.
> >
> > I'm missing something. How is that different from option 3 above?
>
> No silly ":mV" on the file name.

As long as that also means no "silly _mV" in the name. However, if the
choice is between :mV and _mV, please go with :mV.

> > BTW, please note that we're talking about a large set of files that
> > use these units (remaining, last full, design capacity, alarm
> > thresholds, etc.), and not just a single attribute.
>
> Sure, what's wrong with:
> capacity_remaining_power
> capacity_last_full_power
> capacity_design_min_power
> if you can read that from the battery, and:
> capacity_remaining_current
> capacity_last_full_current
> capacity_design_min_current
> if you can read that instead.

Well, "Wh" measures energy and not power, and "Ah" measures electric charge
and not current, so it would be better to make that:

capacity_*_energy (Wh-based)

and

capacity_*_charge (Ah-based)

Also, should we go with mWh/mAh, or with even smaller units because of the
tiny battery-driven devices of tomorrow?

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

2006-11-02 03:45:27

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

On Wed, Nov 01, 2006 at 08:55:40PM -0300, Henrique de Moraes Holschuh wrote:
> On Wed, 01 Nov 2006, Greg KH wrote:
> > On Wed, Nov 01, 2006 at 09:53:12PM +0200, Shem Multinymous wrote:
> > > Hi Greg,
> > >
> > > On 11/1/06, Greg KH <[email protected]> wrote:
> > > >> The suggestions so far were:
> > > >> 1. Append units string to the content of such attribute:
> > > >> /sys/.../capacity_remaining reads "16495 mW".
> > > >> 2. Add a seprate *_units attribute saying what are units for other
> > > >> attribute:
> > > >> /sys/.../capacity_units gives the units for
> > > >> /sys/.../capacity_{remaining,last_full,design,min,...}.
> > > >> 3. Append the units to the attribute names:
> > > >> capacity_{remaining,last_full,design_min,...}:mV.
> > > >
> > > >No, again, one for power and one for current. Two different files
> > > >depending on the type of battery present. That way there is no need to
> > > >worry about unit issues.
> > >
> > > I'm missing something. How is that different from option 3 above?
> >
> > No silly ":mV" on the file name.
>
> As long as that also means no "silly _mV" in the name. However, if the
> choice is between :mV and _mV, please go with :mV.

Neither should be in the name.

Again, people, look how we already do this in the kernel today with the
hwmon drivers. They all just document the units and that's it. No
problems.

> > > BTW, please note that we're talking about a large set of files that
> > > use these units (remaining, last full, design capacity, alarm
> > > thresholds, etc.), and not just a single attribute.
> >
> > Sure, what's wrong with:
> > capacity_remaining_power
> > capacity_last_full_power
> > capacity_design_min_power
> > if you can read that from the battery, and:
> > capacity_remaining_current
> > capacity_last_full_current
> > capacity_design_min_current
> > if you can read that instead.
>
> Well, "Wh" measures energy and not power, and "Ah" measures electric charge
> and not current, so it would be better to make that:
>
> capacity_*_energy (Wh-based)
>
> and
>
> capacity_*_charge (Ah-based)

Ok, that's fine with me.

> Also, should we go with mWh/mAh, or with even smaller units because of the
> tiny battery-driven devices of tomorrow?

Well, what do the tiny battery-driven devices of today provide (like the
Nokia 770, other cell phones, smart hand-helds, etc.) Those should give
you a good idea if that would be needed or not.

thanks,

greg k-h

2006-11-02 07:57:53

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On 10/31/2006, man with no name wrote:
> In the case at hand we have mWh and mAh, which measure different
> physical quantities. You can't convert between them unless you have
> intimate knowledge of the battery's chemistry and condition, which we
> don't.

You just need to know the voltage of the battery, what else?

> And it would be nice to also allow for power supply devices that use
> other, incompatible units like "percent" or "minutes" or "hand crank
> revolutions".

Do such batteries exist at the moment, or are you just speculating? I
don't quite see how a battery could report remaining energy in time
units, as power consumption varies over time. Hand crank revolutions
wouldn't be a very useful unit either, unless you know how much energy
a revolution provides, and then you can just convert it. Percent would
make some sense, but you can only express the remaining energy this way,
not the total. And if you know the total in mAh or mWh, you can multiply
by the percentage and you get the remaining energy in the same unit.

--
Jean Delvare

2006-11-02 08:39:45

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Thu, 2006-11-02 at 08:52 +0100, Jean Delvare wrote:
> You just need to know the voltage of the battery, what else?

This isn't as easy as you think. The number of broken BIOS's that have
the current, last full or design either as 0xFFFF, 0xFFFFFF, 0, 1mV, or
10000mV rather than the present (correct) value is a big problem.

We've got quite a bit of code in HAL to try and sort out all the quirks,
although it only works most of the time due to extreme hardware
brokenness.

Richard.

2006-11-02 13:54:29

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Thu, 02 Nov 2006, Jean Delvare wrote:
> On 10/31/2006, man with no name wrote:
> > In the case at hand we have mWh and mAh, which measure different
> > physical quantities. You can't convert between them unless you have
> > intimate knowledge of the battery's chemistry and condition, which we
> > don't.
>
> You just need to know the voltage of the battery, what else?

The error goes way up when you do such calculations. Not that most battery
hardware reports SBS Error margins right, but still...

So doing conversions is not a good idea unless it is from Ah to Coulombs or
something else like that which is an exact conversion.

In ThinkPads, you just need to compare what the various "let's calculate it"
applets say, and the output of tp_smapi (gets remanining time data directly
from the hardware) to see which one is more accurate :-) And the difference
is often quite expressive.

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

2006-11-02 17:51:09

by Bill Davidsen

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

Henrique de Moraes Holschuh wrote:
> On Wed, 01 Nov 2006, Greg KH wrote:
>> On Wed, Nov 01, 2006 at 09:53:12PM +0200, Shem Multinymous wrote:
>>> Hi Greg,
>>>
>>> On 11/1/06, Greg KH <[email protected]> wrote:
>>>>> The suggestions so far were:
>>>>> 1. Append units string to the content of such attribute:
>>>>> /sys/.../capacity_remaining reads "16495 mW".
>>>>> 2. Add a seprate *_units attribute saying what are units for other
>>>>> attribute:
>>>>> /sys/.../capacity_units gives the units for
>>>>> /sys/.../capacity_{remaining,last_full,design,min,...}.
>>>>> 3. Append the units to the attribute names:
>>>>> capacity_{remaining,last_full,design_min,...}:mV.
>>>> No, again, one for power and one for current. Two different files
>>>> depending on the type of battery present. That way there is no need to
>>>> worry about unit issues.
>>> I'm missing something. How is that different from option 3 above?
>> No silly ":mV" on the file name.
>
> As long as that also means no "silly _mV" in the name. However, if the
> choice is between :mV and _mV, please go with :mV.
>
>>> BTW, please note that we're talking about a large set of files that
>>> use these units (remaining, last full, design capacity, alarm
>>> thresholds, etc.), and not just a single attribute.
>> Sure, what's wrong with:
>> capacity_remaining_power
>> capacity_last_full_power
>> capacity_design_min_power
>> if you can read that from the battery, and:
>> capacity_remaining_current
>> capacity_last_full_current
>> capacity_design_min_current
>> if you can read that instead.
>
> Well, "Wh" measures energy and not power, and "Ah" measures electric charge
> and not current, so it would be better to make that:
>
> capacity_*_energy (Wh-based)
>
> and
>
> capacity_*_charge (Ah-based)
>
> Also, should we go with mWh/mAh, or with even smaller units because of the
> tiny battery-driven devices of tomorrow?
>
Having seen a French consultant with a Windows laptop reporting mJ
(Joules) I bet that came from the hardware. And given that laptop
batteries run at (almost) constant voltage, could all of these just be
converted to mWh for consistency?

--
Bill Davidsen <[email protected]>
Obscure bug of 2004: BASH BUFFER OVERFLOW - if bash is being run by a
normal user and is setuid root, with the "vi" line edit mode selected,
and the character set is "big5," an off-by-one errors occurs during
wildcard (glob) expansion.

2006-11-02 17:53:41

by Bill Davidsen

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

Jean Delvare wrote:
> On 10/31/2006, man with no name wrote:
>> In the case at hand we have mWh and mAh, which measure different
>> physical quantities. You can't convert between them unless you have
>> intimate knowledge of the battery's chemistry and condition, which we
>> don't.
>
> You just need to know the voltage of the battery, what else?
>
>> And it would be nice to also allow for power supply devices that use
>> other, incompatible units like "percent" or "minutes" or "hand crank
>> revolutions".
>
> Do such batteries exist at the moment, or are you just speculating?

I have seen joules (or mJ) on a laptop. Yes, it was Windows, but I bet
the report came from hardware. Some vendor getting anal about metric?
> I
> don't quite see how a battery could report remaining energy in time
> units, as power consumption varies over time. Hand crank revolutions
> wouldn't be a very useful unit either, unless you know how much energy
> a revolution provides, and then you can just convert it. Percent would
> make some sense, but you can only express the remaining energy this way,
> not the total. And if you know the total in mAh or mWh, you can multiply
> by the percentage and you get the remaining energy in the same unit.
>
> --
> Jean Delvare

--
Bill Davidsen <[email protected]>
Obscure bug of 2004: BASH BUFFER OVERFLOW - if bash is being run by a
normal user and is setuid root, with the "vi" line edit mode selected,
and the character set is "big5," an off-by-one errors occurs during
wildcard (glob) expansion.

2006-11-02 19:19:55

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

On Thu, 2006-11-02 at 12:49 -0500, Bill Davidsen wrote:
> And given that laptop
> batteries run at (almost) constant voltage

No, they really don't.

Richard.

2006-11-02 19:27:19

by Richard B. Johnson

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

----- Original Message -----
From: "Bill Davidsen" <[email protected]>
To: "Jean Delvare" <[email protected]>
Cc: <[email protected]>; "Richard Hughes" <[email protected]>; "David
Woodhouse" <[email protected]>; "Dan Williams" <[email protected]>;
<[email protected]>; <[email protected]>; <[email protected]>;
<[email protected]>; <[email protected]>; <[email protected]>;
"linux-thinkpad mailing list" <[email protected]>; "Pavel
Machek" <[email protected]>
Sent: Thursday, November 02, 2006 12:52 PM
Subject: Re: [PATCH v2] Re: Battery class driver.

> Jean Delvare wrote:
>> On 10/31/2006, man with no name wrote:
>>> In the case at hand we have mWh and mAh, which measure different
>>> physical quantities. You can't convert between them unless you have
>>> intimate knowledge of the battery's chemistry and condition, which we
>>> don't.
>>
>> You just need to know the voltage of the battery, what else?
>>
>>> And it would be nice to also allow for power supply devices that use
>>> other, incompatible units like "percent" or "minutes" or "hand crank
>>> revolutions".
>>
>> Do such batteries exist at the moment, or are you just speculating?
>
> I have seen joules (or mJ) on a laptop. Yes, it was Windows, but I bet the
> report came from hardware. Some vendor getting anal about metric?

The only thing that makes sense with batteries is the total amount of energy
available. Such
energy has the dimension of watt-seconds, i.e., joules, even though a
battery might be rated in ampere-hours.
This is because you can't even guess at the amount of energy remaining by
looking at a battery's voltage. To
get joules, you need to multiply the voltage times the current times the
time for all time, both the charge
time and the discharge time. When charging, the charging efficiency needs
to be taken into account as well.
The charging efficiency varies with the battery chemistry, its type, and its
stored energy. For instance, a fully
charged battery has zero charging efficiency (any current supplied is
converted to heat). A typical battery at
about one-half its capacity converts over 90% of the applied charge to
stored energy.

No known laptop bothers to do this. That's why the batteries fail at the
most inoportune times
and why it will decide to shut down when it feels like it, based totally
upon some detected
voltage drop when a disk-drive started.

Analogic makes a portable CAT Scanner. It can be plugged into an ordinary
wall outlet even though
the X-Ray subsystem takes 10 kilowatts! It does this by storing the needed
energy in batteries. Since
the X-Ray is on only for a short period of time, the system has plenty of
time to charge the batteries
while the image is being processed or reviewed. Since it is against FDA
regulations to expose a
patient to X-Rays unless diagnistically-useful images result, it is
mandatory that the charge state
of the batteries be known at all times so that an image sequence once
started, is guaranteed to
complete. For this, we have a software sampler that calculates the charge
about 1,000 per second.
It does nor assume that the samples are at millisecond intervals, it reads a
hardware timer to get
the elapsed time between each sample. It measures the voltage, the current,
and the time each
sample interval. It accumulates these samples into a charge variable with
the dimension of joules.

> > I
>> don't quite see how a battery could report remaining energy in time
>> units, as power consumption varies over time. Hand crank revolutions
>> wouldn't be a very useful unit either, unless you know how much energy
>> a revolution provides, and then you can just convert it. Percent would
>> make some sense, but you can only express the remaining energy this way,
>> not the total. And if you know the total in mAh or mWh, you can multiply
>> by the percentage and you get the remaining energy in the same unit.
>>
>> --
>> Jean Delvare
>
>
> --
> Bill Davidsen <[email protected]>
> Obscure bug of 2004: BASH BUFFER OVERFLOW - if bash is being run by a
> normal user and is setuid root, with the "vi" line edit mode selected,
> and the character set is "big5," an off-by-one errors occurs during
> wildcard (glob) expansion.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

Cheers,
Dick Johnson
Penguin : Linux version 2.6.16.24 (somewhere) -- They shut off my email at
work!
Book: http://www.AbominableFireug.com

2006-11-02 21:22:09

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

Hi!

> >Well, "Wh" measures energy and not power, and "Ah"
> >measures electric charge
> >and not current, so it would be better to make that:
> >
> >capacity_*_energy (Wh-based)
> >
> >and
> >
> >capacity_*_charge (Ah-based)
> >
> >Also, should we go with mWh/mAh, or with even smaller
> >units because of the
> >tiny battery-driven devices of tomorrow?
> >
> Having seen a French consultant with a Windows laptop
> reporting mJ (Joules) I bet that came from the hardware.
> And given that laptop batteries run at (almost) constant
> voltage, could all of these just be converted to mWh for
> consistency?

li-ions run from 4.2V down to 3.6V without problems, and you can use
them down to 3.0V. I've ran zaurus down to 3.3V, IIRC. That's quite a
big range.

--
Thanks for all the (sleeping) penguins.

2006-11-02 22:02:08

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

Hi!

> Well, "Wh" measures energy and not power, and "Ah" measures electric charge
> and not current, so it would be better to make that:
>
> capacity_*_energy (Wh-based)
>
> and
>
> capacity_*_charge (Ah-based)
>
> Also, should we go with mWh/mAh, or with even smaller units because of the
> tiny battery-driven devices of tomorrow?

Okay... So I have cellphone here, 700mAh battery, ~2.8Wh battery. It
could last 14 days if we were *very* careful.

echo '(700 mA * hour) / (14*day) \ A' | ucalc

ucalc> OK: 0.002083
ucalc>

...that is about 2mA in low power standby mode (but still listening on
GSM, getting calls, etc).

...so, mAh are probably good enough for capacity_*_charge, but would
suck for current power consumption, as difference between 2mA and 3mA
would be way too big. We need some finer unit in that case.

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

2006-11-03 12:46:13

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

On Thu, 02 Nov 2006, Bill Davidsen wrote:
> Having seen a French consultant with a Windows laptop reporting mJ
> (Joules) I bet that came from the hardware. And given that laptop
> batteries run at (almost) constant voltage, could all of these just be
> converted to mWh for consistency?

*No*. That adds quite a lot of error, which can be easily avoided by
providing the _charge and _energy attribute sets.

You can convert between J and Wh and between C to Ah without significant
precision loss. Just don't go cheap on the fixed point calculations, and
make sure the destination unit is small enough not to forsake precision.

We can definately be safe from any precision loss using (10^-6) * (A, Ah, W,
Wh, V) as the base unit, but that will make for long numbers in sysfs with
lots of zeros in many situations (which is MUCH better than precision loss).

We could also use a proper submultiple of J and C instead of Wh and Ah if
we'd rather stick to the SI, that wouldn't be a big problem at all.

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

2006-11-03 13:17:52

by U Kuehn

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

Hi,

Pavel Machek wrote:
>
> echo '(700 mA * hour) / (14*day) \ A' | ucalc
>
> ucalc> OK: 0.002083
> ucalc>
>
> ...that is about 2mA in low power standby mode (but still listening on
> GSM, getting calls, etc).
>
> ...so, mAh are probably good enough for capacity_*_charge, but would
> suck for current power consumption, as difference between 2mA and 3mA
> would be way too big. We need some finer unit in that case.
>

That very much depends on the system. Having a laptop where the current
power consumption is around 10 Watts (or, at about 10 to 12 Volts,
nearly 1 A), having a resolution of 10 or even 100 mA would be OK.
However, on your cellphone with a standby consumption of 2mA, such a
resolution would be meaningless. What kind of resultion does the
hardware usually support?

And then there is the measurement error. Any ideas about what the actual
error ranges are?

Cheers
Ulrich

2006-11-03 13:23:52

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

On Thu, 02 Nov 2006, Richard B. Johnson wrote:
> No known laptop bothers to do this. That's why the batteries fail at the
> most inoportune times and why it will decide to shut down when it feels
> like it, based totally upon some detected voltage drop when a disk-drive
> started.

Weird, I though the whole point behind a SBS hardware stack requiring
something fairly intelligent in the battery pack and allowing for (runtime
switchable!) Ah or Wh modes of operation was to allow vendors to do exactly
that: measure (V,A) permanently while the cells are above the safety cut-off
fuse level, and accumulate it...

Well, IBM embedded a microcontroller of some sort on every SBS ThinkPad
battery pack, and the ThinkPad reports battery data in Wh, so I expected it
to actually do the hard work to know how much energy is still left in the
pack... especially given how much $$$ they want for the packs :-) I have no
idea of what software is really running inside the battery pack, of course,
so maybe the SBS battery EC just sits there doing something else instead of
taking real-time measurements of the battery charge (that wouldn't surprise
me too much...).

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

2006-11-03 14:21:24

by Richard B. Johnson

[permalink] [raw]

Subject: Re: [PATCH v2] Re: Battery class driver.

----- Original Message -----
From: "Henrique de Moraes Holschuh" <[email protected]>
To: "Richard B. Johnson" <[email protected]>
Cc: "Bill Davidsen" <[email protected]>; "Jean Delvare" <[email protected]>;
<[email protected]>; "Richard Hughes" <[email protected]>; "David
Woodhouse" <[email protected]>; "Dan Williams" <[email protected]>;
<[email protected]>; <[email protected]>; <[email protected]>;
<[email protected]>; <[email protected]>; <[email protected]>;
"linux-thinkpad mailing list" <[email protected]>; "Pavel
Machek" <[email protected]>
Sent: Friday, November 03, 2006 8:23 AM
Subject: Re: [PATCH v2] Re: Battery class driver.

> On Thu, 02 Nov 2006, Richard B. Johnson wrote:
>> No known laptop bothers to do this. That's why the batteries fail at the
>> most inoportune times and why it will decide to shut down when it feels
>> like it, based totally upon some detected voltage drop when a disk-drive
>> started.
>
> Weird, I though the whole point behind a SBS hardware stack requiring
> something fairly intelligent in the battery pack and allowing for (runtime
> switchable!) Ah or Wh modes of operation was to allow vendors to do
> exactly
> that: measure (V,A) permanently while the cells are above the safety
> cut-off
> fuse level, and accumulate it...
>
> Well, IBM embedded a microcontroller of some sort on every SBS ThinkPad
> battery pack, and the ThinkPad reports battery data in Wh, so I expected
> it
> to actually do the hard work to know how much energy is still left in the
> pack... especially given how much $$$ they want for the packs :-) I have
> no
> idea of what software is really running inside the battery pack, of
> course,
> so maybe the SBS battery EC just sits there doing something else instead
> of
> taking real-time measurements of the battery charge (that wouldn't
> surprise
> me too much...).

I'm not sure anybody actually embeds a micro. There is some chip, originally
make by National, that
was supposed to monitor the battery state. I know that I have used five
laptops so far and have
never been able to obtain any intellegent operation. They just shut down
when they feel like it.
They do go to "suspend" mode to save power as well, always at the most
inopertune moment.

Maybe the "ThinkPad" actually has some intellegence within. The cost of the
batteries only reflects
the cost of defending lawsuits <grin> and not the cost of its components.
Batteries made in
China seem to become "excited" at inopertune times.

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

Cheers,
Dick Johnson
Penguin : Linux version 2.6.16.24 (somewhere). IT removed email "privileges"
for engineers!
New Book: http://www.AbominableFirebug.com

2006-11-03 15:19:25

by Stefan Seyfried

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

On Thu, Nov 02, 2006 at 12:49:54PM -0500, Bill Davidsen wrote:

> Having seen a French consultant with a Windows laptop reporting mJ

Hehe.... That's the nice thing about SI units.

1W = 1J / 1s

so

1W * 1h = 3600s * 1J / 1s
1mWh = 3.6J

If i did not screw up something :-). I still wonder why they would
report mJ, but probably they like big numbers ;-)
--
Stefan Seyfried
QA / R&D Team Mobile Devices | "Any ideas, John?"
SUSE LINUX Products GmbH, N?rnberg | "Well, surrounding them's out."

2006-11-03 16:11:03

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

On Fri, 03 Nov 2006, Richard B. Johnson wrote:
> I'm not sure anybody actually embeds a micro. There is some chip,

I'd have to crack open a ThinkPad battery to know for sure, as well. That's
why I used "some sort of embedded controller"... I am operating on
second-hand data.

Still, stuff like http://focus.ti.com/docs/prod/folders/print/bq20z90.html
gives me some hope the battery packs have some good stuff in it.

> originally make by National, that was supposed to monitor the battery
> state. I know that I have used five laptops so far and have never been
> able to obtain any intellegent operation. They just shut down when they
> feel like it. They do go to "suspend" mode to save power as well, always
> at the most inopertune moment.

Try a ThinkPad, they are a bit better than that :-) Of course, you have to
disable (or properly configure) stuff that tries to outguess you and are not
really enabled to talk to ThinkPad specifics like what comes with KDE and
GNOME, if you don't want to get surprise suspends.

I am not sure how better a recent ThinkPad (T42 or newer) behaviour would be
by your standards, but still...

> Maybe the "ThinkPad" actually has some intellegence within. The cost of
> the batteries only reflects the cost of defending lawsuits <grin> and not
> the cost of its components. Batteries made in China seem to become
> "excited" at inopertune times.

Well, all of mine come with "made in Japan" seals from Sanyo. I sure hope
this means the cells themselves are not from some third-party with shoddy
quality control and manufacturing processes.

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

2006-11-05 20:52:56

[permalink] [raw]

Subject: Re: [ltp] Re: [PATCH v2] Re: Battery class driver.

On Fri 2006-11-03 14:12:22, U Kuehn wrote:
> Hi,
>
> Pavel Machek wrote:
> >
> > echo '(700 mA * hour) / (14*day) \ A' | ucalc
> >
> > ucalc> OK: 0.002083
> > ucalc>
> >
> > ...that is about 2mA in low power standby mode (but still listening on
> > GSM, getting calls, etc).
> >
> > ...so, mAh are probably good enough for capacity_*_charge, but would
> > suck for current power consumption, as difference between 2mA and 3mA
> > would be way too big. We need some finer unit in that case.
> >
>
> That very much depends on the system. Having a laptop where the current
> power consumption is around 10 Watts (or, at about 10 to 12 Volts,
> nearly 1 A), having a resolution of 10 or even 100 mA would be OK.
> However, on your cellphone with a standby consumption of 2mA, such a
> resolution would be meaningless. What kind of resultion does the
> hardware usually support?

I do not know details. Siemens phones have current monitors, but I do
not recall how accurate those are. Anyway, at least for some
applications mA is not enough, so we probably should use finer
unit. Or use ampers and let kernel be "as precise as it needs" in
simulated floating point.
Pavel

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

2006-11-05 21:02:54