[PATCH] ACPI: battery: register power_supply subdevice even when battery not present

[PATCH] ACPI: battery: register power_supply subdevice even when battery not present

[Alexey Starikovskiy]

From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

Keeping this device around lets userspace know that we have a battery
bay, even if there is nothing in it at the moment.  This is what every
other battery driver does, so ACPI should do it as well.

For example, this means gnome-power-manager will now allow configuring
behaviour for "on battery power" if the battery bay was empty on
startup. (Thanks to Maxim Levitsky for pointing this out).

There is no reason to preserve the old behaviour.  We now correctly
provide the "present" attribute, which will return "0" when the battery
is removed.  HAL was already trying to check this attribute when
the old behaviour was implemented.  This should not break any version
of HAL.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/battery.c |   36 ++++++++++++++++--------------------
 1 files changed, 16 insertions(+), 20 deletions(-)


diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index db78b6e..5b9fd78 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -546,8 +546,6 @@ static int sysfs_add_battery(struct acpi_battery *battery)
 
 static void sysfs_remove_battery(struct acpi_battery *battery)
 {
-	if (!battery->bat.dev)
-		return;
 	device_remove_file(battery->bat.dev, &alarm_attr);
 	power_supply_unregister(&battery->bat);
 	battery->bat.dev = NULL;
@@ -568,9 +566,6 @@ static int acpi_battery_update(struct acpi_battery *battery)
 	if (result)
 		return result;
 	if (!acpi_battery_present(battery)) {
-#ifdef CONFIG_ACPI_SYSFS_POWER
-		sysfs_remove_battery(battery);
-#endif
 		battery->update_time = 0;
 		return 0;
 	}
@@ -582,10 +577,6 @@ static int acpi_battery_update(struct acpi_battery *battery)
 		acpi_battery_quirks(battery);
 		acpi_battery_init_alarm(battery);
 	}
-#ifdef CONFIG_ACPI_SYSFS_POWER
-	if (!battery->bat.dev)
-		sysfs_add_battery(battery);
-#endif
 	return acpi_battery_get_state(battery);
 }
 
@@ -877,9 +868,7 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event)
 					dev_name(&device->dev), event,
 					acpi_battery_present(battery));
 #ifdef CONFIG_ACPI_SYSFS_POWER
-	/* acpi_battery_update could remove power_supply object */
-	if (battery->bat.dev)
-		power_supply_changed(&battery->bat);
+	power_supply_changed(&battery->bat);
 #endif
 }
 
@@ -904,17 +893,24 @@ static int acpi_battery_add(struct acpi_device *device)
 	acpi_battery_update(battery);
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	result = acpi_battery_add_fs(device);
+	if (result)
+		goto fail;
 #endif
-	if (!result) {
-		printk(KERN_INFO PREFIX "%s Slot [%s] (battery %s)\n",
-			ACPI_BATTERY_DEVICE_NAME, acpi_device_bid(device),
-			device->status.battery_present ? "present" : "absent");
-	} else {
+#ifdef CONFIG_ACPI_SYSFS_POWER
+	result = sysfs_add_battery(battery);
+	if (result)
+		goto fail;
+#endif
+	printk(KERN_INFO PREFIX "%s Slot [%s] (battery %s)\n",
+		ACPI_BATTERY_DEVICE_NAME, acpi_device_bid(device),
+		device->status.battery_present ? "present" : "absent");
+	return 0;
+
+fail:
 #ifdef CONFIG_ACPI_PROCFS_POWER
-		acpi_battery_remove_fs(device);
+	acpi_battery_remove_fs(device);
 #endif
-		kfree(battery);
-	}
+	kfree(battery);
 	return result;
 }
 

Re: [PATCH] ACPI: battery: register power_supply subdevice even when battery not present

[Alan Jenkins]

[CC power_supply maintainers]

I tried to fix the ACPI battery driver so it created a power supply
device even if the battery is not present.  All the other battery
drivers do this.  It would let e.g. gnome-power-manager to detect the
presence of a battery bay, and allow configuration of battery
behaviour even when the battery has been removed.

Unfortunately this is not as easy as I thought.  We would need to
either modify or work around the generic power supply class, so I
would like to ask your opinion on this.

On 9/11/09, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> Indeed, it works.
>
>
> Just one issue though, if I plug in the battery after system boot, it is
> detected a an 'energy battery' (energy_battery_props are used)
>
> ACPI seems to be ok:
...
> PBIF at offset 0 is set correctly to 1.

Your ACPI is fine - there's a fatal flaw in my patch.  It doesn't
evaluate _BIF until the battery is inserted.  At that point it is too
late to change the set of properties on the battery device, because
it's already been created.

The problem is that this power_unit value is allowed to change between
mW and mA when the battery is hotplugged.  And it looks like it does
on some machines, going from 0 to 1 when a battery is inserted.  So we
can't just read it at load time.

