[RESEND] [PATCH 1/3] ACPI: battery: Fix CONFIG_ACPI_SYSFS_POWER=n

[RESEND] [PATCH 1/3] ACPI: battery: Fix CONFIG_ACPI_SYSFS_POWER=n

[Alan Jenkins]

Disabling CONFIG_ACPI_SYSFS_POWER changes the behaviour of
acpi_battery_update().  It will call acpi_battery_get_info()
even if the battery is not present.  I haven't noticed this
causing any problem, but it does look like a bad idea.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
CC: Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com>
---
 drivers/acpi/battery.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 58b4517..908edce 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -501,13 +501,13 @@ static int acpi_battery_update(struct acpi_battery *battery)
 	result = acpi_battery_get_status(battery);
 	if (result)
 		return result;
-#ifdef CONFIG_ACPI_SYSFS_POWER
 	if (!acpi_battery_present(battery)) {
+#ifdef CONFIG_ACPI_SYSFS_POWER
 		sysfs_remove_battery(battery);
+#endif
 		battery->update_time = 0;
 		return 0;
 	}
-#endif
 	if (!battery->update_time ||
 	    old_present != acpi_battery_present(battery)) {
 		result = acpi_battery_get_info(battery);
-- 
1.6.3.2

[RESEND] [PATCH 1/3] ACPI: battery: Fix CONFIG_ACPI_SYSFS_POWER=n

[Alan Jenkins]

Disabling CONFIG_ACPI_SYSFS_POWER changes the behaviour of
acpi_battery_update().  It will call acpi_battery_get_info()
even if the battery is not present.  I haven't noticed this
causing any problem, but it does look like a bad idea.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
CC: Alexey Starikovskiy <astarikovskiy@suse.de>
---
 drivers/acpi/battery.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 58b4517..908edce 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -501,13 +501,13 @@ static int acpi_battery_update(struct acpi_battery *battery)
 	result = acpi_battery_get_status(battery);
 	if (result)
 		return result;
-#ifdef CONFIG_ACPI_SYSFS_POWER
 	if (!acpi_battery_present(battery)) {
+#ifdef CONFIG_ACPI_SYSFS_POWER
 		sysfs_remove_battery(battery);
+#endif
 		battery->update_time = 0;
 		return 0;
 	}
-#endif
 	if (!battery->update_time ||
 	    old_present != acpi_battery_present(battery)) {
 		result = acpi_battery_get_info(battery);
-- 
1.6.3.2

[RESEND] [PATCH 2/3] ACPI: battery drivers should call power_supply_changed()

[Alan Jenkins]

Calling kobject_uevent() directly is a layering violation.  In
particular, it means we'll miss updating the generic LED trigger.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
CC: Alexey Starikovskiy <astarikovskiy@suse.de>
---
 drivers/acpi/battery.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 908edce..7d31a23 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -811,7 +811,7 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event)
 #ifdef CONFIG_ACPI_SYSFS_POWER
 	/* acpi_batter_update could remove power_supply object */
 	if (battery->bat.dev)
-		kobject_uevent(&battery->bat.dev->kobj, KOBJ_CHANGE);
+		power_supply_changed(&battery->bat);
 #endif
 }
 
-- 
1.6.3.2

[RESEND] [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.

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>
CC: 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 7d31a23..f424033 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);
 }
 
@@ -809,9 +800,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_batter_update could remove power_supply object */
-	if (battery->bat.dev)
-		power_supply_changed(&battery->bat);
+	power_supply_changed(&battery->bat);
 #endif
 }
 
@@ -832,17 +821,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;
 }
 
-- 
1.6.3.2

Re: [RESEND] [PATCH 1/3] ACPI: battery: Fix CONFIG_ACPI_SYSFS_POWER=n

[Maxim Levitsky]

Hi,

So this is newer version of this patchset.
When this will be merged?


Last patch doesn't apply, seems that parts of yours patch were applied.
I applied it manually.
Will soon test.


Best regards,
	Maxim Levitsky

Re: [RESEND] [PATCH 1/3] ACPI: battery: Fix CONFIG_ACPI_SYSFS_POWER=n

