ACPI: Battery: Add bit flags

PATCH In review Alexey Starikovskiy 5 participants 15 messages Oct 15, 2009 – Mar 16, 2010

Subsystems (2)

ACPI Power supply

lore

Share & tools

lore

Alexey Starikovskiy

author

2009-10-15 10:32 UTC #0 lore raw

PATCH 1/4 ACPI: Battery: Add bit flags

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/battery.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)

drivers/acpi/battery.c +14 ~~-12~~

index 3f4602b..7adc1fa 100644
@@ -88,10 +88,13 @@ static const struct acpi_device_id battery_device_ids[] = {
 
 MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
-/* For buggy DSDTs that report negative 16-bit values for either charging
- * or discharging current and/or report 0 as 65536 due to bad math.
- */
-#define QUIRK_SIGNED16_CURRENT 0x0001
+enum {
+	ACPI_BATTERY_ALARM_PRESENT,
+	/* For buggy DSDTs that report negative 16-bit values for either charging
+	* or discharging current and/or report 0 as 65536 due to bad math.
+	*/
+	ACPI_BATTERY_QUIRK_SIGNED16_CURRENT,
+};
 
 struct acpi_battery {
 	struct mutex lock;

@@ -118,8 +121,7 @@ struct acpi_battery {
 	char oem_info[32];
 	int state;
 	int power_unit;
-	u8 alarm_present;
-	long quirks;
+	unsigned long flags;
 };
 
 #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);

@@ -399,7 +401,7 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
 	battery->update_time = jiffies;
 	kfree(buffer.pointer);
 
-	if ((battery->quirks & QUIRK_SIGNED16_CURRENT) &&
+	if (test_bit(ACPI_BATTERY_QUIRK_SIGNED16_CURRENT, &battery->flags) &&
 	    battery->rate_now != -1)
 		battery->rate_now = abs((s16)battery->rate_now);

@@ -412,7 +414,8 @@ static int acpi_battery_set_alarm(struct acpi_battery *battery)
 	union acpi_object arg0 = { .type = ACPI_TYPE_INTEGER };
 	struct acpi_object_list arg_list = { 1, &arg0 };
 
-	if (!acpi_battery_present(battery)|| !battery->alarm_present)
+	if (!acpi_battery_present(battery)||
+	    !test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags))
 		return -ENODEV;
 
 	arg0.integer.value = battery->alarm;

@@ -437,10 +440,10 @@ static int acpi_battery_init_alarm(struct acpi_battery *battery)
 	/* See if alarms are supported, and if so, set default */
 	status = acpi_get_handle(battery->device->handle, "_BTP", &handle);
 	if (ACPI_FAILURE(status)) {
-		battery->alarm_present = 0;
+		clear_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags);
 		return 0;
 	}
-	battery->alarm_present = 1;
+	set_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags);
 	if (!battery->alarm)
 		battery->alarm = battery->design_capacity_warning;
 	return acpi_battery_set_alarm(battery);

@@ -510,9 +513,8 @@ static void sysfs_remove_battery(struct acpi_battery *battery)
 
 static void acpi_battery_quirks(struct acpi_battery *battery)
 {
-	battery->quirks = 0;
 	if (dmi_name_in_vendors("Acer") && battery->power_unit) {
-		battery->quirks |= QUIRK_SIGNED16_CURRENT;
+		set_bit(ACPI_BATTERY_QUIRK_SIGNED16_CURRENT, &battery->flags);
 	}
 }

Alexey Starikovskiy

author

2009-10-15 10:32 UTC #1 lore raw

PATCH 2/4 POWER: Add support for cycle_count

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/power/power_supply_sysfs.c |    1 +
 include/linux/power_supply.h       |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

drivers/power/power_supply_sysfs.c +1 -0

index 0814439..2a52345 100644
@@ -96,6 +96,7 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(present),
 	POWER_SUPPLY_ATTR(online),
 	POWER_SUPPLY_ATTR(technology),
