I am not exactly sure about this one ... what other power_supply class drivers
do? Should I fix HAL instead (but then, I do not know whether HAL is the only
application that is using this interface).
Andrey Borzenkov wrote:
> I am not exactly sure about this one ... what other power_supply class drivers
> do? Should I fix HAL instead (but then, I do not know whether HAL is the only
> application that is using this interface).
>
>
Hm, do you need separate set of properties for that? You could register either
of existing two, and read function will not allow read of anything but "present".
IMHO, this is what other modules do (/drivers/power)
One remaining trick here, you need to call unregister/register for power_supply
if you change attributes -- so please check if your patched driver survives
insertion of the battery.
Regards,
Alex.
On Saturday 27 October 2007, Alexey Starikovskiy wrote:
> Andrey Borzenkov wrote:
> > I am not exactly sure about this one ... what other power_supply class
> > drivers do? Should I fix HAL instead (but then, I do not know whether HAL
> > is the only application that is using this interface).
>
> Hm, do you need separate set of properties for that? You could register
> either of existing two, and read function will not allow read of anything
> but "present". IMHO, this is what other modules do (/drivers/power)
Do they have different set of properties depending on underlying hardware that
you can't query unless hardware is present? I'd rather avoid adding fake
attributes; but I do not actually care so which one do you prefer? :)
> One remaining trick here, you need to call unregister/register for
> power_supply if you change attributes -- so please check if your patched
> driver survives insertion of the battery.
>
Neither does your code (nor kpowersave :) ) Remove battery and set of
attributes is "stuck" instead of being reset to only fixed set of power
device attributes (basically "info"). The only call to power_supply_register
is in acpi_battery_add and as far as I can tell this is executed on adding
*slot* not when content of this slot changes.
Andrey Borzenkov wrote:
> On Saturday 27 October 2007, Alexey Starikovskiy wrote:
>> Andrey Borzenkov wrote:
>>> I am not exactly sure about this one ... what other power_supply class
>>> drivers do? Should I fix HAL instead (but then, I do not know whether HAL
>>> is the only application that is using this interface).
>> Hm, do you need separate set of properties for that? You could register
>> either of existing two, and read function will not allow read of anything
>> but "present". IMHO, this is what other modules do (/drivers/power)
>
> Do they have different set of properties depending on underlying hardware that
> you can't query unless hardware is present? I'd rather avoid adding fake
> attributes; but I do not actually care so which one do you prefer? :)
I prefer less code :)
>
>> One remaining trick here, you need to call unregister/register for
>> power_supply if you change attributes -- so please check if your patched
>> driver survives insertion of the battery.
>>
>
>
> Neither does your code (nor kpowersave :) ) Remove battery and set of
> attributes is "stuck" instead of being reset to only fixed set of power
> device attributes (basically "info"). The only call to power_supply_register
> is in acpi_battery_add and as far as I can tell this is executed on adding
> *slot* not when content of this slot changes.
Point is -- it does not break :) If you start to play with shorter length of
attributes table and don't call unregister/register, power_supply may
go past the end of table.
On Sat, Oct 27, 2007 at 08:54:30PM +0400, Andrey Borzenkov wrote:
> I am not exactly sure about this one ... what other power_supply class drivers
> do? Should I fix HAL instead (but then, I do not know whether HAL is the only
> application that is using this interface).
Well, PROP_PRESENT wasn't my idea, currently it's used by pmu and
olpc drivers becuase it's not trivial to register/unregister their
batteries on physical insertion/removal. I have some plans to teach
at least pmu batteries to not use PROP_PRESENT. I don't have any
OLPC, thus I can't convert it.
To sum this: the good way to handle "missing" batteries is to
unregister them, so they'll not show up in the /sys/class/power_supply.
Is that possible with the ACPI?
The good example is ds2760 batteries:
drivers/power/ds2760_battery.c - is platform device
drivers/w1/slaves/w1_ds2760.c - is w1 slave, which
registers/unregisters pdevs on the detection/removal.
> Subject: [PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent
Bad idea. Don't use present attribute, if possible.
> From: Andrey Borzenkov <[email protected]>
>
> Ensure that we always have "present" attribute in sysfs. This is compatible
> with procfs case where we had "present: no" if battery was not available.
>
> This fixes HAL battery detection where it does pretend battery is present
> but canot provide any value for it.
>
> Signed-off-by: Andrey Borzenkov <[email protected]>
>
> ---
>
> drivers/acpi/battery.c | 11 ++++++++++-
> 1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 681e26b..6e67fcd 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -187,6 +187,10 @@ static int acpi_battery_get_property(struct power_supply *psy,
> return 0;
> }
>
> +static enum power_supply_property missing_battery_props[] = {
> + POWER_SUPPLY_PROP_PRESENT,
> +};
> +
> static enum power_supply_property charge_battery_props[] = {
> POWER_SUPPLY_PROP_STATUS,
> POWER_SUPPLY_PROP_PRESENT,
> @@ -389,8 +393,13 @@ static int acpi_battery_update(struct acpi_battery *battery)
> {
> int saved_present = acpi_battery_present(battery);
> int result = acpi_battery_get_status(battery);
> - if (result || !acpi_battery_present(battery))
> + if (result)
> return result;
> + if (!acpi_battery_present(battery)) {
> + battery->bat.properties = missing_battery_props;
> + battery->bat.num_properties = ARRAY_SIZE(missing_battery_props);
> + return result;
> + }
> if (saved_present != acpi_battery_present(battery) ||
> !battery->update_time) {
> battery->update_time = 0;
--
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2
On Sat, 2007-10-27 at 22:42 +0400, Anton Vorontsov wrote:
> Well, PROP_PRESENT wasn't my idea, currently it's used by pmu and
> olpc drivers becuase it's not trivial to register/unregister their
> batteries on physical insertion/removal. I have some plans to teach
> at least pmu batteries to not use PROP_PRESENT. I don't have any
> OLPC, thus I can't convert it.
Actually it's not hard to do that. It was done this way in response to a
request from the userspace side. IIRC.
--
dwmw2
On Sat, Oct 27, 2007 at 03:32:04PM -0400, David Woodhouse wrote:
> On Sat, 2007-10-27 at 22:42 +0400, Anton Vorontsov wrote:
> > Well, PROP_PRESENT wasn't my idea, currently it's used by pmu and
> > olpc drivers becuase it's not trivial to register/unregister their
> > batteries on physical insertion/removal. I have some plans to teach
> > at least pmu batteries to not use PROP_PRESENT. I don't have any
> > OLPC, thus I can't convert it.
>
> Actually it's not hard to do that.
I didn't say it's hard. But we don't have any interrupts for the
battery events, thus we have to implement polling.
> It was done this way in response to a
> request from the userspace side. IIRC.
Oh. Userspace tried to do something weird then, I think. Generally
it's just sane to unregister absent hardware.
--
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2
On Saturday 27 October 2007, Anton Vorontsov wrote:
> On Sat, Oct 27, 2007 at 08:54:30PM +0400, Andrey Borzenkov wrote:
> > I am not exactly sure about this one ... what other power_supply class
> > drivers do? Should I fix HAL instead (but then, I do not know whether HAL
> > is the only application that is using this interface).
>
> Well, PROP_PRESENT wasn't my idea, currently it's used by pmu and
> olpc drivers becuase it's not trivial to register/unregister their
> batteries on physical insertion/removal. I have some plans to teach
> at least pmu batteries to not use PROP_PRESENT. I don't have any
> OLPC, thus I can't convert it.
>
> To sum this: the good way to handle "missing" batteries is to
> unregister them, so they'll not show up in the /sys/class/power_supply.
Well, in this case HAL behaviour makes sense (default to present == true
if "present" attribute is missing)
> Is that possible with the ACPI?
>
At least looking in ACPI specs, this looks possible. What currently is
presented as battery object is actually battery bay according to ACPI spec:
Unlike most other devices, when a battery is inserted or removed from the
system, the device itself (the
battery bay) is still considered to be present in the system.
When battery is inserted/removed, ACPI notifies us and we can check whether
battery is actually present and update registration accordingly. From the
sysfs structure POV probably nothing has to be changed; just when and how
power_supply is registered
under /sys/devices/LNXSYSTM:00/device:00/PNP0C0A:0n
Alexey, does it make sense (or doable)?
On Sunday 28 October 2007, Andrey Borzenkov wrote:
> On Saturday 27 October 2007, Anton Vorontsov wrote:
> > On Sat, Oct 27, 2007 at 08:54:30PM +0400, Andrey Borzenkov wrote:
> > > I am not exactly sure about this one ... what other power_supply class
> > > drivers do? Should I fix HAL instead (but then, I do not know whether
> > > HAL is the only application that is using this interface).
> >
> > Well, PROP_PRESENT wasn't my idea, currently it's used by pmu and
> > olpc drivers becuase it's not trivial to register/unregister their
> > batteries on physical insertion/removal. I have some plans to teach
> > at least pmu batteries to not use PROP_PRESENT. I don't have any
> > OLPC, thus I can't convert it.
> >
> > To sum this: the good way to handle "missing" batteries is to
> > unregister them, so they'll not show up in the /sys/class/power_supply.
>
> Well, in this case HAL behaviour makes sense (default to present == true
> if "present" attribute is missing)
>
> > Is that possible with the ACPI?
>
> At least looking in ACPI specs, this looks possible. What currently is
> presented as battery object is actually battery bay according to ACPI spec:
>
> Unlike most other devices, when a battery is inserted or removed from the
> system, the device itself (the
> battery bay) is still considered to be present in the system.
>
> When battery is inserted/removed, ACPI notifies us and we can check whether
> battery is actually present and update registration accordingly. From the
> sysfs structure POV probably nothing has to be changed; just when and how
> power_supply is registered
> under /sys/devices/LNXSYSTM:00/device:00/PNP0C0A:0n
>
> Alexey, does it make sense (or doable)?
or would you take attached patch as prototype? :) This works on my system and
does not crash it immediately. Here is event sequence I get:
UEVENT[1193556388.161539] add /module/battery (module)
ACTION=add
DEVPATH=/module/battery
SUBSYSTEM=module
SEQNUM=3243
UEVENT[1193556388.177069] add /bus/acpi/drivers/battery (drivers)
ACTION=add
DEVPATH=/bus/acpi/drivers/battery
SUBSYSTEM=drivers
SEQNUM=3244
UEVENT[1193556388.212721]
add /devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
(power_supply)
ACTION=add
DEVPATH=/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
SUBSYSTEM=power_supply
SEQNUM=3245
UEVENT[1193556388.223113]
change /devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
(power_supply)
ACTION=change
DEVPATH=/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
SUBSYSTEM=power_supply
POWER_SUPPLY_NAME=BAT1
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_STATUS=Full
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_TECHNOLOGY=Unknown
POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
POWER_SUPPLY_VOLTAGE_NOW=0
POWER_SUPPLY_CURRENT_NOW=0
POWER_SUPPLY_ENERGY_FULL_DESIGN=38880000
POWER_SUPPLY_ENERGY_FULL=37530000
POWER_SUPPLY_ENERGY_NOW=0
POWER_SUPPLY_MODEL_NAME=XM2038P04
POWER_SUPPLY_MANUFACTURER=
SEQNUM=3246
physically remove battery
UEVENT[1193556483.326303]
remove /devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
(power_supply)
ACTION=remove
DEVPATH=/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
SUBSYSTEM=power_supply
POWER_SUPPLY_NAME=BAT1
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_PRESENT=0
SEQNUM=3252
battery back
UEVENT[1193556510.339045]
add /devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
(power_supply)
ACTION=add
DEVPATH=/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
SUBSYSTEM=power_supply
SEQNUM=3253
UEVENT[1193556510.339387]
change /devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
(power_supply)
ACTION=change
DEVPATH=/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
SUBSYSTEM=power_supply
POWER_SUPPLY_NAME=BAT1
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_STATUS=Charging
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_TECHNOLOGY=Unknown
POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
POWER_SUPPLY_VOLTAGE_NOW=11340000
POWER_SUPPLY_CURRENT_NOW=13197000
POWER_SUPPLY_ENERGY_FULL_DESIGN=38880000
POWER_SUPPLY_ENERGY_FULL=37530000
POWER_SUPPLY_ENERGY_NOW=37519000
POWER_SUPPLY_MODEL_NAME=XM2038P04
POWER_SUPPLY_MANUFACTURER=
SEQNUM=3254
We'll probably need to teach user space to distinguish between battery and
battery bay (and not to crash when battery is removed :) ) but that is all
doable once we settle on kernel implementation.