[Alan Jenkins]

Maxim Levitsky wrote:
> Hi,
>
> So this is newer version of this patchset.
> When this will be merged?
>
>
> Last patch doesn't apply, seems that parts of yours patch were applied.
> I applied it manually.
>   
> Will soon test.
>
>
> Best regards,
> 	Maxim Levitsky
>   

I will submit "To: len" next time round.

You're right, it no longer applies cleanly to acpi-test.  ["patch" is
happy to apply it and warn about "fuzz", but git-am is more strict.  It
doesn't look like there's a real conflict.]

I look forward to your results.  Please tell me the diff you end up with
for the last patch and what tree you applied it on top of.  That way, if
I add "Tested-by: Maxim..." I can be certain we're talking about the
same patch :-).

Thanks!
Alan

Re: [RESEND] [PATCH 1/3] ACPI: battery: Fix CONFIG_ACPI_SYSFS_POWER=n

[Maxim Levitsky]

On Wed, 2009-09-09 at 18:22 +0100, Alan Jenkins wrote: 
> Maxim Levitsky wrote:
> > Hi,
> >
> > So this is newer version of this patchset.
> > When this will be merged?
> >
> >
> > Last patch doesn't apply, seems that parts of yours patch were applied.
> > I applied it manually.
> >   
> > Will soon test.
> >
> >
> > Best regards,
> > 	Maxim Levitsky
> >   
> 
> I will submit "To: len" next time round.
> 
> You're right, it no longer applies cleanly to acpi-test.  ["patch" is
> happy to apply it and warn about "fuzz", but git-am is more strict.  It
> doesn't look like there's a real conflict.]
I applied it on top of both vanilla and linux-next. Here it really
doesn't apply (with or without 'fuzz')

> 
> I look forward to your results.  Please tell me the diff you end up with
> for the last patch and what tree you applied it on top of.  That way, if
> I add "Tested-by: Maxim..." I can be certain we're talking about the
> same patch :-).
> 
> Thanks!
> Alan

I did that few days ago (applied patch manually, without much thinking)

Unfortunately, resulting kernel oopses if battery is present, and
otherwise, if I plug batter later on, many battery statistics are
missing (probably some locks are held)

I will soon investigate this issue, more deeply.

Best regards,
Maxim Levitsky

Re: [RESEND] [PATCH 1/3] ACPI: battery: Fix CONFIG_ACPI_SYSFS_POWER=n

[Alan Jenkins]

[-- Attachment #1: Type: text/plain, Size: 1826 bytes --]

On 9/9/09, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Wed, 2009-09-09 at 18:22 +0100, Alan Jenkins wrote:
>> Maxim Levitsky wrote:
>> > Hi,
>> >
>> > So this is newer version of this patchset.
>> > When this will be merged?
>> >
>> >
>> > Last patch doesn't apply, seems that parts of yours patch were applied.
>> > I applied it manually.
>> >
>> > Will soon test.
>> >
>> >
>> > Best regards,
>> > 	Maxim Levitsky
>> >
>>
>> I will submit "To: len" next time round.
>>
>> You're right, it no longer applies cleanly to acpi-test.  ["patch" is
>> happy to apply it and warn about "fuzz", but git-am is more strict.  It
>> doesn't look like there's a real conflict.]
> I applied it on top of both vanilla and linux-next. Here it really
> doesn't apply (with or without 'fuzz')
>
>>
>> I look forward to your results.  Please tell me the diff you end up with
>> for the last patch and what tree you applied it on top of.  That way, if
>> I add "Tested-by: Maxim..." I can be certain we're talking about the
>> same patch :-).
>>
>> Thanks!
>> Alan
>
> I did that few days ago (applied patch manually, without much thinking)
>
> Unfortunately, resulting kernel oopses if battery is present, and
> otherwise, if I plug batter later on, many battery statistics are
> missing (probably some locks are held)
>
> I will soon investigate this issue, more deeply.
>
> Best regards,
> Maxim Levitsky

