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

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.

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, so
it should be fine.

Signed-off-by: Alan Jenkins <redacted>

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 9208e97..9331f01 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c

@@ -487,8 +487,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;

@@ -502,9 +500,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;
 	}

@@ -515,10 +510,6 @@ static int acpi_battery_update(struct acpi_battery *battery)
 			return result;
 		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);
 }
 

@@ -810,9 +801,7 @@ static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
 					dev_name(&device->dev), event,
 					acpi_battery_present(battery));
 #ifdef CONFIG_ACPI_SYSFS_POWER
-	/* acpi_batter_update could remove power_supply object */
-	if (battery->bat.dev)
-		power_supply_changed(&battery->bat);
+	power_supply_changed(&battery->bat);
 #endif
 }
 

@@ -837,6 +826,11 @@ static int acpi_battery_add(struct acpi_device *device)
 	if (result)
 		goto end;
 #endif
+#ifdef CONFIG_ACPI_SYSFS_POWER
+	result = sysfs_add_battery(battery);
+	if (result)
+		goto end;
+#endif
 	status = acpi_install_notify_handler(device->handle,
 					     ACPI_ALL_NOTIFY,
 					     acpi_battery_notify, battery);

On Tue, 2009-05-12 at 15:51 +0100, Alan Jenkins wrote:

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.

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, so
it should be fine.

Signed-off-by: Alan Jenkins <redacted>

What happened to this patch?

I still get the issue this patch attempts to fix:

maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC  BAT0
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$ uname -r
2.6.31-rc8-next-20090904-next
maxim@maxim-laptop:~/software/kernel/linux-2.6$ 


When I unplug the battery, its sysfs entry disappears.
Thus if system was booted without battery, there will be no way to know
system has one.

This patch doesn't apply.

Best regards,
	Maxim Levitsky

Maxim Levitsky wrote:

On Tue, 2009-05-12 at 15:51 +0100, Alan Jenkins wrote:
  

quoted

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.

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, so
it should be fine.

Signed-off-by: Alan Jenkins <redacted>
    

What happened to this patch?

I still get the issue this patch attempts to fix:

maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC  BAT0
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$ uname -r
2.6.31-rc8-next-20090904-next
maxim@maxim-laptop:~/software/kernel/linux-2.6$


When I unplug the battery, its sysfs entry disappears.
Thus if system was booted without battery, there will be no way to know
system has one.

This patch doesn't apply.
  

I rebased these patches a while back

http://patchwork.kernel.org/patch/33118/
http://patchwork.kernel.org/patch/33119/
http://patchwork.kernel.org/patch/33120/

you should find these will still apply.

Regards
Alan

On Mon, 2009-09-07 at 13:29 +0100, Alan Jenkins wrote: 

Maxim Levitsky wrote:

quoted

On Tue, 2009-05-12 at 15:51 +0100, Alan Jenkins wrote:
  

quoted

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.

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, so
it should be fine.

Signed-off-by: Alan Jenkins <redacted>
    

What happened to this patch?

I still get the issue this patch attempts to fix:

maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC  BAT0
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$ uname -r
2.6.31-rc8-next-20090904-next
maxim@maxim-laptop:~/software/kernel/linux-2.6$ 


When I unplug the battery, its sysfs entry disappears.
Thus if system was booted without battery, there will be no way to know
system has one.

This patch doesn't apply.
  

I rebased these patches a while back

http://patchwork.kernel.org/patch/33118/
http://patchwork.kernel.org/patch/33119/
http://patchwork.kernel.org/patch/33120/

you should find these will still apply.

Regards
Alan

Any progress on adding special battery bay device?

Best regards,
Maxim Levitsky

Maxim Levitsky wrote:

On Mon, 2009-09-07 at 13:29 +0100, Alan Jenkins wrote: 
  

quoted

Maxim Levitsky wrote:
    

quoted

On Tue, 2009-05-12 at 15:51 +0100, Alan Jenkins wrote:
  
      

quoted

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.

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, so
it should be fine.

Signed-off-by: Alan Jenkins <redacted>
    
        

What happened to this patch?

I still get the issue this patch attempts to fix:

maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC  BAT0
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$ uname -r
2.6.31-rc8-next-20090904-next
maxim@maxim-laptop:~/software/kernel/linux-2.6$ 


When I unplug the battery, its sysfs entry disappears.
Thus if system was booted without battery, there will be no way to know
system has one.

This patch doesn't apply.
  
      

I rebased these patches a while back

http://patchwork.kernel.org/patch/33118/
http://patchwork.kernel.org/patch/33119/
http://patchwork.kernel.org/patch/33120/

you should find these will still apply.

Regards
Alan
    

Any progress on adding special battery bay device?

Best regards,
Maxim Levitsky
  

No, sorry.

I did think of one complication - suspend/resume.  (That's not blocking
me though, it's just a matter of time).  Assume the obvious device tree:

    ACPI BAT0 -> battery bay device -> [ battery device ]

The user might suspend with the battery removed, and then add the
battery and resume.  But

    1) Parent devices are resumed before their children.
    2) The device core won't let you add a child device to a device
    which is currently suspended.

So when BAT0 resumes, the battery bay device is still suspended. 
Therefore the resume handler for BAT0 cannot directly create the battery
device. 

So we either need to