+	POWER_SUPPLY_ATTR(cycle_count),
 	POWER_SUPPLY_ATTR(voltage_max),
 	POWER_SUPPLY_ATTR(voltage_min),
 	POWER_SUPPLY_ATTR(voltage_max_design),

include/linux/power_supply.h +1 -0

index b5d096d..ebd2b8f 100644
@@ -82,6 +82,7 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_CYCLE_COUNT,
 	POWER_SUPPLY_PROP_VOLTAGE_MAX,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN,
 	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,

Len Brown

reviewer

2010-01-15 21:59 UTC #2 lore raw

Re: [PATCH 2/4] POWER: Add support for cycle_count

applied to acpi-test

thanks,
Len Brown, Intel Open Source Technology Center

Alexey Starikovskiy

author

2009-10-15 10:32 UTC #3 lore raw

PATCH 3/4 ACPI: SBS: Export cycle_count

In reply to Alexey Starikovskiy

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/sbs.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

drivers/acpi/sbs.c +5 -0

index 52b9db8..38412ec 100644
@@ -217,6 +217,9 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_TECHNOLOGY:
 		val->intval = acpi_battery_technology(battery);
 		break;
+	case POWER_SUPPLY_PROP_CYCLE_COUNT:
+		val->intval = battery->cycle_count;
+		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
 		val->intval = battery->design_voltage *
 			acpi_battery_vscale(battery) * 1000;

@@ -276,6 +279,7 @@ static enum power_supply_property sbs_charge_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_CYCLE_COUNT,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_CURRENT_NOW,

@@ -560,6 +564,7 @@ static int acpi_battery_read_info(struct seq_file *seq, void *offset)
 		   battery->design_voltage * acpi_battery_vscale(battery));
 	seq_printf(seq, "design capacity warning: unknown\n");
 	seq_printf(seq, "design capacity low:     unknown\n");
+	seq_printf(seq, "cycle count:		  %i\n", battery->cycle_count);
 	seq_printf(seq, "capacity granularity 1:  unknown\n");
 	seq_printf(seq, "capacity granularity 2:  unknown\n");
 	seq_printf(seq, "model number:            %s\n", battery->device_name);

Len Brown

reviewer

2010-01-15 22:00 UTC #4 lore raw

Re: [PATCH 3/4] ACPI: SBS: Export cycle_count

applied to acpi-test

thanks,
Len Brown, Intel Open Source Technology Center

Alexey Starikovskiy

author

2009-10-15 10:32 UTC #5 lore raw

PATCH 4/4 ACPI: Battery: Add support for _BIX extended info method

In reply to Alexey Starikovskiy

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/battery.c |   59 +++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 53 insertions(+), 6 deletions(-)

drivers/acpi/battery.c +53 -6

index 7adc1fa..4ac8f2d 100644
@@ -54,6 +54,7 @@
 #define ACPI_BATTERY_DEVICE_NAME	"Battery"
 #define ACPI_BATTERY_NOTIFY_STATUS	0x80
 #define ACPI_BATTERY_NOTIFY_INFO	0x81
+#define ACPI_BATTERY_NOTIFY_THRESHOLD   0x82
 
 #define _COMPONENT		ACPI_BATTERY_COMPONENT

