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

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

[Alan Jenkins]

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 <alan-jenkins@tuffmail.co.uk>

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);

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

[Maxim Levitsky]

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 <alan-jenkins@tuffmail.co.uk>

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

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

[Alan Jenkins]

Maxim Levitsky wrote:
> 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 <alan-jenkins@tuffmail.co.uk>
>>     
>
> 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

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

[Maxim Levitsky]

On Mon, 2009-09-07 at 13:29 +0100, Alan Jenkins wrote: 
> Maxim Levitsky wrote:
> > 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 <alan-jenkins@tuffmail.co.uk>
> >>     
> >
> > 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

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

[Alan Jenkins]

Maxim Levitsky wrote:
> On Mon, 2009-09-07 at 13:29 +0100, Alan Jenkins wrote: 
>   
>> Maxim Levitsky wrote:
>>     
>>> 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 <alan-jenkins@tuffmail.co.uk>
>>>>     
>>>>         
>>> 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

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

[Maxim Levitsky]

On Sat, 2009-10-31 at 10:48 +0000, Alan Jenkins wrote:
> Maxim Levitsky wrote:
> > On Mon, 2009-09-07 at 13:29 +0100, Alan Jenkins wrote: 
> >   
> >> Maxim Levitsky wrote:
> >>     
> >>> 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 <alan-jenkins@tuffmail.co.uk>
> >>>>     
> >>>>         
> >>> 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

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

[Alan Jenkins]

Maxim Levitsky wrote:
> On Sat, 2009-10-31 at 10:48 +0000, Alan Jenkins wrote:
>   
>> Maxim Levitsky wrote:
>>     
>>> On Mon, 2009-09-07 at 13:29 +0100, Alan Jenkins wrote: 
>>>   
>>>       
>>>> Maxim Levitsky wrote:
>>>>     
>>>>         
>>>>> 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 <alan-jenkins@tuffmail.co.uk>
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> 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