a) provide some sort of resume mechanism attached to the battery bay
device, or
b) use a different device tree and use symlinks to represent the
relationship i.e.

ACPI BAT0 -> battery bay device (with "battery" symlink)
                   -> battery device (with "bay" symlink)

I favour b).

Regards
Alan

On Sat, 2009-10-31 at 10:48 +0000, Alan Jenkins wrote:

Maxim Levitsky wrote:

quoted

On Mon, 2009-09-07 at 13:29 +0100, Alan Jenkins wrote: 
  

quoted

Maxim Levitsky wrote:
    

quoted

On Tue, 2009-05-12 at 15:51 +0100, Alan Jenkins wrote:
  
      

quoted

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.

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, so
it should be fine.

Signed-off-by: Alan Jenkins <redacted>
    
        

What happened to this patch?

I still get the issue this patch attempts to fix:

maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC  BAT0
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$ uname -r
2.6.31-rc8-next-20090904-next
maxim@maxim-laptop:~/software/kernel/linux-2.6$ 


When I unplug the battery, its sysfs entry disappears.
Thus if system was booted without battery, there will be no way to know
system has one.

This patch doesn't apply.
  
      

I rebased these patches a while back

http://patchwork.kernel.org/patch/33118/
http://patchwork.kernel.org/patch/33119/
http://patchwork.kernel.org/patch/33120/

you should find these will still apply.

Regards
Alan
    

Any progress on adding special battery bay device?

Best regards,
Maxim Levitsky
  

No, sorry.

I did think of one complication - suspend/resume.  (That's not blocking
me though, it's just a matter of time).  Assume the obvious device tree:

    ACPI BAT0 -> battery bay device -> [ battery device ]

The user might suspend with the battery removed, and then add the
battery and resume.  But

    1) Parent devices are resumed before their children.
    2) The device core won't let you add a child device to a device
    which is currently suspended.

So when BAT0 resumes, the battery bay device is still suspended. 
Therefore the resume handler for BAT0 cannot directly create the battery
device. 

So we either need to

a) provide some sort of resume mechanism attached to the battery bay
device, or
b) use a different device tree and use symlinks to represent the
relationship i.e.

ACPI BAT0 -> battery bay device (with "battery" symlink)
                   -> battery device (with "bay" symlink)

I favour b).

Regards
Alan

Me too.

In fact, I don't think there is need for relationship between bay and
battery.

All I need is some way for kernel to notify that batteries might became
present,  so it would be enough to create a dummy child of the BAT0
exactly like you said.

Probably even it is possible not to use symlinks.


Best regards,
	Maxim Levitsky

Maxim Levitsky wrote:

On Sat, 2009-10-31 at 10:48 +0000, Alan Jenkins wrote:
  

quoted

Maxim Levitsky wrote:
    

quoted

On Mon, 2009-09-07 at 13:29 +0100, Alan Jenkins wrote:
  
      

quoted

Maxim Levitsky wrote:
    
        

quoted

On Tue, 2009-05-12 at 15:51 +0100, Alan Jenkins wrote:
  
      
          

quoted

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.

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, so
it should be fine.

Signed-off-by: Alan Jenkins <redacted>
    
        
            

What happened to this patch?

I still get the issue this patch attempts to fix:

maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC  BAT0
maxim@maxim-laptop:~/software/kernel/linux-2.6$
ls /sys/class/power_supply/
AC
maxim@maxim-laptop:~/software/kernel/linux-2.6$ uname -r
2.6.31-rc8-next-20090904-next
maxim@maxim-laptop:~/software/kernel/linux-2.6$


When I unplug the battery, its sysfs entry disappears.
Thus if system was booted without battery, there will be no way to know
system has one.

This patch doesn't apply.
  
      
          

I rebased these patches a while back

http://patchwork.kernel.org/patch/33118/
http://patchwork.kernel.org/patch/33119/
http://patchwork.kernel.org/patch/33120/

you should find these will still apply.

Regards
Alan
    
        

Any progress on adding special battery bay device?

Best regards,
Maxim Levitsky
  
      

No, sorry.

I did think of one complication - suspend/resume.  (That's not blocking
me though, it's just a matter of time).  Assume the obvious device tree:

    ACPI BAT0 -> battery bay device -> [ battery device ]

The user might suspend with the battery removed, and then add the
battery and resume.  But

    1) Parent devices are resumed before their children.
    2) The device core won't let you add a child device to a device
    which is currently suspended.

So when BAT0 resumes, the battery bay device is still suspended. Therefore the resume handler for BAT0 cannot directly create the battery
device.

So we either need to

a) provide some sort of resume mechanism attached to the battery bay
device, or
b) use a different device tree and use symlinks to represent the
relationship i.e.

ACPI BAT0 -> battery bay device (with "battery" symlink)
                   -> battery device (with "bay" symlink)

I favour b).

Regards
Alan
    

Me too.

In fact, I don't think there is need for relationship between bay and
battery.

All I need is some way for kernel to notify that batteries might became
present,  so it would be enough to create a dummy child of the BAT0
exactly like you said.

Probably even it is possible not to use symlinks.


Best regards,
	Maxim Levitsky
  

I believe there are systems with more than one battery.  I think we should be able to tell userspace "there is one battery present, called BAT0, and one empty battery bay, called BAT1".  The symlinks would provide a reasonably clean way to indicate which battery bay is empty.

Regards
Alan