Re: [PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent
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)? signature.asc Description: This is a digitally signed message part.
Re: [PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent
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. signature.asc Description: This is a digitally signed message part.
Re: [PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent
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).
Subject: [PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent
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;
signature.asc
Description: This is a digitally signed message part.