@@ -90,6 +91,7 @@ MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
 enum {
 	ACPI_BATTERY_ALARM_PRESENT,
+	ACPI_BATTERY_XINFO_PRESENT,
 	/* For buggy DSDTs that report negative 16-bit values for either charging
 	* or discharging current and/or report 0 as 65536 due to bad math.
 	*/

@@ -112,6 +114,12 @@ struct acpi_battery {
 	int design_voltage;
 	int design_capacity_warning;
 	int design_capacity_low;
+	int cycle_count;
+	int measurement_accuracy;
+	int max_sampling_time;
+	int min_sampling_time;
+	int max_averaging_interval;
+	int min_averaging_interval;
 	int capacity_granularity_1;
 	int capacity_granularity_2;
 	int alarm;

@@ -200,6 +208,9 @@ static int acpi_battery_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_TECHNOLOGY:
 		val->intval = acpi_battery_technology(battery);
 		break;
+	case POWER_SUPPLY_PROP_CYCLE_COUNT:
+		val->intval = battery->cycle_count;
+		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
 		val->intval = battery->design_voltage * 1000;
 		break;

@@ -241,6 +252,7 @@ static enum power_supply_property charge_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_CYCLE_COUNT,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_CURRENT_NOW,

@@ -256,6 +268,7 @@ static enum power_supply_property energy_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_CYCLE_COUNT,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_CURRENT_NOW,

@@ -307,6 +320,28 @@ static struct acpi_offsets info_offsets[] = {
 	{offsetof(struct acpi_battery, oem_info), 1},
 };
 
+static struct acpi_offsets extended_info_offsets[] = {
+	{offsetof(struct acpi_battery, power_unit), 0},
+	{offsetof(struct acpi_battery, design_capacity), 0},
+	{offsetof(struct acpi_battery, full_charge_capacity), 0},
+	{offsetof(struct acpi_battery, technology), 0},
+	{offsetof(struct acpi_battery, design_voltage), 0},
+	{offsetof(struct acpi_battery, design_capacity_warning), 0},
+	{offsetof(struct acpi_battery, design_capacity_low), 0},
+	{offsetof(struct acpi_battery, cycle_count), 0},
+	{offsetof(struct acpi_battery, measurement_accuracy), 0},
+	{offsetof(struct acpi_battery, max_sampling_time), 0},
+	{offsetof(struct acpi_battery, min_sampling_time), 0},
+	{offsetof(struct acpi_battery, max_averaging_interval), 0},
+	{offsetof(struct acpi_battery, min_averaging_interval), 0},
+	{offsetof(struct acpi_battery, capacity_granularity_1), 0},
+	{offsetof(struct acpi_battery, capacity_granularity_2), 0},
+	{offsetof(struct acpi_battery, model_number), 1},
+	{offsetof(struct acpi_battery, serial_number), 1},
+	{offsetof(struct acpi_battery, type), 1},
+	{offsetof(struct acpi_battery, oem_info), 1},
+};
+
 static int extract_package(struct acpi_battery *battery,
 			   union acpi_object *package,
 			   struct acpi_offsets *offsets, int num)

@@ -352,22 +387,29 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
 {
 	int result = -EFAULT;
 	acpi_status status = 0;
+	char *name = test_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags)?
+			"_BIX" : "_BIF";
+
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 
 	if (!acpi_battery_present(battery))
 		return 0;
 	mutex_lock(&battery->lock);
-	status = acpi_evaluate_object(battery->device->handle, "_BIF",
-				      NULL, &buffer);
+	status = acpi_evaluate_object(battery->device->handle, name,
+						NULL, &buffer);
 	mutex_unlock(&battery->lock);
 
 	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BIF"));
+		ACPI_EXCEPTION((AE_INFO, status, "Evaluating %s", name));
 		return -ENODEV;
 	}
-
-	result = extract_package(battery, buffer.pointer,
-				 info_offsets, ARRAY_SIZE(info_offsets));
+	if (test_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags))
+		result = extract_package(battery, buffer.pointer,
+				extended_info_offsets,
+				ARRAY_SIZE(extended_info_offsets));
+	else
+		result = extract_package(battery, buffer.pointer,
+				info_offsets, ARRAY_SIZE(info_offsets));
 	kfree(buffer.pointer);
 	return result;
 }

@@ -592,6 +634,7 @@ static int acpi_battery_print_info(struct seq_file *seq, int result)
 	seq_printf(seq, "design capacity low:     %d %sh\n",
 		   battery->design_capacity_low,
 		   acpi_battery_units(battery));