I've applied the patches from the mailing list, initially on -rc8
using git-am, then rebased them onto acpi-test without conflicts.  I
don't have GIT hosting, so here's a git bundle.  If you're still
having problems, you should be able to get an _exact_ copy of my tree
with

$ git fetch alan.bundle battery-check:battery-check
$ git checkout battery-check

so long as you have the acpi-test commit I based it on.

Thanks
Alan

[-- Attachment #2: alan.bundle --]
[-- Type: application/octet-stream, Size: 2468 bytes --]

Re: [RESEND] [PATCH 1/3] ACPI: battery: Fix CONFIG_ACPI_SYSFS_POWER=n

[Maxim Levitsky]

On Thu, 2009-09-10 at 12:11 +0100, Alan Jenkins wrote: 
> On 9/9/09, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > On Wed, 2009-09-09 at 18:22 +0100, Alan Jenkins wrote:
> >> Maxim Levitsky wrote:
> >> > Hi,
> >> >
> >> > So this is newer version of this patchset.
> >> > When this will be merged?
> >> >
> >> >
> >> > Last patch doesn't apply, seems that parts of yours patch were applied.
> >> > I applied it manually.
> >> >
> >> > Will soon test.
> >> >
> >> >
> >> > Best regards,
> >> > 	Maxim Levitsky
> >> >
> >>
> >> I will submit "To: len" next time round.
> >>
> >> You're right, it no longer applies cleanly to acpi-test.  ["patch" is
> >> happy to apply it and warn about "fuzz", but git-am is more strict.  It
> >> doesn't look like there's a real conflict.]
> > I applied it on top of both vanilla and linux-next. Here it really
> > doesn't apply (with or without 'fuzz')
> >
> >>
> >> I look forward to your results.  Please tell me the diff you end up with
> >> for the last patch and what tree you applied it on top of.  That way, if
> >> I add "Tested-by: Maxim..." I can be certain we're talking about the
> >> same patch :-).
> >>
> >> Thanks!
> >> Alan
> >
> > I did that few days ago (applied patch manually, without much thinking)
> >
> > Unfortunately, resulting kernel oopses if battery is present, and
> > otherwise, if I plug batter later on, many battery statistics are
> > missing (probably some locks are held)
> >
> > I will soon investigate this issue, more deeply.
> >
> > Best regards,
> > Maxim Levitsky
> 
> I've applied the patches from the mailing list, initially on -rc8
> using git-am, then rebased them onto acpi-test without conflicts.  I
> don't have GIT hosting, so here's a git bundle.  If you're still
> having problems, you should be able to get an _exact_ copy of my tree
> with
> 
> $ git fetch alan.bundle battery-check:battery-check
> $ git checkout battery-check
> 
> so long as you have the acpi-test commit I based it on.
> 
> Thanks
> Alan

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:

                Device (BAT0)
                {
                    Name (_HID, EisaId ("PNP0C0A"))
                    Name (_UID, 0x01)
                    Name (_PCL, Package (0x01)
                    {
                        \_SB
                    })
                    Name (PBIF, Package (0x0D)
                    {
                        0x01, 
                        0x0FA0, 
                        0x0FA0, 
                        0x01, 
                        0x39D0, 
                        0x0190, 
                        0x78, 
                        0x0108, 
                        0x0EC4, 
                        "Li_Ion 4000mA ", 
                        "", 
                        "Lion", 
                        "Acer   "
                    })
                    Name (PBST, Package (0x04)
                    {
                        0x01, 
                        0xFFFFFFFF, 
                        0xFFFFFFFF, 
                        0x39D0
                    })
                    Method (_STA, 0, NotSerialized)
                    {
                        If (ECON)
                        {
                            If (\_SB.PCI0.LPC.EC0.BAL1)
                            {
                                Sleep (0x14)
                                Return (0x1F)
                            }
                            Else
                            {
                                Sleep (0x14)
                                Return (0x0F)
                            }
                        }
                        Else
                        {
                            Sleep (0x14)
                            Return (0x1F)
                        }
                    }

                    Method (_BIF, 0, NotSerialized)
                    {
                        If (ECON)
                        {
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BDC0, Index (PBIF, 0x01))
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BFC0, Index (PBIF, 0x02))
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BDV0, Index (PBIF, 0x04))
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BDC0, Local2)
                            Divide (Local2, 0x64, Local6, Local2)
                            Multiply (Local2, 0x05, Local3)
                            Store (Local3, Index (PBIF, 0x05))
                            Multiply (Local2, 0x03, Local4)
                            Store (Local4, Index (PBIF, 0x06))
                            Store (\_SB.PCI0.LPC.EC0.BTY0, Local1)
                            Sleep (0x14)
                            If (LEqual (Local1, 0x01))
                            {
                                Store ("GC86508SAT0 ", Index (PBIF, 0x09))
                                Store ("SANYO ", Index (PBIF, 0x0C))
                            }
                            Else
                            {
                                If (LEqual (Local1, 0x02))
                                {
                                    Store ("GC86503SY90 ", Index (PBIF, 0x09))
                                    Store ("SONY ", Index (PBIF, 0x0C))
                                }
                                Else
                                {
                                    If (LEqual (Local1, 0x04))
                                    {
                                        Store ("GC86503PAG0 ", Index (PBIF, 0x09))
                                        Store ("PANASONIC ", Index (PBIF, 0x0C))
                                    }
                                    Else
                                    {
                                        If (LEqual (Local1, 0x05))
                                        {
                                            Store ("GC86508SM60 ", Index (PBIF, 0x09))
                                            Store ("SAMSUNG ", Index (PBIF, 0x0C))
                                        }
                                        Else
                                        {
                                            Store ("BCL3100LiON ", Index (PBIF, 0x09))
                                            Store ("COMPAL ", Index (PBIF, 0x0C))
                                        }
                                    }
                                }
                            }
                        }

                        Return (PBIF)
                    }

                    Method (_BST, 0, NotSerialized)
                    {
                        If (ECON)
                        {
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BST0, Local0)
                            And (Local0, 0x07, Local0)
                            Store (Local0, Index (PBST, 0x00))
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.GAU0, Local2)
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BPV0, Local3)
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BFC0, Local1)
                            Sleep (0x14)
                            If (Local2)
                            {
                                Multiply (Local2, Local1, Local2)
                                Divide (Local2, 0x64, Local6, Local2)
                                If (Local6)
                                {
                                    Increment (Local2)
                                }
                            }

                            Store (\_SB.PCI0.LPC.EC0.BAC0, Local1)
                            If (And (Local1, 0x8000, Local1))
                            {
                                Store (\_SB.PCI0.LPC.EC0.BAC0, Local1)
                                Subtract (0xFFFF, Local1, Local1)
                            }
                            Else
                            {
                                Store (0x00, Local1)
                            }

                            Sleep (0x14)
                            Store (Local1, Index (PBST, 0x01))
                            Store (Local2, Index (PBST, 0x02))
                            Store (Local3, Index (PBST, 0x03))
                        }

                        Return (PBST)
                    }
                }





PBIF at offset 0 is set correctly to 1.


Best regards,
	Maxim Levitsky

Re: [RESEND] [PATCH 1/3] ACPI: battery: Fix CONFIG_ACPI_SYSFS_POWER=n

[Len Brown]

> 
> $ git fetch alan.bundle battery-check:battery-check
> $ git checkout battery-check
> 
> so long as you have the acpi-test commit I based it on.

apparently I don't.  Can you re-send this as a plain patch?

thanks,
-Len

Re: [RESEND] [PATCH 1/3] ACPI: battery: Fix CONFIG_ACPI_SYSFS_POWER=n

[Alan Jenkins]

On 12/10/09, Len Brown <lenb@kernel.org> wrote:
>>
>> $ git fetch alan.bundle battery-check:battery-check
>> $ git checkout battery-check
>>
>> so long as you have the acpi-test commit I based it on.
>
> apparently I don't.  Can you re-send this as a plain patch?
>
> thanks,
> -Len

Sure.  I'll resend the first two patches in this series (the patch 3/3
was wrong and should not be applied).

Thanks
Alan