I can think of some more complex ways to do this

1) Destroy and recreate the battery device on hotplug.
2) Modify the generic power supply class to allow changing the set of attribute.
3) Restructure the interface to provide a "battery bay" device as a parent.

1) and 2) are hacks.  1) could at least cause annoyingly spurious UI
events.  2) sounds like a bad idea, but existing userspace _might_
handle it because it already happens at registration time.  (Power
supply attributes aren't available on the initial ADD uevent; they are
added in a follow-up CHANGE uevent).

3) could be backwards compatible and relatively straightforward.  The
"battery bay" could be a new type of power supply device, with no
attributes of its own.  It could be created automatically for non-acpi
batteries.  Userspace would have to be taught about it, but perhaps
this is a good time to do so since HAL is currently being replaced by
DevKit.

Alan

Re: [PATCH] ACPI: battery: register power_supply subdevice even when battery not present

[Anton Vorontsov]

On Fri, Sep 11, 2009 at 01:28:42PM +0100, Alan Jenkins wrote:
> [CC power_supply maintainers]
> 
> I tried to fix the ACPI battery driver so it created a power supply
> device even if the battery is not present.  All the other battery
> drivers do this.  It would let e.g. gnome-power-manager to detect the
> presence of a battery bay, and allow configuration of battery
> behaviour even when the battery has been removed.

This was discussed several times already.

http://lkml.org/lkml/2009/4/27/314
http://lkml.org/lkml/2007/10/27/170

> Unfortunately this is not as easy as I thought.

And not possible for certain batteries.

[...]
> I can think of some more complex ways to do this
> 
> 1) Destroy and recreate the battery device on hotplug.
> 2) Modify the generic power supply class to allow changing the set of attribute.
> 3) Restructure the interface to provide a "battery bay" device as a parent.
> 
> 1) and 2) are hacks.  1) could at least cause annoyingly spurious UI
> events.  2) sounds like a bad idea, but existing userspace _might_
> handle it because it already happens at registration time.  (Power
> supply attributes aren't available on the initial ADD uevent; they are
> added in a follow-up CHANGE uevent).
> 
> 3) could be backwards compatible and relatively straightforward.  The
> "battery bay" could be a new type of power supply device, with no
> attributes of its own.

Yes, that would be nice. There could be some attributes though,
I can think of a temperature sensor (measures ambient/bay
temperature), a battery presence/lock sensor etc.

IIRC, battery bay in iPaq hx4700 devices can report if a battery is
locked (that's all it can do, but still useful).

> It could be created automatically for non-acpi batteries.

Why? If there is no bay, we don't need any device for it.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] ACPI: battery: register power_supply subdevice even when battery not present

[Alan Jenkins]

On 9/11/09, Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> On Fri, Sep 11, 2009 at 01:28:42PM +0100, Alan Jenkins wrote:
>> [CC power_supply maintainers]
>>
>> I tried to fix the ACPI battery driver so it created a power supply
>> device even if the battery is not present.  All the other battery
>> drivers do this.  It would let e.g. gnome-power-manager to detect the
>> presence of a battery bay, and allow configuration of battery
>> behaviour even when the battery has been removed.
>
> This was discussed several times already.
>
> http://lkml.org/lkml/2009/4/27/314
> http://lkml.org/lkml/2007/10/27/170
>
>> Unfortunately this is not as easy as I thought.
>
> And not possible for certain batteries.
>
> [...]
>> I can think of some more complex ways to do this
>>
>> 1) Destroy and recreate the battery device on hotplug.
>> 2) Modify the generic power supply class to allow changing the set of
>> attribute.
>> 3) Restructure the interface to provide a "battery bay" device as a
>> parent.
>>
>> 1) and 2) are hacks.  1) could at least cause annoyingly spurious UI
>> events.  2) sounds like a bad idea, but existing userspace _might_
>> handle it because it already happens at registration time.  (Power
>> supply attributes aren't available on the initial ADD uevent; they are
>> added in a follow-up CHANGE uevent).
>>
>> 3) could be backwards compatible and relatively straightforward.  The
>> "battery bay" could be a new type of power supply device, with no
>> attributes of its own.
>
> Yes, that would be nice. There could be some attributes though,
> I can think of a temperature sensor (measures ambient/bay
> temperature), a battery presence/lock sensor etc.
>
> IIRC, battery bay in iPaq hx4700 devices can report if a battery is
> locked (that's all it can do, but still useful).
>
>> It could be created automatically for non-acpi batteries.
>
> Why? If there is no bay, we don't need any device for it.

I thought it would be easier for userspace... but actually there's no
point since programs will still have to handle older kernels.

Ok, I'll write 3) and see what happens.

Alan