+	seq_printf(seq, "cycle count:		  %i\n", battery->cycle_count);
 	seq_printf(seq, "capacity granularity 1:  %d %sh\n",
 		   battery->capacity_granularity_1,
 		   acpi_battery_units(battery));

@@ -843,6 +886,7 @@ static int acpi_battery_add(struct acpi_device *device)
 {
 	int result = 0;
 	struct acpi_battery *battery = NULL;
+	acpi_handle handle;
 	if (!device)
 		return -EINVAL;
 	battery = kzalloc(sizeof(struct acpi_battery), GFP_KERNEL);

@@ -853,6 +897,9 @@ static int acpi_battery_add(struct acpi_device *device)
 	strcpy(acpi_device_class(device), ACPI_BATTERY_CLASS);
 	device->driver_data = battery;
 	mutex_init(&battery->lock);
+	if (ACPI_SUCCESS(acpi_get_handle(battery->device->handle,
+			"_BIX", &handle)))
+		set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
 	acpi_battery_update(battery);
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	result = acpi_battery_add_fs(device);

Alan Jenkins

2009-10-16 09:08 UTC #6 lore raw

Re: [PATCH 4/4] ACPI: Battery: Add support for _BIX extended info method

Quoted 42 lines · Alexey Starikovskiy

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/battery.c |   59
+++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 53 insertions(+), 6 deletions(-)

drivers/acpi/battery.c +8 -0

index 7adc1fa..4ac8f2d 100644
@@ -54,6 +54,7 @@
 #define ACPI_BATTERY_DEVICE_NAME	"Battery"
 #define ACPI_BATTERY_NOTIFY_STATUS	0x80
 #define ACPI_BATTERY_NOTIFY_INFO	0x81
+#define ACPI_BATTERY_NOTIFY_THRESHOLD   0x82
 
 #define _COMPONENT		ACPI_BATTERY_COMPONENT

@@ -90,6 +91,7 @@ MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
 enum {
 	ACPI_BATTERY_ALARM_PRESENT,
+	ACPI_BATTERY_XINFO_PRESENT,
 	/* For buggy DSDTs that report negative 16-bit values for either charging
 	* or discharging current and/or report 0 as 65536 due to bad math.
 	*/

@@ -112,6 +114,12 @@ struct acpi_battery {
 	int design_voltage;
 	int design_capacity_warning;
 	int design_capacity_low;
+	int cycle_count;
+	int measurement_accuracy;
+	int max_sampling_time;
+	int min_sampling_time;
+	int max_averaging_interval;
+	int min_averaging_interval;
 	int capacity_granularity_1;
 	int capacity_granularity_2;
 	int alarm;

drivers/acpi/battery.c static int acpi_battery_get_property(struct power_supply · line 214

Quoted 7 lines

drivers/acpi/battery.c +3 -0

@@ -200,6 +208,9 @@ static int acpi_battery_get_property(struct power_supply
 *psy,
 	case POWER_SUPPLY_PROP_TECHNOLOGY:
 		val->intval = acpi_battery_technology(battery);
 		break;
+	case POWER_SUPPLY_PROP_CYCLE_COUNT:
+		val->intval = battery->cycle_count;
+		break;

So userspace is supposed to realise that "0" means "this attribute is not supported", as opposed to "I am a brand new battery and don't remember being factory-tested".

How about returning -1 for batteries without _BIX?

<reads spec>. Particularly since Cycle Count in _BIX is spec'ed to return 0xFFFFFFFF for unknown, which we will return as -1, no?

Regards
Alan

Alexey Starikovskiy

author

2009-10-16 09:47 UTC #7 lore raw

PATCH 1/1 ACPI: Battery: Add support for _BIX extended info method

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/battery.c |   61 +++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 55 insertions(+), 6 deletions(-)

drivers/acpi/battery.c +55 -6

index 7adc1fa..4d272db 100644
@@ -54,6 +54,7 @@
 #define ACPI_BATTERY_DEVICE_NAME	"Battery"
 #define ACPI_BATTERY_NOTIFY_STATUS	0x80
 #define ACPI_BATTERY_NOTIFY_INFO	0x81
+#define ACPI_BATTERY_NOTIFY_THRESHOLD   0x82
 
 #define _COMPONENT		ACPI_BATTERY_COMPONENT

@@ -90,6 +91,7 @@ MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
 enum {
 	ACPI_BATTERY_ALARM_PRESENT,
+	ACPI_BATTERY_XINFO_PRESENT,
 	/* For buggy DSDTs that report negative 16-bit values for either charging
 	* or discharging current and/or report 0 as 65536 due to bad math.
 	*/

@@ -112,6 +114,12 @@ struct acpi_battery {
 	int design_voltage;
 	int design_capacity_warning;
 	int design_capacity_low;
+	int cycle_count;
+	int measurement_accuracy;
+	int max_sampling_time;
+	int min_sampling_time;
+	int max_averaging_interval;
+	int min_averaging_interval;
 	int capacity_granularity_1;
 	int capacity_granularity_2;
 	int alarm;

@@ -200,6 +208,9 @@ static int acpi_battery_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_TECHNOLOGY:
 		val->intval = acpi_battery_technology(battery);
 		break;
+	case POWER_SUPPLY_PROP_CYCLE_COUNT:
+		val->intval = battery->cycle_count;
+		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
 		val->intval = battery->design_voltage * 1000;
 		break;

@@ -241,6 +252,7 @@ static enum power_supply_property charge_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_CYCLE_COUNT,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_CURRENT_NOW,

@@ -256,6 +268,7 @@ static enum power_supply_property energy_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_CYCLE_COUNT,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_CURRENT_NOW,

@@ -307,6 +320,28 @@ static struct acpi_offsets info_offsets[] = {
 	{offsetof(struct acpi_battery, oem_info), 1},
 };
 
+static struct acpi_offsets extended_info_offsets[] = {
+	{offsetof(struct acpi_battery, power_unit), 0},
+	{offsetof(struct acpi_battery, design_capacity), 0},
+	{offsetof(struct acpi_battery, full_charge_capacity), 0},
+	{offsetof(struct acpi_battery, technology), 0},
+	{offsetof(struct acpi_battery, design_voltage), 0},
+	{offsetof(struct acpi_battery, design_capacity_warning), 0},
+	{offsetof(struct acpi_battery, design_capacity_low), 0},
+	{offsetof(struct acpi_battery, cycle_count), 0},
+	{offsetof(struct acpi_battery, measurement_accuracy), 0},
+	{offsetof(struct acpi_battery, max_sampling_time), 0},
+	{offsetof(struct acpi_battery, min_sampling_time), 0},
+	{offsetof(struct acpi_battery, max_averaging_interval), 0},
+	{offsetof(struct acpi_battery, min_averaging_interval), 0},
+	{offsetof(struct acpi_battery, capacity_granularity_1), 0},
+	{offsetof(struct acpi_battery, capacity_granularity_2), 0},
+	{offsetof(struct acpi_battery, model_number), 1},
+	{offsetof(struct acpi_battery, serial_number), 1},
+	{offsetof(struct acpi_battery, type), 1},
+	{offsetof(struct acpi_battery, oem_info), 1},
+};
+
 static int extract_package(struct acpi_battery *battery,
 			   union acpi_object *package,
 			   struct acpi_offsets *offsets, int num)

@@ -352,22 +387,31 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
 {
 	int result = -EFAULT;
 	acpi_status status = 0;
+	char *name = test_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags)?
+			"_BIX" : "_BIF";
+
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 
 	if (!acpi_battery_present(battery))
 		return 0;
 	mutex_lock(&battery->lock);
-	status = acpi_evaluate_object(battery->device->handle, "_BIF",
-				      NULL, &buffer);
+	status = acpi_evaluate_object(battery->device->handle, name,
+						NULL, &buffer);
 	mutex_unlock(&battery->lock);
 
 	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BIF"));
+		ACPI_EXCEPTION((AE_INFO, status, "Evaluating %s", name));
 		return -ENODEV;
 	}
-
-	result = extract_package(battery, buffer.pointer,
-				 info_offsets, ARRAY_SIZE(info_offsets));
+	if (test_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags))
+		result = extract_package(battery, buffer.pointer,
+				extended_info_offsets,
+				ARRAY_SIZE(extended_info_offsets));
+	else {
+		battery->cycle_count = -1;
+		result = extract_package(battery, buffer.pointer,
+				info_offsets, ARRAY_SIZE(info_offsets));
+	}
 	kfree(buffer.pointer);
 	return result;
 }

@@ -592,6 +636,7 @@ static int acpi_battery_print_info(struct seq_file *seq, int result)
 	seq_printf(seq, "design capacity low:     %d %sh\n",
 		   battery->design_capacity_low,
 		   acpi_battery_units(battery));
+	seq_printf(seq, "cycle count:		  %i\n", battery->cycle_count);
 	seq_printf(seq, "capacity granularity 1:  %d %sh\n",
 		   battery->capacity_granularity_1,
 		   acpi_battery_units(battery));

@@ -843,6 +888,7 @@ static int acpi_battery_add(struct acpi_device *device)
 {
 	int result = 0;
 	struct acpi_battery *battery = NULL;
+	acpi_handle handle;
 	if (!device)
 		return -EINVAL;
 	battery = kzalloc(sizeof(struct acpi_battery), GFP_KERNEL);

@@ -853,6 +899,9 @@ static int acpi_battery_add(struct acpi_device *device)
 	strcpy(acpi_device_class(device), ACPI_BATTERY_CLASS);
 	device->driver_data = battery;
 	mutex_init(&battery->lock);
+	if (ACPI_SUCCESS(acpi_get_handle(battery->device->handle,
+			"_BIX", &handle)))
+		set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
 	acpi_battery_update(battery);
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	result = acpi_battery_add_fs(device);

Henrique de Moraes Holschuh

2009-10-17 15:41 UTC #8 lore raw

Re: [PATCH 4/4] ACPI: Battery: Add support for _BIX extended info method

In reply to Alan Jenkins

Quoted 5 lines · Alan Jenkins

So userspace is supposed to realise that "0" means "this attribute is not supported", as opposed to "I am a brand new battery and don't remember being factory-tested".
How about returning -1 for batteries without _BIX?

How about NOT registering attributes that don't exist? That's the proper way of doing things in sysfs.

When that's not possible, I'd suggest doing the right thing and returning an error of some sort (ENXIO, ENOTSUP, etc)...

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Alexey Starikovskiy

2010-03-14 22:16 UTC #9 lore raw

Re: [PATCH 4/4] ACPI: Battery: Add support for _BIX extended info method

Henrique de Moraes Holschuh пишет:

Quoted 6 lines · Alan Jenkins

Quoted 5 lines
So userspace is supposed to realise that "0" means "this attribute is not supported", as opposed to "I am a brand new battery and don't remember being factory-tested".

Quoted 7 lines

Quoted 2 lines
How about returning -1 for batteries without _BIX?
How about NOT registering attributes that don't exist? That's the proper way of doing things in sysfs.

That is going to be too complex (2 duplicated arrays)

Quoted 4 lines

When that's not possible, I'd suggest doing the right thing and returning an error of some sort (ENXIO, ENOTSUP, etc)...

None of power class members does it, and all ACPI classes are returning -1 in not-supporting case.

Regards,
Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Henrique de Moraes Holschuh

2010-03-15 11:38 UTC #10 lore raw

Re: [PATCH 4/4] ACPI: Battery: Add support for _BIX extended info method

Quoted 6 lines · Alexey Starikovskiy

Henrique de Moraes Holschuh ??????????:
Quoted 5 lines · Alan Jenkins
Quoted 4 lines
So userspace is supposed to realise that "0" means "this attribute is not supported", as opposed to "I am a brand new battery and don't remember being factory-tested".

Quoted 6 lines

Quoted 5 lines
Quoted 1 line
How about returning -1 for batteries without _BIX?
How about NOT registering attributes that don't exist? That's the proper way of doing things in sysfs.
That is going to be too complex (2 duplicated arrays)

It would be the Right Thing to do. Still, you could return a proper error as I suggested.

Quoted 5 lines

Quoted 3 lines
When that's not possible, I'd suggest doing the right thing and returning an error of some sort (ENXIO, ENOTSUP, etc)...
None of power class members does it, and all ACPI classes are returning -1 in not-supporting case.

That just means the ACPI sysfs conversion on that area was not as good as it should have been. It is hardly the only place where the sysfs ABI is not well implemented, but that doesn't mean the breakage should remain, or that it should be made worse.

What is the technical case to returning invalid values for something that is not supported when you could return a proper error on open() instead (since you're not going to do the Right Thing and not register that attribute it in the first place)? Not only the "-1" way wastes more system resources (all clients have to do open+read+close), it also needs userspace to special case something, and that is always a Bad Idea for *many* reasons.

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Alexey Starikovskiy

author

2010-03-15 20:17 UTC #11 lore raw

Re: [PATCH 4/4] ACPI: Battery: Add support for _BIX extended info method

Henrique de Moraes Holschuh пишет:

Quoted 6 lines

That just means the ACPI sysfs conversion on that area was not as good as it should have been. It is hardly the only place where the sysfs ABI is not well implemented, but that doesn't mean the breakage should remain, or that it should be made worse.
What is the technical case to returning invalid values for something that is not supported when you could return a proper error on open() instead (since

Power Class interface does not give me control over open() as far as I know.

Quoted 4 lines

you're not going to do the Right Thing and not register that attribute it in the first place)? Not only the "-1" way wastes more system resources (all clients have to do open+read+close), it also needs userspace to special case something, and that is always a Bad Idea for *many* reasons.

If you know how this could be done, please show the patch...

Thanks,
Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Henrique de Moraes Holschuh

2010-03-16 00:12 UTC #12 lore raw

Re: [PATCH 4/4] ACPI: Battery: Add support for _BIX extended info method

Quoted 9 lines · Alexey Starikovskiy

Henrique de Moraes Holschuh ??????????:
Quoted 6 lines
That just means the ACPI sysfs conversion on that area was not as good as it should have been. It is hardly the only place where the sysfs ABI is not well implemented, but that doesn't mean the breakage should remain, or that it should be made worse.
What is the technical case to returning invalid values for something that is not supported when you could return a proper error on open() instead (since
Power Class interface does not give me control over open() as far as I know.

Hmm, you're correct. We can return errors, but they'll be on read()/write(), not open().

Quoted 5 lines

Quoted 4 lines
you're not going to do the Right Thing and not register that attribute it in the first place)? Not only the "-1" way wastes more system resources (all clients have to do open+read+close), it also needs userspace to special case something, and that is always a Bad Idea for *many* reasons.
If you know how this could be done, please show the patch...

Let me look at that code, and I will get back to you. Might have to use the recently introduced sysfs helpers or some somewhat ugly crap I had to add to thinkpad-acpi to do just that, though.

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Len Brown

reviewer

2010-01-15 22:03 UTC #13 lore raw

Re: [PATCH 4/4] ACPI: Battery: Add support for _BIX extended info method

In reply to Alexey Starikovskiy

applied to acpi-test with checkpatch error fixed:

ERROR: spaces required around that '?' (ctx:VxE)
#106: FILE: drivers/acpi/battery.c:390:
+ char *name = test_bit(ACPI_BATTERY_XINFO_PRESENT,
&battery->flags)?

total: 1 errors, 0 warnings, 135 lines checked

thanks,
Len Brown, Intel Open Source Technology Center

Len Brown

reviewer

2010-01-15 21:59 UTC #14 lore raw

In reply to Alexey Starikovskiy

applied to acpi-test as below -- with the checkpatch issues fixed...

thanks,
Len Brown, Intel Open Source Technology Center

commit 7b3bcc4a1a7cd2d53b403ca29d06ceb5fa617eb7

Author: Alexey Starikovskiy <[email protected]>
Date:   Thu Oct 15 14:31:24 2009 +0400

ACPI: Battery: Add bit flags

Signed-off-by Alexey Starikovskiy <[email protected]>

Signed-off-by Len Brown <[email protected]>

drivers/acpi/battery.c +14 ~~-12~~

index cada73f..b2b48f8 100644
@@ -88,10 +88,13 @@ static const struct acpi_device_id battery_device_ids[] = {
 
 MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
-/* For buggy DSDTs that report negative 16-bit values for either charging
- * or discharging current and/or report 0 as 65536 due to bad math.
- */
-#define QUIRK_SIGNED16_CURRENT 0x0001
+enum {
+	ACPI_BATTERY_ALARM_PRESENT,
+	/* For buggy DSDTs that report negative 16-bit values for either charging
+	* or discharging current and/or report 0 as 65536 due to bad math.
+	*/
+	ACPI_BATTERY_QUIRK_SIGNED16_CURRENT,
+};
 
 struct acpi_battery {
 	struct mutex lock;

@@ -118,8 +121,7 @@ struct acpi_battery {
 	char oem_info[32];
 	int state;
 	int power_unit;
-	u8 alarm_present;
-	long quirks;
+	unsigned long flags;
 };
 
 #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);

@@ -399,7 +401,7 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
 	battery->update_time = jiffies;
 	kfree(buffer.pointer);
 
-	if ((battery->quirks & QUIRK_SIGNED16_CURRENT) &&
+	if (test_bit(ACPI_BATTERY_QUIRK_SIGNED16_CURRENT, &battery->flags) &&
 	    battery->rate_now != -1)
 		battery->rate_now = abs((s16)battery->rate_now);

@@ -412,7 +414,8 @@ static int acpi_battery_set_alarm(struct acpi_battery *battery)
 	union acpi_object arg0 = { .type = ACPI_TYPE_INTEGER };
 	struct acpi_object_list arg_list = { 1, &arg0 };
 
-	if (!acpi_battery_present(battery)|| !battery->alarm_present)
+	if (!acpi_battery_present(battery)||
+	    !test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags))
 		return -ENODEV;
 
 	arg0.integer.value = battery->alarm;

@@ -437,10 +440,10 @@ static int acpi_battery_init_alarm(struct acpi_battery *battery)
 	/* See if alarms are supported, and if so, set default */
 	status = acpi_get_handle(battery->device->handle, "_BTP", &handle);
 	if (ACPI_FAILURE(status)) {
-		battery->alarm_present = 0;
+		clear_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags);
 		return 0;
 	}
-	battery->alarm_present = 1;
+	set_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags);
 	if (!battery->alarm)
 		battery->alarm = battery->design_capacity_warning;
 	return acpi_battery_set_alarm(battery);

@@ -510,9 +513,8 @@ static void sysfs_remove_battery(struct acpi_battery *battery)
 
 static void acpi_battery_quirks(struct acpi_battery *battery)
 {
-	battery->quirks = 0;
 	if (dmi_name_in_vendors("Acer") && battery->power_unit) {
-		battery->quirks |= QUIRK_SIGNED16_CURRENT;
+		set_bit(ACPI_BATTERY_QUIRK_SIGNED16_CURRENT, &battery->flags);
 	}
 }